fix: two shipped features that never worked — Pixel Provenance and the Zapper by doublegate · Pull Request #383 · doublegate/RustyNES

doublegate · 2026-08-16T21:09:52Z

Two shipped features that never worked, both reported by the maintainer, both
broken since the release that introduced them.

They share a root shape worth naming up front: in each case the core logic was
well unit-tested and the path a user actually exercises was tested by nothing.
That is the same shape as issue #360 (MovieUi::after_frame works in production,
no test calls it) — three instances now, two of them from the same release train.

1. Pixel Provenance (broken since v2.3.2 "Lucid")

Defect A — run-ahead erased the record before the UI could read it

run_ahead defaults to 1, and RunAhead::finish → Nes::restore_quiet →
restore_inner unconditionally cleared both provenance stores.

That clear is correct for a user-driven save-state load and for netplay
rollback. It is wrong here for a reason that has nothing to do with the restore:
run-ahead's rollback is the last thing before the frontend releases the
emulator lock, so the panel's first opportunity to look is always after it. It
did not discard a stale timeline — it discarded the record for the frame on
screen.

finish now carries both stores around the restore (Nes::take_provenance /
put_provenance — a move of two boxed stores, skipped when neither is armed).
Every other caller still clears; restore_inner sees None and its own
reasoning is untouched.

A comment two lines above that clear asserted the opposite of what the code
did — "leaving exactly the visible frame's writes" — and
docs/pixel-provenance.md repeated it verbatim. The prose asserting the intent
is what stopped anyone checking the code against it. Both corrected here.

Defect B — clicking a pixel was never implemented

The panel offered two DragValue spinboxes and no hit-test; the only Sense in
the file was Sense::hover() on a colour swatch. The docs said "Point at any
pixel" while two later lines in the same document correctly said it pins a
coordinate — the document contradicted itself and the wrong half was the one
users read first.

The NES image is a raw wgpu letterbox blit, not an egui widget, so there is
nothing to hit-test. The click is now captured in the winit handler and converted
by a new gfx::window_to_nes_pixel, which shares one BlitTransform with
letterbox_uniform so the picker and the shader cannot drift — a second
independent derivation of the letterbox is precisely the bug class being
repaired. Its test round-trips against the shader's own uniform rather than a
third re-derivation.

Also fixed while here

The panel mirrored the core's armed flags in frontend state, which desynced
permanently once a ROM load installed a fresh Nes (checkbox ticked, core
unarmed, no recovery but unticking and re-ticking). The core is now the single
source of truth.
A cleared record rendered as fact — every field of one reads as a confident
"scanline 0, dot 0, backdrop, palette $0000". PixelProvenance::is_recorded()
(keyed on dot != 0, unreachable for a real record) now separates not armed
from nothing recorded yet from off-screen.

2. The Zapper: Duck Hunt could never score

The gun fired and nothing could be hit, at any aim point — the maintainer
tried many positions on the ducks before reporting it.

Duck Hunt's protocol is "the gun must see nothing for one frame, then a
bright spot in the next". Bus::sample_zapper_light() runs at the end of
run_frame, so the light bit a read returns during frame N was sampled from
frame N−1. The game received its probe exactly inverted — bright on the
blanked frame, dark on the target frame — and discarded the shot before
hit-testing. Aiming was irrelevant, which is exactly the symptom.

The beam-relative light model (zapper_temporal_light, opt-in since v2.2.3)
is now the default: the bit derives from where the CRT beam is at the moment
of the read, with the ~19-26-scanline photodiode hold the NESdev wiki describes.

A second defect had to go with it — the beam-relative sampler read aperture rows
the beam had not finished painting, which still hold the previous frame, so
it asserted light on an all-black screen. Measured directly: at scanline 96 the
beam was 5 dots into row 96 and the sampler saw the previous frame's sky at
luma 152 on a frame whose mean luma was 0. aperture_is_bright_painted now
excludes rows at or after the current scanline.

Why it shipped that way

v2.2.3 kept the model off because "no pass/fail light-gun test ROM exists… the
supported titles re-poll every frame and are satisfied by either model." The
second half was false, and the first was beside the point — the game is the
oracle. That reasoning is corrected in the source and in accuracy-ledger.md
rather than merely overridden.

Evidence

Measured A/B — same ROM, aim (93,156), identical inputs:

