Skip to content

[Agent] feat(ds): Metal sub-rectangle dual-screen framebuffer rendering#3509

Open
github-actions[bot] wants to merge 7 commits intodevelopfrom
agent/issue-3373
Open

[Agent] feat(ds): Metal sub-rectangle dual-screen framebuffer rendering#3509
github-actions[bot] wants to merge 7 commits intodevelopfrom
agent/issue-3373

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

Summary

Implements GPU-side Metal sub-rectangle rendering for Nintendo DS dual-screen skins, completing the rendering half of the DS dual-screen feature (skin loading was enabled in PR #3377).

  • New Metal shader (dual_screen_blit.metal) — renders a sub-rectangle of the source texture to an arbitrary viewport position in a single render pass
  • PVMetalViewController+DualScreen — defines DualScreenRenderInfo (source UV rect + destination view rect) and extends the Metal renderer to split a combined framebuffer into two independently-positioned quads
  • PVEmulatorViewController+MetalDualScreen — bridges the DeltaSkin layout system to the Metal renderer: reads DeltaSkinScreen.inputFrame/outputFrame from the active skin, computes normalised rects, and installs them on PVMetalViewController
  • applyDualScreenViewport integration — the Metal dual-screen path is tried first for DS cores with an active skin; falls back to the legacy view-resizing path if no skin is available
  • DS coressupportsSkins = true enabled for PVMelonDSCore and PVDesmume2015Core

How it works

The DS emulator produces a combined 256×384 framebuffer (top screen rows 0–191, bottom screen rows 192–383). Each frame, the Metal renderer:

  1. Clears the full drawable to black
  2. Draws a quad mapping y ∈ [0, 0.5] of the texture to the skin's top-screen outputFrame
  3. Draws a quad mapping y ∈ [0.5, 1] of the texture to the skin's bottom-screen outputFrame

Both quads are encoded in a single render pass with no intermediate textures or copies. Portrait (stacked) and landscape (side-by-side) skin layouts are both supported by using the skin's per-screen outputFrame rects directly.

Test plan

  • Load a DS ROM with a dual-screen skin active — both screens render at correct skin-defined positions
  • Load a DS ROM without a skin — falls back to legacy combined-viewport positioning (no regression)
  • Toggle skin on/off mid-game — Metal dual-screen layout is set/cleared correctly
  • Rotate device — layout recomputes to match new orientation
  • Enable image smoothing — bilinear sampler applied to both screens
  • Verify tvOS build compiles (Metal dual-screen path is #if !os(tvOS) guarded)

Part of #3373

🤖 Generated with Claude Code

@github-actions github-actions bot requested a review from JoeMatt as a code owner March 25, 2026 07:56
github-actions bot added a commit that referenced this pull request Mar 25, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions github-actions bot enabled auto-merge (squash) March 25, 2026 07:57
@github-actions github-actions bot added agent-work PR or issue being worked on by the AI agent ai-reviewing AI code review in progress labels Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

🤖 PR created. AI review starting automatically.

@JoeMatt JoeMatt requested a review from Copilot March 25, 2026 08:15
@JoeMatt JoeMatt self-assigned this Mar 25, 2026
@JoeMatt JoeMatt added system-NDS regarding Nintendo DS [non-core specific] priority:High High priority skins Related to controller skins / Delta skins labels Mar 25, 2026
@github-actions github-actions bot removed the ai-reviewing AI code review in progress label Mar 25, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements GPU-side Metal sub-rectangle rendering for Nintendo DS dual-screen skins by splitting the combined DS framebuffer into per-screen quads and wiring DeltaSkin layout data into the Metal render path.

Changes:

  • Added a dual-screen rendering path in PVMetalViewController that draws multiple sub-rect blits in one render pass.
  • Added DeltaSkin → Metal bridging to compute per-screen source UV rects and destination view rects.
  • Enabled supportsSkins = true for melonDS and DeSmuME 2015 cores, plus added release notes/What’s New entry.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
PVUI/Sources/PVUIBase/PVGLViewController/PVMetalViewController.swift Routes directRender through a dual-screen render path when a layout is installed.
PVUI/Sources/PVUIBase/PVGLViewController/PVMetalViewController+DualScreen.swift Defines DualScreenRenderInfo and encodes multi-quad sub-rectangle blits per frame.
PVUI/Sources/PVUIBase/PVEmulatorVC/PVEmulatorViewController+MetalDualScreen.swift Computes dual-screen rects from DeltaSkin screen groups and installs them on PVMetalViewController.
PVUI/Sources/PVUIBase/PVEmulatorVC/PVEmulatorViewController+DualScreen.swift Integrates the Metal dual-screen path into applyDualScreenViewport.
PVUI/Sources/PVSwiftUI/Resources/whats-new.json Adds a new “DS Dual-Screen Skins” What’s New entry.
PVShaders/Sources/PVShaders/Resources/Metal/Blitters/dual_screen_blit.metal Adds a Metal shader intended for dual-screen sub-rectangle blitting.
Cores/melonDS/PVMelonDSCore/Core/PVMelonDSCore.swift Enables skin support for melonDS.
Cores/Desmume2015/PVDesmume2015Core/Core/PVDesmume2015Core.swift Enables skin support for DeSmuME 2015.
.changelog/3509.md Adds changelog fragment describing the feature.

Comment on lines +154 to +158
let sampler = renderSettings.smoothingEnabled ? linearSampler : pointSampler
encoder.setRenderPipelineState(pipeline)
encoder.setFragmentTexture(sourceTexture, index: 0)
if let sampler { encoder.setFragmentSamplerState(sampler, index: 0) }

Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dual_screen_ps declares an explicit sampler [[sampler(0)]], but the code only binds a sampler when linearSampler/pointSampler are non-nil. Some pipeline setup paths in PVMetalViewController (e.g. createBasicShadersWithDefaultLibrary) never create these sampler states, which would leave sampler(0) unbound and can trigger a Metal validation/GPU error. Ensure a sampler state is always created/bound here (or change the fragment shader to use a constexpr sampler so it doesn't require binding).

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +14
// dual_screen_blit.metal
// PVShaders
//
// Metal shaders for DS dual-screen sub-rectangle blitting.
//
// Each screen is rendered by a separate draw call in a single render pass.
// The vertex buffer carries interleaved (NDC-position, UV) data so the vertex
// function needs no uniforms — all per-screen geometry is baked in by the CPU.
//
// Usage pattern (per screen):
// 1. Compute 4 vertices of the form float4(ndcX, ndcY, srcU, srcV)
// arranged as a triangle-strip (TL, TR, BL, BR).
// 2. Call setVertexBytes(_:length:index:) with buffer index 0.
// 3. drawPrimitives(.triangleStrip, vertexStart: 0, vertexCount: 4)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file adds dual_screen_blit.metal, but the dual-screen pipeline in PVMetalViewController+DualScreen currently compiles shader source from an inline string instead of loading functions from the module’s default Metal library. If the intent is to ship this shader via PVShaders, it should be referenced/registered (or the extra file removed) to avoid dead/duplicated resources.

Copilot uses AI. Check for mistakes.
Comment on lines +32 to +39
var canUseMetalDualScreenRendering: Bool {
#if os(tvOS)
return false
#else
guard gpuViewController is PVMetalViewController else { return false }
guard core.supportsDualScreens else { return false }
guard isDeltaSkinEnabled, currentSkin != nil else { return false }
return true
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

canUseMetalDualScreenRendering is gated on core.supportsDualScreens, which is also true for non-DS dual-screen systems (e.g. emuThree/3DS). Since applyMetalDualScreenLayout assumes a DS-style combined framebuffer split and DS-specific defaults (256×384, top/bottom halves), this should be additionally gated to Nintendo DS (e.g. core.systemIdentifier == SystemIdentifier.DS.rawValue) to avoid accidentally taking the DS split path for other dual-screen cores if/when they gain skins + Metal rendering.

Copilot uses AI. Check for mistakes.
Comment on lines +571 to +588
},
{
"version": "3.8.0",
"title": "DS Dual-Screen Skins",
"features": [
{
"symbolName": "rectangle.split.1x2.fill",
"symbolColor": "blue",
"title": "Nintendo DS Dual-Screen Skins",
"subtitle": "DS games now support full DeltaSkin overlays with GPU-accelerated dual-screen rendering. Both top and bottom screens are positioned independently according to your skin layout."
},
{
"symbolName": "square.3.layers.3d",
"symbolColor": "purple",
"title": "Metal Sub-Rectangle Rendering",
"subtitle": "The DS framebuffer is split into two sub-rectangles on the GPU in a single render pass — no extra copies, minimal overhead."
}
]
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per repo release-versioning rules, whats-new.json entries should only be added when the target version has been explicitly confirmed by a maintainer (and version numbers must not be guessed). Adding a new entry for "version": "3.8.0" here looks speculative—please remove it or update it to the confirmed release version once provided.

Suggested change
},
{
"version": "3.8.0",
"title": "DS Dual-Screen Skins",
"features": [
{
"symbolName": "rectangle.split.1x2.fill",
"symbolColor": "blue",
"title": "Nintendo DS Dual-Screen Skins",
"subtitle": "DS games now support full DeltaSkin overlays with GPU-accelerated dual-screen rendering. Both top and bottom screens are positioned independently according to your skin layout."
},
{
"symbolName": "square.3.layers.3d",
"symbolColor": "purple",
"title": "Metal Sub-Rectangle Rendering",
"subtitle": "The DS framebuffer is split into two sub-rectangles on the GPU in a single render pass — no extra copies, minimal overhead."
}
]

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +5
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations.

### Changed
- **melonDS / DeSmuME skin support** — `supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog fragments follow the style rule “plain English, no trailing period”. This entry ends with a period; please drop the trailing punctuation to match existing changelog formatting.

Suggested change
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations.
### Changed
- **melonDS / DeSmuME skin support**`supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed.
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations
### Changed
- **melonDS / DeSmuME skin support**`supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed

Copilot uses AI. Check for mistakes.
DLOG("🎮 Applied Metal dual-screen layout")
return
}
// Layout computation failed — fall through to the legacy path.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If canUseMetalDualScreenRendering is true but applyMetalDualScreenLayout() returns false, this falls through to the legacy positioning path without clearing any previously-installed metalVC.dualScreenLayout. Several failure paths in applyMetalDualScreenLayout early-return without clearing, so you can end up with a stale dual-screen layout still active while the legacy viewport-resize logic runs. Consider clearing the Metal layout on failure before falling back (or ensure applyMetalDualScreenLayout clears layout on every failure path).

Suggested change
// Layout computation failed — fall through to the legacy path.
// Layout computation failed — clear any existing Metal dual-screen layout
// before falling back to the legacy viewport positioning path.
clearMetalDualScreenLayout()

Copilot uses AI. Check for mistakes.
Comment on lines +2427 to +2443
renderDualScreenLayout(encoder: renderEncoder,
sourceTexture: inputTexture,
drawableSize: drawableSize,
flipY: flipY)
renderEncoder.endEncoding()
commandBuffer.present(drawable)
commandBuffer.addCompletedHandler { [weak self] buffer in
if let error = buffer.error {
ELOG("GPU error during dual-screen rendering: \(error)")
self?.recoverFromGPUError()
}
}
commandBuffer.commit()
previousCommandBuffer = commandBuffer
frameCount += 1
markFramePresented()
return
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

directRender switches to the dual-screen path whenever dualScreenLayout != nil, but renderDualScreenLayout can early-return (empty layout or pipeline build failure). In that case this path still ends encoding + presents the drawable, resulting in a cleared black frame with no fallback. Consider requiring a non-empty layout and having renderDualScreenLayout report success so directRender can fall back to the standard blit when nothing was drawn (or clear dualScreenLayout on failure).

Suggested change
renderDualScreenLayout(encoder: renderEncoder,
sourceTexture: inputTexture,
drawableSize: drawableSize,
flipY: flipY)
renderEncoder.endEncoding()
commandBuffer.present(drawable)
commandBuffer.addCompletedHandler { [weak self] buffer in
if let error = buffer.error {
ELOG("GPU error during dual-screen rendering: \(error)")
self?.recoverFromGPUError()
}
}
commandBuffer.commit()
previousCommandBuffer = commandBuffer
frameCount += 1
markFramePresented()
return
let didRenderDualScreen = renderDualScreenLayout(encoder: renderEncoder,
sourceTexture: inputTexture,
drawableSize: drawableSize,
flipY: flipY)
if didRenderDualScreen {
renderEncoder.endEncoding()
commandBuffer.present(drawable)
commandBuffer.addCompletedHandler { [weak self] buffer in
if let error = buffer.error {
ELOG("GPU error during dual-screen rendering: \(error)")
self?.recoverFromGPUError()
}
}
commandBuffer.commit()
previousCommandBuffer = commandBuffer
frameCount += 1
markFramePresented()
return
}
// If dual-screen rendering did not produce any output, fall back to the
// standard fullscreen blit/filter path below without presenting a black frame.

Copilot uses AI. Check for mistakes.
Comment on lines +54 to +88
// MARK: Inline Metal source

/// Metal source for the dual-screen sub-rectangle blit shaders.
///
/// Compiled at runtime so we don't need to load from a bundle — matches the
/// pattern already used by `createBasicShaders()`.
static let dualScreenShaderSource = """
#include <metal_stdlib>
using namespace metal;

struct DSVSOut {
float4 position [[position]];
float2 texCoord;
};

// buffer(0): array of float4 (NDC.xy, UV.zw) for a 4-vertex triangle strip
vertex DSVSOut dual_screen_vs(
uint vid [[vertex_id]],
constant float4 *vertices [[buffer(0)]])
{
DSVSOut out;
out.position = float4(vertices[vid].xy, 0.0f, 1.0f);
out.texCoord = vertices[vid].zw;
return out;
}

fragment half4 dual_screen_ps(
DSVSOut in [[stage_in]],
texture2d<half> source [[texture(0)]],
sampler samp [[sampler(0)]])
{
half4 c = source.sample(samp, in.texCoord);
c.a = 1.0h;
return c;
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo already has a Metal shader bundle in PVShaders (loaded via MetalShaderManager.shared.shaderBundle), and this PR also adds dual_screen_blit.metal. Keeping a separate inline dualScreenShaderSource duplicates the shader code and risks divergence. Prefer building the pipeline from the default library (bundle) and remove the inline source, or remove the .metal resource if runtime compilation is the intended approach.

Suggested change
// MARK: Inline Metal source
/// Metal source for the dual-screen sub-rectangle blit shaders.
///
/// Compiled at runtime so we don't need to load from a bundle — matches the
/// pattern already used by `createBasicShaders()`.
static let dualScreenShaderSource = """
#include <metal_stdlib>
using namespace metal;
struct DSVSOut {
float4 position [[position]];
float2 texCoord;
};
// buffer(0): array of float4 (NDC.xy, UV.zw) for a 4-vertex triangle strip
vertex DSVSOut dual_screen_vs(
uint vid [[vertex_id]],
constant float4 *vertices [[buffer(0)]])
{
DSVSOut out;
out.position = float4(vertices[vid].xy, 0.0f, 1.0f);
out.texCoord = vertices[vid].zw;
return out;
}
fragment half4 dual_screen_ps(
DSVSOut in [[stage_in]],
texture2d<half> source [[texture(0)]],
sampler samp [[sampler(0)]])
{
half4 c = source.sample(samp, in.texCoord);
c.a = 1.0h;
return c;
}

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +65
let orientation: DeltaSkinOrientation = view.bounds.width > view.bounds.height ? .landscape : .portrait
let traits = DeltaSkinTraits(device: skinDevice,
displayType: .standard,
orientation: orientation,
gameIdentifier: game?.title)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This extension says it “mirrors currentDualScreenViewportFrame()”, but it derives orientation from view.bounds rather than using the same orientation source (currentOrientation) used by the legacy dual-screen layout. During rotation (before/while layout settles), these can disagree and cause the wrong skin traits/screen group to be selected. Consider reusing the same orientation logic as applyDualScreenViewport to keep Metal and legacy paths consistent.

Copilot uses AI. Check for mistakes.
Comment on lines +2 to +5
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations.

### Changed
- **melonDS / DeSmuME skin support** — `supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed.
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changelog fragments follow the style rule “plain English, no trailing period”. This entry ends with a period; please drop the trailing punctuation to match existing changelog formatting.

Suggested change
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations.
### Changed
- **melonDS / DeSmuME skin support**`supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed.
- **DS Dual-Screen Metal Rendering** — Nintendo DS games with a DeltaSkin active now render both screens as GPU-side sub-rectangle blits in a single Metal render pass. The combined 256×384 DS framebuffer is split into top (rows 0–191) and bottom (rows 192–383) viewports that match the skin's screen layout, supporting portrait (stacked) and landscape (side-by-side) configurations
### Changed
- **melonDS / DeSmuME skin support**`supportsSkins` is now `true` for both DS cores, enabling DeltaSkin overlays and controller layouts when a compatible skin is installed

Copilot uses AI. Check for mistakes.
github-actions bot added a commit that referenced this pull request Mar 25, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@JoeMatt
Copy link
Copy Markdown
Member

JoeMatt commented Mar 25, 2026

@claude please audit all Copilot review comments on this PR. For each comment:

  1. Evaluate whether the concern is valid
  2. Fix any valid issues (compile errors, logic bugs, missing null checks, incorrect API usage, etc.)
  3. Note/dismiss nits that are style-only with a brief explanation

After addressing the review comments, do a self-review pass:

  • Verify all changed files compile (check for unresolved symbols, missing imports, type mismatches)
  • Check for any dead code, unused variables, or missing test coverage
  • Ensure multi-platform compatibility (iOS + tvOS + macOS Catalyst)

Then re-request Copilot review.

@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Mar 25, 2026

Rebase Complete

Resolved

  • Agent started
  • Fetched PR branch agent/issue-3373
  • Conflict 1: PVMetalViewController.swift — hoisted flipY before do block; placed dual-screen path outside do { defer { endEncoding } } to preserve Metal encoding order
  • Conflict 2: whats-new.json (add/add) — kept all develop entries + PR's 3.8.0 DS entry
  • Conflict 3: whats-new.json (fix commit) — removed 3.8.0 DS entry per fix commit intent; kept all develop entries
  • Force-pushed rebased branch
  • CI validation re-triggered

Completed: 2026-04-01

@github-actions github-actions bot added the ai-reviewing AI code review in progress label Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

✅ Fixes pushed — addressed all 10 Copilot review comments (8 distinct issues)

Fixed (logic bugs / correctness):

  • Unbound sampler — Changed dual_screen_ps fragment shader to use constexpr sampler (eliminates GPU validation error from unbound sampler(0) slot); removed now-unused external sampler binding
  • Black frame on failurerenderDualScreenLayout now returns Bool; directRender falls through to the standard fullscreen blit instead of presenting a black frame when the layout is empty or the pipeline fails
  • Stale Metal layout on fallback — Added clearMetalDualScreenLayout() call when applyMetalDualScreenLayout() returns false, preventing a stale dual-screen layout from persisting while the legacy viewport path runs
  • DS-only gatingcanUseMetalDualScreenRendering now checks SystemIdentifier(rawValue: core.systemIdentifier ?? "") == .DS; avoids accidentally taking the DS framebuffer-split path for 3DS/emuThree dual-screen cores
  • Orientation consistencyapplyMetalDualScreenLayout now uses currentOrientation (same source as the legacy dual-screen path) instead of view.bounds aspect ratio, preventing wrong skin traits during rotation

Fixed (dead code / duplicates):

  • Removed dual_screen_blit.metal — file was dead code; the inline shader source in PVMetalViewController+DualScreen.swift is the single source of truth

Dismissed as style-only (fixed anyway):

  • Speculative whats-new.json version — Removed the 3.8.0 entry since the version is unconfirmed by a maintainer
  • Changelog trailing periods — Removed trailing . from both changelog entries per repo style convention

@github-actions github-actions bot removed the ai-reviewing AI code review in progress label Mar 25, 2026
@JoeMatt JoeMatt requested a review from Copilot March 25, 2026 19:38
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comment on lines +54 to +61
// MARK: Inline Metal source

/// Metal source for the dual-screen sub-rectangle blit shaders.
///
/// Compiled at runtime so we don't need to load from a bundle — matches the
/// pattern already used by `createBasicShaders()`.
static let dualScreenShaderSource = """
#include <metal_stdlib>
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description mentions a new dual_screen_blit.metal shader file, but the implementation compiles an inline Metal source string at runtime. Please either add the promised .metal resource, or update the PR description / comments so it matches the actual approach (runtime makeLibrary(source:)).

Copilot uses AI. Check for mistakes.
Comment on lines +216 to +230
// Metal dual-screen path: for DS cores (melonDS, DeSmuME) with an active
// skin, hand the sub-rectangle layout to PVMetalViewController so it can
// render both screens in a single GPU pass instead of resizing the view.
if canUseMetalDualScreenRendering {
if applyMetalDualScreenLayout() {
DLOG("🎮 Applied Metal dual-screen layout")
return
}
// Layout computation failed — clear any stale Metal layout before
// falling back to the legacy viewport-resize path.
clearMetalDualScreenLayout()
} else {
// Skin disabled or core not Metal-based; clear any stale layout.
clearMetalDualScreenLayout()
}
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new Metal dual-screen path returns early after installing the layout, but the existing DeltaSkinColorBarsFrameUpdated dual-screen notification handler still calls applyFrameToGPUView(...) for dual-screen systems. That can resize the Metal view back to the combined rect (undoing expandMetalViewToFillParent) and break the destination rect math/clipping. Consider updating the notification handler (or observer setup) to skip view-resizing when canUseMetalDualScreenRendering is true, and instead recompute/install the Metal layout on frame updates.

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +44
public struct DualScreenRenderInfo: Sendable {
/// Normalized (0…1) source sub-rectangle inside the combined input texture.
public let normalizedSourceRect: CGRect
/// Destination in the Metal view's UIKit-point coordinate space.
public let viewDestRect: CGRect

public init(normalizedSourceRect: CGRect, viewDestRect: CGRect) {
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DualScreenRenderInfo is declared public, but it appears to be used only internally by PVMetalViewController/PVEmulatorViewController within PVUIBase. If it’s not intended as part of the module’s public API surface, consider making it internal to avoid committing to a public ABI/API you may want to change later.

Suggested change
public struct DualScreenRenderInfo: Sendable {
/// Normalized (0…1) source sub-rectangle inside the combined input texture.
public let normalizedSourceRect: CGRect
/// Destination in the Metal view's UIKit-point coordinate space.
public let viewDestRect: CGRect
public init(normalizedSourceRect: CGRect, viewDestRect: CGRect) {
struct DualScreenRenderInfo: Sendable {
/// Normalized (0…1) source sub-rectangle inside the combined input texture.
let normalizedSourceRect: CGRect
/// Destination in the Metal view's UIKit-point coordinate space.
let viewDestRect: CGRect
init(normalizedSourceRect: CGRect, viewDestRect: CGRect) {

Copilot uses AI. Check for mistakes.
Comment on lines +196 to +199
encoder.setVertexBytes(&vertices,
length: vertices.count * MemoryLayout<SIMD4<Float>>.stride,
index: 0)
encoder.drawPrimitives(type: .triangleStrip, vertexStart: 0, vertexCount: 4)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setVertexBytes(&vertices, ...) is passing a pointer to the Swift Array value (metadata), not to the contiguous SIMD4<Float> element storage. This will produce incorrect vertex data (and can crash) at runtime. Use vertices.withUnsafeBytes { ... } / withUnsafeBufferPointer to pass the element bytes, or allocate a fixed-size struct/MTLBuffer for the 4 vertices.

Suggested change
encoder.setVertexBytes(&vertices,
length: vertices.count * MemoryLayout<SIMD4<Float>>.stride,
index: 0)
encoder.drawPrimitives(type: .triangleStrip, vertexStart: 0, vertexCount: 4)
vertices.withUnsafeBytes { vertexBytes in
guard let baseAddress = vertexBytes.baseAddress else { return }
encoder.setVertexBytes(baseAddress,
length: vertexBytes.count,
index: 0)
encoder.drawPrimitives(type: .triangleStrip, vertexStart: 0, vertexCount: 4)
}

Copilot uses AI. Check for mistakes.
@github-actions github-actions bot added the needs-fixes AI review found issues; auto-fixer dispatched label Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

✅ Fixes pushed — addressed Copilot review comments:

  1. DualScreenRenderInfo access level — changed from public to internal; the type is used only within PVUIBase and was not part of the module's intended public API
  2. setVertexBytes unsafe pointer — replaced &vertices with vertices.withUnsafeBytes { ptr in ... } to correctly pass a pointer to the contiguous element storage rather than the Swift Array metadata
  3. Notification handler guardhandleSkinFrameUpdated now skips applyFrameToGPUView when canUseMetalDualScreenRendering is true, preventing the skin-frame notification from resizing the Metal view back to the combined rect and undoing expandMetalViewToFillParent
  4. Shader comment — updated inline dualScreenShaderSource comment to accurately describe the runtime makeLibrary(source:) approach

Other comments were already addressed in prior commits (orientation uses currentOrientation, applyDualScreenViewport already clears stale Metal layout before fallback, directRender already falls through to standard blit when renderDualScreenLayout returns false) or refer to files not present in this PR's diff.

@github-actions github-actions bot added ai-reviewing AI code review in progress and removed needs-fixes AI review found issues; auto-fixer dispatched ai-reviewing AI code review in progress labels Mar 25, 2026
@github-actions
Copy link
Copy Markdown
Contributor Author

github-actions bot commented Apr 1, 2026

Rebase Conflicts Resolved

Rebased agent/issue-3373 onto origin/develop — 3 conflicts resolved across 7 commits.

Conflict 1PVMetalViewController.swift (feat commit):

The PR added a dual-screen early-return path. Develop had since wrapped the render pipeline code in a do { defer { renderEncoder.endEncoding() } } block for Metal encoding order safety.

Resolution: hoisted let flipY before the do block and placed the dual-screen path (with its own explicit endEncodingpresentcommit) before the do block. Kept HEAD's if !filterApplied { drawPrimitives } inside the do block unchanged.

Conflict 2whats-new.json (chore commit, add/add):

Both develop and the PR added new entries at the end of the JSON array.

Resolution: kept all develop entries (3.1.0 through 3.9.7), placed the PR's 3.8.0 DS entry before them.

Conflict 3whats-new.json (fix commit):

The fix commit explicitly removed the 3.8.0 DS entry ("version unconfirmed"). This conflicted with the entry added during conflict 2.

Resolution: removed the 3.8.0 DS entry (honouring the fix commit's explicit intent), kept all develop entries.

github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 2, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot added a commit that referenced this pull request Apr 3, 2026
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions bot and others added 7 commits April 4, 2026 02:00
Implements GPU-side split rendering for DS dual-screen skins.

- Add `dual_screen_blit.metal` shader for sub-rectangle blitting
- Add `PVMetalViewController+DualScreen` with DualScreenRenderInfo,
  pipeline setup, and single-pass dual-screen render method
- Add `dualScreenLayout` and `dualScreenBlitPipeline` properties to
  PVMetalViewController; hook into directRender
- Add `PVEmulatorViewController+MetalDualScreen` to compute per-screen
  DualScreenRenderInfo from the active skin's DeltaSkinScreen groups and
  install them on PVMetalViewController
- Route DS dual-screen applyDualScreenViewport through the Metal path
  when a compatible skin is active; clear layout when skins are disabled
- Enable supportsSkins=true for PVMelonDSCore and PVDesmume2015Core

The combined 256×384 DS framebuffer is split into top (y=0..191) and
bottom (y=192..383) sub-rectangles, each rendered to the viewport
position defined by the skin's DeltaSkinScreen.outputFrame.

Part of #3373

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…#3509)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Use constexpr sampler in dual_screen_ps to eliminate unbound sampler(0)
  slot; drop unused external sampler binding in renderDualScreenLayout
- Remove dead dual_screen_blit.metal (inline source is the chosen approach)
- Gate canUseMetalDualScreenRendering to SystemIdentifier.DS only to avoid
  accidentally using DS framebuffer-split logic for 3DS/emuThree cores
- renderDualScreenLayout now returns Bool; directRender falls through to
  standard fullscreen blit instead of presenting a black frame on failure
- Clear stale Metal dual-screen layout when applyMetalDualScreenLayout
  returns false before falling back to the legacy viewport path
- Use currentOrientation (not view.bounds aspect) for skin trait computation
  to stay consistent with the legacy dual-screen path during rotation
- Remove speculative whats-new.json version 3.8.0 entry (version unconfirmed)
- Drop trailing periods from changelog fragment entries

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…es, Metal layout guard

- Make DualScreenRenderInfo internal (was public with no public consumers)
- Fix setVertexBytes to use withUnsafeBytes for correct element-storage pointer
- Guard handleSkinFrameUpdated against calling applyFrameToGPUView when Metal
  dual-screen layout is active, preventing it from undoing expandMetalViewToFillParent

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Sort DS screens by (minY, minX) for stable landscape ordering when
  both screens share the same outputFrame.minY
- Add `isMetalDualScreenActive` property (set by apply/clear) so the
  DeltaSkin frame-update handler correctly skips applyFrameToGPUView
  only when Metal dual-screen is truly active, not just eligible

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix default inputFrame UV split for DeSmuME2015 (bufferSize=2048×2048):
  the old 0–0.5 split sampled the full texture width/height, but the valid
  DS content only occupies the top-left 256×384 region; now normalises by
  DS native dimensions (256×192 per screen) so UVs land on correct texels
  for both padded (2048×2048) and native-size (256×384) textures
- Add dual_screen_blit.metal to PVShaders/Blitters as the canonical source
  of the dual-screen shaders; add comment in PVMetalViewController+DualScreen
  explaining why the inline copy is used at runtime (cross-bundle access)
- Drop trailing period from .changelog/3509.md to match project style

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-work PR or issue being worked on by the AI agent priority:High High priority skins Related to controller skins / Delta skins system-NDS regarding Nintendo DS [non-core specific]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants