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

CHANGELOG.md

@@ -16,6 +16,108 @@ cycle-accurate core later replaced.
 
 ### Fixed
 
+- **Pixel Provenance now works.** The v2.3.2 "Lucid" marquee returned an empty
+  report for effectively every user, from release until now, because of two
+  independent defects.
+
+  **Run-ahead erased the record before the UI could read it.** Run-ahead defaults
+  to 1, and its per-frame rollback (`RunAhead::finish` → `Nes::restore_quiet`)
+  unconditionally cleared both provenance stores. That clear is right for a
+  save-state load and for netplay rollback, and 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 was always after the wipe. 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), keeping exactly the visible
+  frame's records. Every other caller still clears, unchanged.
+
+  **Clicking a pixel was never implemented.** The panel offered two coordinate
+  spinboxes and no click hit-test, while the docs and release notes said "point
+  at"/"pin" a pixel. Clicking the game view now pins that pixel. The NES image is
+  a raw wgpu blit rather than an egui widget, so the click is captured in the
+  winit handler and converted by a new `gfx::window_to_nes_pixel`, which inverts
+  the blit's own letterbox/crop transform — correct at any window size, pixel
+  aspect and overscan crop, and `None` on a letterbox bar.
+
+  Also fixed while here: the panel mirrored the core's armed flags in frontend
+  state, which desynced permanently the moment a ROM load installed a fresh
+  `Nes` (checkbox ticked, core unarmed, no way back but unticking and re-ticking)
+  — the core is now the single source of truth; and the panel rendered a cleared
+  record as fact, because every field of one reads as a confident "scanline 0,
+  dot 0, backdrop, palette `$0000`". It now distinguishes not-armed from
+  nothing-recorded-yet from off-screen.
+
+  **Why it went unnoticed:** the core data structures were well unit-tested and
+  the frontend wiring was tested by nothing — the same shape as issue #360 in the
+  same release train. `runahead.rs` even carried tests pinning the determinism of
+  the very code path that destroyed this telemetry. The new regression net drives
+  the run-ahead cycle with provenance armed and asserts a record survives, with a
+  plain-run control so a failure cannot be misread as a bad assertion, plus three
+  tests for the coordinate converter — one round-tripping it against the shader's
+  own uniform rather than a third re-derivation of the letterbox.
+
+  Two comments and four documentation claims asserted the opposite of the code
+  and are corrected in the same change, including one in `CHANGELOG-FULL.md`'s
+  spec (`docs/pixel-provenance.md`) that contradicted itself two sections apart.
+
+  Emulation is untouched: the new core methods are additive and output-only, so
+  **AccuracyCoin holds at exactly 141/141** (RAM decoder) with nestest 0-diff —
+  verified, not asserted.
+
+- **Duck Hunt is playable: a Zapper shot can finally score.** The gun fired and
+  nothing could ever be hit — at any aim point, in any part of a duck.
+
+  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 therefore received its probe **exactly inverted**: on the
+  blanked frame it read the previous, bright frame; on the target frame it read
+  the blanked one. The shot was discarded before hit-testing, which is why aiming
+  made no difference.
+
+  The **beam-relative light model is now the default** (`zapper_temporal_light`,
+  opt-in since v2.2.3). It derives the light bit from where the CRT beam is at
+  the moment of the read — dark before the beam paints the aim row, lit for the
+  ~19-26-scanline photodiode hold, dark once drained — which is what the hardware
+  does and what the frame 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. Rows at or after the current
+  scanline are now excluded (`aperture_is_bright_painted`).
+
+  **The reason it shipped that way was a wrong claim, not a missing oracle.**
+  v2.2.3 kept the model off because "no pass/fail light-gun test ROM exists… the
+  supported titles are satisfied by either model". The second half was false, and
+  the first was beside the point: the game is the oracle. Measured A/B on the same
+  ROM, aim and inputs — frame model: score 000000, duck still flying;
+  beam-relative: score 000500, duck marked hit. Pinned by
+  `duck_hunt_zapper_shot_can_score`, which asserts Duck Hunt's own scoreboard and
+  was mutation-checked (with the model forced off it fails on identical score
+  pixels). New `zapper_light_probe` diagnostic reproduces the whole sequence from
+  the game's `$4017` traffic.
+
+  This changes emulation behaviour when a Zapper is attached, so the gates were
+  re-run rather than assumed: **AccuracyCoin 141/141** (RAM decoder), nestest
+  0-diff, 2,038 workspace tests green. Pass `set_zapper_temporal_light(false)` to
+  restore the pre-v2.3.6 model.
+
+- **The Zapper's aim was off by the letterbox.** Its cursor mapping stretched the
+  256×240 image across the whole window, so the aim was wrong by the bar size
+  whenever the window did not match the NES aspect, and a click on a black bar
+  registered as a hit on a real pixel — while the comment directly above it
+  claimed "letterbox bars read as off-screen — the correct Zapper 'no light'
+  behavior", which a full-window stretch cannot produce. It now shares
+  `gfx::window_to_nes_pixel` with the provenance picker, so bars are genuinely
+  dark and the aim tracks the pixel actually under the cursor at any window size,
+  pixel-aspect setting or overscan crop. The Input Display's on-screen indicator
+  uses the same converter, so the HUD and the core agree. The Vaus paddle keeps
+  its full-window sweep deliberately: a knob has no off-screen state, and how far
+  the hand travels per turn is a feel decision no oracle adjudicates.
+
 - **The libretro `.info` description is corrected.** It now advertises native
   `RETRO_ENVIRONMENT_SET_MEMORY_MAPS` support and native Game Genie cheats —
   both long-standing capabilities that the description omitted — plus the two

crates/rustynes-core/src/bus.rs

@@ -858,7 +858,9 @@ impl LockstepBus {
             vs_4016_bit1: false,
             vs_4016_bit1_dirty: false,
             expansion_device: [None, None],
-            zapper_temporal_light: false,
+            // v2.3.6: ON by default. See `set_zapper_temporal_light` — the frame
+            // model made a Duck Hunt hit impossible.
+            zapper_temporal_light: true,
             famicom_mic: false,
             nt_mirroring_override: None,
             #[cfg(feature = "debug-hooks")]
@@ -1628,22 +1630,37 @@ impl LockstepBus {
         }
     }
 
-    /// A3 (v2.2.3): enable the **beam-relative** Zapper light model.
+    /// Enable the **beam-relative** Zapper light model.
     ///
-    /// Default **off**, which keeps the frame-granular model and therefore
-    /// byte-identical output on every shipped build.
+    /// **Default ON since v2.3.6.** The light bit is derived from where the CRT
+    /// beam is at the moment of the `$4016`/`$4017` read: dark before the beam
+    /// paints the aim row, lit while the photodiode holds (~19-26 scanlines,
+    /// per the `NESdev` wiki's capacitor model), dark once it drains. That is what
+    /// real hardware does, and the frame-granular model structurally cannot
+    /// express it — it returns one answer for the whole frame, sampled at
+    /// end-of-frame, so every read during frame N reports frame N-1.
     ///
-    /// With it on, the light bit is derived from where the CRT beam is at the
-    /// moment of the `$4016`/`$4017` read rather than from the completed frame:
-    /// dark before the beam paints the aim row, lit while the photodiode holds
-    /// (~19-26 scanlines), dark once it drains. That is what real hardware
-    /// does, and the frame model structurally cannot express it — it returns
-    /// one answer for the whole frame.
+    /// # Why it was promoted (v2.3.6)
     ///
-    /// Opt-in rather than promoted because there is **no pass/fail light-gun
-    /// test ROM** to adjudicate it: the supported titles re-poll every frame and
-    /// are satisfied by either model, so promoting it would change output with
-    /// no oracle able to confirm the change is an improvement.
+    /// A3 (v2.2.3) shipped this off, on the reasoning that "there is no pass/fail
+    /// light-gun test ROM… the supported titles re-poll every frame and are
+    /// satisfied by either model". **The second half of that was false**, and no
+    /// test ROM was needed to show it — the game itself is the oracle.
+    ///
+    /// *Duck Hunt* requires the gun to see **nothing for one frame** and then a
+    /// bright spot in the next. Under the frame model it received exactly the
+    /// inverse: on the blanked frame it read the previous (bright) frame's
+    /// answer, and on the target frame it read the blanked frame's. The shot was
+    /// discarded before hit-testing, so the gun fired and **nothing could ever be
+    /// hit** — reported by the maintainer, then reproduced headlessly from the
+    /// game's own `$4017` traffic (`zapper_light_probe`).
+    ///
+    /// Measured A/B on the same ROM, aim and inputs: frame model → score 000000,
+    /// duck still flying; beam-relative → score 000500, duck marked hit.
+    ///
+    /// Turning it off restores the pre-v2.3.6 frame-granular behaviour.
+    /// Deterministic either way: the answer is a pure function of framebuffer +
+    /// aim + scanline and holds no state, so it adds nothing to serialize.
     pub const fn set_zapper_temporal_light(&mut self, on: bool) {
         self.zapper_temporal_light = on;
     }

crates/rustynes-core/src/input_device.rs