light model	score	duck
frame-granular (shipped)	`000000`	still flying
beam-relative (this PR)	`000500`	marked hit

The protocol itself, from the game's own $4017 traffic: frame 129 all 0x48
(sees nothing, 1918 polls), frame 130 aim_luma 255 with 0x40 — light
detected.

New tests — all mutation-checked

runahead_preserves_pixel_provenance, with
plain_run_leaves_pixel_provenance_populated as a control, so a failure
cannot be misread as a bad assertion or a ROM that emits nothing.
runahead_preserves_the_provenance_arm.
Three gfx::tests::window_to_nes_pixel_*, one round-tripping the picker
against the shader uniform.
duck_hunt_zapper_shot_can_score — asserts Duck Hunt's own scoreboard;
with the model forced off it fails on identical score pixels (428 = 428).

New zapper_light_probe diagnostic reproduces the whole sequence headlessly from
bus traffic and dumps frames, so the next light-gun question is measurable rather
than argued.

Verification

The Zapper change alters emulation behaviour when a Zapper is attached, so
the contract was verified, not assumed:

AccuracyCoin 141/141 (RAM decoder — authoritative; no failing tests)
nestest 0-diff
2,038 workspace tests green, zero failures
fmt, workspace clippy, all four native feature combos, both wasm32
invocations, rustdoc, no_std cross-compile, pre-commit — clean

Reviewer notes

This is a deliberate default change, not additive.
set_zapper_temporal_light(false) restores the pre-v2.3.6 model.
duck_hunt_zapper_shot_can_score pins a specific deterministic replay. If a
future core change shifts that timeline it fails as "replay drifted", which the
assertion message states explicitly.
The Zapper's aim mapping now shares window_to_nes_pixel, so a letterbox bar
reads as "no light" — which its own comment always claimed it did, while in
fact stretching the image across the bars. The Vaus paddle deliberately keeps
its full-window sweep: a knob has no off-screen state.
Both fixes were manually confirmed by the maintainer in the running app.

Addresses the maintainer reports on Pixel Provenance and Duck Hunt.

🤖 Generated with Claude Code

