feat(rdp): chroma-key alpha compare (key_en, R-10) - #160
Merged
Conversation
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>
|
Warning Review limit reached
Next review available in: 10 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 (6)
📝 WalkthroughWalkthroughThe RDP now decodes chroma-key enablement and per-channel widths, computes key-derived alpha in ChangesRDP chroma-key alpha compare
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant VectorGenerator
participant Rdp
participant ChromaKeyMin
participant ConformanceTest
VectorGenerator->>Rdp: emit key-enabled triangle commands
Rdp->>Rdp: decode key_en and key_width
Rdp->>ChromaKeyMin: combine 17-bit channel values with key widths
ChromaKeyMin-->>Rdp: return clamped alpha
Rdp-->>ConformanceTest: render framebuffer
ConformanceTest->>ConformanceTest: compare with Angrylion golden vector
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: 5
🤖 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 11-23: Update the older [Unreleased] entry below the RDP
chroma-key entry to remove claims that key_width is not stored or chroma-key
alpha comparison is deferred. Revise or merge it so [Unreleased] consistently
documents the implemented final key_en behavior described in the new entry.
In `@crates/rustyn64-rdp/src/lib.rs`:
- Around line 5213-5227: Add a test case to
chroma_key_min_folds_and_takes_the_minimum using at least one col17 value at or
above 0x10000, exercising the negative result from SIGN(col17, 17). Assert the
expected folded minimum so incorrect signed 17-bit sign extension is detected
while preserving the existing cases.
- Around line 1644-1657: The decode test for the key-setting dispatcher must
independently verify all three key_width channels. Update the test covering
OP_SET_KEY_GB and OP_SET_KEY_R to use distinct R/G/B width values, assert
key_width[0], key_width[1], and key_width[2] after dispatch, and remove any
stale wording claiming width is unused; preserve the existing centre/scale
assertions.
- Around line 900-902: Add backwards-compatible deserialization for the newly
added RDP fields key_en and key_width in the Rdp serialization path, using serde
defaults or an equivalent versioned migration. Ensure EmuCore::restore can
decode existing save-state blobs missing these fields while preserving the
current values for newly serialized states.
In `@crates/rustyn64-test-harness/vectors-gen/driver.c`:
- Around line 956-975: The conformance fixture does not currently observe
key-derived alpha because alpha comparison is disabled and rasterization
replaces combiner alpha with coverage. In
crates/rustyn64-test-harness/vectors-gen/driver.c:956-975, enable a non-zero
alpha-compare threshold or add a direct pre-coverage assertion using values that
distinguish chroma_key_min; regenerate
crates/rustyn64-test-harness/tests/vectors/tex_tri_chromakey_alpha_16.rvec:1. In
crates/rustyn64-test-harness/tests/rdp_conformance.rs:462-476, narrow the
end-to-end claim or assert failure when key alpha is bypassed. Update
docs/accuracy-ledger.md:391 and docs/rdp.md:407-412 to distinguish helper/unit
evidence from end-to-end evidence, ensuring the accuracy oracle does not claim
coverage for unobserved behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 78061b69-33e7-4174-8795-649447bac6e0
📒 Files selected for processing (7)
CHANGELOG.mdcrates/rustyn64-rdp/src/lib.rscrates/rustyn64-test-harness/tests/rdp_conformance.rscrates/rustyn64-test-harness/tests/vectors/tex_tri_chromakey_alpha_16.rveccrates/rustyn64-test-harness/vectors-gen/driver.cdocs/accuracy-ledger.mddocs/rdp.md
…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>
Owner
Author
Antigravity adjudication (commit 226cb6e)No blocking issues; 1 adopted, 1 fixed, 2 rejected.
|
Antigravity review (Gemini via Ultra)This PR implements RDP chroma-key alpha comparison ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
doublegate
added a commit
that referenced
this pull request
Aug 1, 2026
…/43) (#242) `conformance_gpu::census` replays all 43 registered `.rvec` vectors through parallel-rdp and grades each against Angrylion's golden framebuffer — the same independent oracle the software rasterizer is graded by, not one implementation against the other. 42 of 43 match on both paths: triangles (flat, shaded, textured, Z-buffered, fractional, negative-slope, right-major), texture loads (Load Tile, Load Block, TLUT, CI4/CI8, 4-bit), the combiner, the blender, dither, tile clamp/mask/mirror/shift, and the 3-point filter. The one divergence runs the opposite way from the expected one: parallel-rdp does not implement the `key_en` chroma-key alpha compare, which RustyN64's software rasterizer does (#160). Verified in its source — `op_set_other_modes` never decodes bit 8 of `words[0]`, `set_color_key` routes `key_center`/ `key_scale` only to combiner mux inputs, `key_width` is written and never read, and no shader mentions a key. The gate asserts the exact census, so a vector leaving the known-gap set fails as loudly as one joining it. `GpuRdp` now owns its RDRAM instead of borrowing it: the buffer must be aligned to the device's `minImportedHostPointerAlignment` or the direct host-import path is silently replaced by a staging copy, and a contract a caller can satisfy by accident is worse than no contract. The alignment is queried from the device rather than hardcoded. RDRAM is reached through `with_rdram`/`with_rdram_mut`, which use upstream's coherency handshake and report a failed publish rather than dropping it. Byte order was the silent hazard: parallel-rdp stores RDRAM in native little-endian word order while RustyN64 stores big-endian bytes. Pinned by an involution test, an exact-permutation test, and a cross-mechanism test that catches two consistently-wrong-but-mutually-inverse helpers. Not integrated: no Bus wiring, no shared RDRAM with a running machine, no frontend feature, no dirty-region sync and therefore no ADR 0004 determinism claim. Adjudicated 15 bot findings across two reviewers: 14 adopted, 1 rejected with reasons on the PR.
doublegate
added a commit
that referenced
this pull request
Aug 2, 2026
Three ADRs for the three items in the declined backlog that measure positive. Each is written on the re-derived numbers rather than the ones they were declined on, since those were shares of a frame that has since halved. 0018 (async GPU RDP) accepts shape (b) — present one frame late — as an opt-in that is off by default, because its cost is a frame of presentation latency and ~3.5% is not enough to spend a user's latency budget for them. Records that shape (a) is unavailable: `present` stages RDRAM before enqueueing, so submitting mid-frame would change which memory each command reads. Also names a GPU-to-GPU ordering hazard the plan missed, which is NOT the CPU-side tracker ADR 0014 §6 describes. 0019 (GPU as the machine's rasterizer) accepts A4 as an opt-in but BLOCKS building it until the GPU/Angrylion census reaches 43/43. This is the finding that changed the shape of the decision: the GPU is currently LESS complete than the software rasterizer it would replace (`key_en`, #160), so shipping it for ~2.5% would trade correctness for frame time. The ADR inverts the justification — A4 is worth building as an ACCURACY change, with the frame time as a side effect, and that reframing is what makes the gate load-bearing. 0020 amends 0016 to accept the SIMD exception for `multiply_lane` only, and says plainly that the technique got worse while the context got better: 0016 declined 1.056x, this accepts 1.045x. The four gates carry forward unrelaxed, and the ADR states in advance what would make it a mistake — that a hard-to-write equivalence test is a reason to stop, not to sample. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The follow-on to #159 (chroma-key combiner inputs): this wires the
key_enalpha-compare keying path — the other half of N64 chroma-keying. It also stores the per-channelkey_widththat #159 deliberately left out ("lands with its consumer") — the consumer is here.Changes
Set Other Modesbit 40 (hi >> 8 & 1) →OtherModes.key_en;Set Key GB/Rnow also decodekey_width(GBhi[23:12]/[11:0], Rlo[31:16]).combiner_1cycle): whenkey_en,Rdp::combineoutputs the sub-A chromabypass colour (clamped) as RGB, and the pixel alpha ischroma_key_minover the pre->>817-bit combined colour (combine_channel_17bit, matchingcolor_combiner_equation) and the key widths — per channelSIGN(col,17)folded (-k, or-k+0x10when the low nibble is 8),+ (width<<4),min-of-3, clamp[0,0xff].key_enso the common combiner path is byte-identical — all 31 prior RDP conformance vectors pass unchanged.Verification
tex_tri_chromakey_alpha_16.rvec—key_en+ Shade sub-A; Angrylion's golden is0x4321(shade RGB + the keyalpha's alpha bit), which RustyN64 matches byte-for-byte. Mutation-verified non-vacuous: forcing the common path (clearingkey_en) outputs the combined colour + alpha-combiner result and fails.chroma_key_min_folds_and_takes_the_minimum— hand-computed unit test covering the fold, thenibble==8special case, and the clamp.cargo test --workspace, fmt, clippy-D warnings, rustdoc-D warnings, no_std, markdownlint.Still deferred under R-10: noise (un-oracled — Angrylion fakes it with a validation PRNG), the derivative
lod_frac, and the YUVK0–K3convert. n64-systemtest impact: none (no RDP-combiner coverage).🤖 Generated with Claude Code