test(rdp): oracle-validate the 4-bit texture path (ledger R-7) - #142
Conversation
Studying the Angrylion oracle source (tex.c:526-533) corrected a misconception behind ledger R-7. There is NO 4-bit texel LOAD on the N64: a 4-bit texture-image load is invalid and crashes the RDP pipeline. Games load 4-bit textures by lying about the format -- an 8-bit texture image + 8-bit LOAD tile loads the packed bytes raw, then a SEPARATE 4-bit render tile extracts nibbles at fetch. RustyN64 already implements both halves (8-bit load + 4-bit fetch_texel for I4/IA4/CI4), so that canonical path already works -- R-7's "4-bit texels not loaded, needs nibble addressing" framing was wrong. New conformance vector tex_tri_i4_16 proves it: eight I4 texels packed two-per-byte (0xFD 0xB9 0x75 0x31), an 8-bit Load Tile, then a 4-bit (format I, size 0) render tile sampled across the triangle. RustyN64 reproduces Angrylion BYTE-FOR-BYTE. The eight intensities are descending and non-zero (texel 0 = white 0xFFFF, not black), so the test is non-vacuous: an all-zero (no-op) load would render black and fail. Since the load is format-agnostic (a raw byte copy), this validates the load mechanism for all 4-bit formats; the per-format decoders are separately unit-tested. No production code change (the bail on a 4-bit texture image already matches the hardware's invalid-load behaviour). R-7's genuinely-remaining gaps are now precisely characterized and stay open: the 32-bit Load Block split, and a direct 4-bit LOAD tile paired with an 8-bit texture image (the ti_size-vs-tile.size load granularity). - vectors-gen/driver.c: V20 tex_tri_i4_16 generator (+ Angrylion golden .rvec). - rdp_conformance.rs: the assertion. - Ledger R-7, docs/rdp.md, CHANGELOG corrected to the accurate hardware model. Gates: full rdp_conformance suite (22 pass), fmt --check, clippy -D warnings, markdownlint on the touched docs, check_no_roms clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 14 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)
📝 WalkthroughWalkthroughAdds an Angrylion-validated I4 textured-triangle vector, its regression test, generated RVEC payload, and documentation updates describing the canonical load/fetch path and remaining deferred cases. ChangesI4 texture conformance
| Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
Adopt Antigravity review: - Nitpick: clarify the LOAD tile's line=1 is one 64-bit word (not a byte count). - Suggestion (R-7 count 90 vs neighbours' 93): the reviewer flagged a real inconsistency but the fix direction is inverted — 90 is authoritative (docs/STATUS.md, after Phase 5 dropped 93->90); the surrounding 93s are stale. Resolve by pointing the R-7 narrative at STATUS.md rather than citing a number, per the docs-as-spec rule, avoiding both the stale 93 and a ledger-wide count sweep in this focused PR. Rejected: the uint8_t-vs-uint16_t preload suggestion — the .rvec byte emit is explicitly big-endian and host-independent (documented in driver.c), the u16 array is the established Vector.texels pattern, and the byte-for-byte pass proves the byte order is correct. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Adjudicated: Suggestion 1 ( Suggestion 2 (R-7 count "90" vs neighbours' "93") — Real inconsistency, but the fix direction is inverted: 90 is authoritative ( Nitpick ( |
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/rdp.md`:
- Around line 281-289: Update the later summary near the remaining deferred-work
section to remove the generic “4-bit loading” wording and explicitly name only
the two open cases: the 32-bit Load Block split and direct 4-bit LOAD-tile
granularity with an 8-bit texture image. Preserve the clarification that 4-bit
texture-image loads are invalid and must not be described as an implementation
target.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 56081dbb-9cb7-470c-857d-f602cd3a3f9d
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustyn64-test-harness/tests/rdp_conformance.rscrates/rustyn64-test-harness/tests/vectors/tex_tri_i4_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/accuracy-ledger.mddocs/rdp.md
…bbit) Two later spots in rdp.md still said generic '4-bit loading (nibble Load Tile/ Load Block) pending R-7', conflicting with the corrected model earlier in the same file. Reconcile both to the accurate statement: there is no 4-bit load; 4-bit textures load as 8-bit + render with a 4-bit tile (validated by tex_tri_i4_16), and only the 32-bit Load Block split + a direct 4-bit LOAD tile remain deferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Antigravity review (Gemini via Ultra)This PR adds an RDP conformance test ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
Motivation
Continuing gap-analysis Stage D. With the CPU-timing cluster done/deferred, this
is the first RDP residual, validated against the Angrylion
.rvecconformanceoracle. It closes the validated part of ledger R-7 (4-bit textures) and — more
importantly — corrects a misconception the ledger carried.
The finding
Studying the Angrylion oracle's source (
tex.c:526-533) shows there is no 4-bittexel load on the N64: a 4-bit texture-image load is invalid and crashes the
RDP pipeline (
rdp_pipeline_crashed = 1). Games load 4-bit textures by lyingabout the format — an 8-bit texture image + 8-bit LOAD tile loads the packed
bytes raw, then a separate 4-bit render tile extracts nibbles at fetch.
RustyN64 already implements both halves (the 8-bit load, and
fetch_texelforI4/IA4/CI4), so the canonical 4-bit path already works. R-7's "4-bit texels not
loaded, needs nibble addressing" framing was wrong — 4-bit is never DMA'd as
nibbles.
What this PR does
tex_tri_i4_16: eight I4 texels packed two-per-byte(
0xFD 0xB9 0x75 0x31), an 8-bitLoad Tile, then a 4-bit render tile sampledacross the triangle. RustyN64 reproduces Angrylion byte-for-byte.
(texel 0 = white
0xFFFF), so an all-zero no-op load would render black andfail — the golden distinguishes a correct load from a load that did nothing.
already matches hardware's invalid-load behaviour.
docs/rdp.md, andCHANGELOGto the accurate model,and precisely characterizes the two genuinely-remaining gaps (the 32-bit
Load Blocksplit; a direct 4-bit LOAD tile with an 8-bit texture image — theti_size-vs-tile.sizeload granularity).Gates run locally
cargo test -p rustyn64-test-harness --test rdp_conformance— 22 pass, 1 ignoredcargo fmt --all --check·cargo clippy -p rustyn64-test-harness --all-targets -- -D warningspre-commit run markdownlinton the touched docs ·scripts/check_no_roms.shclean🤖 Generated with Claude Code