@@ -282,12 +282,45 @@ impl ZapperState {
     /// temporal model differs from the frame model ONLY in *when* it samples,
     /// never in what counts as light.
     fn aperture_is_bright(framebuffer: &[u8], x: u16, y: u16) -> bool {
+        Self::aperture_is_bright_painted(framebuffer, x, y, None)
+    }
+
+    /// [`Self::aperture_is_bright`], restricted to rows the CRT beam has
+    /// **finished painting this frame**.
+    ///
+    /// `painted_before` is the scanline the beam is currently on: rows at or
+    /// after it are excluded, because the current row is only part-way drawn and
+    /// later rows still hold the PREVIOUS frame's pixels. `None` means the whole
+    /// framebuffer is current, which is true only for the end-of-frame sampler.
+    ///
+    /// # Why this is load-bearing
+    ///
+    /// A photodiode can only respond to light the phosphor has already emitted.
+    /// Without this clip the beam-relative model samples stale rows and reports
+    /// light on a screen that is entirely black — measured directly on *Duck
+    /// Hunt*'s light-test frame, where at scanline 96 the beam was 5 dots into
+    /// row 96 and the sampler read the previous frame's bright sky (`aim_luma
+    /// 152` on a frame whose mean luma is 0), then did it again at scanline 97
+    /// via the still-unpainted row 97 of the 3x3 aperture.
+    ///
+    /// That matters because *Duck Hunt* requires the gun to see **nothing** for
+    /// one frame before it will accept a shot, so a false positive here discards
+    /// every shot: the gun fires and nothing can ever be hit (v2.3.6).
+    fn aperture_is_bright_painted(
+        framebuffer: &[u8],
+        x: u16,
+        y: u16,
+        painted_before: Option<u16>,
+    ) -> bool {
         const W: i32 = 256;
         const H: i32 = 240;
         let (ax, ay) = (i32::from(x), i32::from(y));
         if ax >= W || ay >= H {
             return false; // aimed off-screen: never sees light
         }
+        // Rows `>= painted_before` are not yet emitted this frame. `None` (the
+        // end-of-frame sampler) admits the whole screen.
+        let row_limit = painted_before.map_or(H, i32::from);
         let mut bright = 0u32;
         let r = ZAPPER_APERTURE_RADIUS;
         for dy in -r..=r {
@@ -296,6 +329,9 @@ impl ZapperState {
                 if !(0..W).contains(&px) || !(0..H).contains(&py) {
                     continue; // aperture clipped by the screen edge
                 }
+                if py >= row_limit {
+                    continue; // the beam has not finished this row this frame
+                }
                 // px/py are now bounded to the screen, so the linear index is
                 // non-negative and fits a usize.
                 let Ok(idx) = usize::try_from((py * W + px) * 4) else {
@@ -333,7 +369,9 @@ impl ZapperState {
     /// * before the beam reaches the aim row (`scanline < y`) — dark, because
     ///   this frame has not painted it yet;
     /// * from the aim row until the hold expires — bright iff the aperture is
-    ///   bright, the same aperture test [`Self::sample_light`] uses;
+    ///   bright **over the rows the beam has already finished**, per
+    ///   `aperture_is_bright_painted` (a plain code span, not an intra-doc link:
+    ///   that item is private and `rustdoc::private_intra_doc_links` is denied);
     /// * after the hold — dark again, the capacitor having drained.
     ///
     /// Holding **no extra state** is deliberate: light is derived on demand at
@@ -342,10 +380,26 @@ impl ZapperState {
     /// and keeps the determinism contract (same framebuffer + aim + scanline
     /// always yields the same answer).
     ///
-    /// One consequence is physically right rather than a compromise: the
-    /// aperture rows *below* the beam still hold the previous frame's pixels,
-    /// which is exactly what the sensor sees, since the beam has not repainted
-    /// them yet.
+    /// # A wrong claim this used to make (v2.3.6)
+    ///
+    /// This doc previously ended: *"One consequence is physically right rather
+    /// than a compromise: the aperture rows below the beam still hold the
+    /// previous frame's pixels, which is exactly what the sensor sees, since the
+    /// beam has not repainted them yet."*
+    ///
+    /// **That is backwards.** A photodiode responds to light the phosphor has
+    /// *emitted*; a row the beam has not reached this frame is emitting nothing,
+    /// and its stale framebuffer contents are an artefact of how the emulator
+    /// stores pixels, not something a sensor could see. Reading those rows made
+    /// the model report light on an all-black screen — measured at scanline 96,
+    /// where the beam was 5 dots into row 96 and the sampler returned the
+    /// previous frame's sky at luma 152 on a frame whose mean luma was 0.
+    ///
+    /// Because *Duck Hunt* requires the gun to see nothing for one frame before
+    /// it will accept a shot, that false positive discarded every shot: the gun
+    /// fired and no duck could ever be hit. The rows are now clipped, and the
+    /// paragraph is kept rather than deleted because the plausible-sounding
+    /// wrong reasoning is what made the defect look intentional.
     #[must_use]
     pub fn light_at_scanline(&self, framebuffer: &[u8], scanline: u16) -> bool {
         let y = self.y;
@@ -355,7 +409,9 @@ impl ZapperState {
         if scanline - y >= ZAPPER_LIGHT_HOLD_SCANLINES {
             return false; // photodiode has drained
         }
-        Self::aperture_is_bright(framebuffer, self.x, y)
+        // Only rows the beam has FINISHED this frame can have emitted light —
+        // see `aperture_is_bright_painted` for what goes wrong without this.
+        Self::aperture_is_bright_painted(framebuffer, self.x, y, Some(scanline))
     }
 
     /// The device byte as [`Self::read`] would return it, but using the
@@ -1898,11 +1954,26 @@ mod tests {
         z.set(100, 0, true); // aim on visible row 0, trigger pulled
         let fb = fb_with_target(100, 0);
 
-        // Row 0 itself is inside the hold window: light IS detected there.
+        // v2.3.6: reading WHILE the beam is on the aim row reports no light —
+        // the row is only part-way painted, so the phosphor has not emitted it
+        // yet and the framebuffer still holds the previous frame there. (This
+        // assertion read "light IS detected at row 0" until v2.3.6; sampling
+        // rows the beam had not finished is what let the sensor report light on
+        // a fully black screen, which made a Duck Hunt hit impossible. See
+        // `aperture_is_bright_painted`.)
         assert_eq!(
             z.read_at_scanline(&fb, 0) & 0b0000_1000,
+            0b0000_1000,
+            "the aim row is still being painted at scanline == y: no light yet"
+        );
+
+        // The first scanline PAST the aim row: the row is complete, the
+        // photodiode is inside its hold window, so light IS detected. This is
+        // the contrast the fallback below is measured against.
+        assert_eq!(
+            z.read_at_scanline(&fb, 1) & 0b0000_1000,
             0,
-            "row 0 detects light (contrast for the fallback below)"
+            "scanline 1 detects the light emitted by the completed row 0"
         );
 
         // The real pre-render line (261 NTSC) is already no-light via the normal