feat(r18): first rendered frame from a commercial cartridge - #186
Conversation
Paper Mario renders real geometry through the full LLE path -- retail HLE boot, the game's own code, its graphics microcode on the LLE RSP, the DPC seam, the LLE RDP, and Bus::scanout_scaled. Committed as screenshots/paper-mario-first-commercial-frame.png: 87 distinct RGBA5551 values, 75,840 of 75,840 pixels lit, held stably from frame 120 through 270 of a 300-frame run. This falsifies R-18's headline claim. The earlier conclusion that "the RDP rasterises real geometry to black" was title-specific, not a pipeline defect. The remaining gap is per-title coverage: Ocarina 27,651 commands but 98% black clear, World Driver Championship 45 commands and one distinct value, Super Mario 64 and Banjo-Tooie zero commands. Two measurement defects fixed in the same change. commercial_boot counted lit pixels through the superseded 1:1 Bus::scanout rather than Bus::scanout_scaled, the path the frontend actually presents, so every number R-18 ever quoted came from a buffer no user sees. And Bus::scanout_scaled's own rustdoc still claimed it was "not yet wired into the frontend" -- false since #158. The evidence standard R-18 itself sets was held rather than assumed: on the same run Ocarina scored 62,963 lit pixels and, rendered to PNG and looked at, is pure uninitialised-RDRAM noise. Pixel count cannot distinguish it from Paper Mario's real frame, so screenshots/README.md requires looking. A suspicion was raised and withdrawn by measurement: the 625-wide scan-out looked like PRESCALE_H leaking into the width term, but VI_X_SCALE = 0x200 is 0.5 in 2.10, so 320 upscales to 640 less the 8/7-px minhpass/maxhpass crop = 625. Correct as-is. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint -- all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 42 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 (2)
📝 WalkthroughWalkthroughThe change aligns commercial-boot pixel diagnostics with the frontend’s scaled scan-out path, updates scan-out documentation, and records revised R-5 and R-18 accuracy findings alongside Paper Mario’s first commercial rendered frame evidence. ChangesCommercial frame evidence
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (8 passed)
Comment |
![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 `@CHANGELOG.md`:
- Around line 11-35: Run the repository’s pinned Markdown validation locally
with `pre-commit run markdownlint --all-files`, and resolve any reported issues
in the changelog while preserving its content.
In `@crates/rustyn64-test-harness/tests/commercial_boot.rs`:
- Around line 46-52: Update the R-5 entry in accuracy-ledger.md to list
commercial_boot as a scanout_scaled caller and remove the outdated statement
that it will migrate from Bus::scanout. Preserve the prior unscaled-caller
statement only if clearly marked as historical, ensuring the ledger reflects the
current caller contract.
In `@docs/accuracy-ledger.md`:
- Line 413: Normalise the R-18 evidence across all listed sites: in
docs/accuracy-ledger.md lines 413-413, mark stale tlut_en wording as historical
and update vector/test totals from docs/STATUS.md; in
crates/rustyn64-test-harness/tests/commercial_boot.rs lines 99-103, describe
non_black_pixels using the returned scaled dimensions and w*h denominator; in
CHANGELOG.md lines 17-18, correct the Paper Mario total; and in
screenshots/README.md lines 16-18, make the 625×237 dimensions and recorded
pixel total refer to the same frame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4fe5457-58f0-4354-ba77-7fe44b0f0a57
⛔ Files ignored due to path filters (1)
screenshots/paper-mario-first-commercial-frame.pngis excluded by!**/*.png
📒 Files selected for processing (5)
CHANGELOG.mdcrates/rustyn64-core/src/bus.rscrates/rustyn64-test-harness/tests/commercial_boot.rsdocs/accuracy-ledger.mdscreenshots/README.md
…denominators Adopts three review findings. Antigravity (blocking): scanout_scaled returns (0, 0) when the destination is too small, which arrived here as non_black_pixels == 0 -- a buffer-sizing mistake wearing the exact costume of "this title renders nothing". The buffer was 640x480; PAL is 576 lines, so every PAL title would have read as blank. Sized to 720x576 and the returned dimensions are now carried in BootResult. That immediately split a diagnosis that was invisible before: four of five staged titles report scanout=0x0 (the VI is blanked, no frame scanned at all) while 007 reports 625x237 with 0/148125 (a real frame that is genuinely all black). All five previously reported an identical "0 non-black pixels". CodeRabbit: the evidence record mixed denominators -- 75,840/75,840 is the 1:1 320x237 count, quoted beside the committed PNG's 625x237 scaled dimensions. Both figures are now stated with their path in the ledger, CHANGELOG and screenshots/README. CodeRabbit: R-5 listed commercial_boot among the callers that "migrate as R-18 needs them"; it has now migrated, recorded without rewriting the prior claim. Antigravity (nit): screenshots/README notes the dimensions are this NTSC capture's, not a fixed expectation. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint -- all green. commercial_boot re-run locally. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adjudication — Antigravity reviewBlocking finding: ADOPTED, and it was right in a way worth recording. Fixed in
Both halves of the suggestion are taken: the buffer is now It immediately split a diagnosis that was invisible. Re-running the capstone locally: Four titles have a blanked VI — no frame is scanned at all. 007 scans a real frame that is genuinely all black. Those are different failures with different causes, and all five previously reported an identical "0 non-black pixels". This finding directly improved the instrument. Nitpick: ADOPTED. Thanks — the blocking one was a genuine catch. |
…t the pipeline docs/STATUS.md is the single source of truth, and four of its rows asserted that a commercial title "scans out no frame" or that a title frame awaits a VI/RI/F3DEX dependency. Paper Mario falsifies the first and narrows the second. Superseded in place with the date and the reason, not silently rewritten: the pipeline is not the gap; per-title coverage is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
407-408: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not claim that PAL output fits the frontend buffer.
Bus::scanout_scaledcan produce up to 625 scan lines, while the frontend backing buffer is 640×480. Its(0, 0)oversize guard therefore makes PAL output black even with interlace/serrate deferred. Expand the frontend buffer and bounds, or document the PAL limitation accurately; the current claim is false.🤖 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 `@CHANGELOG.md` around lines 407 - 408, Correct the changelog entry describing Bus::scanout_scaled so it no longer implies PAL output fits the 640×480 frontend buffer. Document that PAL-sized output can exceed the buffer and trigger the (0, 0) black-frame guard, or update the entry only after the frontend buffer and bounds are expanded.
🤖 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 `@screenshots/README.md`:
- Around line 16-20: Add a citation to the paragraph in screenshots/README.md
beside the claims about VI_X_SCALE, 2.10 semantics, and minhpass/maxhpass
cropping, referencing the relevant R-5 measurement or primary source documented
in docs/accuracy-ledger.md. Keep the existing explanation and dimensions
unchanged.
---
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 407-408: Correct the changelog entry describing
Bus::scanout_scaled so it no longer implies PAL output fits the 640×480 frontend
buffer. Document that PAL-sized output can exceed the buffer and trigger the (0,
0) black-frame guard, or update the entry only after the frontend buffer and
bounds are expanded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 561735a9-6e02-4ad9-bf4c-d19061b9bace
📒 Files selected for processing (4)
CHANGELOG.mdcrates/rustyn64-test-harness/tests/commercial_boot.rsdocs/accuracy-ledger.mdscreenshots/README.md
The geometry paragraph introduced VI_X_SCALE = 0x200, 2.10 fixed-point step semantics, and the 8/7-px minhpass/maxhpass crop as hardware behaviour with no citation. Every hardware constant in this repo cites a primary source or a ledgered measurement, so the paragraph now names both: N64brew Video Interface for the scale semantics, and ledger R-5 -- validated RGB byte-for-byte against Angrylion through the .vivec vectors -- for the overscan and crop. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR migrates the Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Adopts two Antigravity suggestions. The R-18 text referenced the scanout_dims field as though it landed here; it landed in #186. Now cited by PR number so the claim is traceable to the change that made it true rather than to whichever PR happens to mention it. The Cpu comment carried a paragraph about the Stage-A sweep that missed the stale TODO. That belongs in the commit message and the ledger, not beside a struct definition -- a reader here needs to know where the CP0/TLB/CP1/LL state lives and why (ADR 0007), which the remaining text states positively. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot fault (#187) * fix(cpu): retire a stale TODO; record that the stalled titles wait, not fault Two unrelated-looking findings, both instances of "nothing fails when a claim goes stale". crates/rustyn64-cpu/src/lib.rs carried TODO(T-CPU-01) listing the branch-delay-slot latch, CP0 registers, TLB entries, the CP1 register file and the LL/SC link bit as unimplemented. All five exist, one level down in Pipeline, because they are pipelined rather than architectural state (ADR 0007): pipeline.cop0, pipeline.tlb, pipeline.fpr, pipeline.ll_bit, and in_delay_slot riding in the inter-stage latch. This is the false-incompleteness class the Stage-A comment sweep existed to remove, and one it missed. Ledger R-18 records why the non-rendering commercial titles stall, and corrects two claims I made earlier in the same session. The "hard AdES fault loop" was a sticky-register artefact. Cause.ExcCode survives the handler returning, so sampling it showed AdES on 10,575 of 12,000 samples. Counting transitions instead gives exactly ONE address error over 1,332,906,106 retired instructions for Banjo-Kazooie and exactly ONE over 1,995,894,174 for 1080. One fault, then a billion instructions of spinning -- the fault is not what stops them. And our AdES is correct in both cases. Banjo-Kazooie faults on SD t0, 88(k0) inside libultra's exception preamble, to an address that is 8-byte aligned and correctly sign-extended -- but Status=0x6D016CAA gives KSU=1 (Supervisor) with SX=0, and KSEG0 does not exist in Supervisor mode. 1080 faults in IPL3 running from DMEM on a genuinely odd address with KSU=0. Three emulator-side hypotheses were refuted by reading the code rather than patching it: the EXL/ERL kernel forcing, the 64-bit-operation reservation, and kernel_segment's CKSEG0 arm are all already correct. CpU exceptions are benign, established by a control rather than by argument: Paper Mario raises 18 while rendering correctly in the same run. What is actually open: all four stalled titles spin, and two of them never raise an address error at all, so the shared cause is something they wait for that never arrives. The interrupt/DMA-completion path is the first place to look. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint -- all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs: attribute scanout_dims to #186; trim the process narrative Adopts two Antigravity suggestions. The R-18 text referenced the scanout_dims field as though it landed here; it landed in #186. Now cited by PR number so the claim is traceable to the change that made it true rather than to whichever PR happens to mention it. The Cpu comment carried a paragraph about the Stage-A sweep that missed the stale TODO. That belongs in the commit message and the ledger, not beside a struct definition -- a reader here needs to know where the CP0/TLB/CP1/LL state lives and why (ADR 0007), which the remaining text states positively. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Paper Mario renders real geometry through the full LLE path — retail HLE boot → the game's own code → its graphics microcode on the LLE RSP → the DPC seam → the LLE RDP →
Bus::scanout_scaled. Committed asscreenshots/paper-mario-first-commercial-frame.png.Why this is evidence and not another lit-pixel claim
Ledger R-18 records that "lit pixel count" was cited for weeks and was wrong — uninitialised RDRAM is non-black, so a broken machine scores 90% as easily as a working one. R-18 therefore admits only two kinds of evidence: a byte-comparison against a committed golden, or someone actually looking.
That standard was held, not assumed. On the same 300-frame run:
The counts are comparable; the images are not. Hence
screenshots/README.mdrequires looking before adding a file.What this changes about R-18
The headline claim is falsified. The earlier conclusion that "the RDP rasterises real geometry to black" was title-specific, not a pipeline defect — the pipeline produces a correct picture end-to-end. Coverage now stratifies by title (120 frames):
0x0001(black clear), 1,440 px litTwo claims in R-18's opening column are now wrong and are retained only as the historical observation (immutable-reference rule, cf. R-5):
VI_CTRLdoes not stay 0 andVI_ORIGINis not never set — both are programmed by every title that boots (SM640x13016, Mario Kart 640x3116, both TYPE=2).Two measurement defects fixed in the same change
commercial_bootcounted through the superseded 1:1Bus::scanout, notBus::scanout_scaled— the path the frontend actually presents (emu.rs:310). Every lit-pixel number R-18 ever quoted came from a buffer no user sees, off by the realVI_X_SCALEupscale (~2×).Bus::scanout_scaled's own rustdoc still said it was "not yet wired into the frontend" — false since feat(frontend): present the accurate VI scan-out (wire scanout_scaled, R-5) #158, which wired it.A suspicion raised and withdrawn by measurement
The 625-wide scan-out looked like
PRESCALE_Hleaking into the width term. It is correct:VI_X_SCALE = 0x200is 0.5 in 2.10, so 320 upscales to 640, less the 8/7-pxminhpass/maxhpasscrop = 625. Recorded in the ledger so it is not re-investigated.Gates run locally
cargo fmt --all --check·cargo clippy --workspace --all-targets -D warnings·cargo test --workspace·RUSTDOCFLAGS="-D warnings" cargo doc·pre-commit run markdownlint --all-files— all green. No ROMs are committed; only the rendered output, which thecommercial-romspolicy explicitly permits.