feat(harness): pin Load Block's inclusive texel count against Angrylion - #184
Conversation
…on (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>
|
Warning Review limit reached
Next review available in: 11 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)
📝 WalkthroughWalkthroughAdds an Angrylion golden vector and replay test for 16-bit ChangesLoad Block oracle coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
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/STATUS.md`:
- Line 257: Regenerate the page-wide STATUS metrics from the current test run:
update the stale “passes 712 tests” claim near the test-count summary to include
the newly added load_block_count_16_matches_angrylion test, and revise the
paragraph after the accuracy table to reflect that the accuracy battery is
available and passes 100% (52/52), matching the table.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17186546-0ad1-4a8f-9495-9f4f6cab4e13
📒 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/load_block_count_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/STATUS.mddocs/accuracy-ledger.md
… 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>
|
Both Antigravity suggestions adopted.
Hardcoded Worth flagging the difference from #182, where I rejected what looked like the same suggestion: there it was offered as bare The other 20 vectors still pass literals. Deliberately not swept here: that is a mechanical change across the whole generator and would bury a one-vector fix in it. |
…xecuted 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>
Antigravity review (Gemini via Ultra)This PR adds a minimal 16-bit single-line conformance vector ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
First
Load Blockcoverage, and a settled ambiguityLoad Block(0x33) had zero vector coverage — every texture vector loadedthrough
Load Tile— while Ocarina of Time issues 4,374Load Blocks of its74,508 commands. Dominant load path in a live retail stream, entirely unpinned.
The ambiguity
§0x33's prose says "
lower_right.s - upper_left.sdetermines the number oftexels" — no
+1— whileLoad Tileis inclusive andload_blockcomputesshi - slo + 1. The two readings genuinely disagree, and prose cannot decide it.A minimal vector can
load_block_count_16usesuls = 0, lrs = 1— a load of either one texel ortwo — on a single line with
dxt = 0.That minimality is the whole design. Two earlier, larger
Load Blockvectors wereauthored and discarded because their goldens showed Angrylion reading unwritten
TMEM; the fix was to remove layout from the experiment rather than keep guessing
at it. With one line, two texels and no
dxt, neither the odd-line swap nor anymulti-line TMEM arrangement can confound the answer.
Angrylion loads two. The count is inclusive,
shi - slo + 1is correct,and RustyN64 matches the golden byte-for-byte.
+1turns it red.What this does not claim
Load Blockis no longer wholly unpinned, but the coverage is minimal byconstruction. Still untested on this path:
dxt,The ledger says so explicitly rather than implying the command is now covered —
one narrow vector is not coverage of a command, and recording it as such is how
the next person avoids assuming otherwise.
Net effect on R-18
The leading
Load Blockhypothesis joins the others as refuted. R-18's videocause remains open, and the honest position is that four suspects have now been
eliminated by oracle rather than by argument.
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