fix(rdp): gate the TLUT lookup on tlut_en, not the tile format (R-18) - #185
Conversation
Set Other Modes.tlut_en (bit 47) was not decoded at all, so the colour-index palette lookup keyed off the tile's format field. That is wrong in both directions: a CI tile with tlut_en clear was still palette-mapped, and a non-CI tile with it set was not. The oracle settled the tlut_en = 0 behaviour rather than it being guessed. ci4_tlut_disabled_16 is byte-identical to tex_tri_ci4_tlut_16 apart from that single bit, and the two goldens are the full palette versus all black. The fix reproduces the observed result and says so: 0x2F does not document what the hardware does with un-TLUT'd index data, so no reinterpretation of the index bits is invented, only the outcome the oracle produces. tlut_type (bit 46) is decoded alongside it so the flag is no longer silently ignored, but IA16 palettes stay deferred - the lookup assumes RGBA16 and implementing IA16 without a vector would be inventing behaviour. An existing unit test, fetch_texel_ci_through_the_tlut, went red and was right to: it exercised the palette path while leaving tlut_en clear, so it passed only because the gate did not exist. Both of its Rdp instances now set the flag they depend on. Same shape as the five fill_rectangle tests that were asserting FILL-mode behaviour without selecting FILL mode. Battery is 53 probes (40 RDP + 13 VI). Mutation-checked by removing the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 3 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 RDP now decodes ChangesTLUT gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SetOtherModes
participant RDP
participant TextureMemory
participant Framebuffer
SetOtherModes->>RDP: Decode tlut_en and tlut_type
RDP->>TextureMemory: Fetch CI texel
alt tlut_en set
RDP->>TextureMemory: Resolve texel through TLUT
RDP->>Framebuffer: Write palette colour
else tlut_en clear
RDP->>Framebuffer: Write zero RGBA
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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 `@crates/rustyn64-rdp/src/lib.rs`:
- Around line 3178-3179: Extend the existing set_other_modes_decodes_fields test
to construct input with distinct bit-15 and bit-14 values, then assert both
decoded tlut_en and tlut_type fields independently. Ensure the chosen vector
would fail for swapped extraction or if either decode is removed.
In `@crates/rustyn64-test-harness/vectors-gen/driver.c`:
- Around line 1181-1217: Correct the coverage comments in
V39_CI4_TLUT_DISABLED_16: replace the claim that all eight indices are sampled
with wording that distinguishes the eight loaded palette entries from the six
exercised by the geometry. Keep the existing geometry and disabled-TLUT
assertion unchanged.
In `@docs/accuracy-ledger.md`:
- Line 413: Clarify the historical “38 RDP vectors, 51 battery probes” statement
in the R-18 ledger entry by explicitly labeling it as a dated historical count
and scoping it to the earlier snapshot. Preserve the later current counts and
reference docs/STATUS.md as the authoritative source for present totals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8f5a35ee-9189-41dc-98da-98295351e932
📒 Files selected for processing (8)
CHANGELOG.mdcrates/rustyn64-rdp/src/lib.rscrates/rustyn64-test-harness/src/conformance.rscrates/rustyn64-test-harness/tests/rdp_conformance.rscrates/rustyn64-test-harness/tests/vectors/ci4_tlut_disabled_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/STATUS.mddocs/accuracy-ledger.md
My own doc comment said keying off the format is "wrong in both directions" and implied the fix addressed both. It addresses one. Implemented: a CI tile with tlut_en clear is no longer palette-mapped, pinned by ci4_tlut_disabled_16. Deferred, and now stated: a NON-CI tile with tlut_en set is still not palette-mapped, though hardware would sample it through the TLUT. No vector covers that case, and the RGBA/IA/I formats index the palette differently enough that implementing it from the prose alone would be inventing behaviour. It stays wrong-but-recorded until a vector defines it - the same posture as tlut_type's IA16 palettes. This is the comment-is-not-an-implementation hazard once more, and in the PR that fixes an instance of it: the doc described the complete rule while the code implemented half, and nothing failed because a comment cannot fail. Recorded in the ledger too, so the deferral is visible outside the source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Antigravity — the doc claims both directions, the code implements one. ADOPTED, and this is the sharper catch. My comment said keying off the format is "wrong in both directions" and implied the fix addressed both. It addresses one:
I have not implemented the second direction because no vector covers it, and the RGBA/IA/I formats index the palette differently enough that deriving it from prose alone would be inventing behaviour — the same reason Worth naming: this is the comment-is-not-an-implementation hazard, occurring inside the PR that fixes an instance of it. The doc described the complete rule while the code implemented half, and nothing failed — because a comment cannot fail. That is the whole reason this project keeps a rule about it. On |
…ntly Three review findings. docs/rdp.md's T-32-003 section documented CI8/CI4 "through the TLUT" with no mention of tlut_en, so the spec described the old, format-keyed behaviour. It now records the gate, the vector that pins it, and both limits: a non-CI tile with tlut_en set is still not palette-mapped, and tlut_type's IA16 palettes are deferred. Recording the limits matters more than recording the fix - the spec is what someone reads before assuming a case works. set_other_modes_decodes_fields did not assert tlut_en or tlut_type. They are ADJACENT bits (47 and 46), so a swapped extraction is the likely error and would pass any test that sets both or neither, and only bit 47 has vector coverage. Each is now asserted with the other clear, in both polarities. Mutation-checked by swapping the two extractions: red. V39's geometry comment was copied from V37 and described V37's history, which read as a claim about this vector. Reworded to what it actually tests: the geometry spans eight columns, but with tlut_en clear every column is black whatever its index, so this vector proves the GATE and V37 proves the per-index resolution. Identical geometry is deliberate - it makes the two goldens differ by exactly one bit of input. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Docs-As-Spec Sync — ADOPTED. It now records the gate, the vector that pins it, and both limits: a non-CI tile with The limits are the part that matters more than the fix. The spec is what someone reads before assuming a case works — documenting only the working direction is how the next person concludes non-CI TLUT sampling is supported and spends a day finding out otherwise. |
Antigravity review (Gemini via Ultra)This PR gates RDP texture lookup for color-indexed (CI) tiles on the Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
|
The remaining Docs-As-Spec Sync warning is stale: CodeRabbit last reviewed at Merging on that basis. CI 9/9, all three review threads adjudicated and resolved, no outside-diff findings. |
Adopts three CodeRabbit findings. set_controllers_connected had no test -- every controller test ran on the default mask, so a no-op setter would have passed. the_configured_port_mask_ reaches_the_joybus_dispatch inverts the default (port 0 empty, port 1 populated) and asserts BOTH directions from one setter call, so a setter writing a constant cannot satisfy it. Mutation-checked: stubbing the setter to a no-op turns it red. mark_no_device's doc claimed the connected/unsupported-command path was its ONLY route; a missing EEPROM on channel 4 reaches it too. Corrected to say what is actually true -- it is the route this test drives, and it was the only route any test could take before the empty-port guard was refactored to delegate. Ledger R-18 said "tlut_en remains undecoded" in one clause while recording it as FIXED in #185 in another. Labelled as the historical statement it was, rather than rewritten, per the append-only rule the row follows. 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>
… screen (R-18) (#190) * feat(pif): report "no device" on empty ports -- SM64 renders its title screen Super Mario 64 renders its title screen (screenshots/super-mario-64-title.png): Mario's head, textured cap with the M logo, over the tiled SUPER MARIO 64 background. 125,278 RDP commands, 138,474/148,125 pixels lit at 625x237, through the full LLE path. Viewed and confirmed, not counted. The unblocking defect was in the PIF, not the RDP. SM64 was halting in its own assert path -- a B -1 self-loop at 0x80246DD8, reached by falling through BNE t1, zero, +4 where t1 is a byte at 0x8032D590 that is never written -- immediately after an osCreateThread call. Our PIF answered the 0x00/0xFF info command as a connected controller on ALL FOUR joybus channels, so osContInit reported four pads on a one-pad console. PIF-NUS.md, RX byte: special flags, is explicit: bit 7 (0x80) is "set if the handshake failed because no device appears to be connected to the joybus channel". We never set it. A second, pre-existing defect sat next to it. mark_no_device wrote the flag to resp - 1, which for a block laid out TX RX tt[tx_len] rr[rx_len] with resp = i + 2 + tx_len is the LAST TX DATA BYTE, not the RX byte at i + 1. Since run_channel returns early on tx_len == 0 the two offsets could never coincide, so the flag had never once landed where software reads it -- while a doc comment asserted it did. Both fixed and separately mutation-checked. The second test exists only because the first mutation PASSED: the empty-port guard short-circuits before run_channel, so nothing exercised mark_no_device at all until a test was written for a connected channel given an unsupported command. Measured effect: Super Mario 64 0 -> 125,278 RDP commands and a title screen; Banjo-Kazooie 0 -> 133,625 commands and real 3D geometry, committed as a known-imperfect frame (geometry, textures and depth right; colours carry a blue/yellow cast, an open combiner/texel-format issue). A measurement trap is recorded with it: a 240-frame sample reported SM64 at scanout 0x0 and looked like a geometry failure. It was a transient -- one frame with H_VIDEO = 0 mid-reprogram of the VI. Sampling at 60/120/180/300 shows a stable 625x237 throughout. Still open: Jet Force Gemini and Banjo-Tooie remain at 0 RDP commands, and 1080 Snowboarding still faults in IPL3 on an odd DMEM address. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint, n64-systemtest (Phase 1 Failed: 0, suite-wide 90) -- all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor(pif): expose port occupancy; delegate to mark_no_device Adopts three Antigravity suggestions. set_controllers_connected / controllers_connected replace the hardcoded private field, so a frontend can attach pads to ports 2-4 instead of the PIF permanently reporting single-player. The empty-port path now delegates to mark_no_device(i + 1) rather than poking ram directly, centralising the bounds check. This also strengthened the mutation check: reverting the offset now turns BOTH no-device tests red rather than one, because the two paths share the write. The tests derive the RX-byte offset from the documented frame layout (SKIPS + 1) instead of counting backwards from resp with magic subtraction, and assert the layout itself so the derivation cannot drift silently. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings -- all green. Both mutations re-checked after the refactor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(pif): pin the port mask through dispatch; fix two stale doc claims Adopts three CodeRabbit findings. set_controllers_connected had no test -- every controller test ran on the default mask, so a no-op setter would have passed. the_configured_port_mask_ reaches_the_joybus_dispatch inverts the default (port 0 empty, port 1 populated) and asserts BOTH directions from one setter call, so a setter writing a constant cannot satisfy it. Mutation-checked: stubbing the setter to a no-op turns it red. mark_no_device's doc claimed the connected/unsupported-command path was its ONLY route; a missing EEPROM on channel 4 reaches it too. Corrected to say what is actually true -- it is the route this test drives, and it was the only route any test could take before the empty-port guard was refactored to delegate. Ledger R-18 said "tlut_en remains undecoded" in one clause while recording it as FIXED in #185 in another. Labelled as the historical statement it was, rather than rewritten, per the append-only rule the row follows. 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
A real defect, fixed
Set Other Modes.tlut_en(bit 47) was not decoded at all, so thecolour-index palette lookup keyed off the tile's format field. Wrong in both
directions:
tlut_enclear was still palette-mapped;The oracle defined the behaviour, not me
ci4_tlut_disabled_16is byte-identical totex_tri_ci4_tlut_16apart fromthat single bit, so any difference is attributable to it. The goldens:
tlut_ensetf801 07c1 003f ffff ffc1 07ff f83f 8421— the full palettetlut_enclear0001 0001 0001 0001 0001 0001 0001 0001— all blackThe fix reproduces that observed result and says so in the code: §0x2F does not
document what the hardware does with un-TLUT'd index data, so no
reinterpretation of the index bits is invented — only the outcome.
tlut_type(bit 46) is decoded alongside it so the flag is no longer silentlyignored, but IA16 palettes stay deferred — the lookup assumes RGBA16, and
implementing IA16 without a vector would be inventing behaviour.
A test that was passing for the wrong reason
fetch_texel_ci_through_the_tlutwent red, and was right to. It exercised thepalette path while leaving
tlut_enclear — so it passed only because thegate did not exist, asserting the palette while describing a machine that had not
asked for one. Both of its
Rdpinstances now set the flag they depend on.Same shape as the five
fill_rectangle_*tests earlier in this series, whichasserted FILL-mode behaviour without ever selecting FILL mode. A test that does
not configure the feature it names will pass on the bug and fail on the fix.
Verification
ci4_tlut_disabled_16red.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: n64-systemtest is unchanged at 90 suite-wide, Phase 1
Failed: 0— it has no RDP render-path coverage, so no movement was expected here.
🤖 Generated with Claude Code