feat(rdp): wire register-sourced exotic combiner inputs (R-10) - #147
Conversation
Route PRIM_LOD_FRAC and the Set Convert K4/K5 constants through the colour combiner instead of reading zero (ledger R-10, gap-analysis Stage D). - Set Prim Color (0x3A) extracts prim_lod_frac + min_level from its word-0 low byte; a new Set Convert (0x2C) dispatch extracts K4/K5 (raw 9-bit, lo[17:9]/lo[8:0]). - The combiner input mux wires prim_lod_frac to RGB mul-select 14 and alpha mul-select 6, K4 to RGB sub-B select 7, and K5 to RGB mul-select 15. - Two non-vacuous Angrylion conformance vectors pin the wiring byte-for-byte: tex_tri_primlodfrac_16 (One * prim_lod_frac, gray vs black if unwired) and tex_tri_convert_k45_16 ((One - K4) * K5, gray vs black if unwired). - Two mutation-checked unit tests (combine_cycle_routes_prim_lod_frac, combine_cycle_routes_convert_k4_k5). Still open (read as zero until their machinery exists): noise, the derivative-computed lod_frac, the chroma-key centre/scale, and the YUV convert K0-K3 coefficients. Docs: ledger R-10 RESOLUTION, docs/rdp.md combiner scope, CHANGELOG. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 minute 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 (5)
📝 WalkthroughWalkthroughThe RDP now latches ChangesRegister-sourced combiner inputs
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RDPCommandDispatcher
participant Rdp
participant combined_color
participant Combiner
participant ConformanceHarness
RDPCommandDispatcher->>Rdp: latch PRIM_LOD_FRAC, K4, and K5
Rdp->>combined_color: provide latched render state
combined_color->>Combiner: supply combiner inputs
Combiner->>ConformanceHarness: produce framebuffer output
ConformanceHarness->>ConformanceHarness: compare against RVEC golden vectors
Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ector Adjudicating the Antigravity review on #147. The bot flagged K4/K5 as stored raw-unsigned without sign extension. The sign extension is real but lives one layer down in `combine_channel` (`special_expand` for the K4 sub-B input, `sext9` for the K5 mul), matching Angrylion, which likewise stores K4/K5 raw 0..511 (`rdp_set_convert`) and sign-extends in the equation (`special_9bit_exttable` / `SIGNF(c,9)`). Signing at decode would double-apply. The positive `tex_tri_convert_k45_16` vector could not reach the bit-8-set path, so this adds `tex_tri_convert_kneg_16` (negative `K4 = 0x1C0` = -64, golden `0x5295` gray) that oracle-proves it byte-for-byte — a raw-positive read would clamp black. Also sharpens the K4/K5 doc comments to name the exact mechanism and cite Angrylion, so the raw-storage decision reads as deliberate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adjudication of the Antigravity reviewThanks — the review surfaced a genuinely under-tested path, and the fix is a stronger oracle rather than a code change. Details per item: Blocking issue (K4/K5 sign extension) — rejected as stated, but hardenedThe premise ("sign extension never happens") is incorrect: it happens one layer below the input mux, in
To make this oracle-proven rather than reasoned, I added Suggestion (sign-extend at decode) — rejectedSame reason: Angrylion holds the raw value and expands in the equation. Signing Nitpick (doc comments) — adopted with modificationThe old comments were accurate but terse. I rewrote the Fixed in |
Adjudicating the second Antigravity review on #147. - CombinerInputs gains #[non_exhaustive], matching Rdp — both grow every sprint as more combiner inputs are wired, so a new field stays a compatible change. (Rdp was already non_exhaustive; that half of the finding did not apply.) - Remove the min_level field: it was extracted from Set Prim Color but never read. Unread state is drift; it lands with its LOD/mip consumer. The word layout stays documented in the dispatch comment. - Note that Set Convert's ignored lo[31:18] carries the deferred YUV K0..K3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adjudication of the second Antigravity reviewFixed in Blocking (public-API breaking change without
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustyn64-rdp/src/lib.rs (1)
629-652: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefresh the combiner contract rustdoc.
The public
combine_cycledocumentation still says exotic inputs read as zero, but these arms now wirePRIM_LOD_FRAC, K4, and K5. Restrict that claim to noise, derivative LOD, key inputs, and K0–K3 so the code contract matches R-10 anddocs/rdp.md.Also applies to: 694-695
🤖 Prompt for 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. In `@crates/rustyn64-rdp/src/lib.rs` around lines 629 - 652, Update the public rustdoc for combine_cycle to remove the broad claim that exotic inputs read as zero. Restrict that statement to noise, derivative LOD, key inputs, and K0–K3, while documenting PRIM_LOD_FRAC, K4, and K5 as implemented inputs consistent with the rgb_input_c and corresponding channel mappings.
🤖 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 13-16: Update the shared min_level documentation: in CHANGELOG.md
lines 13-16, remove “and min_level” from the Set Prim Color extraction claim; in
docs/accuracy-ledger.md line 391, state that min_level remains deferred in bits
12:8 and is not stored, while preserving the prim_lod_frac low-byte extraction
description.
In `@crates/rustyn64-test-harness/tests/rdp_conformance.rs`:
- Around line 419-423: Correct the documentation for the Set Convert K4/K5 case
near tex_tri_convert_k45_16 to identify the bit-8-clear vector’s golden result
as 0x94A5 instead of 0x5295. Keep 0x5295 associated only with the negative-K4
vector and clarify the comments so the two R-10 conformance cases are not
conflated.
In `@docs/accuracy-ledger.md`:
- Line 391: Update the R-10 resolution entry to explicitly state the measured
impact on n64-systemtest’s failing-assertion count, or state that the impact was
not measured. Keep the existing Angrylion, conformance-vector, and oracle
results unchanged.
- Line 391: Update the R-10 resolution text to state only that PRIM_LOD_FRAC is
extracted from the low byte of the Set Prim Color word-0; remove any claim that
min_level is latched or stored, and keep min_level deferred with the
derivative-computed LOD machinery.
In `@docs/rdp.md`:
- Around line 398-405: Update the R-10 scope in the documentation to include
tex_tri_convert_kneg_16 alongside tex_tri_primlodfrac_16 and
tex_tri_convert_k45_16. Preserve the existing description while explicitly
listing all three conformance vectors, including the negative-K4 raw 9-bit
storage and sign-extension coverage.
---
Outside diff comments:
In `@crates/rustyn64-rdp/src/lib.rs`:
- Around line 629-652: Update the public rustdoc for combine_cycle to remove the
broad claim that exotic inputs read as zero. Restrict that statement to noise,
derivative LOD, key inputs, and K0–K3, while documenting PRIM_LOD_FRAC, K4, and
K5 as implemented inputs consistent with the rgb_input_c and corresponding
channel mappings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4cbc00c4-d9f3-48d6-96f5-28bd6a0568b5
📒 Files selected for processing (9)
CHANGELOG.mdcrates/rustyn64-rdp/src/lib.rscrates/rustyn64-test-harness/tests/rdp_conformance.rscrates/rustyn64-test-harness/tests/vectors/tex_tri_convert_k45_16.rveccrates/rustyn64-test-harness/tests/vectors/tex_tri_convert_kneg_16.rveccrates/rustyn64-test-harness/tests/vectors/tex_tri_primlodfrac_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/accuracy-ledger.mddocs/rdp.md
…puts Adjudicating the third Antigravity review on #147. CombinerInputs is transient per-pixel state (built in combined_color, never stored in System), so it is never part of a save-state — yet it derived Serialize/Deserialize, which is what led the reviewer to flag a (non-existent) save-state compat break. Dropping the unused derives removes the confusion at its root and is the correct design for transient state. For the record on the reviewer's premise: save-states use bincode over the whole System (a positional format), so #[serde(default)] would not help missing fields anyway; the persistent RDP fields live on Rdp (which IS serialised, per ADR 0004's requirement that new hidden state be reachable by the serialiser) and same-version round-trip holds. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adjudication of the third Antigravity reviewFixed in Blocking (missing
|
Adjudicating the CodeRabbit inline findings and one Antigravity nitpick on #147: - CHANGELOG + ledger: min_level is no longer extracted (removed in 67f4812) — drop the stale "and min_level" claim; state it stays deferred. - rdp_conformance.rs: the tex_tri_convert_k45_16 doc comment named the wrong golden (0x5295, which is the kneg vector's) — the positive k45 golden is 0x94a5. Corrected. - ledger R-10: state the n64-systemtest impact explicitly (none — the suite has no RDP-combiner coverage; count unchanged at 90), per the ledger convention. - rdp.md: add the tex_tri_convert_kneg_16 vector to the R-10 scope (all three vectors now listed). - Clarify the combine_cycle_passes_texel0_through rgb_b comment: select 7 is K4 (0 in this default input), not the zero fall-through arm. Docs/comments only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adjudication of the fourth Antigravity reviewFixed in Blocking (removing serde derives +
|
Adjudicating the fifth Antigravity review on #147. CombinerInputs is transient per-pixel state with no external consumers (nothing outside rustyn64-rdp references it, CombineCycle, combine, or combine_cycle). Making it `pub(crate)` (and demoting combine/combine_cycle, its only public users, to `pub(crate)`) enforces in the type system what the doc comment already claimed — it is not part of this crate's public API — and permanently settles the recurring "removing serde derives / adding fields is a breaking public change" finding: it is no longer public. Dropped the now-redundant `#[non_exhaustive]` too (a new field is always compatible within the crate). Also hardens `combine_cycle_passes_texel0_through`: use zero-select 8 for rgb_b instead of 7 (which now maps to K4), so the pass-through test no longer depends on K4's default value. No behaviour change; the combiner output is identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adjudication of the fifth Antigravity reviewFixed in Blocking (removing serde derives from a
|
Antigravity review (Gemini via Ultra)This PR wires register-sourced exotic RDP combiner inputs ( Blocking issues
Suggestions
Nitpicks
Automated first-pass review by |
Adjudication of the sixth Antigravity reviewNo code change this round — the findings are self-contradictory, factually incorrect, already-satisfied, or a standing policy disagreement. Adjudicated below. Blocking (visibility reduction + version bump) — rejected
Suggestion (mark
|
…Antigravity) Adopt all 3 Antigravity suggestions (no blocking): - Add the end-to-end Angrylion conformance vector `tex_tri_chromakey_16` (the main suggestion — matching the #147 K4/K5 / PRIM_LOD_FRAC precedent). Set Key GB/R program centre [0x20,0x40,0x60] / scale [0x40,0x80,0xC0]; the combine (One-centre)*scale>>8 = [56,96,120] -> RGBA5551 0x3b1f, which RustyN64 matches byte-for-byte (black if unwired -> non-vacuous). The Set Combine word differs from V27 only in rgb_b (7->6 KeyCentre) and rgb_c (15->6 KeyScale); alpha/cyc0 identical. This validates the decode + mux end-to-end through the real pipeline, beyond the unit tests. - Document the hi word (width_g[23:12], width_b[11:0]) in OP_SET_KEY_GB, matching OP_SET_KEY_R's width_r note. - Clarify the rgb_input_b fallback comment (no R-10-deferred select remains for B). Gates: workspace test (incl. 32 rdp_conformance), fmt, clippy, rustdoc, markdownlint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(rdp): chroma-key combiner inputs (Set Key R/GB, R-10) Wire the last register-sourced R-10 combiner inputs. `Set Key GB` (0x2A) and `Set Key R` (0x2B) now decode the per-channel chroma-key centre and scale (bit-layout ported from Angrylion rdp_set_key_gb/_r), and they route through the combiner as KeyCentre (RGB sub-B select 6) and KeyScale (RGB mul-select 6) instead of reading zero — matching Angrylion combiner.c cases 6. - The key *width* is not stored: it drives only the deferred chroma-key alpha compare (key_en), not the combiner mux, so it lands with that consumer (the min_level precedent — no unread state). - Two mutation-checked unit tests: set_key_decodes_centre_and_scale_per_channel (distinct per-channel values pin the decode field positions) and combine_cycle_routes_chroma_key ((One-centre)*scale>>8 → [56,96,120]; unwiring either input changes the result). Same register-sourced-combiner-input pattern as #147 (PRIM_LOD_FRAC / K4 / K5). n64-systemtest impact: none (no RDP-combiner coverage). Still deferred under R-10: noise, derivative lod_frac, the chroma-key alpha compare, and YUV convert K0-K3. Gates: workspace test, fmt, clippy, rustdoc, no_std, markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(rdp): add chroma-key .rvec conformance vector + decode docs (#159 Antigravity) Adopt all 3 Antigravity suggestions (no blocking): - Add the end-to-end Angrylion conformance vector `tex_tri_chromakey_16` (the main suggestion — matching the #147 K4/K5 / PRIM_LOD_FRAC precedent). Set Key GB/R program centre [0x20,0x40,0x60] / scale [0x40,0x80,0xC0]; the combine (One-centre)*scale>>8 = [56,96,120] -> RGBA5551 0x3b1f, which RustyN64 matches byte-for-byte (black if unwired -> non-vacuous). The Set Combine word differs from V27 only in rgb_b (7->6 KeyCentre) and rgb_c (15->6 KeyScale); alpha/cyc0 identical. This validates the decode + mux end-to-end through the real pipeline, beyond the unit tests. - Document the hi word (width_g[23:12], width_b[11:0]) in OP_SET_KEY_GB, matching OP_SET_KEY_R's width_r note. - Clarify the rgb_input_b fallback comment (no R-10-deferred select remains for B). Gates: workspace test (incl. 32 rdp_conformance), fmt, clippy, rustdoc, markdownlint. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(rdp): refresh stale R-10 comments after wiring chroma-key (#159 CodeRabbit) CodeRabbit's review caught nearby comments still describing the chroma-key inputs as deferred/read-as-zero after this PR wired them: - The CombinerInputs population comment, the `combine_cycle` rustdoc, and the CombinerInputs struct doc now list prim-LOD-frac / K4 / K5 / chroma-key centre-scale as the wired register-sourced inputs, and noise / derivative lod_frac / YUV K0-K3 as the still-deferred (read-zero) ones. Docs only. Gates: fmt, clippy, rustdoc clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…ge fixes CodeRabbit (5 comments — 3 adopted, 2 addressed, 1 rejected w/ citation): - MAJOR (driver.c): the vector didn't observe the key alpha (the 16-bit alpha bit came from coverage, not chroma_key_min). Reworked tex_tri_chromakey_alpha_16 to enable alpha-compare (bit 0) with a Set Blend Color threshold of 0x80: the Shade triangle is drawn only where chroma_key_min >= 0x80 (combine yields exactly 0x80). Now a chroma_key_min off by -1 makes the triangle vanish (mutation-verified), and the golden genuinely observes the key alpha. Narrowed the test/ledger/rdp.md claims. - MAJOR (serde/save-state): REJECTED the serde(default) fix — bincode is positional so serde(default) is a no-op (engineering-lessons; the #147 precedent), and adding RDP register state matches #159's key_center/key_scale addition this same session; the 0.x save-state format evolves with emulated state (ADR 0005 reserves format stability for the stable release). No versioning system exists or is in scope. - Assert distinct per-channel key_width in the decode test (renamed). - Add a bit-16-set (negative) col17 case to the chroma_key_min unit test. - Fix the contradictory #159 CHANGELOG "still deferred" line. Antigravity (no blocking — 1 adopted, 1 fixed, 2 rejected): - Fix the width_r comment (lo[27:16], 12-bit, not [31:16]). - Mask col17 to 17 bits inside chroma_key_min (defensive). - Reject: the 2-cycle "unpopulated combined" concern — the key_en branch runs AFTER the two_cycle cyc0 eval, so inp.combined is populated. - Reject: rgb_input_a is already bound to a local `a` (called once). Gates: workspace test, fmt, clippy, rustdoc, no_std, markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(rdp): chroma-key alpha compare (key_en, R-10) The follow-on to #159's chroma-key combiner inputs: the key_en alpha-compare keying path (Angrylion combiner_1cycle). Set Other Modes bit 40 (hi>>8&1) decodes to OtherModes.key_en, and Set Key GB/R now also store the per-channel key_width (GB hi[23:12]/[11:0], R lo[31:16]) — now that key_en consumes it. When key_en, Rdp::combine takes the key path: the RGB output is the sub-A "chromabypass" colour (clamped) and the pixel alpha is chroma_key_min over the pre->>8 17-bit combined colour (combine_channel_17bit = ((A-B)*C + (D<<8) + 0x80) & 0x1ffff, matching color_combiner_equation) and the key widths — per channel SIGN(col,17) folded (-k, or -k+0x10 when the low nibble is 8), +(width<<4), min-of-3, clamp [0,0xff]. The new behaviour is GATED on key_en, so the common combiner path is byte- identical — all 31 prior RDP conformance vectors pass unchanged. Validated byte-for-byte against Angrylion by tex_tri_chromakey_alpha_16 (key_en + Shade sub-A -> golden 0x4321 = shade RGB + alpha bit; clearing key_en outputs the combined colour + alpha-combiner result, mutation-verified non-vacuous) plus a hand-computed chroma_key_min unit test. Still deferred under R-10: noise (un-oracled), the derivative lod_frac, YUV K0-K3. Gates: workspace test (incl. 33 rdp_conformance), fmt, clippy, rustdoc, no_std, markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(rdp): adjudicate #160 bot review — observable key alpha + coverage fixes CodeRabbit (5 comments — 3 adopted, 2 addressed, 1 rejected w/ citation): - MAJOR (driver.c): the vector didn't observe the key alpha (the 16-bit alpha bit came from coverage, not chroma_key_min). Reworked tex_tri_chromakey_alpha_16 to enable alpha-compare (bit 0) with a Set Blend Color threshold of 0x80: the Shade triangle is drawn only where chroma_key_min >= 0x80 (combine yields exactly 0x80). Now a chroma_key_min off by -1 makes the triangle vanish (mutation-verified), and the golden genuinely observes the key alpha. Narrowed the test/ledger/rdp.md claims. - MAJOR (serde/save-state): REJECTED the serde(default) fix — bincode is positional so serde(default) is a no-op (engineering-lessons; the #147 precedent), and adding RDP register state matches #159's key_center/key_scale addition this same session; the 0.x save-state format evolves with emulated state (ADR 0005 reserves format stability for the stable release). No versioning system exists or is in scope. - Assert distinct per-channel key_width in the decode test (renamed). - Add a bit-16-set (negative) col17 case to the chroma_key_min unit test. - Fix the contradictory #159 CHANGELOG "still deferred" line. Antigravity (no blocking — 1 adopted, 1 fixed, 2 rejected): - Fix the width_r comment (lo[27:16], 12-bit, not [31:16]). - Mask col17 to 17 bits inside chroma_key_min (defensive). - Reject: the 2-cycle "unpopulated combined" concern — the key_en branch runs AFTER the two_cycle cyc0 eval, so inp.combined is populated. - Reject: rgb_input_a is already bound to a local `a` (called once). Gates: workspace test, fmt, clippy, rustdoc, no_std, markdownlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Motivation
Ledger residual R-10 (gap-analysis Stage D toward
v0.8.0 "Breadth"): the colourcombiner modelled the common inputs but read every exotic input as zero. This PR
closes the register-sourced subset — the inputs that need no new pipeline, only a
register decode and a mux wire.
Changes
Set Prim Color(0x3A) now extractsprim_lod_frac(andmin_level) from itsword-0 low byte.
Set Convert(0x2C) dispatch extractsK4/K5(raw 9-bit,lo[17:9]/lo[8:0]).prim_lod_frac→ RGB mul-select 14 + alpha mul-select 6,K4→ RGB sub-B select 7,K5→ RGB mul-select 15.Oracle validation (pin-the-oracle-first)
Two new non-vacuous Angrylion conformance vectors, byte-for-byte:
tex_tri_primlodfrac_16One * prim_lod_frac(0x80)0x8421graytex_tri_convert_k45_16(One - K4) * K5(0x40/0xC0)0x94a5grayPlus two mutation-checked unit tests (
combine_cycle_routes_prim_lod_frac,combine_cycle_routes_convert_k4_k5); the K5 select was reverted to confirm both theunit test and the conformance vector go red, then restored.
Still open (documented in R-10, read as zero)
Noise (needs a per-pixel noise source), the derivative-computed
lod_frac(needs theLOD/mip pipeline, pairs with R-13), the chroma-key centre/scale (needs the key registers +
key_en), and the YUV convertK0–K3coefficients.Gates run locally
cargo fmt --all --check,cargo clippy --workspace --all-targets -D warnings,cargo test --workspace(29 conformance incl. 2 new, 104 rdp units incl. 2 new),RUSTDOCFLAGS=-D warnings cargo doc,no_stdthumbv7em build, andpre-commit run markdownlint— all green.🤖 Generated with Claude Code