Skip to content

Commit 37b4e28

Browse files
fix: address Copilot review — dual-screen rendering correctness
- Add dualScreenPipelineBuildFailed flag to skip per-frame pipeline retry and log spam after a build failure; reset on layout reinstall - Apply outputFrame normalization heuristic (same as normalizeFrame()) to handle Delta skin JSON that stores pixel-space rather than 0-1 coords - Replace isMetalDualScreenActive checks in skin frame handler with direct dualScreenLayout != nil checks for more accurate fallback detection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent f51fb54 commit 37b4e28

File tree

4 files changed

+41
-11
lines changed

4 files changed

+41
-11
lines changed

PVUI/Sources/PVUIBase/PVEmulatorVC/PVEmulatorViewController+DualScreen.swift

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -544,10 +544,11 @@ extension PVEmulatorViewController {
544544
if self.core.coreIdentifier?.contains("emuThree") == true || self.core.coreIdentifier?.contains("3DS") == true {
545545
DLOG("🎮 SKIN: Applying dual screen viewport for emuThreeDS: \(combinedRect)")
546546
self.applyDualScreenViewportForEmuThree(frame: combinedRect)
547-
} else if self.isMetalDualScreenActive {
548-
// Metal dual-screen is actively handling layout via sub-rectangle blits;
549-
// skip applyFrameToGPUView to avoid overriding expandMetalViewToFillParent.
550-
DLOG("🎮 SKIN: Metal dual-screen active, skipping applyFrameToGPUView")
547+
} else if (self.gpuViewController as? PVMetalViewController)?.dualScreenLayout != nil {
548+
// Metal dual-screen layout is installed and handling rendering via
549+
// sub-rectangle blits; skip applyFrameToGPUView to avoid overriding
550+
// the view that expandMetalViewToFillParent already sized to full frame.
551+
DLOG("🎮 SKIN: Metal dual-screen layout active, skipping applyFrameToGPUView")
551552
} else {
552553
DLOG("🎮 SKIN: Applying dual screen viewport (standard): \(combinedRect)")
553554
self.applyFrameToGPUView(combinedRect)
@@ -567,7 +568,7 @@ extension PVEmulatorViewController {
567568
DLOG("🎮 SKIN: Timeout - only one screen received, applying single frame: \(frame)")
568569
if self.core.coreIdentifier?.contains("emuThree") == true || self.core.coreIdentifier?.contains("3DS") == true {
569570
self.applyDualScreenViewportForEmuThree(frame: frame)
570-
} else if !self.isMetalDualScreenActive {
571+
} else if (self.gpuViewController as? PVMetalViewController)?.dualScreenLayout == nil {
571572
self.applyFrameToGPUView(frame)
572573
}
573574
self.currentTargetFrame = frame

PVUI/Sources/PVUIBase/PVEmulatorVC/PVEmulatorViewController+MetalDualScreen.swift

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,24 @@ extension PVEmulatorViewController {
147147

148148
// --- Destination (view-space points) ---
149149
// Mirror the calculation in currentDualScreenViewportFrame() exactly.
150-
let inLayout = CGRect(x: outputFrame.minX * scaledW,
151-
y: outputFrame.minY * scaledH,
152-
width: outputFrame.width * scaledW,
153-
height: outputFrame.height * scaledH)
150+
// Skin outputFrames are *supposed* to be 0–1 normalised relative to
151+
// mappingSize, but some JSON skins store raw pixel coordinates instead.
152+
// Apply the same heuristic used by normalizeFrame(): if any component
153+
// exceeds 1.0, divide by mappingSize to convert to the 0–1 range first.
154+
let normOutput: CGRect
155+
if outputFrame.origin.x > 1.0 || outputFrame.origin.y > 1.0 ||
156+
outputFrame.size.width > 1.0 || outputFrame.size.height > 1.0 {
157+
normOutput = CGRect(x: outputFrame.minX / mappingSize.width,
158+
y: outputFrame.minY / mappingSize.height,
159+
width: outputFrame.width / mappingSize.width,
160+
height: outputFrame.height / mappingSize.height)
161+
} else {
162+
normOutput = outputFrame
163+
}
164+
let inLayout = CGRect(x: normOutput.minX * scaledW,
165+
y: normOutput.minY * scaledH,
166+
width: normOutput.width * scaledW,
167+
height: normOutput.height * scaledH)
154168
let destRect = CGRect(x: xOff + inLayout.midX - inLayout.width / 2,
155169
y: yOff + inLayout.midY - inLayout.height / 2,
156170
width: inLayout.width,
@@ -168,6 +182,8 @@ extension PVEmulatorViewController {
168182

169183
ILOG("dual-screen metal: installing layout with \(renderInfos.count) screens")
170184
metalVC.dualScreenLayout = renderInfos
185+
// Reset failure flag so the pipeline gets another build attempt on new layout.
186+
metalVC.dualScreenPipelineBuildFailed = false
171187
isMetalDualScreenActive = true
172188

173189
// Expand the Metal view to fill the parent so both screen quads are visible.

PVUI/Sources/PVUIBase/PVGLViewController/PVMetalViewController+DualScreen.swift

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,11 +98,14 @@ extension PVMetalViewController {
9898
// MARK: Pipeline setup
9999

100100
/// Compiles and caches the dual-screen blit pipeline.
101-
/// Safe to call multiple times; no-ops when the pipeline already exists.
101+
/// Safe to call multiple times; no-ops when the pipeline already exists or after a build failure.
102102
func buildDualScreenBlitPipelineIfNeeded() {
103103
guard dualScreenBlitPipeline == nil else { return }
104+
// Skip retry after a failure to avoid per-frame shader compilation attempts.
105+
guard !dualScreenPipelineBuildFailed else { return }
104106
guard let device = device else {
105107
ELOG("dual-screen: cannot build pipeline – Metal device is nil")
108+
dualScreenPipelineBuildFailed = true
106109
return
107110
}
108111

@@ -111,12 +114,14 @@ extension PVMetalViewController {
111114
library = try device.makeLibrary(source: Self.dualScreenShaderSource, options: nil)
112115
} catch {
113116
ELOG("dual-screen: shader compile error: \(error)")
117+
dualScreenPipelineBuildFailed = true
114118
return
115119
}
116120

117121
guard let vertFn = library.makeFunction(name: "dual_screen_vs"),
118122
let fragFn = library.makeFunction(name: "dual_screen_ps") else {
119123
ELOG("dual-screen: shader functions missing from compiled library")
124+
dualScreenPipelineBuildFailed = true
120125
return
121126
}
122127

@@ -130,6 +135,7 @@ extension PVMetalViewController {
130135
ILOG("dual-screen: render pipeline ready")
131136
} catch {
132137
ELOG("dual-screen: pipeline creation failed: \(error)")
138+
dualScreenPipelineBuildFailed = true
133139
}
134140
}
135141

PVUI/Sources/PVUIBase/PVGLViewController/PVMetalViewController.swift

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,10 @@ class PVMetalViewController : PVGPUViewController, PVRenderDelegate, MTKViewDele
243243
/// Built lazily the first time it is needed.
244244
var dualScreenBlitPipeline: MTLRenderPipelineState? = nil
245245

246+
/// Set to `true` after a failed attempt to build `dualScreenBlitPipeline` so
247+
/// subsequent frames skip the (expensive) retry and log the error only once.
248+
var dualScreenPipelineBuildFailed: Bool = false
249+
246250
/// Controls whether VSync is enabled (synchronizes rendering with display refresh rate)
247251
public var vsyncEnabled: Bool = true
248252

@@ -2468,7 +2472,10 @@ class PVMetalViewController : PVGPUViewController, PVRenderDelegate, MTKViewDele
24682472
// renderDualScreenLayout produced no output (empty layout or pipeline
24692473
// failure) — fall through to the standard fullscreen blit below to
24702474
// avoid presenting a black frame.
2471-
WLOG("dual-screen: renderDualScreenLayout produced no output, falling back to standard blit")
2475+
if !dualScreenPipelineBuildFailed {
2476+
WLOG("dual-screen: renderDualScreenLayout produced no output, falling back to standard blit")
2477+
dualScreenPipelineBuildFailed = true
2478+
}
24722479
}
24732480

24742481
/// Local scope so `endEncoding` runs before `present`/`commit` (function-scoped `defer` would run too late and trip Metal debug `encoding in progress`).

0 commit comments

Comments
 (0)