…e Zapper Both were reported by the maintainer, both had been broken since the release that introduced them, and both were invisible because the core logic was unit-tested while the path a user actually exercises was tested by nothing. Pixel Provenance (broken since v2.3.2 "Lucid") ---------------------------------------------- Two independent defects, either of which alone made the panel useless. 1. Run-ahead erased the record before the UI could read it. `run_ahead` defaults to 1, and `RunAhead::finish` -> `Nes::restore_quiet` -> `restore_inner` unconditionally cleared both provenance stores. That clear is correct for a user-driven save-state load and for netplay rollback; it is wrong here for a reason that has nothing to do with the restore itself. Run-ahead's rollback is the LAST thing before the frontend releases the emulator lock, so the panel's first opportunity to look is always after it. The clear did not discard a stale timeline — it discarded the record for the frame on screen. `finish` now carries both stores AROUND the restore via new `Nes::take_provenance` / `put_provenance` (a move of two boxed stores, skipped entirely when neither is armed). Every other caller still clears, unchanged; `restore_inner`'s own reasoning is untouched because it sees `None`. A comment two lines above that clear claimed the opposite of what the code did — "leaving exactly the visible frame's writes" — and `pixel-provenance.md` repeated it. The prose asserting the intent is what stopped anyone checking. 2. Clicking a pixel was never implemented. The panel offered two `DragValue` spinboxes and no hit-test; the only `Sense` in the file was `Sense::hover()` on a colour swatch. The NES image is a raw wgpu letterbox blit, not an egui widget, so there is nothing to hit-test: the click is now captured in the winit handler and converted by a new `gfx::window_to_nes_pixel`. That converter inverts the blit's own transform and shares one `BlitTransform` with `letterbox_uniform`, so picker and shader cannot drift — a second independent derivation of the letterbox is exactly the bug class under repair. Its test round-trips against the shader's own uniform rather than a third re-derivation. Also: the panel mirrored the core's armed flags in frontend state, which desynced permanently once a ROM load installed a fresh `Nes` (checkbox ticked, core unarmed, no way back but unticking); the core is now the single source of truth. And a cleared record rendered as fact, because every field of one reads as a confident "scanline 0, dot 0, backdrop, palette $0000" — `PixelProvenance::is_recorded()` (keyed on `dot != 0`, unreachable for a real record) now separates not-armed from nothing-recorded-yet from off-screen. The Zapper: Duck Hunt could never score (broken since the Zapper shipped) ------------------------------------------------------------------------- The gun fired and nothing could be hit, at any aim point. Duck Hunt's protocol is "the gun must see NOTHING for one frame, then a bright spot in the next". `Bus::sample_zapper_light()` runs at the END of `run_frame`, so the light bit a read returns during frame N was sampled from frame N-1. The game received its probe exactly inverted — bright on the blanked frame, dark on the target frame — and discarded the shot before hit-testing. Aiming was irrelevant, which is precisely what was reported. The beam-relative light model (`zapper_temporal_light`, opt-in since v2.2.3) is now the DEFAULT. It derives the bit from where the CRT beam is at the moment of the read, with the ~19-26-scanline photodiode hold the NESdev wiki describes — what the hardware does, and what a frame-granular model structurally cannot express. A second defect had to go with it: the beam-relative sampler read aperture rows the beam had not FINISHED painting, which still hold the previous frame, so it asserted light on an all-black screen. Measured directly — at scanline 96 the beam was 5 dots into row 96 and the sampler saw the previous frame's sky at luma 152 on a frame whose mean luma was 0. `aperture_is_bright_painted` now excludes rows at or after the current scanline. Why it shipped that way is worth recording: v2.2.3 kept the model off because "no pass/fail light-gun test ROM exists... the supported titles re-poll every frame and are satisfied by either model". The second half was false, and the first was beside the point — the game is the oracle. That reasoning is corrected in the source and in `accuracy-ledger.md` rather than merely overridden. Evidence and gates ------------------ Measured A/B, same ROM, aim (93,156) and inputs: frame model -> score 000000, duck still flying beam-relative -> score 000500, duck marked hit Protocol, from the game's own $4017 traffic: frame 129 all 0x48 (sees nothing, 1918 polls), frame 130 aim_luma 255 with 0x40 (light detected). New tests, both mutation-checked: runahead::tests::runahead_preserves_pixel_provenance, with plain_run_leaves_pixel_provenance_populated as a control so a failure cannot be misread as a bad assertion; three gfx::tests::window_to_nes_pixel_*; duck_hunt_zapper_shot_can_score, which asserts Duck Hunt's own scoreboard and fails on identical score pixels (428 = 428) with the model forced off. New `zapper_light_probe` diagnostic reproduces the whole sequence headlessly from bus traffic and dumps frames, so the next light-gun question is measurable. The Zapper change alters emulation behaviour when a Zapper is attached, so the contract was verified rather than assumed: AccuracyCoin 141/141 (RAM decoder, no failing tests), nestest 0-diff, 2,038 workspace tests green. fmt, workspace clippy, all four native feature combos, both wasm32 invocations, rustdoc, no_std cross-compile and pre-commit all clean. The Zapper's aim mapping also now shares `window_to_nes_pixel`, so a letterbox bar reads as "no light" — which its own comment always claimed it did while in fact stretching the image across the bars. The Vaus paddle deliberately keeps its full-window sweep: a knob has no off-screen state. Addresses the maintainer reports on Pixel Provenance and Duck Hunt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

coderabbitai · 2026-08-16T21:10:04Z

Important

Review available on request

🔍 Trigger review

Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment @coderabbitai review to review the latest changes. For a full review, comment @coderabbitai full review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a9ebbf18-2f49-41da-9273-218604f85336

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

Copilot

Pull request overview

Fixes two user-facing features that shipped broken: the Pixel Provenance debugger panel (run-ahead rollback wiped records and click-to-pick was missing) and NES Zapper scoring in Duck Hunt (frame-granular sampling inverted the game’s “dark frame then bright frame” protocol). The PR also aligns documentation/specs with actual behavior and adds regression tests/diagnostics to exercise the real user paths.

Changes:

