fix(frontend): apply HD-pack tile substitution at render (align runtime key with real-Mesen format) (v1.7.1 #3) - #154
Conversation
…me key with real-Mesen format) (v1.7.1 #3) A loaded HD pack (e.g. the user's *Zelda Remastered*, ~15,849 rules) parsed and reported success, but nothing changed on screen — Reset / Power-Cycle did not help. Root cause — render dispatch, not key derivation. The present arm splits into two branches: a `needs_nes` branch (taken when the debugger overlay is visible OR a Cheats / ROM-Database tool panel is open) and the common `else` branch. Only the `else` branch ran `HdCompositor::composite` + `render_hd_with_overlay`; the `needs_nes` branch always presented the stock NES framebuffer via `render_with_overlay`, so a loaded pack was silently inert whenever the debugger / a nes-tool panel was open. The runtime tile-key derivation was VERIFIED correct end-to-end against the real `<ver>106` pack + a live ROM: every rendered tile's key (the CRC-32 of its 16 CHR bytes read from the live pattern-space snapshot, the exact form the ADR-0018 loader stores from the `tileData` field) hit the loaded rule set — 441/441 tile-bearing cells matched. So the loader↔compositor key contract was already sound; the substitution simply never executed in the `needs_nes` branch. Fix: the `needs_nes` branch now captures the same per-frame snapshots the common branch does (the PPU per-pixel tile-source telemetry, the 8 KiB CHR pattern space, and the watched-memory set) under the held emu lock, runs the compositor, and presents the upscaled buffer through `render_hd_with_overlay` (the deep debugger / tool panels still draw on top via the `overlay` closure). The branch's `nes` binding moves from `as_ref` to `as_mut` so the side-effect-free `peek_ppu` / `ppu_bus_peek` / `cpu_bus_peek` snapshot reads can run there. Byte-identical-default guarantee: all of this is `#[cfg(all(feature = "hd-pack", not(target_arch = "wasm32")))]` frontend code, and the snapshot + composite is skipped entirely when no compositor is active. With `hd-pack` off (the shipped default) the native / `no_std` / wasm builds stay byte-identical, and the core PPU `hd_tile_source` export remains output-only telemetry. AccuracyCoin holds 100% (139/139) with `hd-pack` ON and OFF. Adds a unit test (`real_format_tile_substitutes_when_chr_matches_and_falls_ through_otherwise`) proving the loader-key ↔ runtime-key alignment end-to-end through `composite`: a real-format `tileData` tile substitutes when the live CHR hash matches, and a non-matching CHR tile falls through to the original. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where HD-pack tile substitution was not applied when the debugger overlay or tool panels were open. It updates the locked rendering branch (needs_nes) to capture HD snapshots, run the compositor, and present the upscaled buffer, and adds a corresponding unit test and documentation updates. The reviewer feedback suggests dropping the emu lock before running the CPU-heavy HD composite operation to minimize lock contention and prevent emulation thread starvation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Fixes an HD-pack rendering dispatch bug in rustynes-frontend where HD tile substitution was skipped whenever the “needs_nes” (debugger overlay / NES tool panels) render branch was taken, causing loaded packs to appear inert despite being parsed successfully.
Changes:
- Run HD-pack snapshot capture +
HdCompositor::compositein theneeds_nesrender branch, and present viarender_hd_with_overlaywhen an HD compositor is active. - Add an end-to-end unit test validating real-format
<tile>tileDatakey alignment throughcomposite(substitutes on match; falls through on mismatch). - Update frontend docs and
CHANGELOG.mdto document the fixed behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/frontend.md | Updates render-branch documentation to reflect HD-pack compositing now happening in the locked needs_nes path. |
| crates/rustynes-frontend/src/hdpack.rs | Adds a new compositor/key-alignment unit test using a real-format <ver>106 tileData example. |
| crates/rustynes-frontend/src/app.rs | Ensures the needs_nes branch captures HD inputs, composites, and presents the HD frame while still allowing deep overlays to draw on top. |
| CHANGELOG.md | Records the user-visible fix for HD-pack substitution when debugger/tool panels are open. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The new compositor key-alignment test declared `const BASE` after statements; clippy --all-targets (the scripting,hd-pack CI gate) rejects items after statements. Converted it to a `let` binding (a statement, not an item). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review) Restructure the needs_nes render branch (the path taken when the debugger overlay or a Cheats/ROM-Database tool panel is open) so the CPU-heavy comp.composite no longer runs while the emu lock is held, honouring the frontend discipline (docs/frontend.md): never hold the emu lock during heavy work. Snapshot all HD-composite inputs (framebuffer, per-pixel tile-source, 8 KiB CHR pattern space, watched-memory set) into owned locals under a brief lock scope, DROP the guard, run comp.composite + render_hd_with_overlay outside the lock, then re-acquire the lock only to hand the debugger pass a live &mut Nes for render_shell. This mirrors how the common else branch already drops the lock before the composite. Also make the hdpack test chr2 immutable and remove the dummy let _ = &mut chr2; line that silenced unused_mut (it could trip clippy::let_underscore_drop and confused readers). AccuracyCoin holds 139/139 (hd-pack on and off); the default build stays byte-identical (the change is confined to the overlay-open render path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Fixes v1.7.1 issue #3 — a loaded HD pack (the user's Zelda Remastered, ~15,849 rules) parsed and showed a green "HD pack loaded" status, but nothing changed on screen and Reset / Power-Cycle didn't help.
Root cause — render dispatch (NOT key derivation)
The present arm splits into two branches:
needs_nesbranch — taken when the debugger overlay is visible OR a Cheats / ROM-Database tool panel is open, andelsebranch.Only the
elsebranch ranHdCompositor::composite+render_hd_with_overlay. Theneeds_nesbranch always presented the stock NES framebuffer viarender_with_overlay, so a loaded pack was silently inert whenever the debugger / a nes-tool panel was open.The runtime tile-key derivation was verified correct end-to-end against the real
<ver>106pack + a live ROM: every rendered tile's key (the CRC-32 of its 16 CHR bytes read from the live pattern-space snapshot — the exact form the ADR-0018 loader stores from thetileDatafield) hit the loaded rule set, 441/441 tile-bearing cells matched. The loader↔compositor key contract was already sound; the substitution simply never executed in theneeds_nesbranch.Fix
The
needs_nesbranch now captures the same per-frame snapshots the common branch does (PPU per-pixel tile-source telemetry, the 8 KiB CHR pattern space, the watched-memory set) under the held emu lock, runs the compositor, and presents the upscaled buffer throughrender_hd_with_overlay(the deep debugger / tool panels still draw on top via theoverlayclosure). The branch'snesbinding movesas_ref→as_mutso the side-effect-freepeek_ppu/ppu_bus_peek/cpu_bus_peeksnapshot reads can run there.Adds a unit test proving the loader-key ↔ runtime-key alignment end-to-end through
composite: a real-formattileDatatile substitutes when the live CHR hash matches, and a non-matching CHR tile falls through to the original (GPU blit itself is maintainer-manual).Byte-identical-default guarantee
All of this is
#[cfg(all(feature = "hd-pack", not(target_arch = "wasm32")))]frontend code, skipped entirely when no compositor is active. Withhd-packoff (the shipped default) the native /no_std/ wasm builds stay byte-identical; the core PPUhd_tile_sourceexport remains output-only telemetry. AccuracyCoin 100% (139/139) withhd-packON and OFF.Gates (all green)
cargo fmt --all --checkcargo clippy --workspace --all-targets -- -D warningscargo clippy -p rustynes-frontend --all-targets --features scripting,hd-pack -- -D warnings(the hd-pack gate) and--features retroachievements--no-default-features --features wasm-canvas)RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-depscargo build -p rustynes-core --target thumbv7em-none-eabihf --no-default-featurescargo test --workspace --features test-roms→ 1905 passed, 20 ignoredcargo test -p rustynes-frontend --features hd-pack→ 40 passed (incl. the new compositor/key test)hd-packON (-p rustynes-test-harness --features test-roms,hd-pack) and OFFMaintainer verification (GPU / visual)
Re-test with a real pack (e.g.
~/Downloads/UnZipMeFirstZeldaRemasteredV1_3/ZeldaRemastered) on a Zelda PRG0 ROM: the HD tiles should now substitute on screen even with the debugger overlay or the Cheats / ROM-Database panel open. The actual GPU blit (render_hd_with_overlay) is GPU-side and can't be exercised headlessly. HD<bgm>/<sfx>audio is a separate, already-wired$4100tap unaffected by this dispatch bug (left as-is).🤖 Generated with Claude Code