diff --git a/crates/rustynes-frontend/src/debugger/atlas_panel.rs b/crates/rustynes-frontend/src/debugger/atlas_panel.rs index 90405fd1..2a225ee3 100644 --- a/crates/rustynes-frontend/src/debugger/atlas_panel.rs +++ b/crates/rustynes-frontend/src/debugger/atlas_panel.rs @@ -95,6 +95,11 @@ pub struct AtlasPanel { verify_batch_requested: bool, /// The address whose detail is expanded. selected: Option, + /// An address the user asked to send to RAM Watch, with the label the atlas + /// can honestly attach to it. Drained by the overlay after this panel's + /// render, for the same reason as the two emulator actions: the destination + /// panel's state is not reachable from here. + watch_requested: Option<(u16, String)>, /// Status / cost line. status: String, } @@ -114,6 +119,7 @@ impl Default for AtlasPanel { verify_requested: None, verify_batch_requested: false, selected: None, + watch_requested: None, status: String::new(), } } @@ -129,6 +135,27 @@ impl AtlasPanel { pub fn clear(&mut self) { *self = Self::default(); } + + /// Take a pending RAM Watch export, if one was requested this frame. + /// + /// Taken rather than read so a single click seeds a single entry. The + /// alternative — leaving the request standing and de-duplicating downstream — + /// makes the panel's behaviour depend on the destination list's contents, + /// which is exactly the coupling this hand-off exists to avoid. + pub fn take_watch_request(&mut self) -> Option<(u16, String)> { + self.watch_requested.take() + } + + /// Report the outcome of an export back onto the panel's status line. + /// + /// The destination window is drawn EARLIER in the overlay's pass than this + /// one, so a seed made here does not appear over there until the next frame. + /// Without this line the user's evidence that anything happened is a list + /// they may not be looking at — an export that silently succeeds and one that + /// silently does nothing look identical. + pub fn note_export(&mut self, text: impl Into) { + self.status = text.into(); + } } /// Draw the RAM Atlas window. `nes` is `Some` only when a ROM is loaded under the @@ -438,9 +465,56 @@ fn detail(ui: &mut egui::Ui, state: &mut AtlasPanel, l: &Label, can_run: bool) { } } } + + // Offered for EVERY address, including `Untested` and `Inert`. + // + // Restricting the export to verified-live addresses would be the + // paternalistic reading of this panel's honesty rule. The rule is that a + // claim must carry its evidence, not that unverified addresses are + // unusable — an `Untested` sparse byte is a perfectly good thing to watch + // while forming a hypothesis, and `Inert` is documented here as NOT + // meaning unused. What the rule requires is that the exported entry say + // which of the three it was, which `watch_label` does. + // + // Not gated on `can_run` either: this writes to another panel's list and + // never touches the emulator, so the netplay / TAS / hardcore predicate + // that gates Observe and Verify does not apply to it. + if ui + .button(ic(glyph::EYE, "Send to RAM Watch")) + .on_hover_text( + "Adds this address to Tools -> Analysis -> Memory Compare's RAM \ + Watch list, labelled with the atlas verdict that produced it.", + ) + .clicked() + { + state.watch_requested = Some((l.addr, watch_label(l, state.lens))); + } }); } +/// The label an exported address carries into RAM Watch. +/// +/// Three things, in the order a reader needs them: the address, the behavioural +/// class, and the verification verdict **with its lens**. The lens is not +/// decoration — liveness is relative to the observable, so "live" without it is +/// the over-claim the atlas exists to avoid, and the watch list is precisely +/// where that claim would outlive the panel that qualified it. +/// +/// `Untested` is spelled out rather than omitted. An entry with no verdict and +/// an entry that was never tested look the same once the atlas is gone. +fn watch_label(l: &Label, lens: Observable) -> String { + let verdict = match l.liveness { + Liveness::Untested => "unverified".to_owned(), + Liveness::Live => format!("LIVE via {}", lens_name(lens)), + Liveness::Inert => format!("inert via {}", lens_name(lens)), + }; + format!( + "${:04X} atlas: {}, {verdict}", + l.addr, + behaviour_name(l.behaviour) + ) +} + /// The threshold that produced this label, in words. fn why(l: &Label, frames: u32) -> String { let transitions = frames.saturating_sub(1); @@ -741,6 +815,85 @@ mod tests { } } + /// The exported label must name the LENS a verdict came from. Liveness is + /// relative to the observable — a byte is `Live` through work RAM and may be + /// `Inert` through the screen — and the watch list is exactly where an + /// unqualified "LIVE" would outlive the panel that qualified it. + #[test] + fn an_exported_verdict_carries_its_lens() { + let l = label(0x0071, Behaviour::RisingCounter, Liveness::Live); + let text = watch_label(&l, Observable::Wram); + assert!(text.contains("0071"), "the address is missing: {text}"); + assert!(text.contains("rising"), "the behaviour is missing: {text}"); + assert!(text.contains("LIVE"), "the verdict is missing: {text}"); + assert!( + text.contains(lens_name(Observable::Wram)), + "the lens is missing, so the verdict over-claims: {text}" + ); + // The same label under a different lens must READ differently. Naming + // the lens is only worth anything if the name tracks the argument. + assert_ne!( + text, + watch_label(&l, Observable::Framebuffer), + "the lens name did not change with the lens" + ); + } + + /// `Untested` is spelled out rather than left blank. Once the atlas is gone, + /// an entry with no verdict and an entry that was never tested look the same, + /// which is the exact collapse this panel refuses to make elsewhere. + #[test] + fn an_unverified_export_says_so() { + let l = label(0x0300, Behaviour::Sparse, Liveness::Untested); + let text = watch_label(&l, Observable::Framebuffer); + assert!( + text.contains("unverified"), + "an untested address exported without saying so: {text}" + ); + assert!( + !text.contains("LIVE"), + "an untested address claimed a verdict: {text}" + ); + // And it must NOT name a lens: nothing was observed through one, so + // citing one would dress a hypothesis as a measurement. + assert!( + !text.contains(lens_name(Observable::Framebuffer)), + "an untested address cited a lens it never used: {text}" + ); + } + + /// The request is TAKEN, so one click seeds one entry. Left standing it would + /// re-fire every frame until the destination happened to de-duplicate it, + /// which makes this panel's behaviour depend on another panel's contents. + #[test] + fn a_watch_request_is_drained_by_the_first_taker() { + let mut p = AtlasPanel { + watch_requested: Some((0x0071, "atlas".to_owned())), + ..AtlasPanel::default() + }; + assert_eq!(p.take_watch_request(), Some((0x0071, "atlas".to_owned()))); + assert_eq!( + p.take_watch_request(), + None, + "the request survived being taken and would seed a second entry" + ); + } + + /// A ROM change must discard a pending export too. An address is a fact about + /// one cartridge's memory map; seeding it into a watch list after a different + /// game has loaded is the same class of stale claim as keeping the labels. + #[test] + fn clearing_discards_a_pending_export() { + let mut p = AtlasPanel { + watch_requested: Some((0x0071, "atlas".to_owned())), + status: "stale".to_owned(), + ..AtlasPanel::default() + }; + p.clear(); + assert_eq!(p.take_watch_request(), None); + assert!(p.status.is_empty()); + } + /// A ROM change must discard the whole atlas. Two thousand labels describing /// a cartridge that is no longer loaded is a worse lie than one stale number, /// because it looks like a map. diff --git a/crates/rustynes-frontend/src/debugger/memory_compare_panel.rs b/crates/rustynes-frontend/src/debugger/memory_compare_panel.rs index 53cdba13..ce6a49bb 100644 --- a/crates/rustynes-frontend/src/debugger/memory_compare_panel.rs +++ b/crates/rustynes-frontend/src/debugger/memory_compare_panel.rs @@ -112,6 +112,18 @@ impl CompareTo { } } +/// What [`MemoryComparePanelState::seed_watch`] did. +/// +/// Two outcomes rather than a `bool`, so a caller's status line cannot describe +/// one as the other. Neither is an error. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum SeedOutcome { + /// A new entry was appended. + Added, + /// An entry for that address was already present; the list is unchanged. + AlreadyWatched, +} + /// One RAM Watch entry. #[derive(Debug, Clone)] struct WatchEntry { @@ -242,6 +254,38 @@ impl MemoryComparePanelState { self.watch_addr_text.clear(); self.watch_label_text.clear(); } + + /// Add a watch entry on behalf of another panel. + /// + /// The RAM Atlas classifies every byte of work RAM but deliberately offers + /// no way to keep an address once the panel is closed — an atlas is a + /// snapshot of one observation window, and it is cleared at every ROM + /// transition. RAM Watch is where an address the user cares about already + /// lives, so the export target is this list rather than a second one. + /// + /// `Size::U8` is not a parameter: the atlas classifies **bytes**, and its + /// evidence (change count, direction, range) is per byte. Seeding a `u16` + /// from a byte-scoped verdict would attach that evidence to a second address + /// nothing was ever observed about. The user can widen the entry afterwards, + /// which is a decision they have made rather than one made for them. + /// + /// Returns which of the two things happened, because they are different and + /// the caller has to say which. A second click on an address already watched + /// must not report "added" (the list did not grow) and must not report a + /// failure (nothing went wrong) — the same refusal to collapse distinct + /// outcomes that keeps `Untested` and `Inert` apart in the atlas itself. + pub fn seed_watch(&mut self, addr: u16, label: String) -> SeedOutcome { + if self.watches.iter().any(|w| w.addr == addr) { + return SeedOutcome::AlreadyWatched; + } + self.watches.push(WatchEntry { + addr, + size: Size::U8, + label, + frozen: None, + }); + SeedOutcome::Added + } } /// Copy the 2 KB work RAM via the (logically side-effect-free) peek. @@ -581,6 +625,44 @@ fn parse_wch(text: &str) -> Vec { mod tests { use super::*; + /// The export path from the RAM Atlas. A second click on the same address + /// must be reported as "already there", never as an addition — the list did + /// not grow, and a status line saying it did is a false report of work done. + #[test] + fn seeding_the_same_address_twice_adds_one_entry() { + let mut s = MemoryComparePanelState::default(); + assert_eq!(s.seed_watch(0x0071, "first".to_owned()), SeedOutcome::Added); + assert_eq!(s.watches.len(), 1); + assert_eq!( + s.seed_watch(0x0071, "second".to_owned()), + SeedOutcome::AlreadyWatched + ); + assert_eq!(s.watches.len(), 1, "a duplicate address grew the list"); + assert_eq!( + s.watches[0].label, "first", + "the duplicate overwrote the original entry's label" + ); + } + + /// A seeded entry is byte-scoped, because the atlas's evidence is per byte. + /// Seeding a `u16` would attach a byte's verdict to a second address nothing + /// was ever observed about. + #[test] + fn a_seeded_entry_is_one_byte_and_unfrozen() { + // The panel's own size selector is deliberately set to something else: + // the seed must not inherit whatever the user last picked in the Add row. + let mut s = MemoryComparePanelState { + watch_size: Size::U32, + ..MemoryComparePanelState::default() + }; + s.seed_watch(0x0300, "atlas".to_owned()); + assert_eq!(s.watches[0].size, Size::U8); + assert!( + s.watches[0].frozen.is_none(), + "an exported watch arrived frozen, which would write to the game" + ); + } + #[test] fn op_predicates() { assert!(Op::Eq.apply(5, 5)); diff --git a/crates/rustynes-frontend/src/debugger/mod.rs b/crates/rustynes-frontend/src/debugger/mod.rs index 760cfa1c..e20916ac 100644 --- a/crates/rustynes-frontend/src/debugger/mod.rs +++ b/crates/rustynes-frontend/src/debugger/mod.rs @@ -2305,6 +2305,30 @@ impl DebuggerOverlay { nes.as_deref_mut(), atlas_writes_locked, ); + // The atlas -> RAM Watch export (v2.3.9 item C). Dispatched here + // rather than inside the panel because the destination is another + // panel's private state, and the overlay is the only place that holds + // both. + // + // The destination window is opened as part of the export. Landing an + // entry in a list the user cannot see is the shape of defect this + // release keeps finding: it succeeds, reports nothing, and is + // indistinguishable from having done nothing at all. Memory Compare + // is drawn EARLIER in this pass, so the seeded row itself appears on + // the next frame — which is why the outcome is also reported on the + // atlas's own status line, where the user is already looking. + if let Some((addr, label)) = self.atlas_ui.take_watch_request() { + let outcome = self.memory_compare_ui.seed_watch(addr, label); + self.show_memory_compare = true; + self.atlas_ui.note_export(match outcome { + memory_compare_panel::SeedOutcome::Added => { + format!("${addr:04X} added to RAM Watch (Memory Compare).") + } + memory_compare_panel::SeedOutcome::AlreadyWatched => { + format!("${addr:04X} was already in the RAM Watch list.") + } + }); + } } // v2.3.8 "Parallax" — the Divergence Lens. Gated on the SAME predicate as // the RAM Atlas above, and for identical reasons rather than by analogy: diff --git a/docs/ram-atlas.md b/docs/ram-atlas.md index c9da72f9..72362d06 100644 --- a/docs/ram-atlas.md +++ b/docs/ram-atlas.md @@ -240,16 +240,67 @@ measurement. - An atlas is ROM-bound and is discarded on every ROM transition via `DebuggerOverlay::clear_rom_bound_analysis`. 2048 stale labels are a worse lie than one stale number, because they look like a map. +- **Send to RAM Watch** exports the selected address into Memory Compare's watch + list. See below. + +## Export to RAM Watch + +An atlas is deliberately discarded at every ROM transition, which is correct and +has a cost: the address a user just *verified* by spending trials on it +disappears with everything else. `Send to RAM Watch` is the way out — the +destination is Memory Compare's existing watch list rather than a second list, +because that is already the tool that keeps addresses. + +Four properties, each of which is the honesty rule applied to a different edge: + +- **The label carries the verdict AND its lens.** Liveness is relative to the + observable (`## Liveness is relative to its lens` above), so an unqualified + "LIVE" in a list that outlives this panel is exactly the over-claim the panel + exists to avoid. `Untested` is spelled out rather than left blank — once the + atlas is gone, an entry with no verdict and an entry that was never tested look + identical — and an untested export names **no** lens, because nothing was + observed through one. +- **Every address is exportable**, including `Inert` and `Untested`. Restricting + the button to verified-live addresses would be the paternalistic reading of the + rule; the rule is that a claim carries its evidence, not that unverified + addresses are unusable. An `Untested` sparse byte is a good thing to watch while + forming a hypothesis, and `Inert` does not mean unused (`## What a label does + not mean`). +- **The entry is byte-scoped and unfrozen.** The atlas's evidence is per byte, so + seeding a `u16` would attach a byte's verdict to a second address nothing was + observed about; the user can widen it afterwards, which is a decision they made. + A frozen watch entry writes to the game, and an export must not. +- **A duplicate is reported as a duplicate.** A second click on an address already + watched did not grow the list. `SeedOutcome` keeps "added" and "already there" + apart, because reporting either as the other is a false report — the same + refusal that keeps `Untested` and `Inert` apart. + +The button is **not** gated on the netplay / TAS / hardcore predicate that gates +Observe and Verify (`## Unavailable during locked sessions`): it writes to another +panel's list and never advances the emulator. + +Memory Compare is drawn earlier in the overlay's pass than the atlas, so the +seeded row itself appears on the **next** frame. The outcome is therefore also +reported on the atlas's own status line, where the user is already looking — an +export that silently succeeds and one that silently does nothing are otherwise +indistinguishable. ## Deliberately not implemented Named here so they read as decisions rather than oversights: -- **Export paths** — seeding the Watch and Cheat panels, the Lua API, and - RetroAchievements authoring. Additive on top of the labels; better shaped once - the labels have been used in anger. -- **Per-game persistence** — an atlas keyed on the ROM hash via the - `rustynes-gamedb` overlay. +- **The remaining export paths** — the Cheat panel, the Lua API, and + RetroAchievements authoring. RAM Watch is implemented (above); the other three + are additive on top of the same request/dispatch shape and better decided once + the first one has been used in anger. A cheat is also a *write*, so it needs the + locked-session predicate the watch export correctly does without. +- **Per-game persistence** — an atlas keyed on the ROM hash. The obvious form + is unsafe: a restored verdict without its evidence is a claim that cannot be + checked, and this panel's whole argument is that its output can be. A restored + verdict *with* its evidence is still a statement about the game state the + observation ran in, which the next session does not share. The RAM Watch export + above is the durable path in the meantime, and RAM Watch already has `.wch` + save and load. - **Coordinate cross-referencing against OAM** — the original design sketched correlating candidate addresses against sprite X/Y. It is not implemented, and the `Behaviour` set makes no coordinate claim, so nothing currently over-states