feat(harness): pin the CI4 + TLUT path against Angrylion (R-18) - #182
Conversation
Verifying the Load Tlut encoding against N64brew ...Commands 0x30 found two errors in my first attempt, both of which had made Angrylion render a near-empty palette: - lower_right.s is command bits 23:12, so an 8-entry palette is (8-1) << 2 = 0x1C shifted by TWELVE. I had shifted by 14, which decodes as 29 entries. - the section's own Hazards require the TLUT tile to be 4-bit and neither RGBA nor YUV. I had set it 16-bit; only the TEXTURE IMAGE is 16-bit. Re-authored to the spec, Angrylion renders the intended palette - f801 07c1 003f ffff ffc1 07ff - and RustyN64 matches it byte-for-byte. tex_tri_ci4_tlut_16 is now committed, the first vector to exercise Load Tlut at all: the colour-indexed path had zero oracle coverage before this, which is why nobody could say whether it worked. Battery is 51 probes (38 RDP + 13 VI). Mutation-checked: changing tlut_lookup's stride from the quadrupled 8 to 4 turns it red at pixel (3,2). This settles the previous entry's provisional result. CI4 + TLUT is now GENUINELY eliminated as R-18's cause rather than weakly de-prioritised, and it is eliminated by a committed oracle instead of by inference. tlut_en remains undecoded and is still a real defect, but demonstrably not this one - the vector passes because our lookup keys off the CI format, which here coincides with tlut_en being set. R-18's video cause stays open. The remaining texture suspects are the ones this vector does not cover: Load Block vs Load Tile addressing at real texture sizes, the odd-line swap, and mip/LOD tile selection under a live command stream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 10 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 (4)
📝 WalkthroughWalkthroughThe PR adds a committed CI4/TLUT Angrylion conformance vector, registers it in the shared RDP battery, adds regression coverage, and updates R-18 and accuracy documentation to reflect the corrected ChangesCI4 TLUT conformance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VectorDriver
participant Rvec
participant ConformanceTest
participant Angrylion
VectorDriver->>Rvec: Emit tex_tri_ci4_tlut_16
ConformanceTest->>Rvec: Load committed vector
ConformanceTest->>Angrylion: Compare rendered output
Angrylion-->>ConformanceTest: Return golden result
Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
The V37 comment cited N64brew 0x30's hazard that the TLUT tile must not use RGBA or YUV, and then encoded fmt = 0, which IS RGBA. The comment was right and the code disagreed with it - the exact hazard this project has hit four times before. Now fmt = CI(2) with size 0 (4-bit), so the encoding matches both the hazard and the sentence above it. Regenerated; RustyN64 still matches Angrylion byte-for-byte and the mutation check still holds, which is itself informative: neither implementation's Load Tlut consults the format field here, so the hazard is about hardware behaviour the software reference does not model. Following it anyway costs nothing and keeps the vector a faithful command stream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Adjudicating the Antigravity review — one adopted, two rejected with evidence. TLUT tile format is RGBA, contradicting the comment — ADOPTED, and it is the best of the three. The comment cited §0x30's hazard that the TLUT tile must not use RGBA or YUV, and then encoded Now
uint32_t preload_len = v->n_texels * 2u;
Host little-endian byte order — REJECTED, premise incorrect. The emitter composes big-endian explicitly rather than memcpy-ing host rdram_put_word(byte_addr + i * 2u, (hi << 16) | lo);That is the same path every committed texture vector already uses; if it inverted byte order, all 38 of them would be wrong rather than passing. Worth noting the concern was reasonable — it just does not survive reading the emitter. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 43-48: Correct the vector count from 39 to 38 in CHANGELOG.md
lines 43-48 and docs/accuracy-ledger.md line 413, preserving the surrounding
text; the resulting battery total is 51 with 13 VI vectors. docs/STATUS.md
requires no direct change because it remains the source of truth for current
accuracy-battery counts.
In `@crates/rustyn64-test-harness/vectors-gen/driver.c`:
- Around line 1204-1205: Update the TEX_BLOCK test in the vector-generation
driver to sample all eight TLUT entries, including entries 6 and 7 represented
by 0xF83F and 0x8421. Extend the draw or add a second draw with terminal-entry
addressing, and update the committed golden expectation so mutations truncating
the Load Tlut range or mishandling the final entries fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 79a08e9f-077d-4e32-b5fa-aa66fa32dae1
📒 Files selected for processing (7)
CHANGELOG.mdcrates/rustyn64-test-harness/src/conformance.rscrates/rustyn64-test-harness/tests/rdp_conformance.rscrates/rustyn64-test-harness/tests/vectors/tex_tri_ci4_tlut_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/STATUS.mddocs/accuracy-ledger.md
Two review findings. The triangle only covered six columns, so palette entries 6 and 7 were declared and never fetched - the vector claimed eight distinct entries and proved six. Widening the edges (XH/XM start at column 0, DxMDy 2.0) makes every row reach all eight, and the golden's last rows now read f801 07c1 003f ffff ffc1 07ff f83f 8421: all eight entries rendered, none missing. RustyN64 still matches byte-for-byte, and the stride mutation still turns it red. The count was wrong: 38 RDP vectors are registered, not 39. The 39 is the TEST count - 38 vectors plus the fuzz-corpus test - and docs/STATUS.md already had 38/51 correctly, so the ledger and CHANGELOG were contradicting the file that is the single source of truth for it. Both now say 38 RDP vectors, 51 battery probes. The first is the more instructive: a vector that declares more coverage than it exercises is the same shape as an assertion a broken machine also satisfies. The palette had eight entries because eight made the test look thorough, and two of them were decoration until now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR adds a CI4 texture with TLUT lookup RDP test vector ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
… length Review follow-up, both suggestions adopted. TEX_BLOCK_COUNT read like a quantity while holding texel colours; it is now TEX_BLOCK_COUNT_TEXELS. The vector is about a COUNT question, which made the old name actively misleading next to the lrs/uls fields it sits beside. The preload length is now sizeof(...) / sizeof(uint16_t) rather than a literal 2, so it cannot drift if the array changes. Note the divisor is load-bearing: the field is a texel count, not a byte count, so a bare sizeof would declare twice the payload - which is why I rejected the same suggestion in its unqualified form on #182. Here it was offered correctly. The other 20 vectors still pass literals. Not swept in this PR: that is a mechanical change across the whole generator and would bury a one-vector fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…on (#184) * feat(harness): pin Load Block's inclusive texel count against Angrylion (R-18) Load Block (0x33) had zero vector coverage - every texture vector loaded through Load Tile - while Ocarina issues 4,374 Load Blocks of its 74,508 commands. It was the dominant load path in a live retail stream and entirely unpinned. The open question was the texel count. 0x33's prose says "lower_right.s - upper_left.s determines the number of texels" with no +1, while Load Tile is inclusive and load_block computes shi - slo + 1. The two readings disagree and prose cannot settle it. A minimal vector can. load_block_count_16 uses uls=0, lrs=1 - a load of either one texel or two - on a single line with dxt=0, so neither the odd-line swap nor any multi-line TMEM layout can confound the answer. That minimality is the point: two earlier, larger Load Block vectors were authored and discarded because their goldens showed Angrylion reading unwritten TMEM, and the fix was to remove layout from the experiment rather than to keep guessing at it. Angrylion loads two. The count is inclusive, our shi - slo + 1 is correct, and RustyN64 matches the golden byte-for-byte. Mutation-checked: dropping the +1 turns it red. Battery is 52 probes (39 RDP + 13 VI). So the leading Load Block hypothesis joins the others as refuted. The path is no longer wholly unpinned, but the coverage is minimal by construction - multi-line loads, the odd-line swap under a real dxt, and non-16-bit texel sizes all remain untested, and the ledger says so rather than implying the command is now covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(vectors): name the texel array for its contents; compute its length Review follow-up, both suggestions adopted. TEX_BLOCK_COUNT read like a quantity while holding texel colours; it is now TEX_BLOCK_COUNT_TEXELS. The vector is about a COUNT question, which made the old name actively misleading next to the lrs/uls fields it sits beside. The preload length is now sizeof(...) / sizeof(uint16_t) rather than a literal 2, so it cannot drift if the array changes. Note the divisor is load-bearing: the field is a texel count, not a byte count, so a bare sizeof would declare twice the payload - which is why I rejected the same suggestion in its unqualified form on #182. Here it was offered correctly. The other 20 vectors still pass literals. Not swept in this PR: that is a mechanical change across the whole generator and would bury a one-vector fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(status): refresh the test count and stop calling the battery unexecuted Two stale claims in the file that is meant to be the single source of truth for current numbers. The test count said 712. The actual figure from cargo test --workspace is 803, so it had been stale across many PRs rather than by the one this branch adds - the review flagged it as off by one, and it was off by 91. The corpus paragraph still listed the accuracy battery among the oracles that are "staged only - an oracle on disk that no gate executes yet", directly contradicting both the 100% (52/52) row in the table above it and the line that says the probe battery is now real. The battery does execute; only the real-ROM visual goldens and the commercial ROMs remain staged, and the sentence now says that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
The colour-indexed path had zero oracle coverage
No committed vector exercised
Load Tlutat all before this one — which is whynobody could say whether CI4 worked, and why it looked like a plausible cause for
R-18's black frame.
Verifying the encoding first found two errors in my own vector
The previous PR recorded a provisional result because my first attempt
disagreed with Angrylion and I could not tell whose fault that was. Checking
N64brew …/Commands §0x30 settled it — the vector was wrong, twice:
lower_right.sis command bits 23:12. An 8-entry palette is(8-1) << 2 = 0x1Cshifted by 12. I had shifted by 14, which decodes as 29entries — so Angrylion dutifully loaded 29 entries from a 16-byte source.
Hazards). I had set it 16-bit. Only the texture image is 16-bit.
Result
Re-authored to the spec, Angrylion renders the intended palette and RustyN64
matches it byte-for-byte:
tex_tri_ci4_tlut_16is committed. Battery is now 51 probes (38 RDP + 13 VI).Mutation-checked: changing
tlut_lookup's stride from the quadrupled 8 to 4turns it red at pixel (3,2).
What this settles, and what it does not
Settles: CI4 + TLUT is now genuinely eliminated as R-18's cause — by a
committed oracle rather than by inference. The previous entry's "weakly
de-prioritised" is upgraded and the ledger says so.
Does not settle:
tlut_en(bit 47) is still undecoded and still a realdefect — but demonstrably not this one. The vector passes because our lookup
keys off the CI format, which here coincides with
tlut_enbeing set; a vectorwith
tlut_enclear would still diverge.R-18's video cause remains open. The remaining texture suspects are precisely
the ones this vector does not cover:
Load BlockvsLoad Tileaddressing atreal texture sizes, the odd-line swap, and mip/LOD tile selection under a live
command stream.
Note on process
I discarded the first golden rather than committing it. In a test-as-spec project
a wrong golden is worse than no golden — it converts an authoring error into the
specification, and every later result built on it stops being evidence.
Gates
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace,RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps,pre-commit run markdownlint --all-files— each run separately, exitstatus checked.
Oracle: no emulation behaviour change (a new vector + docs), so n64-systemtest
is unchanged at 90 suite-wide, Phase 1
Failed: 0, perdocs/STATUS.md.🤖 Generated with Claude Code