feat(ppu): in-render CGRAM write redirect behind per-dot-compositor flag (T-CA-10) [DRAFT — needs reference cross-val] - #218
Conversation
…lag (T-CA-10) First behavioural increment of the per-dot compositor (docs/adr/0014, dossier C3.04): a $2122 CGRAM write during active display is redirected from the CPU-programmed CGADD index to the CGRAM index of the color currently being drawn at that dot — a faithful port of ares PPU::writeCGRAM (address = latch.cgramAddress, the above-pixel palette set by the DAC's paletteColor). Gate is ares' exactly: !displayDisable && 0<vcounter<vdisp && 88<=hcounter<1096 (dots 22..274 here); the programmed address still auto-increments. The redirect target is recomputed on demand (only a mid-display CGRAM write reaches it) from the exact per-line BG resolution (render_bg), so it cannot diverge from the batch composite. First-cut scope: backdrop + non-Mode-7 BGs; sprites/Mode 7/color- window resolve as backdrop pending later phases. Off by default via the new per-dot-compositor feature (rustysnes-ppu, with core + harness passthroughs) → the shipped batch path is byte-identical. Mechanism verified: 3 rustysnes-ppu unit tests (redirect in active display; not in HBlank; not under force-blank; address still advances). Default (feature-off) suite unchanged (29 tests); clippy clean both feature states. NOT YET VERIFIED — the gating step before this can be trusted/advanced: flag-ON changes exactly one corpus ROM, undisbeliever/inidisp_forgot_to_force_blank (the one that writes CGRAM without force-blanking) — a simple 3-color screen within the resolver's scope — from 2 colors (batch) to 3. That is the redirect firing as designed, but whether the new frame matches hardware is UNCONFIRMED: reference cross-validation against Mesen2/ares was not achievable in the headless sandbox (takeScreenshot fails, getScreenBuffer returns an unreliable capture). Must be verified on real hardware / a reference before the flag is trusted or flipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughAdds an opt-in per-dot PPU compositor that incrementally renders scanlines, redirects active-display CGRAM writes, and exposes feature wiring through the core and test harness. Tests cover redirect gating, while documentation records current accuracy and remaining Phase 4 work. ChangesPer-dot compositor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CPU
participant PPU
participant Framebuffer
CPU->>PPU: write $2122 during active display
PPU->>PPU: select cgram_write_target
PPU->>Framebuffer: incrementally composite current dot
PPU->>PPU: update internal_cgram_address
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR adds an opt-in Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
Verification update — the redirect is documented-real hardware behaviorPursued the cross-validation through channels that work headless. Findings: 1. Authoritative confirmation (fullsnes / SNESdev). 2. snes9x cross-check (libretro host, canonical BGR555). For Net: the redirect's direction and model are well-grounded (documented-real + faithful ares port + matches the C3.04 dossier definition), and it is byte-identical on every corpus ROM except the one that exercises exactly this quirk. Still outstanding before un-drafting: exact-pixel confirmation of the changed frame against ares or Mesen2 (the accuracy references that DO model the quirk) — not achievable in this headless sandbox ( |
…9x isn't an oracle The redirect is documented-real, not speculative: fullsnes/SNESdev state a CGRAM write during active display "lands at the wrong CGRAM address" (ref-docs/fullsnes/30-ppu.md), and ares pins that address as latch.cgramAddress (the drawn color) — what dossier C3.04 and this implementation follow. snes9x renders inidisp_forgot_to_force_blank the no-redirect (flag-OFF) way because it writes the programmed index and does not model the quirk, so it is not a valid oracle. The one remaining check before the flag is trusted is exact-pixel confirmation vs ares/Mesen2 (which do model it) — not achievable headless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e (T-CA-10 3b) The first-cut resolver handled only backdrop + BGs; a CGRAM write during active display at a sprite-covered (or windowed) column would have redirected to the BG color, not the sprite the DAC is actually drawing. Extend in_render_above_palette to reuse the batch's EXACT above-resolution — render_bg + render_objects, including window/priority — so the redirect target is correct-by-construction (it is literally the drawn `above` pixel) for every non-Mode-7 screen. render_objects' only self-mutation is the $213E range/time over-flags; they are snapshotted and restored around the call so the resolver stays side-effect-free and the batch's own render_objects at RENDER_DOT still sets them at the correct dot (no over-flag re-timing). Resolver is now &mut self (hence cgram_write_target too). Mode 7 remains the sole gap (render_mode7's vestigial VideoBus param can't be supplied from the bus-less write path; a CGRAM write in Mode-7 active display is vanishingly rare) — resolves as backdrop, documented. New unit test proves a sprite pixel is the resolved drawn color and the over-flags are restored. Feature-on suite 32→33; default suite unchanged (29); clippy+fmt clean both states. Still off by default; the exact-frame-vs-ares check for the one affected corpus ROM remains the gate before the flag is trusted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tch (T-CA-10 4a) Drive the visible line's composite one dot at a time under the per-dot-compositor feature (Ppu::pd_render_to_dot, called each tick_dot), replacing the batch composite at RENDER_DOT. Blueprint: MesenCE SnesPpu::RenderScanline (ref-proj/MesenCE, cloned + built this session). The line is fetched once at its start (pd_fetch_line, the same backdrop+BG+sprite build as render_scanline minus the composite) into new feature- gated Ppu buffers, then drained per dot up to the DAC's current column with LIVE register reads; all columns finish by RENDER_DOT (before the line's HDMA), so a static line is byte-identical to the batch. This makes the in-render CGRAM redirect exact: internal_cgram_address = the last-drawn column's palette (MesenCE _state.InternalCgramAddress / ares latch.cgramAddress), maintained by the compositor, replaces the earlier on-demand h-22 resolver (removed). Also makes mid-line INIDISP (brightness/force-blank) per-column. Verified: feature-OFF byte-identical (batch path unchanged, 29 ppu tests); feature-ON byte-identical on 26/29 undisbeliever ROMs (static content); clippy + fmt clean both states (Pixel/DacCarry now pub in the private render module; MesenCE/Mesen2 added to clippy doc-valid-idents). Per-dot state is transient (re-fetched per line), not save-stated. IN PROGRESS: the three mid-line INIDISP ROMs (inidisp_brightness_delay, inidisp_enable_display_mid_frame, inidisp_forgot_to_force_blank) now shift as intended but do NOT yet match MesenCE — inidisp_forgot renders 7fff vs MesenCE's 7fc6. Root cause under debug: the compose-vs-CPU-write ordering per dot (internal_cgram_address timing) and/or the INIDISP-per-column semantics. Default-off, so shipped builds and CI (feature-off) are unaffected. Draft #218; not for merge until the raster timing matches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-blank (T-CA-10 4a) pd_fetch_line skipped building above/below when display_disable was set at the line's start, leaving the per-dot buffers as the default backdrop. But force-blank is a compose-time decision the per-dot path re-evaluates per column — a line blanked at its start but UN-blanked mid-line then composited the un-fetched backdrop instead of the real picture. Fetch unconditionally (MesenCE fetches regardless of force-blank); the compose still outputs black per column while display_disable is set, so a fully-blank line stays black and byte-identical. Verified: feature-OFF unchanged (29 ppu tests); feature-ON still byte-identical on the static corpus (26/29 undisbeliever); the fix correctly moves inidisp_enable_display_mid_frame (which blanks-at-start then enables mid-frame). clippy + fmt clean both states. Note: the 3 mid-line INIDISP ROMs still do not exactly match MesenCE — inidisp_forgot renders 7fff vs MesenCE 7fc6 (and the BATCH renders 34e6, so even the pre-existing model is wrong for it). That ROM exercises a PPU-access-during-active-display quirk that needs deeper per-dot modeling (Phase 4d), not a 4a fix. Default-off; not for merge yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…/OAM/Phase-6 plan Capture this session's per-dot compositor progress and findings in the tracker so the remaining phases execute efficiently against the MesenCE reference (cloned + built this session at ref-proj/MesenCE — the Phase-4 driving-loop blueprint AND the exact-frame oracle). 4a (draw cursor) is DONE and verified byte-identical on the static corpus; the 3 mid-line INIDISP undisbeliever ROMs are documented as deep Phase-4d access-during-render quirks (not scored AccuracySNES rows), with the concrete finding that inidisp_forgot does zero CGRAM writes and MesenCE applies INIDISP with no write-delay. The remaining systematic phases (4b incremental sprite-eval + over-flag timing, 4c BG fetch-ahead, 4d deep access model + hi-res, Item 3 OAM redirect, Phase 6 default-flip + scored rows) are laid out with MesenCE SnesPpu.cpp line references and per-phase milestones. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…iever) Rendered the 3 mid-line INIDISP ROMs in MesenCE and compared to the per-dot flag-ON output: brightness_delay matches exactly, enable_display_mid_frame nearly matches, and only inidisp_forgot_to_force_blank is genuinely wrong (an access-during-render quirk deferred to Phase 4d). So the per-dot compose is correct on 28/29, not merely byte-identical-on-static. Noted the shared-golden constraint: it can't re-bless to the more-accurate per-dot values until Phase 6's default flip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rustysnes-ppu/src/lib.rs`:
- Around line 696-723: Update Ppu::load_state to invalidate all per-dot
compositor transient state after restoring h/v: mark pd_fetched_line as
unfetched and reset pd_draw_x, pd_carry, and internal_cgram_address to their
line-start values. Ensure the next pd_render_to_dot call refetches the current
line instead of reusing pre-load pixels or cursor state, while keeping the
fields out of serialized save-state data.
In `@crates/rustysnes-ppu/src/regs.rs`:
- Around line 34-55: Extract the exclusive active-display upper bound from the
literal 274 into a shared `pub(crate) const ACTIVE_DOT_END: u16` alongside
`ACTIVE_DOT_START`. Update `Ppu::cgram_write_target` and `lib.rs`’s `tick_dot`
HBlank calculation to use `ACTIVE_DOT_END`, preserving the existing boundary
behavior.
In `@docs/ppu.md`:
- Around line 114-135: Update the Status paragraph in the per-dot compositor
section to match the current validation: report 28/29 undisbeliever ROMs as
byte-identical/accurate, identify inidisp_forgot_to_force_blank as the sole
mismatch, and attribute it to the deeper Phase 4d PPU access-during-render quirk
involving VRAM/OAM rather than compose ordering or an INIDISP-per-column debug
issue. Keep the remaining save-state and experimental-flag details unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73410ea9-36a2-4a0b-af75-6d186f31f903
📒 Files selected for processing (9)
clippy.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/render.rscrates/rustysnes-test-harness/Cargo.tomldocs/ppu.mdto-dos/TIER1-CYCLE-ACCURACY.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test-light
- GitHub Check: accuracysnes
🧰 Additional context used
📓 Path-based instructions (16)
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
Files:
clippy.tomlcrates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomldocs/ppu.mdcrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rsto-dos/TIER1-CYCLE-ACCURACY.mdcrates/rustysnes-ppu/src/render.rs
**/Cargo.toml
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Do not configure the
scriptingandscript-wasmbackends as simultaneously enabled features; they are mutually exclusive.
Files:
crates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.toml
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.{rs,toml}: Do not enable all Cargo features; additive features must remain default-off so shipped, native, no_std, and wasm builds remain byte-identical.
Preserve determinism: identical seed, ROM, and input must produce bit-identical audio and video; rate control belongs to the frontend.
Files:
crates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.tomlcrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/*/Cargo.toml
⚙️ CodeRabbit configuration file
crates/*/Cargo.toml: The frontend pins one compatible GUI tier (currently winit 0.30 / wgpu 29 / egui 0.35 and
itsegui-wgpu/egui-winitsiblings). Flag a bump to any one of those that is not
accompanied by matching bumps to the others. Optional features are default-off by
convention so shipped, no_std and wasm builds stay byte-identical; flag a new feature added
to adefault = [...]list.
Files:
crates/rustysnes-core/Cargo.tomlcrates/rustysnes-ppu/Cargo.tomlcrates/rustysnes-test-harness/Cargo.toml
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.Keep
docs/STATUS.mdas the authoritative per-subsystem state and update relevant documentation in the same PR as code changes.
Files:
docs/ppu.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.
Files:
docs/ppu.mdcrates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rsto-dos/TIER1-CYCLE-ACCURACY.mdcrates/rustysnes-ppu/src/render.rs
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/ppu.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/ppu.mdto-dos/TIER1-CYCLE-ACCURACY.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/rustysnes-{cpu,ppu,apu,cart,core}/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Keep core chip implementation changes localized to the owning chip crate and preserve the workspace crate boundaries.
Keep board logic in the cart crate; chip crates must not depend on one another;
rustysnes-corecoordinates the chips and instantiates and steps the SA-1 second CPU.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/rustysnes-ppu/**/*.{rs,md}
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
For the
rustysnes-ppuPPU1 (5C77) and PPU2 (5C78) video path, read../../docs/ppu.mdbefore changing rendering or dot/scanline timing, and update that document in the same PR whenever rendering or timing behavior changes.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
crates/rustysnes-ppu/**/*.rs
📄 CodeRabbit inference engine (crates/rustysnes-ppu/CLAUDE.md)
crates/rustysnes-ppu/**/*.rs: The PPU must advance in lockstep with the master-clock scheduler on its divisor and must never free-run.
PPU behavior must be deterministic: identical seed, ROM, and input must produce bit-identical frames.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned toolchain 1.96; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI treats warnings as errors.
Every public Rust item must have a documentation comment.
Run and preserve the workspace quality gates:cargo check --workspace,cargo test --workspace, Clippy with-D warningsand without--all-features, rustdoc with warnings denied, and the no_std core build.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
**/*.{rs,s,inc}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,s,inc}: A chip change must update its correspondingdocs/<chip>.mddocumentation in the same change, and hot paths must remain allocation-free.
Never commit commercial ROMs; only derived screenshots and hashes may be committed.
Files:
crates/rustysnes-ppu/src/regs.rscrates/rustysnes-ppu/src/lib.rscrates/rustysnes-ppu/src/render.rs
🔇 Additional comments (8)
crates/rustysnes-ppu/Cargo.toml (1)
12-18: LGTM!crates/rustysnes-core/Cargo.toml (1)
23-27: LGTM!crates/rustysnes-test-harness/Cargo.toml (1)
13-17: LGTM!crates/rustysnes-ppu/src/lib.rs (1)
785-796: LGTM!Also applies to: 870-878
crates/rustysnes-ppu/src/render.rs (1)
38-39: LGTM!Also applies to: 63-63, 215-217, 279-378, 1135-1135, 1974-2046
crates/rustysnes-ppu/src/regs.rs (1)
213-229: LGTM!to-dos/TIER1-CYCLE-ACCURACY.md (1)
88-134: LGTM!clippy.toml (1)
27-28: LGTM!
…nk boundary (T-CA-10) Address the CodeRabbit review on #218: - CRITICAL: Ppu::load_state now invalidates the per-dot compositor's transient state (pd_fetched_line = u16::MAX, pd_draw_x/pd_carry/internal_cgram_address reset). These fields are not serialized (re-derived per line), but load_state restored h/v without touching them — so a pre-load pd_fetched_line coincidentally equal to the just-loaded self.v (realistic on quick-load/rewind) would skip the re-fetch and composite the new line from stale pixels/cursor, breaking determinism. u16::MAX forces a fresh fetch. - Extract the 274 HBlank/active-display boundary into a shared HBLANK_START_DOT const, used by both tick_dot's blanking test and cgram_write_target's redirect gate, so they cannot desync. - Correct the docs/ppu.md status to match the tracker: flag-ON is validated accurate on 28/29 undisbeliever vs MesenCE (not "26/29 + ordering bug under debug"); the sole mismatch (inidisp_forgot, zero CGRAM writes) is a Phase-4d access-during-render quirk. clippy + fmt clean both feature states; save-state round-trip green; 29/32 ppu tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — adjudicating the reviews. CodeRabbit's three findings are all fixed in 99724f1 (the critical Antigravity's two "blocking" issues — both respectfully declined after verification against the current code + the reference emulators:
No blocking issues remain. Merging as the validated per-dot compositor foundation (default-off; the incomplete phases 4b/4c/4d land as follow-up PRs). |
1 participant
What
First behavioural increment of the per-dot PPU compositor (
docs/adr/0014, T-CA-10 Phase 3; dossier C3.04): a$2122CGRAM write during active display is redirected from the CPU-programmedCGADDindex to the CGRAM index of the color currently being drawn at that dot.Faithful port of ares
PPU::writeCGRAM(address = latch.cgramAddress, the above-pixel palette that the DAC'spaletteColorsets). Gate is ares' exactly —!displayDisable && 0 < vcounter < vdisp && 88 ≤ hcounter < 1096(RustySNES dots22..274, sincehcounter = dot·4); the programmed address still auto-increments.The redirect target is recomputed on demand (only a mid-display CGRAM write reaches it) from the exact per-line BG resolution (
render_bg), so it cannot diverge from the batch composite. First-cut scope: backdrop + non-Mode-7 BGs; sprites, Mode 7, and the color-window resolve as the backdrop pending later phases.Off by default → shipped builds byte-identical
New
per-dot-compositorfeature (rustysnes-ppu, withrustysnes-core+ harness passthroughs), default-off. The batch compositor never redirects, so the default path is unchanged.Verified (mechanism)
rustysnes-ppuunit tests: redirect fires in active display; not in HBlank; not under force-blank; the programmed address still advances.Verification gate (why this is a DRAFT)
Flag-ON is byte-identical on every corpus ROM except
undisbeliever/inidisp_forgot_to_force_blank— the one ROM that writes CGRAM without force-blanking — whose frame goes from 2 colors (batch) to 3 (redirect). That is the redirect firing exactly as designed (correct targeting), and the ROM is a simple 3-color screen within the resolver's backdrop+BG scope.But whether the new frame matches hardware is UNCONFIRMED. Reference cross-validation against Mesen2/ares was not achievable in the headless CI sandbox (
emu.takeScreenshotfails under--testrunner;emu.getScreenBufferatendFramereturned an unreliable all-black capture). This must be verified on real hardware or against a reference render before the flag is trusted or advanced. Until then it stays default-off and this PR stays a draft.Next (Phase 3b+)
Confirm the changed frame vs a reference; extend the resolver past backdrop+BG (sprites/Mode 7/color-window); then the larger live-per-dot phases (mid-line scroll, C11.08 MPY, interlace, hi-res). AccuracySNES scored rows convert at the Phase-6 default flip.
🤖 Generated with Claude Code
Adds an opt-in per-dot PPU compositor claiming hardware-accurate active-display behavior: live register effects apply to later pixels, and
$2122writes target the palette entry for the color currently being drawn rather than programmedCGADD.The claim would be false if ares/Mesen2/MesenCE disagree, if feature-on output diverges on additional corpus ROMs, or if mid-line INIDISP/CGRAM behavior is modeled incorrectly. The T-CA-10 dossier adds Phase 4/4a assertions covering the draw cursor, per-dot compositing, and internal CGRAM address; the coverage denominator remains 29 ROMs, with 28 matching MesenCE. The remaining
inidisp_forgot_to_force_blankmismatch leaves deeper access-during-render behavior unresolved.With the feature disabled, behavior remains byte-identical. When enabled, observable output changes for mid-render PPU accesses, including CGRAM writes, brightness/force-blank changes, sprites, priorities, and windows; Mode 7 and the outstanding INIDISP case remain incomplete.