docs(ppu): identify the inidisp gap's exact model difference; correct C3.04 → C3.12 - #327
Conversation
… C3.04 -> C3.12
The last per-dot framebuffer gap was documented only as "an
internal_cgram_address draw-ordering detail". The concrete difference:
MesenCE updates InternalCgramAddress inside GetRgbColor, called ONLY
when `color > 0` (SnesPpu.cpp:1114) and PER LAYER during
tilemap render -- so a transparent pixel leaves the previous
opaque column's value standing, and several layers may update
it within one column.
RustySNES assigns it once per column, UNCONDITIONALLY, from the
COMPOSITED pixel at the draw cursor.
Structurally different models, not an off-by-one.
Gate-on-opaque is NOT the fix, and this was measured rather than assumed:
wrapping the assignment in `if ap.opaque` moves the ROM's hash from
0xaeb678a4165b28c5 to 0xa55bd66a1e6dd125 -- still not the
MesenCE-agreeing golden -- because gating the COMPOSITE is not the same
as gating each LAYER FETCH.
CORRECTION to a recorded claim: the AccuracySNES row that breaks under
gate-on-opaque is C3.12 ("CGRAM taken in render"), NOT C3.04 ("H counter
advances"), which passes. C3.12 is the row that asserts the redirect
target directly, so its failure means the cart row and MesenCE's model
disagree about a backdrop column -- that needs adjudicating before either
is changed.
The real fix tracks the redirect target in the FETCH stage, per layer,
updating only for a non-zero colour index in MesenCE's layer order. That
is a Phase 4c-shaped change, which is why it is still open.
Docs only; render.rs restored byte-clean from a pre-edit snapshot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe changelog and PPU specification document the ChangesCGRAM redirect documentation
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
Antigravity review (Gemini via Ultra)This PR updates Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
There was a problem hiding this comment.
Pull request overview
Documents the exact behavioral-model mismatch behind the remaining inidisp_forgot_to_force_blank per-dot framebuffer gap, and corrects the previously recorded AccuracySNES row attribution (C3.04 → C3.12). This fits the codebase by tightening the PPU behavior spec/docs without changing emulator behavior.
Changes:
- Expand
docs/ppu.mdwith the identified model difference (MesenCE per-layer,color > 0gating vs RustySNES per-column composited assignment) and record why “gate-on-opaque” is not equivalent. - Add an Unreleased changelog entry summarizing the above and recording the
C3.12correction.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/ppu.md | Adds a detailed, dated explanation of the INIDISP/CGRAM-redirect model difference and corrects the AccuracySNES row attribution. |
| CHANGELOG.md | Records the documentation finding/correction in the Unreleased notes. |
| column. RustySNES assigns it **once per column, unconditionally, from the composited pixel** at | ||
| the draw cursor (`render.rs`, `pd_draw_columns`). Those are structurally different models, not an |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/ppu.md`:
- Around line 169-175: Revise the earlier redirect discussion in docs/ppu.md
around the descriptions of the last-drawn column and draw-cursor/fetch-ahead
alignment. Remove claims that MesenCE uses compositor-level composited-column
state or that the mismatch is solely cursor alignment, and make the prose
consistent with the authoritative per-layer, transparent-pixel-retaining model
while keeping RustySNES’s once-per-column composited assignment distinct.
🪄 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: 9a1e6491-ca76-4f74-a878-e743257ca0de
📒 Files selected for processing (2)
CHANGELOG.mddocs/ppu.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (7)
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/.
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.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/ppu.mdCHANGELOG.md
**/*
📄 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.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/ppu.mdCHANGELOG.md
docs/**/*
📄 CodeRabbit inference engine (docs/testing-strategy.md)
Chip crates should exceed 90% unit-test coverage, and each chip should be fuzzable in isolation.
Files:
docs/ppu.md
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.mdCHANGELOG.md
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
🔇 Additional comments (3)
CHANGELOG.md (1)
14-32: LGTM!docs/ppu.md (2)
177-188: LGTM!
169-175: 🎯 Functional CorrectnessKeep the conditional wording.
GetRgbColoris called insideif (color > 0), so transparent pixels do not call it.> Likely an incorrect or invalid review comment.
| **The exact model difference, identified 2026-08-02.** MesenCE updates `InternalCgramAddress` | ||
| inside `GetRgbColor`, which it calls **only when `color > 0`** (`SnesPpu.cpp:1114`) — so a | ||
| *transparent* pixel leaves the address holding the previous opaque column's value — and it does so | ||
| **per layer during tilemap render**, in render order, so several layers may update it within one | ||
| column. RustySNES assigns it **once per column, unconditionally, from the composited pixel** at | ||
| the draw cursor (`render.rs`, `pd_draw_columns`). Those are structurally different models, not an | ||
| off-by-one. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the conflicting earlier redirect description.
The text at Line 155 through Line 158 still describes MesenCE as the palette of the last-drawn column maintained by the compositor. The text at Line 164 through Line 167 attributes the mismatch to draw-cursor/fetch-ahead alignment. Lines 169-175 now define a different model: per-layer updates, retained state across transparent pixels, and multiple updates within one column. Revise the earlier paragraphs so docs/ppu.md has one authoritative redirect model and keeps RustySNES's composited assignment separate from MesenCE's behavior.
As per path instructions, docs are the spec, not a history log; prose must not drift from the code it describes.
🤖 Prompt for 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.
In `@docs/ppu.md` around lines 169 - 175, Revise the earlier redirect discussion
in docs/ppu.md around the descriptions of the last-drawn column and
draw-cursor/fetch-ahead alignment. Remove claims that MesenCE uses
compositor-level composited-column state or that the mismatch is solely cursor
alignment, and make the prose consistent with the authoritative per-layer,
transparent-pixel-retaining model while keeping RustySNES’s once-per-column
composited assignment distinct.
Source: Path instructions
The last per-dot framebuffer gap was documented only as "an
internal_cgram_addressdraw-ordering detail". That is now a concrete diff.The model difference
InternalCgramAddressis updatedGetRgbColor, called only whencolor > 0(SnesPpu.cpp:1114), per layer during tilemap render — a transparent pixel leaves the previous opaque column's value standing, and several layers may update it within one columnStructurally different models, not an off-by-one.
Gate-on-opaque is not the fix — measured, not assumed
Wrapping the assignment in
if ap.opaquemoves the ROM's hash from0xaeb678a4165b28c5to0xa55bd66a1e6dd125— still not the MesenCE-agreeing golden. Gating the composite is not the same as gating each layer fetch, which is exactly why the naive fix falls short.Correction to a recorded claim
The AccuracySNES row that breaks under gate-on-opaque is
C3.12("CGRAM taken in render"), notC3.04("H counter advances") as previously recorded.C3.04passes.That correction matters beyond bookkeeping:
C3.12is the row that asserts the redirect target directly, so its failure means the cart row and MesenCE's model disagree about a backdrop column — which has to be adjudicated before either one is changed.C3.04failing would have implied something quite different (a timing regression), and would have sent the next investigation in the wrong direction.What the real fix looks like
Track the redirect target in the fetch stage, per layer, updating only for a non-zero colour index in MesenCE's layer order. That is a Phase 4c-shaped change, which is why this remains open rather than being landed here.
Docs only;
render.rsrestored byte-clean from a pre-edit snapshot after the experiment.🤖 Generated with Claude Code
Documents the
InternalCgramAddressdiscrepancy between MesenCE and RustySNES during tilemap rendering. MesenCE updates it per layer only for non-zero color indices. RustySNES updates it once per column from the composited pixel.The claim is false if RustySNES matches MesenCE when the address is tracked from the composited pixel. Gating the composited pixel on opacity does not produce matching behavior or ROM hashes. The fix must occur during per-layer fetch processing and is deferred to Phase 4c.
Updates AccuracySNES dossier assertion
C3.04toC3.12.C3.04passes. The coverage denominator does not move.