Preserve pixel provenance + write attribution across run-ahead rollback via stash/restore, and implement click-to-pick by mapping window coordinates back to NES pixels using the blit’s transform.
Promote the beam-relative Zapper light model to the default and fix stale-row sampling in the temporal aperture check so light isn’t detected from unpainted rows.
Add regression tests (run-ahead provenance retention, window→NES mapping, Duck Hunt score change) plus a headless zapper probe, and update docs/changelog accordingly.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
CHANGELOG.md	Documents the shipped/user-visible fixes for Pixel Provenance and Zapper/Duck Hunt, plus related mapping changes.
docs/user-guide/debugger.md	Adds end-user Pixel Provenance usage instructions and version-note about prior broken behavior.
docs/pixel-provenance.md	Updates the spec to match actual lifecycle/invalidation behavior and selection mechanism; records the defects and new regression net.
docs/frontend.md	Documents frontend-specific details: winit click capture and shared blit-inverse coordinate mapping for picker + Zapper.
docs/accuracy-ledger.md	Updates the Zapper row to reflect temporal model promotion to default and the corrected justification/oracle.
crates/rustynes-frontend/src/runahead.rs	Stashes provenance across `restore_quiet` and adds tests ensuring run-ahead preserves provenance and arming state.
crates/rustynes-frontend/src/app.rs	Wires mouse clicks to provenance picking (when panel open) and routes Zapper aim through the shared window→NES mapping.
crates/rustynes-frontend/src/gfx.rs	Introduces `BlitTransform` and `window_to_nes_pixel` (inverse mapping) plus associated tests.
crates/rustynes-frontend/src/debugger/provenance_panel.rs	Removes frontend-mirrored arm flags, adds “click to pin” messaging, and distinguishes “armed but not recorded yet” via `is_recorded`.
crates/rustynes-frontend/src/debugger/mod.rs	Exposes panel-open query and a setter to accept picks coming from winit events.
crates/rustynes-core/src/nes.rs	Adds `take_provenance`/`put_provenance` pass-through and flips Zapper temporal-light default expectation/tests.
crates/rustynes-core/src/bus.rs	Promotes `zapper_temporal_light` default to ON and updates API docs/rationale.
crates/rustynes-core/src/input_device.rs	Adds painted-row clipping for temporal aperture sampling and updates related unit tests.
crates/rustynes-ppu/src/provenance.rs	Adds `PixelProvenance::is_recorded()` and `ProvenanceStash` for moving stores across restore.
crates/rustynes-ppu/src/ppu.rs	Implements `take_provenance`/`put_provenance` on the PPU to support run-ahead stashing.
crates/rustynes-ppu/src/lib.rs	Re-exports `ProvenanceStash` under `debug-hooks`.
crates/rustynes-test-harness/Cargo.toml	Registers the `zapper_light_probe` diagnostic binary (feature-gated).
crates/rustynes-test-harness/tests/input_devices.rs	Adds a Duck Hunt scoring regression test using a gitignored commercial dump.
crates/rustynes-test-harness/src/bin/zapper_light_probe.rs	Adds a headless diagnostic that correlates `$4017` reads with framebuffer brightness and can replay a detected light-test frame.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…model label Two review findings from Copilot on #383, both correct. 1. `light_at_scanline`'s doc ended by justifying the very behaviour v2.3.6 removes: "the aperture rows below the beam still hold the previous frame's pixels, which is exactly what the sensor sees". That is backwards — a photodiode responds to light the phosphor has EMITTED, and a row the beam has not reached is emitting nothing; its stale framebuffer contents are an artefact of how the emulator stores pixels. Reading them is what reported light on an all-black screen and discarded every Duck Hunt shot. The paragraph is corrected in place and KEPT rather than deleted, with the measurement that disproves it. Plausible-sounding wrong reasoning next to code is what made this defect look intentional for four releases; deleting it silently would remove the evidence of how that happens. 2. `zapper_light_probe` still labelled the frame-granular model "SHIPPED DEFAULT", which stopped being true in the same commit that promoted the beam-relative one — making the probe's own A/B output misleading. The default run is now the shipped model; pass `frame-granular` for the superseded one. `temporal` is still accepted so older invocations in notes keep working. The module docs also now list the environment knobs, including the warning that an even number of START presses leaves Duck Hunt PAUSED — which looks exactly like "the Zapper is ignored" and cost this investigation two wrong conclusions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

`light_at_scanline`'s corrected doc linked `[Self::aperture_is_bright_painted]`, which is private, from a public item. `rustdoc::private_intra_doc_links` under `-D warnings` fails the build, and CI caught it. Mine to own: the change that introduced the link was a review fix, and I re-ran clippy for the touched crates but not `RUSTDOCFLAGS="-D warnings" cargo doc --workspace` — the gate that was going to fail. Clippy passing is not evidence about rustdoc. Fixed the way this repo already handles the sibling case (an intra-doc link to a feature-gated dependency, which fails the default doc build for the same class of reason): use a plain code span, and say in the text why it is not a link, so the next person does not "helpfully" restore it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

