diff --git a/CHANGELOG.md b/CHANGELOG.md index 63b32c82..f55d9fea 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/crates/rustynes-core/src/bus.rs b/crates/rustynes-core/src/bus.rs index 8b157652..bbf5a495 100644 --- a/crates/rustynes-core/src/bus.rs +++ b/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; } diff --git a/crates/rustynes-core/src/input_device.rs b/crates/rustynes-core/src/input_device.rs index 928d15a2..887c03ae 100644 --- a/crates/rustynes-core/src/input_device.rs +++ b/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, + ) -> 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 diff --git a/crates/rustynes-core/src/nes.rs b/crates/rustynes-core/src/nes.rs index 772e4c3f..e57c36ab 100644 --- a/crates/rustynes-core/src/nes.rs +++ b/crates/rustynes-core/src/nes.rs @@ -884,6 +884,29 @@ impl Nes { self.bus.ppu.pixel_provenance() } + /// v2.3.6 — move both provenance stores out, leaving them unarmed. + /// + /// For a host that performs a **same-timeline** restore whose result the user + /// is about to inspect. [`Self::restore`] and [`Self::restore_quiet`] both + /// clear the stores, which is correct when the restore replaces the timeline + /// the records describe — and wrong for run-ahead, whose rollback is the last + /// thing before the UI reads, so the clear discards the record for the frame + /// actually on screen. Take before the restore, [`Self::put_provenance`] + /// after. + /// + /// A move, not a copy: the stores are boxed, so this is two pointer moves. + /// See [`rustynes_ppu::ProvenanceStash`]. + #[cfg(feature = "debug-hooks")] + pub const fn take_provenance(&mut self) -> rustynes_ppu::ProvenanceStash { + self.bus.ppu.take_provenance() + } + + /// Put back stores taken by [`Self::take_provenance`]. + #[cfg(feature = "debug-hooks")] + pub fn put_provenance(&mut self, stash: rustynes_ppu::ProvenanceStash) { + self.bus.ppu.put_provenance(stash); + } + /// Resolve a PPU-space nametable address (`$2000-$3EFF`) to the physical /// internal-CIRAM offset it reads, applying the mapper's mirroring and any /// per-game mirroring override. @@ -1399,11 +1422,15 @@ impl Nes { /// A3 (v2.2.3): enable the **beam-relative** Zapper light model. /// - /// Default **off**. See [`crate::bus::LockstepBus::set_zapper_temporal_light`] - /// for the model; in short, the light bit becomes a function of 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 after) - /// instead of one answer for the whole frame. + /// **Default ON since v2.3.6** (was off in v2.2.3-v2.3.5). See + /// [`crate::bus::LockstepBus::set_zapper_temporal_light`] for the model and + /// for why it was promoted; in short, the light bit is a function of 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 after) + /// instead of one answer for the whole frame — and the frame model made a + /// *Duck Hunt* hit impossible. + /// + /// Pass `false` to restore the pre-v2.3.6 frame-granular behaviour. /// /// Deterministic either way: the temporal answer is a pure function of /// framebuffer + aim + current scanline and holds no extra state, so it @@ -1995,9 +2022,20 @@ impl Nes { // any instruction this session executed, so the PCs recorded against // those offsets describe a timeline that no longer exists. Reporting // them would be a confidently wrong answer; reporting nothing until the - // program writes again is the honest one. (Under run-ahead this fires - // once per displayed frame, leaving exactly the visible frame's writes — - // which is the timeline the user is looking at.) + // program writes again is the honest one. + // + // v2.3.6 CORRECTION. This comment used to end by claiming that under + // run-ahead the clear "fires once per displayed frame, leaving exactly + // the visible frame's writes — which is the timeline the user is looking + // at". That was false about the two lines below it, which empty both + // stores completely; and because run-ahead's rollback is the LAST thing + // before the frontend releases the emulator lock, the wipe landed on the + // visible frame's records before any UI could read them. The shipped + // Pixel Provenance inspector therefore rendered an empty report for every + // user with the default `run_ahead = 1`. The clear here is right and + // stays; run-ahead now carries the stores AROUND it (`RunAhead::finish` + // → `Nes::take_provenance` / `put_provenance`), which is what this + // comment always claimed was happening. // // The per-pixel provenance frame is cleared for the same reason, and it // needs saying separately because the obvious analogy is wrong: the @@ -4152,17 +4190,25 @@ mod tests { assert_eq!(nes.nsf_current_song(), 0); } - /// A3 (v2.2.3): the beam-relative Zapper model is OFF by default, so the - /// shipped `$4017` byte is exactly what the frame-granular model produced. + /// v2.3.6: the beam-relative Zapper model is ON by default. + /// + /// It shipped OFF in v2.2.3-v2.3.5 on the reasoning that no light-gun test + /// ROM could adjudicate it and the supported titles were satisfied either + /// way. The second half was false: under the frame-granular model *Duck + /// Hunt* receives its "dark frame then bright frame" probe inverted and can + /// never register a hit. See `LockstepBus::set_zapper_temporal_light`. #[test] - fn zapper_temporal_light_is_off_by_default() { + fn zapper_temporal_light_is_on_by_default() { let mut nes = Nes::from_rom(&synth_nrom(16, 8)).expect("nrom builds"); - assert!(!nes.zapper_temporal_light(), "A3 must default OFF"); + assert!( + nes.zapper_temporal_light(), + "the beam-relative model must default ON from v2.3.6" + ); nes.set_zapper(1, 100, 120, false); - // Toggling it on and back off must restore the default exactly. - nes.set_zapper_temporal_light(true); - assert!(nes.zapper_temporal_light()); + // Toggling it off and back on must restore the default exactly. nes.set_zapper_temporal_light(false); assert!(!nes.zapper_temporal_light()); + nes.set_zapper_temporal_light(true); + assert!(nes.zapper_temporal_light()); } } diff --git a/crates/rustynes-frontend/src/app.rs b/crates/rustynes-frontend/src/app.rs index e4882944..bc01202c 100644 --- a/crates/rustynes-frontend/src/app.rs +++ b/crates/rustynes-frontend/src/app.rs @@ -3844,11 +3844,27 @@ impl App { use crate::config::ExpansionDevice; // Map the cursor X into 0..=255 for the Vaus paddle / aim, and decide // whether the cursor is on the NES screen (Zapper light sensor). + // `on_screen` is the Zapper's light indicator, so it must agree with + // the aim the core actually receives — same converter, same answer + // (v2.3.6). It used to be a third independent full-window stretch, + // which reported "on screen" for a cursor sitting on a black bar. + // The knob keeps the full-window sweep, matching the Vaus branch of + // `mouse_nes` for the same reason given there. let (knob, on_screen) = self.cursor_pos.map_or((0x80u8, false), |(cx, cy)| { let (ww, wh) = self.window_size; let nx = (cx / f64::from(ww.max(1))) * 256.0; - let ny = (cy / f64::from(wh.max(1))) * 240.0; - let on = (0.0..256.0).contains(&nx) && (0.0..240.0).contains(&ny); + let on = crate::gfx::window_to_nes_pixel( + ww, + wh, + self.config.ui.pixel_aspect_correction, + crate::gfx::effective_overscan( + self.config.graphics.hide_overscan, + self.config.graphics.overscan, + ), + cx, + cy, + ) + .is_some(); #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] let knob = nx.clamp(0.0, 255.0) as u8; (knob, on) @@ -3974,36 +3990,52 @@ impl App { run_ahead: self.config.input.run_ahead, #[cfg(not(target_arch = "wasm32"))] expansion: self.config.input.expansion_device, - // Map the cursor (physical window px) to the 256x240 NES screen, - // assuming the framebuffer fills the window (letterbox bars read - // as off-screen — the correct Zapper "no light" behavior). - // v1.5.0 D4 — the Vaus paddle X applies the pointer-scale gain - // around the screen centre (pointer_scale 1.0 = the prior 1:1 map, - // byte-identical); the Zapper aim is unaffected (the gain is only - // meaningful for the paddle and the clamp keeps the aim sane). + // Map the cursor (physical window px) to the 256x240 NES screen. + // + // v2.3.6 workstream 0, defect 2 — the ZAPPER aim now inverts the + // blit's real letterbox/crop transform instead of stretching the + // image across the whole window. The old comment here claimed + // "letterbox bars read as off-screen — the correct Zapper 'no light' + // behavior", but a full-window stretch cannot produce that: every + // position in a bar mapped onto a real, wrong NES pixel, so the light + // sensor read a hit where the screen showed a black bar, and the aim + // was off by the bar size everywhere else. `(u16::MAX, u16::MAX)` is + // the established off-screen sentinel, so a bar is now genuinely dark. + // + // v1.5.0 D4 — the Vaus paddle keeps the full-window sweep with its + // pointer-scale gain, deliberately. A knob has no "off-screen" state, + // and making it jump to the sentinel (or stick) when the cursor + // crosses a bar would be worse, not more correct; how far the hand + // travels per knob turn is a feel decision no oracle adjudicates, so + // it is left exactly as it was rather than changed in passing. #[cfg(not(target_arch = "wasm32"))] mouse_nes: self.cursor_pos.map_or((u16::MAX, u16::MAX), |(cx, cy)| { - let (ww, wh) = self.window_size; - let scale = f64::from(self.config.input.pointer_scale.clamp(0.1, 8.0)); - let raw_x = (cx / f64::from(ww.max(1))) * 256.0; - // Apply the paddle gain (deviation from centre 128, scaled) only - // for the Vaus device; the Zapper keeps the raw cursor map. - let mapped_x = if matches!( + if matches!( self.config.input.expansion_device, crate::config::ExpansionDevice::Vaus ) { - (raw_x - 128.0).mul_add(scale, 128.0) - } else { - raw_x - }; - #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] - let x = mapped_x.clamp(0.0, 255.0) as i64; - #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] - let y = ((cy / f64::from(wh.max(1))) * 240.0) as i64; - ( - u16::try_from(x).unwrap_or(u16::MAX), - u16::try_from(y).unwrap_or(u16::MAX), + let (ww, wh) = self.window_size; + let scale = f64::from(self.config.input.pointer_scale.clamp(0.1, 8.0)); + let raw_x = (cx / f64::from(ww.max(1))) * 256.0; + let mapped_x = (raw_x - 128.0).mul_add(scale, 128.0); + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + let x = mapped_x.clamp(0.0, 255.0) as u16; + #[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] + let y = ((cy / f64::from(wh.max(1))) * 240.0).clamp(0.0, 239.0) as u16; + return (x, y); + } + crate::gfx::window_to_nes_pixel( + self.window_size.0, + self.window_size.1, + self.config.ui.pixel_aspect_correction, + crate::gfx::effective_overscan( + self.config.graphics.hide_overscan, + self.config.graphics.overscan, + ), + cx, + cy, ) + .unwrap_or((u16::MAX, u16::MAX)) }), #[cfg(not(target_arch = "wasm32"))] mouse_pressed: self.mouse_pressed, @@ -8998,6 +9030,35 @@ impl ApplicationHandler for App { .is_some_and(DebuggerOverlay::wants_egui_input); if button == winit::event::MouseButton::Left && !egui_pointer { self.mouse_pressed = state == winit::event::ElementState::Pressed; + // v2.3.6 workstream 0, defect 2 — clicking the game view pins + // that pixel in the Pixel Provenance inspector. The NES image + // is a raw wgpu blit rather than an egui widget, so there is + // no `Response` to hit-test; the conversion inverts the blit's + // own letterbox/crop transform (`gfx::window_to_nes_pixel`) so + // it is correct at any window size, aspect setting and + // overscan crop. A click on a letterbox bar maps to `None` and + // pins nothing, rather than silently pinning an edge pixel. + if state == winit::event::ElementState::Pressed + && let Some((cx, cy)) = self.cursor_pos + && self + .debugger + .as_ref() + .is_some_and(DebuggerOverlay::provenance_open) + && let Some((nx, ny)) = crate::gfx::window_to_nes_pixel( + self.window_size.0, + self.window_size.1, + self.config.ui.pixel_aspect_correction, + crate::gfx::effective_overscan( + self.config.graphics.hide_overscan, + self.config.graphics.overscan, + ), + cx, + cy, + ) + && let Some(dbg) = self.debugger.as_mut() + { + dbg.set_provenance_pick(nx, ny); + } } // v1.2.0 Workstream D — the SNES mouse's right button. if button == winit::event::MouseButton::Right && !egui_pointer { diff --git a/crates/rustynes-frontend/src/debugger/mod.rs b/crates/rustynes-frontend/src/debugger/mod.rs index fc104842..83352fcf 100644 --- a/crates/rustynes-frontend/src/debugger/mod.rs +++ b/crates/rustynes-frontend/src/debugger/mod.rs @@ -1750,6 +1750,25 @@ impl DebuggerOverlay { self.show_cheat || self.show_game_db || self.show_rom_info || self.show_provenance } + /// Whether the **Pixel Provenance** inspector is open. + /// + /// The winit mouse handler asks so it can turn a click on the game view into + /// a pinned pixel — and only while the panel is open, so a click costs + /// nothing (and pins nothing) for every other user. + #[must_use] + pub const fn provenance_open(&self) -> bool { + self.show_provenance + } + + /// Pin the NES pixel the user clicked on the game view (v2.3.6 workstream 0, + /// defect 2). + /// + /// Routed through the overlay rather than threaded into `tool_panels`' + /// signature because the click arrives from winit, outside any egui pass. + pub const fn set_provenance_pick(&mut self, x: u16, y: u16) { + self.provenance_ui.set_pick(x, y); + } + /// Build the egui UI for this frame (the deep-overlay path: chip panels + /// tool panels, all with a live `nes`). Used by [`Self::render`] and by /// [`Self::render_shell`] when the overlay is visible. diff --git a/crates/rustynes-frontend/src/debugger/provenance_panel.rs b/crates/rustynes-frontend/src/debugger/provenance_panel.rs index 86c3ab1b..1e6c9aa8 100644 --- a/crates/rustynes-frontend/src/debugger/provenance_panel.rs +++ b/crates/rustynes-frontend/src/debugger/provenance_panel.rs @@ -33,15 +33,18 @@ use crate::debugger::source_map::SourceMap; use crate::gfx::{NES_H, NES_W}; /// Persistent panel state. +/// +/// Deliberately does NOT mirror the core's armed flags. It used to (v2.3.2), and +/// that mirror silently desynced: loading a ROM installs a brand-new `Nes` whose +/// PPU starts unarmed, while the mirror still read `true`, so the edge-detected +/// push never fired and the core was never re-armed. The checkbox stayed ticked +/// while the report said "enable this, then run a frame" forever, until the user +/// unticked and re-ticked it. The core is now the single source of truth and the +/// desync cannot recur (v2.3.6 workstream 0, defect 3). pub struct ProvenancePanelState { /// Pinned screen coordinate the report describes. px: u32, py: u32, - /// Mirrors the core's "provenance armed" state so the checkbox is stateful - /// across frames without querying the `Nes` before it exists. - prov_armed: bool, - /// Mirrors the core's "write attribution armed" state. - attrib_armed: bool, } impl Default for ProvenancePanelState { @@ -52,12 +55,25 @@ impl Default for ProvenancePanelState { // a user sees a backdrop pixel with nothing to say. px: 128, py: 120, - prov_armed: false, - attrib_armed: false, } } } +impl ProvenancePanelState { + /// Pin the pixel the user clicked on the game view. + /// + /// The click is captured in the winit handler rather than here because the + /// NES image is a raw wgpu blit, not an egui widget — there is no `Response` + /// to hit-test against. `crate::gfx::window_to_nes_pixel` does the screen → + /// NES conversion, inverting the blit's own letterbox/crop transform so a + /// click lands on the pixel actually under the cursor at any window size, + /// aspect setting or overscan crop. + pub const fn set_pick(&mut self, x: u16, y: u16) { + self.px = x as u32; + self.py = y as u32; + } +} + /// A small colour swatch plus its NES palette value. /// /// The swatch is the RAW palette colour. Grayscale / emphasis are reported as @@ -114,12 +130,19 @@ pub fn show( nes: &mut Nes, source_map: &SourceMap, ) { - // Arming toggles are applied BEFORE the body renders, so a click takes - // effect on the frame after next (the core has to run a frame to fill the - // record). The checkbox therefore reflects intent immediately while the - // report below still honestly says "waiting for a frame". - let mut want_prov = state.prov_armed; - let mut want_attrib = state.attrib_armed; + // The CORE is the source of truth for both arm states, re-read every frame. + // A frontend-side mirror desyncs the moment a ROM load installs a new `Nes` + // (v2.3.6 workstream 0, defect 3), and the failure is invisible: the checkbox + // stays ticked over an unarmed core. + // + // Arming toggles are applied AFTER the body renders, so a click takes effect + // on the frame after next (the core has to run a frame to fill the record). + // The checkbox therefore reflects intent immediately while the report below + // still honestly says "waiting for a frame". + let armed_prov = nes.pixel_provenance().is_some(); + let armed_attrib = nes.write_attribution().is_some(); + let mut want_prov = armed_prov; + let mut want_attrib = armed_attrib; super::detachable_window( ctx, @@ -148,26 +171,44 @@ pub fn show( ui.label("Y"); ui.add(egui::DragValue::new(&mut state.py).range(0..=(NES_H - 1))); }); + ui.weak("Or click any pixel on the game view to pin it."); ui.separator(); + // Every way this panel can have no answer is named. A cleared record + // is a valid `PixelProvenance` whose fields all read as a confident + // "scanline 0, dot 0, backdrop, palette $0000" — so without the + // `is_recorded` check below, an empty frame renders as fact. That is + // exactly how the v2.3.2 inspector reported a wiped frame, and why + // the underlying bug went unnoticed from release until a user hit it. let Some(frame) = nes.pixel_provenance() else { - ui.weak("Enable \"Per-pixel provenance\" above, then run a frame."); + ui.weak("Not armed. Tick \"Per-pixel provenance\" above, then run a frame."); return; }; let Some(rec) = frame.get(state.px as usize, state.py as usize) else { ui.weak("(pixel off-screen)"); return; }; + if !rec.is_recorded() { + ui.weak( + "Armed, but nothing has been recorded for this pixel yet — run a \ + frame. (A power-cycle or a save-state load also clears the frame, \ + because its records describe a timeline the restore replaced.)", + ); + return; + } report(ui, &rec, nes, source_map); }, ); - if want_prov != state.prov_armed { - state.prov_armed = want_prov; + // Compared against what the CORE reported at the top of this frame, not + // against a stored mirror — so a `Nes` swapped in underneath us (ROM load) + // simply reads as "not armed" and the next comparison re-arms it. Pushing + // unconditionally would be wrong for the opposite reason: `set_pixel_provenance` + // reallocates the frame, which would discard the record every single frame. + if want_prov != armed_prov { nes.set_pixel_provenance(want_prov); } - if want_attrib != state.attrib_armed { - state.attrib_armed = want_attrib; + if want_attrib != armed_attrib { nes.set_write_attribution(want_attrib); } } diff --git a/crates/rustynes-frontend/src/gfx.rs b/crates/rustynes-frontend/src/gfx.rs index 94f6349b..3cff7ded 100644 --- a/crates/rustynes-frontend/src/gfx.rs +++ b/crates/rustynes-frontend/src/gfx.rs @@ -1948,43 +1948,134 @@ pub(crate) fn letterbox_uniform( par_8_7: bool, overscan: crate::config::Overscan, ) -> [f32; 8] { - let os = overscan.clamped(); - let crop_v = u32::from(os.top) + u32::from(os.bottom); - let crop_h = u32::from(os.left) + u32::from(os.right); - let visible_h = NES_H.saturating_sub(crop_v).max(1); - let visible_w = NES_W.saturating_sub(crop_h).max(1); - let win_aspect = width as f32 / height.max(1) as f32; - // Aspect of the VISIBLE image (square-pixel or 8:7-corrected width over - // the visible height). - let img_w = if par_8_7 { - visible_w as f32 * 8.0 / 7.0 - } else { - visible_w as f32 - }; - let nes_aspect = img_w / visible_h as f32; - let (sx, sy) = if win_aspect > nes_aspect { - (nes_aspect / win_aspect, 1.0) - } else { - (1.0, win_aspect / nes_aspect) - }; - // V crop: scale the [0,1] sample range to the visible rows and offset to - // the top kept row. U crop is the same on the horizontal axis. - let crop_scale_v = visible_h as f32 / NES_H as f32; - let crop_offset_v = f32::from(os.top) / NES_H as f32; - let crop_scale_u = visible_w as f32 / NES_W as f32; - let crop_offset_u = f32::from(os.left) / NES_W as f32; + let t = BlitTransform::new(width, height, par_8_7, overscan); [ - sx, - sy, + t.sx, + t.sy, 0.0, 0.0, - crop_scale_v, - crop_offset_v, - crop_scale_u, - crop_offset_u, + t.crop_scale_v, + t.crop_offset_v, + t.crop_scale_u, + t.crop_offset_u, ] } +/// The blit's window→texture mapping, in one place. +/// +/// [`letterbox_uniform`] serialises this for the shader and +/// [`window_to_nes_pixel`] inverts it for hit-testing. They MUST agree: a picker +/// that re-derives the letterbox independently is a second source of truth that +/// drifts silently, reporting a neighbouring pixel's causal chain as fact. The +/// shared struct makes agreement structural rather than a review obligation +/// (v2.3.6 workstream 0, defect 2). +#[derive(Clone, Copy, Debug)] +pub(crate) struct BlitTransform { + /// Image width as a fraction of the surface (`rect.x`). + sx: f32, + /// Image height as a fraction of the surface (`rect.y`). + sy: f32, + crop_scale_v: f32, + crop_offset_v: f32, + crop_scale_u: f32, + crop_offset_u: f32, +} + +impl BlitTransform { + #[allow(clippy::cast_precision_loss)] // window / NES dims fit in f32. + fn new(width: u32, height: u32, par_8_7: bool, overscan: crate::config::Overscan) -> Self { + let os = overscan.clamped(); + let crop_v = u32::from(os.top) + u32::from(os.bottom); + let crop_h = u32::from(os.left) + u32::from(os.right); + let visible_h = NES_H.saturating_sub(crop_v).max(1); + let visible_w = NES_W.saturating_sub(crop_h).max(1); + let win_aspect = width as f32 / height.max(1) as f32; + // Aspect of the VISIBLE image (square-pixel or 8:7-corrected width over + // the visible height). + let img_w = if par_8_7 { + visible_w as f32 * 8.0 / 7.0 + } else { + visible_w as f32 + }; + let nes_aspect = img_w / visible_h as f32; + let (sx, sy) = if win_aspect > nes_aspect { + (nes_aspect / win_aspect, 1.0) + } else { + (1.0, win_aspect / nes_aspect) + }; + // V crop: scale the [0,1] sample range to the visible rows and offset to + // the top kept row. U crop is the same on the horizontal axis. + Self { + sx, + sy, + crop_scale_v: visible_h as f32 / NES_H as f32, + crop_offset_v: f32::from(os.top) / NES_H as f32, + crop_scale_u: visible_w as f32 / NES_W as f32, + crop_offset_u: f32::from(os.left) / NES_W as f32, + } + } +} + +/// Map a cursor position in **physical surface pixels** to the NES pixel under +/// it, or `None` when the cursor is on a letterbox bar. +/// +/// The exact inverse of what the blit's vertex + fragment shaders do: +/// +/// ```text +/// screen uv = (cx / width, cy / height) // y down, matching the quad +/// image uv = (screen uv - 0.5) / (sx, sy) + 0.5 // undo the letterbox +/// sample uv = image uv * crop_scale + crop_offset// undo the overscan crop +/// nes px = floor(sample uv * (NES_W, NES_H)) +/// ``` +/// +/// `None` for an out-of-image cursor is the honest answer and is load-bearing in +/// both callers: the provenance picker must not report a pixel the user did not +/// click, and the Zapper must read a bar as "no light" — which its own comment +/// always claimed it did, while in fact stretching the image across the bars and +/// mapping every bar position onto a real, wrong NES pixel. +#[must_use] +#[allow(clippy::cast_possible_truncation, clippy::cast_sign_loss)] +pub fn window_to_nes_pixel( + width: u32, + height: u32, + par_8_7: bool, + overscan: crate::config::Overscan, + cx: f64, + cy: f64, +) -> Option<(u16, u16)> { + let xform = BlitTransform::new(width, height, par_8_7, overscan); + let (scale_x, scale_y) = (f64::from(xform.sx), f64::from(xform.sy)); + if scale_x <= 0.0 || scale_y <= 0.0 { + return None; + } + // Surface-relative position, then undo the letterbox to get image-relative. + let screen_u = cx / f64::from(width.max(1)); + let screen_v = cy / f64::from(height.max(1)); + let image_u = (screen_u - 0.5) / scale_x + 0.5; + let image_v = (screen_v - 0.5) / scale_y + 0.5; + // Half-open on the far edge: 1.0 is the first texel past the image, exactly + // as the fragment shader's `> 1.0` clip plus a floor would treat it. + if !(0.0..1.0).contains(&image_u) || !(0.0..1.0).contains(&image_v) { + return None; + } + // Undo the overscan crop to land in full 256x240 texture space. + let tex_u = image_u.mul_add( + f64::from(xform.crop_scale_u), + f64::from(xform.crop_offset_u), + ); + let tex_v = image_v.mul_add( + f64::from(xform.crop_scale_v), + f64::from(xform.crop_offset_v), + ); + let nes_x = (tex_u * f64::from(NES_W)) + .floor() + .clamp(0.0, f64::from(NES_W - 1)) as u16; + let nes_y = (tex_v * f64::from(NES_H)) + .floor() + .clamp(0.0, f64::from(NES_H - 1)) as u16; + Some((nes_x, nes_y)) +} + #[cfg(test)] mod tests { use super::*; @@ -2116,6 +2207,116 @@ mod tests { } } + // --------------------------------------------------------------------- + // window_to_nes_pixel (v2.3.6 workstream 0, defect 2) + // --------------------------------------------------------------------- + + /// The picker must be the exact inverse of the shader, so re-implement the + /// shader's forward map here FROM THE UNIFORM and require a round trip. A + /// test that re-derives the letterbox a third way would only prove the two + /// re-derivations agree with each other. + fn shader_forward(uni: &[f32; 8], nes_x: u16, nes_y: u16) -> (f64, f64) { + // Centre of the NES texel -> sample uv -> image uv -> screen uv. + let tex_u = (f64::from(nes_x) + 0.5) / f64::from(NES_W); + let tex_v = (f64::from(nes_y) + 0.5) / f64::from(NES_H); + let image_u = (tex_u - f64::from(uni[7])) / f64::from(uni[6]); + let image_v = (tex_v - f64::from(uni[5])) / f64::from(uni[4]); + ( + (image_u - 0.5).mul_add(f64::from(uni[0]), 0.5), + (image_v - 0.5).mul_add(f64::from(uni[1]), 0.5), + ) + } + + #[test] + fn window_to_nes_pixel_round_trips_against_the_shader_uniform() { + for &(w, h, par, os) in &[ + (NES_W, NES_H, false, crate::config::Overscan::default()), + ( + NES_W * 4, + NES_H * 4, + false, + crate::config::Overscan::default(), + ), + // Wide window: horizontal bars. Tall window: vertical bars. + (1920, 1080, true, crate::config::Overscan::default()), + (800, 1200, true, crate::config::Overscan::default()), + // Odd size + 8:7 + an asymmetric crop: every term non-trivial. + ( + 1366, + 768, + true, + crate::config::Overscan { + top: 8, + right: 4, + bottom: 8, + left: 12, + }, + ), + ] { + let uni = letterbox_uniform(w, h, par, os); + let osc = os.clamped(); + // Only VISIBLE pixels survive the crop; a cropped-away pixel has no + // window position at all. + let last_x = u16::try_from(NES_W - 1).unwrap() - u16::from(osc.right); + let last_y = u16::try_from(NES_H - 1).unwrap() - u16::from(osc.bottom); + for &(nx, ny) in &[ + (u16::from(osc.left), u16::from(osc.top)), + (128u16, 120u16), + (last_x, last_y), + ] { + let (su, sv) = shader_forward(&uni, nx, ny); + let cx = su * f64::from(w); + let cy = sv * f64::from(h); + let got = window_to_nes_pixel(w, h, par, os, cx, cy); + assert_eq!( + got, + Some((nx, ny)), + "round trip failed at nes=({nx},{ny}) for {w}x{h} par={par} os={osc:?}" + ); + } + } + } + + #[test] + fn window_to_nes_pixel_rejects_the_letterbox_bars() { + // 8:7 on a square window letterboxes top and bottom; a cursor in a bar + // has no NES pixel under it. The Zapper depends on this reading as "no + // light" rather than as a real, wrong coordinate. + let os = crate::config::Overscan::default(); + let (w, h) = (512u32, 1024u32); + assert_eq!( + window_to_nes_pixel(w, h, true, os, 256.0, 1.0), + None, + "top bar" + ); + assert_eq!( + window_to_nes_pixel(w, h, true, os, 256.0, 1023.0), + None, + "bottom bar" + ); + // Dead centre is always inside the image, whatever the aspect. + assert!(window_to_nes_pixel(w, h, true, os, 256.0, 512.0).is_some()); + } + + #[test] + fn window_to_nes_pixel_covers_the_whole_screen_without_gaps_or_overlap() { + // Sweep an exactly-4x window: every NES column and row must be hit, and + // the mapping must be monotone. Catches an off-by-one in the floor or a + // half-texel bias that a centre-only test would miss. + let os = crate::config::Overscan::default(); + let (w, h) = (NES_W * 4, NES_H * 4); + let mut seen_x = vec![false; NES_W as usize]; + let mut prev = 0u16; + for i in 0..w { + let (x, _) = window_to_nes_pixel(w, h, false, os, f64::from(i) + 0.5, 2.0) + .expect("square-pixel 4x fills the window exactly"); + assert!(x >= prev, "x went backwards at column {i}"); + prev = x; + seen_x[x as usize] = true; + } + assert!(seen_x.iter().all(|&s| s), "some NES column was unreachable"); + } + #[test] fn letterbox_uniform_overscan_crops_inner_224_rows() { // v1.0.0 — with the legacy hide-overscan toggle the crop samples rows diff --git a/crates/rustynes-frontend/src/runahead.rs b/crates/rustynes-frontend/src/runahead.rs index b5e778e5..e001eec2 100644 --- a/crates/rustynes-frontend/src/runahead.rs +++ b/crates/rustynes-frontend/src/runahead.rs @@ -72,14 +72,40 @@ impl RunAhead { /// Phase B: roll back to the persistent frame and re-enable rewind /// capture. Call after harvesting the visible framebuffer + audio. /// + /// # Debug provenance (v2.3.6 workstream 0, defect 1) + /// + /// The rollback is carried around the pixel-provenance and write-attribution + /// stores rather than through them. `restore_quiet` clears both, because for + /// its other callers — a user-driven save-state load, netplay rollback — the + /// records describe a timeline the restore replaced, and reporting them would + /// be confidently wrong. + /// + /// Run-ahead is the exception, and not because its restore is different: it + /// is because of *when* it happens. This rollback is the last thing before + /// the frontend releases the emulator lock, so the UI's first opportunity to + /// look is always after it. Clearing here does not discard a stale timeline; + /// it discards the record for the visible frame harvested three lines above, + /// which is the exact frame on screen. Run-ahead defaults to 1, so this made + /// the Pixel Provenance inspector show an empty report for every user from + /// v2.3.2 until this fix. + /// + /// Stashing is a move of two boxed stores, and it is skipped entirely when + /// neither is armed — which is the shipped default. `restore_quiet`'s own + /// reasoning is untouched: it still sees `None` and still clears for + /// everybody else. + /// /// # Panics /// /// Panics if the snapshot fails to restore — impossible for a blob /// produced by `snapshot_core_into` on the same instance (the same /// guarantee netplay rollback relies on). pub fn finish(&mut self, nes: &mut Nes) { + let stash = nes.take_provenance(); nes.restore_quiet(&self.snap_buf) .expect("run-ahead snapshot round-trips on the same instance"); + if stash.is_armed() { + nes.put_provenance(stash); + } nes.set_rewind_capture(true); } @@ -268,6 +294,113 @@ mod tests { } } + /// The pixel a provenance query asks about in the tests below. Screen + /// centre, matching the inspector panel's own default coordinate. + const PROBE_X: usize = 128; + const PROBE_Y: usize = 120; + + /// CONTROL for [`runahead_preserves_pixel_provenance`]: without run-ahead, + /// a plain `run_frame` leaves a populated provenance record. + /// + /// This exists so the sibling test's failure cannot be misread as "the + /// assertion is wrong" or "this ROM emits nothing". `dot` is the marker + /// because `Ppu::emit_pixel` stamps it on every emitted pixel as `x + 1` + /// (so 129 here), while `PixelProvenanceFrame::clear` writes the `Default` + /// record, whose `dot` is 0 — a value no real record can hold. + #[test] + fn plain_run_leaves_pixel_provenance_populated() { + let bytes = rom("assorted/flowing_palette.nes"); + let mut nes = Nes::from_rom(&bytes).expect("rom parses"); + nes.set_pixel_provenance(true); + let mut discard = vec![0.0f32; 8192]; + + for _ in 0..3u32 { + nes.run_frame(); + let _ = nes.drain_audio_into(&mut discard); + } + + let frame = nes + .pixel_provenance() + .expect("armed, so the frame is allocated"); + let rec = frame.get(PROBE_X, PROBE_Y).expect("on-screen coordinate"); + assert_ne!( + rec.dot, 0, + "control failed: a plain run recorded nothing, so the marker used by \ + `runahead_preserves_pixel_provenance` is not valid" + ); + } + + /// v2.3.6 workstream 0, defect 1 — run-ahead must not destroy the VISIBLE + /// frame's provenance record before the UI can read it. + /// + /// `finish` rolls back to the persistent frame with `restore_quiet`, and + /// `Nes::restore_inner` unconditionally clears both provenance stores. That + /// clear is correct for a user-driven save-state load and for netplay + /// rollback, and wrong here: run-ahead's rollback is the LAST thing that + /// happens before the frontend hands the emulator lock to the UI, so the + /// panel could only ever observe the wiped state. Since run-ahead defaults + /// to 1, that made the shipped Pixel Provenance inspector render a + /// complete, confident, entirely empty report for every user. + /// + /// The records kept are the visible frame's — one frame ahead of the + /// restored persistent state, and exactly the frame on screen. + #[test] + fn runahead_preserves_pixel_provenance() { + let bytes = rom("assorted/flowing_palette.nes"); + let mut nes = Nes::from_rom(&bytes).expect("rom parses"); + nes.enable_rewind(); + nes.set_pixel_provenance(true); + nes.set_write_attribution(true); + + let mut ra = RunAhead::default(); + let mut discard = vec![0.0f32; 8192]; + + for _ in 0..3u32 { + ra.run_frame_ahead(&mut nes, 1); + // The frontend harvests the visible framebuffer + audio here. + let _ = nes.drain_audio_into(&mut discard); + ra.finish(&mut nes); + + // ...and only THEN releases the lock, so this is the first moment + // the UI could look. + let frame = nes + .pixel_provenance() + .expect("armed, so the frame is allocated"); + let rec = frame.get(PROBE_X, PROBE_Y).expect("on-screen coordinate"); + assert_ne!( + rec.dot, 0, + "run-ahead's rollback wiped the visible frame's provenance: the \ + inspector panel can never see a record" + ); + } + } + + /// Arming must survive the rollback too — a wiped-and-disarmed store would + /// make the panel say "enable it, then run a frame" forever. + #[test] + fn runahead_preserves_the_provenance_arm() { + let bytes = rom("assorted/flowing_palette.nes"); + let mut nes = Nes::from_rom(&bytes).expect("rom parses"); + nes.enable_rewind(); + nes.set_pixel_provenance(true); + nes.set_write_attribution(true); + + let mut ra = RunAhead::default(); + let mut discard = vec![0.0f32; 8192]; + ra.run_frame_ahead(&mut nes, 2); + let _ = nes.drain_audio_into(&mut discard); + ra.finish(&mut nes); + + assert!( + nes.pixel_provenance().is_some(), + "pixel provenance disarmed by the run-ahead rollback" + ); + assert!( + nes.write_attribution().is_some(), + "write attribution disarmed by the run-ahead rollback" + ); + } + /// Rewind-ring contents: run-ahead must push exactly one (persistent) /// frame per cycle — the hidden + visible frames never land in the /// ring, and the rollback must not clear it. diff --git a/crates/rustynes-ppu/src/lib.rs b/crates/rustynes-ppu/src/lib.rs index 4b24dfa5..2fc2400e 100644 --- a/crates/rustynes-ppu/src/lib.rs +++ b/crates/rustynes-ppu/src/lib.rs @@ -55,8 +55,8 @@ pub use ppu::{HD_CHR_RAM, HD_TILE_NONE, HdSprite, HdTileSource}; #[cfg(feature = "debug-hooks")] pub use provenance::{ CIRAM_LEN as ATTRIB_CIRAM_LEN, OAM_LEN as ATTRIB_OAM_LEN, PALETTE_LEN as ATTRIB_PALETTE_LEN, - PATTERN_ADDR_NONE, PixelLayer, PixelProvenance, PixelProvenanceFrame, SPRITE_SLOT_NONE, - WriteAttrib, WriteAttribution, + PATTERN_ADDR_NONE, PixelLayer, PixelProvenance, PixelProvenanceFrame, ProvenanceStash, + SPRITE_SLOT_NONE, WriteAttrib, WriteAttribution, }; pub use raw_signal::{ ATTENUATION, BLACK, LEVELS, PHASES, RAW_ENTRIES, WHITE, composite_voltage, diff --git a/crates/rustynes-ppu/src/ppu.rs b/crates/rustynes-ppu/src/ppu.rs index aaca3ab4..987374f1 100644 --- a/crates/rustynes-ppu/src/ppu.rs +++ b/crates/rustynes-ppu/src/ppu.rs @@ -2084,6 +2084,40 @@ impl Ppu { } } + /// Move both provenance stores out, leaving the PPU unarmed. + /// + /// Paired with [`Self::put_provenance`] to carry the stores across a + /// same-timeline restore that would otherwise clear them — see + /// [`crate::provenance::ProvenanceStash`] for why run-ahead needs that and + /// save-state loads and netplay rollback do not. + /// + /// `prov_armed` is dropped to `false` alongside the frame it mirrors, so the + /// invariant "`prov_armed` iff `prov_frame.is_some()`" holds while stashed + /// and `emit_pixel` records nothing into the vacated slot. + #[cfg(feature = "debug-hooks")] + pub const fn take_provenance(&mut self) -> crate::provenance::ProvenanceStash { + let stash = crate::provenance::ProvenanceStash { + write_attrib: self.write_attrib.take(), + prov_frame: self.prov_frame.take(), + prov_armed: self.prov_armed, + }; + self.prov_armed = false; + stash + } + + /// Put back stores taken by [`Self::take_provenance`]. + /// + /// Overwrites whatever is currently held, which is what the pairing wants: + /// the only thing that can have appeared in between is a restore's cleared + /// (or absent) store, and the stashed records are the ones the caller means + /// to keep. + #[cfg(feature = "debug-hooks")] + pub fn put_provenance(&mut self, stash: crate::provenance::ProvenanceStash) { + self.write_attrib = stash.write_attrib; + self.prov_frame = stash.prov_frame; + self.prov_armed = stash.prov_armed; + } + /// Freeze the current instruction context as the cause of an OAM DMA burst. /// /// Called by the bus from the `$4014` write, i.e. while diff --git a/crates/rustynes-ppu/src/provenance.rs b/crates/rustynes-ppu/src/provenance.rs index 94b5bf70..e0d46f82 100644 --- a/crates/rustynes-ppu/src/provenance.rs +++ b/crates/rustynes-ppu/src/provenance.rs @@ -320,6 +320,22 @@ pub struct PixelProvenance { pub fine_y: u8, } +impl PixelProvenance { + /// Whether this record was actually emitted, as opposed to being the cleared + /// [`Default`]. + /// + /// `Ppu::emit_pixel` stamps [`Self::dot`] on every pixel it records, and the + /// visible dots are `1..=256` — so dot 0 is unreachable for a real record and + /// is exactly what `clear` leaves behind. Without this a caller cannot tell a + /// cleared record from a genuine backdrop pixel, and reads a confident + /// "scanline 0, dot 0, backdrop, palette $0000" as fact. That is precisely how + /// the v2.3.2 inspector reported a wiped frame (v2.3.6 workstream 0). + #[must_use] + pub const fn is_recorded(&self) -> bool { + self.dot != 0 + } +} + /// Screen width in pixels, and the stride of a [`PixelProvenanceFrame`]. pub const SCREEN_W: usize = 256; /// Screen height in pixels. @@ -391,6 +407,54 @@ impl Default for PixelProvenanceFrame { } } +// --------------------------------------------------------------------------- +// Stashing both stores across a same-timeline restore +// --------------------------------------------------------------------------- + +/// Both provenance stores, moved out of a [`Ppu`] so a caller can put them back. +/// +/// # Why this exists +/// +/// A save-state restore clears both stores, and that is right: the restored +/// bytes were not written by anything this session ran, so the honest answer is +/// "no record" rather than a PC from a timeline that no longer exists. +/// +/// Run-ahead is the one caller for which that is wrong, and it is wrong for a +/// reason that has nothing to do with the restore itself. Its cycle runs the +/// persistent frame, snapshots, runs the hidden and then the **visible** frame, +/// lets the frontend harvest that frame, and only then rolls back. The rollback +/// is therefore the *last* thing to happen before the emulator lock reaches the +/// UI — so a clear there does not discard a stale timeline, it discards the +/// record for the exact frame the user is looking at, before anyone can read it. +/// That is what made the shipped Pixel Provenance inspector render a complete, +/// confident, entirely empty report (v2.3.6 workstream 0, defect 1). +/// +/// Stashing is a **move, not a copy**: both stores are boxed, so this costs two +/// pointer moves per visible frame rather than the ~37 KiB memcpy a snapshot of +/// the contents would. The restore in between sees `None` on both, so its clear +/// is a no-op and its own reasoning is left completely intact — this mechanism +/// changes nothing for save-state loads or for netplay rollback, both of which +/// still want the clear. +/// +/// [`Ppu`]: crate::Ppu +#[derive(Debug, Default)] +pub struct ProvenanceStash { + pub(crate) write_attrib: Option>, + pub(crate) prov_frame: Option>, + pub(crate) prov_armed: bool, +} + +impl ProvenanceStash { + /// Whether either store was armed when this stash was taken. + /// + /// Callers use it to skip the put-back entirely on the overwhelmingly + /// common path where nothing is armed at all. + #[must_use] + pub const fn is_armed(&self) -> bool { + self.prov_armed || self.write_attrib.is_some() + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/rustynes-test-harness/Cargo.toml b/crates/rustynes-test-harness/Cargo.toml index cfc95a3a..f94dc88c 100644 --- a/crates/rustynes-test-harness/Cargo.toml +++ b/crates/rustynes-test-harness/Cargo.toml @@ -120,6 +120,17 @@ name = "dump_battery_ram" path = "src/bin/dump_battery_ram.rs" required-features = ["test-roms"] +# v2.3.6 — Zapper light-timing probe. Answers, from the game's own `$4017` +# traffic, whether a light-gun title ever sees light: per frame it pairs each +# read's returned byte (bit 3 = light NOT detected) with whether the aim point +# was actually bright in that frame's framebuffer. Built for the "Duck Hunt +# fires but never hits" report; needs a gitignored commercial dump, so it skips +# cleanly when absent. +[[bin]] +name = "zapper_light_probe" +path = "src/bin/zapper_light_probe.rs" +required-features = ["commercial-roms", "debug-hooks"] + # v2.3.1 "Plumb Line" — harness-free steady-state frame-cost probe. Profile THIS # instead of the criterion bench: a `perf record` of the bench binary attributes # ~17% of samples to criterion itself (rayon plumbing, libm exp, its sorts), diff --git a/crates/rustynes-test-harness/src/bin/zapper_light_probe.rs b/crates/rustynes-test-harness/src/bin/zapper_light_probe.rs new file mode 100644 index 00000000..fb8155ea --- /dev/null +++ b/crates/rustynes-test-harness/src/bin/zapper_light_probe.rs @@ -0,0 +1,339 @@ +//! v2.3.6 — Zapper light-timing probe: does a light-gun game ever see light? +//! +//! Diagnostic for the maintainer report that *Duck Hunt* fires but never hits. +//! For every frame it records, from the game's own bus traffic, each `$4017` +//! read and the byte the CPU actually received — bit 3 is **light NOT detected** +//! (inverted), bit 4 is the trigger — alongside whether the aperture at the aim +//! point was bright in that frame's completed framebuffer. +//! +//! What to look for: a frame where the framebuffer IS bright at the aim point +//! but every `$4017` read that frame reports "no light", followed by a *later* +//! frame reporting light after the target box is gone. That is a one-frame-stale +//! light signal, and it makes a hit impossible in any game that draws its target +//! and polls in the same frame — which is how *Duck Hunt* works. +//! +//! Run both models and compare — the first is the shipped default, the second +//! the superseded pre-v2.3.6 model: +//! +//! ```text +//! cargo run -p rustynes-test-harness --features commercial-roms,debug-hooks \ +//! --bin zapper_light_probe -- "tests/roms/external/mapper-000-NROM/Duck Hunt.nes" +//! cargo run -p rustynes-test-harness --features commercial-roms,debug-hooks \ +//! --bin zapper_light_probe -- "…/Duck Hunt.nes" frame-granular +//! ``` +//! +//! Useful environment knobs: `ZAPPER_PROBE_AIM=x,y` (aim point), +//! `ZAPPER_PROBE_WARMUP` (frames to reach gameplay — note that pressing START an +//! even number of times leaves *Duck Hunt* PAUSED, which looks exactly like "the +//! Zapper is ignored"), `ZAPPER_PROBE_FRAMES`, `ZAPPER_PROBE_TRACE=1` +//! (instruction-level replay of the first light-test frame) and +//! `ZAPPER_PROBE_PNG_DIR` (dump frames, so a claim can be checked against what +//! is actually on screen). +//! +//! The dump is gitignored (commercial), so this is a local-only diagnostic. + +use std::path::Path; + +use rustynes_core::{Buttons, Nes}; + +/// Dump a frame so the probe's numbers can be checked against what is actually +/// on screen. Reasoning about a light-gun game from luma statistics alone is how +/// this investigation went wrong twice. +fn write_png(path: &Path, fb: &[u8]) { + let file = std::fs::File::create(path).expect("create png"); + let w = std::io::BufWriter::new(file); + let mut enc = png::Encoder::new(w, 256, 240); + enc.set_color(png::ColorType::Rgba); + enc.set_depth(png::BitDepth::Eight); + let mut writer = enc.write_header().expect("png header"); + writer.write_image_data(fb).expect("png data"); +} + +/// Default aim point. Duck Hunt's ducks cross the upper-middle of the play area. +/// Overridable with `ZAPPER_PROBE_AIM=x,y` so a run can be pointed at the target +/// box the previous run located. +const AIM_X_DEFAULT: u16 = 128; +const AIM_Y_DEFAULT: u16 = 96; + +/// Centre of the brightest run of pixels in a frame, if any pixel is bright. +/// +/// On Duck Hunt's target frame the screen is black except for a small white box +/// over each duck, so this locates the box the game is testing — which is where +/// a player who actually hit the duck would have been aiming. +fn brightest_spot(fb: &[u8]) -> Option<(u16, u16)> { + let (mut sx, mut sy, mut n) = (0u32, 0u32, 0u32); + for y in 0..240u32 { + for x in 0..256u32 { + let idx = ((y * 256 + x) * 4) as usize; + let luma = (77 * u32::from(fb[idx]) + + 150 * u32::from(fb[idx + 1]) + + 29 * u32::from(fb[idx + 2])) + >> 8; + if luma >= 0x80 { + sx += x; + sy += y; + n += 1; + } + } + } + (n > 0).then(|| { + ( + u16::try_from(sx / n).unwrap_or(0), + u16::try_from(sy / n).unwrap_or(0), + ) + }) +} + +/// Mirrors `ZapperState::aperture_is_bright` closely enough to say "the target +/// was visible here this frame". Deliberately re-derived rather than exported: +/// the probe asks about the framebuffer, not about what the Zapper concluded. +/// Returns `(centre_luma, pixels_over_threshold)` using the SAME constants the +/// core uses — `ZAPPER_LUMA_THRESHOLD` (`0x80`) over a 3x3 aperture, of which +/// `ZAPPER_APERTURE_MIN_BRIGHT` (2) must pass. Reproduced rather than exported +/// so the probe reports the raw measurement, not the core's verdict. +fn aperture_stats(fb: &[u8], x: u16, y: u16) -> (u16, u32) { + const THRESHOLD: u16 = 0x80; + let (ax, ay) = (i32::from(x), i32::from(y)); + let mut bright = 0u32; + let mut centre = 0u16; + for dy in -1..=1i32 { + for dx in -1..=1i32 { + let (px, py) = (ax + dx, ay + dy); + if !(0..256).contains(&px) || !(0..240).contains(&py) { + continue; + } + let Ok(idx) = usize::try_from((py * 256 + px) * 4) else { + continue; + }; + if idx + 2 >= fb.len() { + continue; + } + let luma = (77 * u16::from(fb[idx]) + + 150 * u16::from(fb[idx + 1]) + + 29 * u16::from(fb[idx + 2])) + >> 8; + if dx == 0 && dy == 0 { + centre = luma; + } + if luma >= THRESHOLD { + bright += 1; + } + } + } + (centre, bright) +} + +#[allow(clippy::too_many_lines)] // a linear diagnostic script; splitting it +// would scatter the frame loop's shared state across helpers for no gain. +fn main() { + let mut args = std::env::args().skip(1); + let path = args + .next() + .unwrap_or_else(|| "tests/roms/external/mapper-000-NROM/Duck Hunt.nes".into()); + // The beam-relative model is the SHIPPED DEFAULT from v2.3.6; pass + // `frame-granular` to probe the superseded pre-v2.3.6 model instead. The + // argument used to be `temporal` (opt-in then), which is still accepted so + // older invocations in notes and logs keep working. + let arg = args.next().unwrap_or_default(); + let temporal = !matches!(arg.as_str(), "frame-granular" | "frame"); + + let Ok(bytes) = std::fs::read(&path) else { + eprintln!("skipping: {path} absent (commercial dump, gitignored)"); + return; + }; + let mut nes = Nes::from_rom(&bytes).expect("rom parses"); + nes.set_zapper_temporal_light(temporal); + // The access log is per-frame and self-clearing, so each frame's reads are + // exactly that frame's. + nes.set_access_logging(true); + println!("rom: {path}"); + println!( + "light model: {}", + if temporal { + "beam-relative (SHIPPED DEFAULT since v2.3.6)" + } else { + "frame-granular (superseded; the model that made a Duck Hunt hit impossible)" + } + ); + + // Aim point, overridable so a run can be pointed at the target box a + // previous run located. + let (aim_x_runtime, aim_y_runtime) = std::env::var("ZAPPER_PROBE_AIM") + .ok() + .and_then(|s| { + let (a, b) = s.split_once(',')?; + Some((a.trim().parse().ok()?, b.trim().parse().ok()?)) + }) + .unwrap_or((AIM_X_DEFAULT, AIM_Y_DEFAULT)); + println!("aim: ({aim_x_runtime}, {aim_y_runtime})"); + + let mut discard = vec![0.0f32; 8192]; + + // Boot and get into gameplay. Duck Hunt's title screen needs Start on the + // standard pad; the warmup is dumped too, because "is the game even in + // gameplay?" turned out to be the question that mattered. + let warmup: u32 = std::env::var("ZAPPER_PROBE_WARMUP") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(300); + for f in 0..warmup { + // START only during the title screen. Holding it into gameplay PAUSES + // Duck Hunt — which looks exactly like "the game ignores the Zapper", + // and cost this investigation two wrong conclusions before a screenshot + // showed the word PAUSE. + let b = if (60..66).contains(&f) { + Buttons::START + } else { + Buttons::empty() + }; + nes.set_buttons(0, b); + nes.set_zapper(1, aim_x_runtime, aim_y_runtime, false); + nes.run_frame(); + let _ = nes.drain_audio_into(&mut discard); + if let Ok(dir) = std::env::var("ZAPPER_PROBE_PNG_DIR") + && f % 20 == 0 + { + write_png( + &Path::new(&dir).join(format!("warm{f:04}.png")), + nes.framebuffer(), + ); + } + } + + println!(); + println!("frame trig fb_bright $4017 reads (value: light?)"); + let mut saw_light = false; + // Duck Hunt blanks the screen for its light test, so a dark frame marks a + // shot actually being processed. Print a window around each one and stay + // quiet otherwise — the interesting event is rare and easy to miss by hand. + let mut window_left = 0u32; + let frames: u32 = std::env::var("ZAPPER_PROBE_FRAMES") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(150); + let mut blanks = 0u32; + let trace_enabled = std::env::var("ZAPPER_PROBE_TRACE").is_ok(); + let mut traced = false; + for f in 0..frames { + // Repeated short pulls, so the run catches the game's post-trigger + // light-test sequence wherever the ducks happen to be. + let trigger = f % 40 < 6; + nes.set_buttons(0, Buttons::empty()); + nes.set_zapper(1, aim_x_runtime, aim_y_runtime, trigger); + // Snapshot before the frame so a light-test frame can be REPLAYED + // instruction-by-instruction once it has been identified — determinism + // is what makes that legitimate. + let pre = if trace_enabled { + nes.snapshot() + } else { + Vec::new() + }; + nes.run_frame(); + let _ = nes.drain_audio_into(&mut discard); + + let (centre, bright_px) = aperture_stats(nes.framebuffer(), aim_x_runtime, aim_y_runtime); + let bright = bright_px >= 2; + let reads: Vec = nes + .accesses() + .iter() + .filter(|a| a.addr == 0x4017 && !a.write) + .map(|a| a.value) + .collect(); + // Mean frame luma separates "the game blanked the screen for the light + // test" from ordinary gameplay, which is how the Duck Hunt sequence is + // recognisable at all. + let fb = nes.framebuffer(); + let mean_luma: u32 = fb.chunks_exact(4).map(|p| u32::from(p[1])).sum::() + / u32::try_from(fb.len() / 4).unwrap_or(1); + let _ = bright; + if let Ok(dir) = std::env::var("ZAPPER_PROBE_PNG_DIR") { + write_png( + &Path::new(&dir).join(format!("f{f:03}.png")), + nes.framebuffer(), + ); + } + // Bit 3 CLEAR == light detected, but only if a Zapper actually answered + // the read: a standard controller returns its shift bit in D0 with D3 + // clear, which is indistinguishable from "light" unless the byte is + // checked as a whole. Print the distinct raw bytes and judge from those. + let mut distinct: Vec = reads.clone(); + distinct.sort_unstable(); + distinct.dedup(); + let lit = reads.iter().filter(|v| *v & 0x08 == 0).count(); + saw_light |= lit > 0; + if mean_luma < 20 { + blanks += 1; + window_left = 6; + // On the target frame the screen is black except the box(es) the + // game is testing. Report where, so a follow-up run can aim there — + // the check that the sensor fires when it should, not just that it + // stays quiet when it should. + if let Some((bx, by)) = brightest_spot(nes.framebuffer()) { + println!(" target box centre at ({bx}, {by}) <-- aim here to test a hit"); + } + if trace_enabled && !traced { + traced = true; + println!(); + println!("--- instruction replay of light-test frame {f} ---"); + println!("sl dot $4017 aim_luma note"); + nes.restore(&pre).expect("snapshot round-trips"); + nes.set_zapper(1, aim_x_runtime, aim_y_runtime, trigger); + // No frame counter on `Nes`, so bound the replay by the + // scanline wrapping back to the top after reaching vblank. + let mut last_sl = i16::MIN; + let mut seen_vblank = false; + for _ in 0..40_000u32 { + nes.step_instruction(); + let now = nes.bus().ppu().scanline(); + if now > 240 { + seen_vblank = true; + } + if seen_vblank && now < 8 { + break; + } + let sl = nes.bus().ppu().scanline(); + if sl == last_sl { + continue; // one sample per scanline shows the shape + } + last_sl = sl; + let dot = nes.bus().ppu().dot(); + let byte = nes.peek(0x4017); + let fb = nes.framebuffer(); + let idx = (usize::from(aim_y_runtime) * 256 + usize::from(aim_x_runtime)) * 4; + let luma = (77 * u32::from(fb[idx]) + + 150 * u32::from(fb[idx + 1]) + + 29 * u32::from(fb[idx + 2])) + >> 8; + let lit = byte & 0x08 == 0; + if lit || sl % 24 == 0 { + println!( + "{sl:<4} {dot:<6} {byte:02X} {luma:<9} {}", + if lit { "LIGHT" } else { "" } + ); + } + } + println!("--- end replay ---"); + println!(); + } + } + if window_left == 0 { + continue; + } + window_left -= 1; + println!( + "{f:5} {:4} aim_luma {centre:3} bright {bright_px}/9 \ + screen_mean {mean_luma:3} {} read(s) bytes {:02X?}", + if trigger { "yes" } else { "-" }, + reads.len(), + distinct, + ); + } + println!(); + println!("blank (light-test) frames observed: {blanks}"); + println!( + "bit 3 clear on at least one read: {} \ + (NOT proof of light on its own — bit 6 is open bus, so a Zapper byte is \ + 0x40 | trigger<<4 | no_light<<3)", + if saw_light { "yes" } else { "no" } + ); +} diff --git a/crates/rustynes-test-harness/tests/input_devices.rs b/crates/rustynes-test-harness/tests/input_devices.rs index 16e5574d..3fe1f104 100644 --- a/crates/rustynes-test-harness/tests/input_devices.rs +++ b/crates/rustynes-test-harness/tests/input_devices.rs @@ -249,3 +249,97 @@ fn input_device_state_types_constructible() { let _ = InputDevice::KonamiHyperShot(rustynes_core::KonamiHyperShotState::new()); let _ = InputDevice::BandaiHyperShot(rustynes_core::BandaiHyperShotState::new()); } + +/// v2.3.6 regression — a Zapper shot in *Duck Hunt* must be able to score. +/// +/// This is the defect the beam-relative light model was promoted to fix. Duck +/// Hunt's protocol is "the gun must see NOTHING for one frame, then a bright +/// spot in the next". Under the frame-granular model the light bit was sampled +/// at end-of-frame, so a read during frame N reported frame N-1: on the blanked +/// probe frame the game saw the previous (bright) frame and on the target frame +/// it saw the blanked one — the probe inverted, the shot discarded before +/// hit-testing, and no duck could ever be hit however well the player aimed. +/// +/// The assertion is the game's own scoreboard, not an internal flag: score is +/// zero at the start of the round and non-zero after a shot aimed at the target +/// box. That is the only oracle that cannot pass while the bug is present. +/// +/// Uses a gitignored commercial dump; skips cleanly when absent, like the other +/// commercial-ROM tests here. +#[test] +fn duck_hunt_zapper_shot_can_score() { + use rustynes_core::Buttons; + + // The target box centre on the light-test frame for this deterministic + // replay, found with `zapper_light_probe`. + const AIM: (u16, u16) = (93, 156); + + // Bottom-right of the play area, where Duck Hunt renders SCORE. Counting + // bright pixels over the digits detects "not all zeroes" without decoding + // the font. + fn score_ink(fb: &[u8]) -> u64 { + let mut ink = 0u64; + for y in 200..232usize { + for x in 184..248usize { + let idx = (y * 256 + x) * 4; + let luma = (77 * u64::from(fb[idx]) + + 150 * u64::from(fb[idx + 1]) + + 29 * u64::from(fb[idx + 2])) + >> 8; + if luma >= 0x80 { + ink += 1; + } + } + } + ink + } + + let manifest = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let root = manifest.parent().and_then(|p| p.parent()).expect("root"); + let path = root.join("tests/roms/external/mapper-000-NROM/Duck Hunt.nes"); + let Ok(bytes) = fs::read(&path) else { + eprintln!("skipping: {} absent (commercial dump)", path.display()); + return; + }; + + let mut nes = Nes::from_rom(&bytes).expect("rom parses"); + assert!( + nes.zapper_temporal_light(), + "this test asserts the SHIPPED default; it is meaningless if the \ + beam-relative model is off" + ); + let mut discard = vec![0.0f32; 8192]; + + // Title -> gameplay. Exactly ONE Start press: holding it into gameplay + // pauses the game, which looks identical to "the Zapper is ignored". + for f in 0..1000u32 { + nes.set_buttons( + 0, + if (60..66).contains(&f) { + Buttons::START + } else { + Buttons::empty() + }, + ); + nes.set_zapper(1, AIM.0, AIM.1, false); + nes.run_frame(); + let _ = nes.drain_audio_into(&mut discard); + } + let ink_before = score_ink(nes.framebuffer()); + + // Pulse the trigger; the round's ducks cross the aim point. + for f in 0..260u32 { + nes.set_buttons(0, Buttons::empty()); + nes.set_zapper(1, AIM.0, AIM.1, f % 40 < 6); + nes.run_frame(); + let _ = nes.drain_audio_into(&mut discard); + } + let ink_after = score_ink(nes.framebuffer()); + + assert_ne!( + ink_after, ink_before, + "Duck Hunt's score never changed: the Zapper shot was discarded before \ + hit-testing (the v2.2.3-v2.3.5 frame-granular light model), or the \ + replay drifted off the round this fixture pins" + ); +} diff --git a/docs/accuracy-ledger.md b/docs/accuracy-ledger.md index 1589090c..fbc23877 100644 --- a/docs/accuracy-ledger.md +++ b/docs/accuracy-ledger.md @@ -40,7 +40,7 @@ disposition under the v2.1.0 "Fathom" accuracy-remediation line | FDS medium model (F4.3) | Byte-stream wire medium: gap / `$80` mark / block / **CRC-16-KERMIT** per block, with **per-block CRC re-emitted on write** (`resynth_block_crc`) and an opt-in **continuous belt-velocity head-seek** model (distance-proportional re-seek, default-off) replacing the fixed-cycle window | **CI-verifiable (synthetic):** `medium_write_verify` BIOS-free oracle — write via the register path, re-walk the wire, assert every block's CRC-16 + gap/mark framing round-trips (`fds::tests::synthetic_write_verify_*`). **Local-only:** the real-BIOS write-CRC path (BIOS recomputes CRC in its own RAM → `$4024`) needs a copyright `disksys.rom`, kept in gitignored `tests/roms/external/` and out of CI | **Shipped (v2.2.0 "Capstone")** — additive: default (model-off, non-writing) `.fds` run is **byte-identical**; new state round-trips the **v4** save-state tail. AccuracyCoin has no FDS ROM, so 141/141 is unaffected | | Famicom microphone ($4016.2) | Not modeled | Built-in controller-2 mic bit surfaced on `$4016` D2 (`Nes::set_microphone`); `famicom_microphone_drives_4016_bit2` bus unit test | **Shipped (v2.2.0)** — additive / default-off (mic released ⇒ `$4016` byte-identical); a `$4016`-only signal (never touches `$4017`). No pass/fail mic ROM exists (real-cart local test only) | | FDS DRAM-refresh-watchdog IRQ (`$4030.D1`) | Not modeled. The FDS BIOS/hardware raises periodic IRQs tied to DRAM-refresh-row-vs-access cycle accounting while `$4023.D0=0`; per upstream (`TakuikaNinja`'s `FDS-4030D1-Addr` research, `NESdev` Wiki) this is itself still under active hardware research and not modeled by most current FDS emulators either | `TakuikaNinja`'s `FDS-4030D1-Addr` probe (gitignored, `tests/roms/external/fds-takuikaninja/`, no permissive license — see `tests/roms/external/README.md`), consumed by the `RUSTYNES_FDS_BIOS`-gated `fds_4030d1_addr_with_real_bios` test | **Out of scope / honest residual, tracked not asserted** — the gated test only proves construction + a bounded real-BIOS run complete without panicking; it does not assert a specific watchdog timing value, since neither RustyNES nor the public research has pinned one yet. Revisit if/when upstream hardware research settles the exact behavior | -| Zapper light-timing | Single-pixel per-frame framebuffer sample | **Photodiode aperture** (3x3 field-of-view, >=2 bright pixels — `ZAPPER_APERTURE_*`) vs the PPU per-dot output, plus (v2.2.3 A3) the **beam-relative temporal model**; `zapper_light_detected_for_bright_region` / `zapper_aperture_rejects_lone_bright_pixel` / `zapper_temporal_light_follows_the_beam` / `zapper_frame_model_is_scanline_invariant_but_temporal_is_not` unit tests | **Hardened (v2.2.0) + temporal model added opt-in (v2.2.3 A3).** The ~19-26-scanline photodiode hold is now modelled: `ZapperState::light_at_scanline` makes light a function of where the CRT beam is at the moment of the read — dark before the beam paints the aim row, lit for `ZAPPER_LIGHT_HOLD_SCANLINES`, dark once drained — which the frame-granular model structurally cannot express (it returns one answer per frame). **Default OFF** (`Nes::set_zapper_temporal_light`): no redistributable pass/fail Zapper ROM exists to adjudicate it, and 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. Deterministic and stateless either way — a pure fn of framebuffer + aim + scanline, so nothing new to serialize and no save-state or rollback impact | +| Zapper light-timing | Single-pixel per-frame framebuffer sample | **Photodiode aperture** (3x3 field-of-view, >=2 bright pixels — `ZAPPER_APERTURE_*`) vs the PPU per-dot output, plus (v2.2.3 A3) the **beam-relative temporal model**; `zapper_light_detected_for_bright_region` / `zapper_aperture_rejects_lone_bright_pixel` / `zapper_temporal_light_follows_the_beam` / `zapper_frame_model_is_scanline_invariant_but_temporal_is_not` unit tests | **Hardened (v2.2.0), temporal model added opt-in (v2.2.3 A3), PROMOTED TO DEFAULT (v2.3.6).** The ~19-26-scanline photodiode hold is modelled: `ZapperState::light_at_scanline` makes light a function of where the CRT beam is at the moment of the read — dark before the beam paints the aim row, lit for `ZAPPER_LIGHT_HOLD_SCANLINES`, dark once drained — which the frame-granular model structurally cannot express (it returns one answer per frame, sampled at end-of-frame, so every read during frame N reports frame N-1). **v2.3.6 CORRECTION:** this row previously justified `Default OFF` with "the supported titles re-poll every frame and are satisfied by either model". **That was false.** *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 that probe inverted (bright on the blanked frame, dark on the target frame) and discarded every shot before hit-testing, so **no duck could ever be hit** — reported by the maintainer, reproduced headlessly from the game's own `$4017` traffic. The claimed absence of an oracle was also wrong: the game itself is one. Also fixed in v2.3.6: the beam-relative sampler read aperture rows the beam had **not finished painting**, so it reported light on an all-black screen (`aperture_is_bright_painted`). Measured A/B, same ROM/aim/inputs — frame model: score 000000, duck flying; beam-relative: score 000500, duck hit. Regression test `duck_hunt_zapper_shot_can_score` asserts the game's own scoreboard, mutation-checked. Deterministic and stateless either way — a pure fn of framebuffer + aim + scanline, so nothing new to serialize and no save-state or rollback impact | | BestEffort mapper tier (26 families, was 112) | Register-decode + save-state round-trip only; off the oracle gate | `mapper_tier_honesty.rs` invariant | **Mostly remediated** (F3): 86 promoted to Curated with commercial-ROM oracle; the 26 left have no cleanly-booting dump (16 NES 2.0 high-id + 8 no-cart + 2 jam-at-boot) | | MMC3 R1/R2 scanline-IRQ (ADR 0002) | ≤1-CPU-cycle differential on 4 `#[ignore]`'d sub-tests; zero game impact | `mmc3_test_2/4` #3 + siblings; `mmc3_r1r2_phase_probe` A12-phase golden probe (v2.1.5, `--features mmc3-a12-phase-probe`) | **CLOSED for the shipping default; axis-B candidate deferred to maintainer** (F5.0, ADR 0002). v2.1.5 direct instrumentation refined the closure: "no post-access qualifying rise" is ROM-specific (holds for the two `scanline_timing` #3 residuals, `irq_post=0`; **false** for `mmc3_test_v1/5`+`/6` #2, `irq_post=4` — post-access IRQ-clocking rises Session B never measured). Every *tested* lever stays non-curative (incl. the `mmc3-m2-phase-irq` deferral, byte-identical status on `/5`+`/6`); the four pins stay `#[ignore]`'d. One untested lever — an ares-style M2-edge-precise falling-edge low-time filter — is deferred to a maintainer decision (needs a sacred-gate-risking substrate change to prototype) | | APU non-linear mixer | Lookup-table matches within the `apu_mixer` band | `apu_mixer` (analog-cancellation, tolerance) | **No stricter oracle** — the LUT already passes; ±4% is honest | diff --git a/docs/frontend.md b/docs/frontend.md index cba5105b..1d3cc1b4 100644 --- a/docs/frontend.md +++ b/docs/frontend.md @@ -1138,6 +1138,35 @@ The panels split by what they need: `&mut Nes` and a per-frame core poll, so they render only while the overlay is visible. `OpenChipPanel` therefore forces the overlay visible. +### Pixel Provenance + +**Tools → Pixel Provenance** pins one screen pixel and reports its whole causal +chain — the dot and scanline that emitted it, the layer that won priority, the +nametable / attribute / pattern addresses of the tile actually on screen, the +palette entry, and the CPU instruction and cycle that last wrote each of those +bytes. Spec: [`pixel-provenance.md`](pixel-provenance.md). + +Two frontend details belong here rather than in the spec: + +- **Selecting a pixel is a raw winit click, not an egui `Response`.** The NES + image is a wgpu letterbox blit drawn under the egui shell, not a widget, so + there is nothing to hit-test. `WindowEvent::MouseInput` converts the cursor + with `gfx::window_to_nes_pixel` and calls `DebuggerOverlay::set_provenance_pick` + — guarded by the same `wants_egui_input` check that keeps a menu click from + firing the Zapper, and only while the panel is open. The X/Y spinboxes remain + for exact coordinates. +- **`gfx::window_to_nes_pixel` is the inverse of the blit, derived from the same + `BlitTransform` the shader uniform is built from.** A picker that re-derived + the letterbox independently would be a second source of truth that drifts + silently and reports a neighbouring pixel's causal chain as fact. It returns + `None` on a letterbox bar, which is load-bearing in both callers: the picker + must not pin a pixel the user did not click, and the **Zapper** must read a bar + as "no light". The Zapper's own mapping was a full-window stretch until v2.3.6 + — its comment claimed bars read as off-screen while in fact every bar position + mapped onto a real, wrong NES pixel — and now shares this converter. The Vaus + paddle deliberately keeps its full-window sweep: a knob has no off-screen + state, and how far the hand travels per turn is a feel decision. + ### Pause and fullscreen Pausing parks the emu thread (no `EmuFrame` pings), so the shell keeps diff --git a/docs/pixel-provenance.md b/docs/pixel-provenance.md index 40e17dbd..4f2af75f 100644 --- a/docs/pixel-provenance.md +++ b/docs/pixel-provenance.md @@ -3,10 +3,15 @@ > **Status:** all four phases implemented (write attribution, per-pixel > provenance, the inspector panel, replay attestation). This document is the > spec, so it is updated in the same change as the code it describes. +> +> **v2.3.6 — the feature did not work as shipped in v2.3.2, and this document +> was part of the reason.** Two defects, and a doc claim covering each. See +> [Two defects, and what they cost](#two-defects-and-what-they-cost) below. ## What the feature answers -Point at any pixel on screen and get the full causal chain that produced it: +Click any pixel on screen — or type its coordinates — and get the full causal +chain that produced it: 1. the PPU **dot and scanline** that emitted it; 2. the **background tile** behind it — nametable address, attribute address, @@ -98,9 +103,29 @@ Both stores — write attribution and the per-pixel frame — are cleared on **power-cycle** and on **both save-state restore paths** (`Nes::restore` and `Nes::restore_quiet`). A restored state's bytes were not written by any instruction this session executed, so the PCs recorded against -those offsets describe a timeline that no longer exists. Under run-ahead the -per-frame `restore_quiet` therefore leaves exactly the visible frame's writes — -which is the timeline the user is looking at. +those offsets describe a timeline that no longer exists. + +**Run-ahead is carried around that clear, not through it** (v2.3.6). +`RunAhead::finish` moves both stores out with `Nes::take_provenance`, lets +`restore_quiet` run, and puts them back with `Nes::put_provenance`. The records +kept are the **visible** frame's — one frame ahead of the restored persistent +state, and exactly the frame on screen. + +This is the one caller that needs the exception, and not because its restore is +different: because of *when* it happens. Run-ahead's rollback is the last thing +before the frontend releases the emulator lock, so the UI's first chance to look +is always after it. Clearing there discards the frame the user is looking at +rather than a stale timeline. Every other caller — a user-driven save-state load, +netplay rollback — still clears, and still should. The stash is a move of two +boxed stores, skipped entirely when neither is armed. + +> **This paragraph used to say the opposite of the code.** Until v2.3.6 it read: +> "Under run-ahead the per-frame `restore_quiet` therefore leaves exactly the +> visible frame's writes — which is the timeline the user is looking at." The +> clear emptied both stores completely, and an identical claim sat in a comment +> beside the clear itself. Since run-ahead defaults to 1, the shipped inspector +> showed an empty report to every user for four releases. The prose asserting the +> intent is what stopped anyone checking the code against it. ### Cost and the determinism contract @@ -236,12 +261,62 @@ The panel pins a screen coordinate and reports, in four sections: CIRAM offsets and the instructions that wrote those bytes, the palette group, the pattern bits, fine scroll, and the pattern address. Shown for sprite pixels too, since the background is what the sprite won priority *over*. -4. **Sprite** — slot, priority, pattern bits and address, the sprite-0 flag, and - the instruction that wrote the slot's OAM bytes. +4. **Sprite** — slot, priority, pattern bits and address, and the sprite-0 flag. + Deliberately **no OAM write-attribution row**: the primary OAM index does not + exist at emit time, so naming a writer would name a different sprite's. See + "What phase 2 cannot answer" above, which this line used to contradict. + +**Selecting a pixel.** Click anywhere on the game view to pin that pixel, or set +the X/Y spinboxes directly. The click is captured in the winit mouse handler +rather than as an egui `Response`, because the NES image is a raw wgpu blit and +not an egui widget — there is nothing to hit-test. `gfx::window_to_nes_pixel` +does the conversion by inverting the blit's own letterbox/crop transform, so it +is correct at any window size, pixel-aspect setting and overscan crop, and a +click on a letterbox bar pins nothing rather than silently pinning an edge pixel. Arming is in the panel: two checkboxes for the provenance frame and the attribution store, both default off. This is the panel's only side effect on the -emulator, and both stores are determinism-neutral. +emulator, and both stores are determinism-neutral. The checkbox state is read +from the core every frame rather than mirrored in the panel, so loading a ROM +(which installs a fresh `Nes`) cannot desync it. + +When the panel has no answer it says which kind of "no answer" it is — not armed, +armed but nothing recorded for this pixel yet, or off-screen. A cleared record is +a valid `PixelProvenance` whose every field reads as a confident "scanline 0, +dot 0, backdrop, palette `$0000`", so without that check an empty frame renders +as fact. `PixelProvenance::is_recorded()` is the discriminator: `emit_pixel` +stamps `dot` on every pixel it records and the visible dots are `1..=256`, so +dot 0 is unreachable for a real record and is exactly what `clear` leaves. + +## Two defects, and what they cost + +Recorded because the shape of the failure matters more than either bug. + +**Defect 1 — run-ahead wiped the record before the UI could read it.** Covered +under [Lifetime and invalidation](#lifetime-and-invalidation) above. + +**Defect 2 — clicking a pixel was never implemented.** The panel offered two +`DragValue` spinboxes and contained no click hit-test at all, while this document +opened with "Point at any pixel on screen" and the release notes said "pin a +screen pixel". Two later lines here — the panel "pins a screen coordinate" and +"the coordinate-picker shape follows `hd_pixel_panel.rs`" — described the real +behaviour accurately, so the document contradicted itself and the wrong half was +the one users read first. + +**What both have in common:** the core data structures were well covered by unit +tests, and the frontend wiring was covered by nothing. No test drove the produce +path with run-ahead on; no test asked whether a click could reach the panel. The +same shape produced issue #360 in the same release train, where `MovieUi::after_frame` +worked in production but no test ever called it. `runahead.rs` even carried tests +pinning the determinism of the exact code path that destroyed this telemetry. + +The regression net added in v2.3.6: +`runahead::tests::runahead_preserves_pixel_provenance` (with +`plain_run_leaves_pixel_provenance_populated` as its control, so a failure cannot +be misread as a bad assertion), `runahead_preserves_the_provenance_arm`, and +three `gfx::tests::window_to_nes_pixel_*` tests — one of which round-trips the +picker against the shader's own uniform rather than against a third re-derivation +of the letterbox. ### Reuse, and one thing deliberately not reused diff --git a/docs/user-guide/debugger.md b/docs/user-guide/debugger.md index 1f4c6de0..c77134bc 100644 --- a/docs/user-guide/debugger.md +++ b/docs/user-guide/debugger.md @@ -191,6 +191,51 @@ set of debugging and authoring tools, reachable from the **Tools** and These are aimed at homebrew developers and TAS authors; you never need them to play a game. +## Pixel Provenance + +**Tools → Pixel Provenance** answers "why is this pixel this colour?" for any +pixel on screen, all the way back to the code that caused it. + +How to use it: + +1. Open **Tools → Pixel Provenance**. +2. Tick **both** checkboxes — *Per-pixel provenance* and *Write attribution*. + They are independent and both default off: the first records which bytes + produced each pixel, the second records which instruction wrote those bytes. + You want both for the full chain. +3. Let the game run at least one frame. +4. **Click any pixel on the game view**, or type coordinates into the X/Y boxes. + +You then get, for that pixel: the PPU dot and scanline that emitted it; whether +the background, a sprite, or the backdrop won; the palette address and entry; the +nametable, attribute and pattern addresses of the tile actually on screen; and — +for each of those bytes — the program counter and CPU cycle of the instruction +that last wrote it. If you have loaded a `.dbg` source map, you also get the +source file and line. + +If the panel has nothing to show it tells you which kind of nothing: not armed, +nothing recorded for that pixel yet, or off-screen. Clicking a black letterbox +bar pins nothing, by design. + +Things worth knowing: + +- **Arming costs memory, not accuracy.** Both stores are output-only. The + framebuffer, the audio and every cycle count are bit-identical whether they are + armed or not. +- **A power-cycle or loading a save state clears the records**, because the + restored bytes were not written by anything the current session ran. Run-ahead + does *not* clear them — you get the frame you are looking at. +- **CHR (pattern) bytes have no attribution row.** They are mapper-owned, so a + byte offset is not a stable identity across a bank switch. The panel reports + which bank supplied them instead. +- **Sprites have no OAM attribution row.** The PPU does not keep the primary OAM + index at emit time, so naming a writer would risk naming a different sprite's. + +> **Fixed in v2.3.6.** In v2.3.2 through v2.3.5 this panel returned an empty +> report for almost everyone: run-ahead (on by default) erased the record before +> the panel could read it, and clicking a pixel was never wired up. If you tried +> it on an older build and nothing happened, that was the bug, not your setup. + ## See also - [Controls](./controls.md) — full rebind flow