diff --git a/CHANGELOG.md b/CHANGELOG.md index 5757ff8f..1648478c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,102 @@ cycle-accurate core later replaced. ## [Unreleased] +Post-v2.3.7 work, landed but not yet cut. + +### Added + +- **The Divergence Lens — which pixels differ, not just which frame, and why.** + (#407, the whole of v2.3.8 "Parallax".) Surfaced as a panel under **Tools → + Analysis**, over a headless `rustynes_probe::divergence` core that is tested + independently of it. `Probe` could already say whether two + configurations of the same ROM diverge and at which frame, because a trial + reduces each frame to one `u64`. That reduction is the right shape for + *detecting* a difference and the wrong shape for *explaining* one: a hash says + frame 412 differs and cannot say which pixel, so it has nothing to hand to + Pixel Provenance, which is where an answer actually lives. + + `divergence::localise` re-runs both configurations to the detected frame, + keeps the full output instead of its hash, and reports the *shape* of the + difference — population count, first pixel in raster order, and the inclusive + bounding box. Count and box separate kinds of bug from each other: one pixel is + a sprite or a palette entry, 256 in a row is a scanline, tens of thousands is a + scroll or a mode change. + + It localises on the **index** framebuffer — 256x240 `u16`s of + `(emphasis << 6) | colour`, the PPU's own per-pixel output before the palette + lookup — which is half the bytes and at least as sensitive, since the RGBA + buffer is a pure function of it given the same palette. + + Three answers, and the third is the point: `Identical`, `Differs`, and + **`Inconclusive`** for an exhausted budget or two trials that cannot be + compared. The Latency Oracle's precedent applies directly: "I stopped looking" + must not arrive wearing the same shape as "they agree". The budget is checked + up front for all four trials, so spending two on detection and then finding the + localisation pair unaffordable cannot consume the budget that would have + answered the question. + + Beyond locating a difference, the Lens **explains** it. Trial-scoped + provenance capture lets a located pixel be handed to the machinery that already + answers "what wrote this, and from which instruction", so the answer is a cause + rather than a coordinate — and it closes v2.3.8 item B without bisection. An + **audio** lens resolves a divergence to the CPU cycle, the cadence at which the + mix is genuinely computed. + + One defect was found and fixed inside the same work: the Lens left the emulator + **thirty frames ahead** of where it started. A trial restores the anchor on the + way in and not on the way out, which is deliberate — it is what lets the Lens + read the trial's final frame off `nes` directly — but the outermost caller has + to put the timeline back, and did not. + +- **A Latency Oracle measurement is remembered per game.** (#410.) Reopening a + game shows what was measured last time instead of an empty panel, keyed on the + ROM SHA-256 in `[input]` — the same key shape `graphics.hd_packs` already uses. + `#[serde(default)]` so an older config loads unchanged, plus + `skip_serializing_if` so the key stays out of the file until there is something + to store: a user who never opens the panel carries nothing and their config is + not rewritten. + + **Remembering is not applying.** Nothing here touches `run_ahead`, and + restoring never queues a pending apply, so a depth measured in an earlier + session is still one explicit click from being applied. An **inconclusive** + result is not remembered at all: a stored "I could not tell" is + indistinguishable from a stored answer once it has lost the context that + produced it. + +- **The two-acquisition lock race is measured rather than reasoned about.** + (#409.) The `needs_nes` render arm — taken exactly when a debugger or tool + panel is open — acquires the emulator lock twice per redraw, and drops it in + between so composite work does not hold the emulator. If the emulation thread + takes the lock in that gap, the screen shows frame N while a panel describes + N+1. + + `Nes::cycle()` is read at both acquisitions and compared: it is cumulative and + monotonic, and `produce_one_frame` holds the lock across a **whole** frame, so + any difference at all means at least one complete frame landed in the gap. + Both a hit count and a denominator are kept — "the race did not fire" and + "nothing was observed" both read as zero hits, and only the denominator + separates them. + +### Changed + +- **The accuracy battery runs at review time, scoped by path.** (#408.) `setup` + computed one `full` flag and `test-roms` ran only when it was true, so a + regular feature PR never ran the battery and an accuracy regression could not + be caught on the PR that caused it — #403 is the worked example. A second + `paths-filter` output covers the chip crates, core, `rustynes-gamedb`, the test + harness and `tests/`, and `test-roms` now runs when it *or* the existing full + flag is true. `rustynes-gamedb` is included for a non-obvious reason: it + rewrites the iNES header on load, so it changes what the emulator *is* before a + cycle runs. + +- **Provisioning steps are bounded, not just the jobs.** (#408, #409.) The + cross-compile gate's `apt-get update && apt-get install` were network fetches + with no timeout of their own, so a stalled mirror hung until the job timeout + fired and the run was reported as cancelled rather than as what it was — four + times during the v2.3.7 cut. Now one bounded, thrice-retried helper, with + elevation outside `timeout` so a killed fetch cannot orphan `apt-get` holding + the dpkg lock. + ## [2.3.7] - 2026-08-19 - "Overtone" (the instruction behind every mixed cycle) An *overtone* is the structure inside a sound that a single pitch reading throws diff --git a/crates/rustynes-frontend/src/config.rs b/crates/rustynes-frontend/src/config.rs index fa99032a..c8ce40f8 100644 --- a/crates/rustynes-frontend/src/config.rs +++ b/crates/rustynes-frontend/src/config.rs @@ -150,15 +150,25 @@ pub struct InputConfig { /// run-ahead depth, which is the field directly above; a measurement and the /// setting it recommends belong together. /// - /// `#[serde(default)]` = empty, so a pre-v2.3.9 config round-trips - /// byte-identically and a user who never opens the panel carries nothing. + /// `#[serde(default)]` = empty, so a pre-v2.3.9 config LOADS unchanged, and + /// `skip_serializing_if` keeps the key out of the file entirely until there + /// is something to store — so a user who never opens the panel carries + /// nothing and their config round-trips byte-identically. + /// + /// The two halves are separate guarantees and only one of them is + /// `default`'s. `#[serde(default)]` alone covers LOADING a file that lacks + /// the key; on SAVE the TOML serializer still emits an empty + /// `[input.latency_reports]` table, so the first save after upgrading would + /// rewrite the file. Review on #414 caught the original claim here, and it + /// was settled by serializing a default `Config` and grepping the output + /// rather than by re-reading the derive. /// /// **Remembering a measurement is not applying it.** Nothing here changes /// `run_ahead`; the panel still requires an explicit Apply, which is the /// separation v2.3.6 built deliberately ("measured" and "applied" are two /// auditable steps). Persisting the recommendation would quietly convert one /// into the other across a restart. - #[serde(default)] + #[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")] pub latency_reports: std::collections::BTreeMap, /// v1.1.0 beta.1 (T-110-B2) — turbo/autofire on the A button: while held, A /// rapid-fires. Off by default (`false`) = byte-identical input. Applied @@ -851,16 +861,24 @@ pub struct GraphicsConfig { #[serde(default)] pub shader_stack: crate::shader_pass::ShaderStackConfig, /// v1.2.0 C2 — saved named shader-stack presets (the CRT preset bank + - /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config is - /// byte-identical. Persisted under `[graphics.shader_presets]`. + /// user-saved stacks). `#[serde(default)]` = empty, so a pre-C2 config LOADS + /// unchanged. Persisted under `[graphics.shader_presets]`. Same correction as + /// `hd_packs` below: `serde(default)` says nothing about what SAVE writes. #[serde(default)] pub shader_presets: crate::shader_pass::ShaderPresetBank, /// v1.2.0 beta.2 (Workstream C3) — per-game HD-pack paths, keyed on the /// ROM SHA-256 (hex). When the loaded ROM's hash has an entry here AND the /// `hd-pack` feature is built in, the frontend loads the referenced pack /// (folder or `.zip`) and substitutes hi-res tiles at blit time. Empty by - /// default and `#[serde(default)]`, so a pre-C3 config is byte-identical - /// and the default presentation is unchanged. Presentation-only. + /// default and `#[serde(default)]`, so a pre-C3 config LOADS unchanged and + /// the default presentation is unchanged. Presentation-only. + /// + /// Deliberately says "loads", not "is byte-identical": `serde(default)` is a + /// LOAD guarantee only, and on save the TOML serializer emits an empty + /// `[graphics.hd_packs]` table. Left as-is rather than given a + /// `skip_serializing_if` like `input.latency_reports`, because that would + /// change the file this shipped feature writes; the wrong half here was the + /// claim, not the behaviour. #[serde(default)] pub hd_packs: std::collections::BTreeMap, /// v1.5.0 "Lens" Workstream D1 — per-side overscan crop, in NES pixels. The @@ -2130,9 +2148,65 @@ fn canonicalize_pad(pad: &PadBindings) -> PadBindings { #[cfg(test)] mod tests { + use super::*; use tempfile::TempDir; + /// `#[serde(default)]` covers LOADING a config that lacks the key. It says + /// nothing about SAVING, and the TOML serializer emits an empty table for an + /// empty map — so a user who never opened the Latency Oracle would have had + /// their config rewritten with a bare `[input.latency_reports]` on the first + /// save after upgrading. Review on #414 caught the claim; this pins the fix. + /// + /// Both directions are asserted, because a `skip_serializing_if` that is too + /// eager would silently discard real measurements. + #[test] + fn an_empty_latency_map_is_not_written_but_a_populated_one_is() { + let empty = toml::to_string_pretty(&Config::default()).expect("serialize default"); + assert!( + !empty.contains("latency_reports"), + "an empty map was written to the config, so an untouched file does \ + not round-trip:\n{empty}" + ); + + let mut c = Config::default(); + c.input.latency_reports.insert( + "deadbeef".to_owned(), + RememberedLatency { + frames: 3, + unanimous: true, + frame_micros: 16_639, + }, + ); + let filled = toml::to_string_pretty(&c).expect("serialize populated"); + assert!( + filled.contains("latency_reports"), + "a real measurement was dropped on save:\n{filled}" + ); + assert!(filled.contains("deadbeef"), "the ROM key was not written"); + + // Round-trip both, because the string checks alone verify the KEY and + // say nothing about the VALUE. `skip_serializing_if` on a field whose + // `Deserialize` had drifted would still produce the right text and load + // back as something else, and it is the load side the documentation + // promises. (Review on #414.) + let empty_back: Config = toml::from_str(&empty).expect("empty config re-parses"); + assert!( + empty_back.input.latency_reports.is_empty(), + "the omitted key did not come back as an empty map" + ); + let filled_back: Config = toml::from_str(&filled).expect("populated config re-parses"); + assert_eq!( + filled_back.input.latency_reports.get("deadbeef").copied(), + Some(RememberedLatency { + frames: 3, + unanimous: true, + frame_micros: 16_639, + }), + "the measurement did not survive a save/load round trip" + ); + } + #[test] fn parse_pal_reads_64_colours_and_rejects_short() { // A 192-byte file → 64 RGB triples, in order. diff --git a/to-dos/plans/v2.3.9-crucible-plan.md b/to-dos/plans/v2.3.9-crucible-plan.md index 0251073d..ca3a1c95 100644 --- a/to-dos/plans/v2.3.9-crucible-plan.md +++ b/to-dos/plans/v2.3.9-crucible-plan.md @@ -431,6 +431,63 @@ Scoped here rather than left implicit, and explicitly lower priority than A5/B: Both are additive and neither touches the deterministic core. +## Item D — the CHANGELOG gate, measured and REJECTED as specified + +Not planned; found while working. `[Unreleased]` was **empty** while three merged +PRs carried user-visible change, including a whole new tool panel: + +| PR | landed | CHANGELOG entry | +| --- | --- | :---: | +| #407 | the Divergence Lens (v2.3.8 "Parallax" marquee) | none | +| #409 | the two-acquisition lock-race measurement | none | +| #410 | per-game Latency Oracle persistence | none | + +So the file that is supposed to be the single source of truth for user-visible +change did not mention a new tool panel. Module 40 says to add the entry in the +same PR and to enforce it with a quality gate, and there is no such gate here. + +### The obvious gate, checked against history rather than argued + +**Proposal:** a PR touching `crates/*/src/**` must also touch `CHANGELOG.md`. + +**Measured** over the last 50 first-parent merges on `main`: it would have gone +red on **8**. Of those, **3** are the genuine misses above. The other **5** are +this repo's normal workflow: + +| merge | why the gate is wrong about it | +| --- | --- | +| `feat(v2.3.6)`: the probe engine + Latency Oracle | entry written in the v2.3.6 cut PR | +| `feat(frontend)`: the Latency Oracle + menu regroup | same | +| `feat`: the RAM Atlas | same | +| `perf(frontend)`: throttle-oscillation instrument | same | +| `docs`: specs + user guide for the v2.3.6 tools | touched only `//!` preambles | + +**A 62% false-positive rate against the project's own history.** Features land +bare and the release-cut PR composes the whole section at once. That is a +deliberate workflow, not sloppiness, and a gate that fires on it would be +suppressed within a week — which is worse than no gate, because a suppressed gate +still reads as coverage. + +### What the measurement actually shows + +The failure mode is not "a feature landed without an entry". It is **"a release +was never cut"**: v2.3.8 "Parallax" landed its marquee and was never tagged, so +the cut PR that would have written the entry never ran, and v2.3.9 opened on top +of an empty `[Unreleased]`. No per-PR gate addresses that, because at the time +each PR merged nothing was wrong. + +**Rejected as specified, and recorded with its numbers** per the convention +`docs/performance.md` sets for rejected optimizations. What was done instead is +the thing the situation actually needed: the section is **backfilled now**, while +the reasons are still recoverable from the commit bodies rather than reconstructed +from diffs at cut time. + +**What would justify revisiting it:** a gate keyed on the *cut*, not the PR — for +example, refusing to tag when `[Unreleased]` is empty while the workspace version +is behind the newest plan doc. That is a release-ceremony check (module 70), it +has no false-positive surface on ordinary PRs, and it targets the mechanism that +actually failed here. Not built in this release; named so it reads as a decision. + ## Item E — the ROM-transition sweep, and the one it found The v2.3.6 hook `DebuggerOverlay::clear_rom_bound_analysis` exists because panel