github-actions · 2026-08-16T21:26:03Z

Antigravity review (Gemini via Ultra)

This PR fixes a regression in the Pixel Provenance inspector where run-ahead erased telemetry before display and hit-testing was unimplemented, and enables the beam-relative Zapper light model by default to fix Duck Hunt hit detection.

Blocking issues

Unvalidated external input reaching a sink: In crates/rustynes-test-harness/src/bin/zapper_light_probe.rs, the ZAPPER_PROBE_PNG_DIR environment variable is directly joined and used in std::fs::File::create(path) (via write_png), and the CLI arg path is passed to std::fs::read(&path). Both need validation or an allowlist before reaching the filesystem sink.
Breaking changes without a version bump: crates/rustynes-core/src/bus.rs changes zapper_temporal_light to default to true. This alters public emulation output behavior and requires a version bump in Cargo.toml.
Silent failure paths (swallowed errors): In crates/rustynes-core/src/input_device.rs and zapper_light_probe.rs, the syntax let Ok(idx) = usize::try_from(...) else { continue; }; silently swallows the error path.
Silent failure paths (ignored return values): There are multiple instances of let _ = nes.drain_audio_into(&mut discard); explicitly ignoring the return value (e.g., in zapper_light_probe.rs, runahead.rs, and input_devices.rs).
Correctness (Build breakage): In crates/rustynes-frontend/src/app.rs, the if ... && let Some(...) && let Some(...) syntax relies on let_chains, which is unstable and will fail to compile on stable Rust.

Suggestions

crates/rustynes-core/src/input_device.rs: Since px and py are explicitly bounded to the screen dimensions right above the assignment, the linear index is logically guaranteed to fit in usize. Use as usize or .expect() rather than silently continue-ing on a swallowed error.
crates/rustynes-test-harness/tests/input_devices.rs: The test duck_hunt_zapper_shot_can_score silently passes if the commercial ROM is absent. Even if intentional, consider logging a warning or using a harness that marks it as explicitly skipped rather than a successful pass to prevent hiding CI regressions.
crates/rustynes-test-harness/src/bin/zapper_light_probe.rs: The variable bright is calculated but then explicitly ignored with let _ = bright;. Consider removing the unused variable entirely.

Nitpicks

crates/rustynes-test-harness/src/bin/zapper_light_probe.rs: Use .unwrap_or("...".into()) instead of .unwrap_or_else(|| "...".into()) since the fallback string is static.

_{Automated first-pass review by agy on a self-hosted runner -- not a human review.}

Copilot AI lite review requested due to automatic review settings August 16, 2026 21:09

Copilot started reviewing on behalf of doublegate August 16, 2026 21:10 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Comment thread crates/rustynes-core/src/input_device.rs

Comment thread crates/rustynes-test-harness/src/bin/zapper_light_probe.rs

doublegate and others added 2 commits August 16, 2026 17:17

doublegate merged commit 97a63d9 into main Aug 16, 2026
29 checks passed

doublegate deleted the fix/v2.3.6-pixel-provenance branch August 16, 2026 21:51

doublegate mentioned this pull request Aug 16, 2026

feat(v2.3.6): the deterministic probe engine and the Latency Oracle measurement #384

Merged

Navigation Menu

Uh oh!

fix: two shipped features that never worked — Pixel Provenance and the Zapper - #383

fix: two shipped features that never worked — Pixel Provenance and the Zapper#383
doublegate merged 3 commits into
mainfrom
fix/v2.3.6-pixel-provenance

doublegate commented Aug 16, 2026

Uh oh!

coderabbitai Bot commented Aug 16, 2026 •

edited

Loading

Review available on request

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

doublegate commented Aug 16, 2026

1. Pixel Provenance (broken since v2.3.2 "Lucid")

Defect A — run-ahead erased the record before the UI could read it

Defect B — clicking a pixel was never implemented

Also fixed while here

2. The Zapper: Duck Hunt could never score

Why it shipped that way

Evidence

New tests — all mutation-checked

Verification

Reviewer notes

Uh oh!

coderabbitai Bot commented Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review available on request

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 16, 2026

Antigravity review (Gemini via Ultra)

Blocking issues

Suggestions

Nitpicks

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Aug 16, 2026 •

edited

Loading