test(rdp): refute the last two RDP mirrored-text candidates (R-18) - #203
Conversation
The mirrored-text defect (GoldenEye's "Nintendo" logo, WCW vs. nWo's banner) had two live candidates left after `tex_tri_mirror_s_16` cleared the tile mirror path. Author both as Angrylion vectors rather than patch on a guess. Both come back green byte-for-byte, so the RDP side of the hunt is exhausted: - `tex_tri_neg_dsdx_16` walks S backwards at one texel per pixel from a base of 7 texels. `mask_s = 0` + `clamp_s` is load-bearing: under an 8-texel wrap the outcomes are indistinguishable, because a missing sign extension gives a stride of 2047 texels and 2047 mod 8 == -1 mod 8, i.e. the bug would wrap to exactly the reversed ramp correct code produces. Clamped, they separate into a reversed ramp versus a solid bar. Column 0 is gray under both, which is why the readout is eight columns wide. - `tex_tri_right_major_16` is the first *textured* `lft = 0` triangle in the battery; the only previous `lft = 0` triangle was FILL-mode with no texture coefficients. It matters because the RDP evaluates the span's texture coordinate at the major edge, so swapping which edge is major swaps which end holds the base coordinate -- and getting that backwards mirrors the span while leaving every left-major vector green. Battery: 54 -> 56 probes (43 RDP + 13 VI), still 100%. Both mutation-checked by corrupting each golden's last byte, which names them individually. Consequence: five RDP hypotheses are now cleared and `Texture Rectangle Flip` is excluded on separate grounds, so the defect is reclassified to the RSP/geometry side -- upstream of the DPC seam, where a vertex transform or viewport sign error would flip a quad whose RDP commands are then rasterised faithfully. Recorded in R-18 with the caveat that these are unit refutations against synthetic vectors, not proof the same paths are configured correctly under a live retail command stream. Also corrects two vector comments that claimed `tex_tri_16` and `tex_tri_base_tile_16` render an eight-texel colour ramp. Their goldens are solid texel 0: `dx.S = 1` is 1/32 of a texel per pixel in the s10.5 domain, so S never leaves the first texel. Both vectors pass and keep the coverage they have, but neither ever pinned a gradient -- and nothing failed when the claim went stale, which is the hazard exactly. Fixes a pre-existing MD040 fence in R-18.md that CI cannot catch (markdownlint is pre-commit only). Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint -- all green.
|
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 (8)
Comment |
Antigravity review on #203, all three items: - TEX_BLOCK with a negative argument: VERIFIED, not a defect. HALVES masks to 16 bits BEFORE the shift, so -0x20 packs as 0xFFE00000 with no bleed into the adjacent T field. Confirmed empirically too -- the emitted golden is the reversed ramp, which only happens if the coefficient reached Angrylion as -32. - Adjacent "colourful"/"colorful" in driver.c: adopted. The inconsistency was mine, introduced into a file with 30 en-GB spellings. The repo is moving to en-US, so driver.c is swept whole (75 replacements) rather than patched at one line. The driver rebuilds and reproduces all 56 goldens byte-for-byte, so no vector changed. The repo-wide sweep follows separately. - R-18.md readability: adopted additively. Its Status section is a single ~48,000-character narrative, which is what made the live scope hard to extract. Added a "Current position" INDEX at the top that supersedes nothing, states that the record wins on any disagreement, and tabulates the nine open items. The history is untouched -- restructuring it would breach the append-only rule the document opens by asserting. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, markdownlint -- all green.
Adjudication of the Antigravity review1.
#define HALVES(hi16, lo16) ((((uint32_t)(hi16) & 0xFFFFu) << 16) | ((uint32_t)(lo16) & 0xFFFFu))Compiled and checked: It is also confirmed empirically, which is the stronger evidence: the emitted golden is the ramp reversed, and that only happens if the coefficient reached Angrylion as −32. A field-bleed would have produced garbage in T, not a clean reversal. Good thing to ask about, though — this is the first negative coefficient in the file, so nothing had exercised that macro path before. 2. The inconsistency was mine: I introduced one 3. Fair, and understated: that I have not restructured it, because the document opens by asserting the append-only rule — superseded claims stay marked rather than being edited away, and re-sorting the narrative would breach that and destroy the chronology that makes the retractions legible. Instead I added a
That gets the readability benefit you asked for without rewriting a word of the history. CodeRabbit hit its review rate limit on the first run; the push above re-triggers it. |
Adopts the second Antigravity review, which is right and overrides its own earlier nitpick: a test-vector PR should not carry 75 unrelated spelling edits. driver.c is restored to its pre-sweep content. The adjacency the first nitpick flagged is resolved without writing either spelling -- the comment now says "a multi-texel ramp" instead of naming a colour, so there is nothing to be inconsistent about. The repo-wide en-US sweep lands in its own PR, driver.c included. Rebuilt and regenerated: all 56 goldens byte-for-byte unchanged.
Adjudication of the second Antigravity reviewSuggestion — the ~25 unrelated spelling edits in You are right, and this is the better call of the two. The sweep only got here because I adopted the first review's nitpick about
The adjacency you flagged first is still resolved — but without writing either spelling. The sentence no longer names a colour at all: -// An earlier revision of this comment said "the colorful ramp"; see V11 for why that
+// An earlier revision of this comment described the picture as a multi-texel ramp;So there is nothing left to be inconsistent about, and no en-GB text was reintroduced either. Both reviews are satisfied without the scope creep. The repo-wide en-US sweep lands in its own PR, Nitpick — the CHANGELOG Rebuilt and regenerated after the revert — all 56 goldens byte-for-byte unchanged, so nothing about the vectors moved. |
Antigravity review (Gemini via Ultra)This PR adds two Angrylion conformance test vectors ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
|
@coderabbitai review |
✅ Action performedReview finished.
|
* chore: write the repo in en-US, and gate it so it stays that way The project standard is en-US. The tree had drifted to en-GB -- and I made that worse: on #202 I rejected a reviewer's generalising -> generalizing suggestion on the measured grounds that the repo was consistently en-GB (37 "behaviour" to 3). The measurement was right and the conclusion was backwards: consistency with a drift is not a reason to keep drifting. 1,505 replacements across 131 files, from 51 stem-level pairs applied case-preservingly, so every inflection of a stem is covered by one entry. Touches prose, doc comments, assertion messages, and private test-function names. NO public item is renamed, so there is no API change -- checked by enumerating `pub fn|struct|enum|const|static|trait|type|mod` names against the stem list and finding none. Also checked in advance: - no CI job or script filters on an affected test name - no excluded directory cites an affected test name, so swept and unswept text cannot desync - no verbatim external quotation is altered (searched for quoted phrases beside a citation marker; the one hit was our own italic emphasis, not a quote) Excluded trees, and why: ref-docs/ immutable research corpus -- corrections land as new dated supplemental files, never in-place rewrites (module 40) n64brew_wiki/ CC BY-SA mirror, quoted verbatim by design ref-proj/ study clones of other emulators third_party/ vendored libdragon; not our prose to edit Two literals are preserved because they are values, not words: `lightgrey` in a shields.io badge URL (a color PARAMETER), and GitHub Actions' own `cancelled` run status where the CHANGELOG quotes it. Stems whose form is correct in en-US too were deliberately left out of the list -- analysis, synthesis, hypothesis, peripheral, exercise, precise, premise, promise, otherwise, likewise, bitwise, controlled, installed, stalled -- since a stem there would corrupt a correct word. Adds scripts/check_en_us.sh and an `en-US spelling` CI job, because a one-off sweep decays: nothing fails when one en-GB form returns, so it survives review and the next has precedent. This repo already demonstrates the pattern -- markdownlint runs in pre-commit only, and an MD040 violation sat unnoticed on main until #203. The gate is mutation-checked in BOTH directions (fails on the unswept tree at 1,355 lines, passes on the swept one at 443 files), strips allowed literals rather than skipping their whole line so a permitted value cannot shield a real hit beside it, and documents a per-line `spell-exempt` escape hatch. Also wired into pre-commit for a faster local loop, with CI as the unskippable gate. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green. * fix(en-us): repair 4 malformed words the sweep produced, and gate the class Adjudicates the Antigravity review on #204, which caught one of four corruptions. A before/after word-pair audit over the whole diff caught all four, and it is the check I should have run before committing the sweep: characteris -> characteriz corrupted characteristics -> characteriztics optimis -> optimiz corrupted optimistic -> optimiztic realis -> realiz corrupted realistically -> realiztically centre -> center corrupted centred -> centerd All four are stems matching INSIDE a word already correct in en-US. The gate passed on the corrupted tree, and correctly so -- the outputs are not en-GB either, so an en-GB pattern cannot see them. The same defect was in the gate itself, in both directions: bare -is stems made it report `characteristics`, `optimistic` and `realistically` as violations. Split the stem list in two -- PLAIN_STEMS matched as substrings, ISE_STEMS requiring an en-GB verb/noun ending -- so `optimis+tic` no longer matches while `optimis+ation` still does. Also: - Added a MALFORMED backstop list so these specific non-words cannot return, with a comment saying plainly that it is a backstop for forms already seen and NOT a substitute for auditing a future sweep's word pairs. - Added `cancelled`/`cancelling` and `centring` as stems. Their omission left real en-GB spellings unswept -- `the_sign_of_a_cancelled_zero_...` (renamed) and two prose sites -- while the value references keep the backticked `cancelled` form that names GitHub Actions' literal status. - `centring` and `cancelling` are listed separately because neither CONTAINS its sibling stem, which is how a stem list silently under-matches. The gate is now mutation-checked three ways: it catches an injected en-GB word, catches an injected malformed word, and does NOT flag optimistic / realistically / characteristics / synthesis / analysis. Antigravity also reported scripts/check_en_us.sh as missing from the patch; rejected as incorrect -- `git ls-files` lists it and the `en-US spelling` CI job PASSED on this PR, which is impossible if the script were absent. Gates: fmt, clippy -D warnings, cargo test --workspace, rustdoc -D warnings, no_std thumbv7em, markdownlint, check_no_roms, check_en_us -- all green.
1 participant
Motivation
The mirrored-text defect visible in the committed commercial screenshots — GoldenEye 007's "Nintendo" logo and WCW vs. nWo's banner both render left-right flipped, while WCW/nWo Revenge's THQ logo is correct — had two live candidates left after
tex_tri_mirror_s_16cleared the tile mirror path. R-18 named them explicitly: a negativeDsDxand the triangle edge/winding path.Per the project's own rule, author the oracle vector rather than patch on a guess.
Result: both refuted
Both new vectors match RustyN64 byte-for-byte against Angrylion, so the RDP side of this hunt is now exhausted rather than merely narrowed.
tex_tri_neg_dsdx_16walks S backwards at one texel per pixel from a base of 7 texels, across a full 8×8 rectangle.mask_s = 0+clamp_sis load-bearing and not tidiness:gray yellow cyan magenta white blue green redgray gray gray gray gray gray gray grayUnder an 8-texel wrap these two are indistinguishable — a missing sign extension gives the stride
0xFFE0= 2047 texels, and2047 mod 8 == -1 mod 8, so the bug would wrap to exactly the reversed ramp correct code produces. Clamping separates them. Column 0 is gray under both, which is what the eight-column readout defeats.tex_tri_right_major_16is the first texturedlft = 0triangle in the battery. Of the 34 triangles indriver.cexactly one setlft = 0(fill_tri_neg_16) and it is FILL-mode with no texture coefficients, so this path was entirely unpinned. It matters because the RDP evaluates the span's texture coordinate at the major edge and steps byDsDxfrom there — swapping which edge is major swaps which end holds the base coordinate, and getting that backwards mirrors the span while leaving every left-major vector in the battery green. Angrylion emits the forward ramp; we match.Battery: 54 → 56 probes (43 RDP + 13 VI), still 100%. Both mutation-checked by corrupting each golden's last byte, which names them individually:
Consequence: the defect is reclassified
Five RDP hypotheses have now been tested and cleared (the
Set Tilemirror_sdecode,mask_coupled's fold, themirror_srender path, negativeDsDx, right-major winding), andTexture Rectangle Flipis excluded on the separate ground that it bails rather than rendering flipped.What is left is upstream of the DPC seam — the geometry producing the vertex X coordinates, i.e. the game's graphics microcode on the LLE RSP. A vertex transform, viewport scale, or vector-unit sign error there would flip a quad while every RDP command it then emits is rasterized faithfully. That moves the defect off R-13/RDP and onto the RSP/geometry side, and it is why no further RDP vector is the right next probe.
Stated caveat, not buried: these are unit refutations against synthetic vectors. They prove the paths are bit-exact when driven directly; they do not prove the same paths are configured correctly under a live retail command stream.
Also: two vector comments were describing a test that does not exist
tex_tri_16's comment claimed "a per-x S gradient (dx.S = 1.0/pixel) so each column reads a different texel". Its golden is solid red — every pixel is texel 0:dx.S = 1is one unit of the s10.5 texel domain — 1/32 of a texel per pixel — so S never leaves texel 0 across eight columns. One texel per pixel is0x20.tex_tri_base_tile_16inherited the same coefficient and the same wrong description ("the colorful ramp").Both vectors pass and keep the coverage they genuinely have (a Load Tile round trip; red-vs-black tile-3 discrimination, which is unaffected), so neither is retired — discarding a passing golden to fix a comment would lose real coverage. But neither ever pinned a gradient, and nothing failed when the claim went stale, which is the hazard
docs/engineering-lessons.md§3.3c describes. Comments corrected in place with the reason recorded.Also fixes a pre-existing MD040 fence in
docs/residuals/R-18.md(landed in #201). markdownlint is pre-commit only and not in any CI job, so this had no gate.Gates run locally
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— all green.Regenerating the vectors reproduced all 54 pre-existing goldens byte-for-byte (only the two new files appear in
git status), which is an incidental reproducibility check on the driver.🤖 Generated with Claude Code