refactor(HDR): decompile ISTemporalAA#2086
Conversation
📝 WalkthroughWalkthroughRefactors the temporal AA pixel shader to typed PS_INPUT/PS_OUTPUT, explicit named resources and a PerGeometry cbuffer; reorganizes shader logic into neighborhood sampling, depth/motion reprojection, luminance clamping, mode-specific temporal blending, transparency handling, and feedback packing. Also moves/updates color, display-mapping, and VR helper utilities and adds a VR test. Changes
Sequence Diagram(s)mermaid PS->>Curr: sample 3×3 neighborhood colors/depths Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
package/Shaders/ISTemporalAA.hlsl (2)
292-296: Transparency check performs 8 texture samples in a loop.The
allTransparentcheck samplesalphaTexfor each of the 8 non-BR neighbors. While this is necessary since each UV differs, consider whether these samples could be combined with the initial neighborhood sampling loop (lines 114-120) to reduce redundant texture fetches ifalphaTexvalues are needed elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISTemporalAA.hlsl` around lines 292 - 296, The loop computing allTransparent repeatedly calls alphaTex.Sample for each uvs[i], causing redundant fetches; integrate alpha sampling into the existing neighborhood sampling pass (the earlier loop that samples neighbors into uvs[]/neighborhood samples) by sampling alphaTex once per neighbor there and storing the results (e.g., an alphaValues[] or alongside the existing sampled data) and then use those stored values in the allTransparent test (the allTransparent variable and the alphaTex.Sample calls) so the separate 8-sample loop is removed and texture fetches are not duplicated.
42-42:JitterAndResis declared but never used in the shader.This constant buffer entry is allocated at
packoffset(c1)but has no references in the shader code. If this is reserved for future use or populated by the engine but not consumed here, consider adding a comment explaining its purpose to avoid confusion during maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISTemporalAA.hlsl` at line 42, The constant buffer entry "JitterAndRes : packoffset(c1)" is declared but never used; either remove it or document/consume it: if it's reserved/populated by the engine leave the declaration but add a clear comment above "JitterAndRes" stating its purpose (e.g., contains frame jitter in .xy and render resolution in .zw), otherwise update the ISTemporalAA shader to actually read JitterAndRes.xy for jitter and JitterAndRes.zw for resolution where jitter/resolution are applied; ensure the comment or usage references the exact symbol "JitterAndRes : packoffset(c1)" so future maintainers understand why it exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/ISTemporalAA.hlsl`:
- Around line 173-178: The comment for the 4-tap blend is wrong: the code
multiplies NeighborWeights.x,y,w,z with colors[4],colors[7],colors[6],colors[3]
respectively (so the actual mapping is x→C, y→D, z→L, w→LD). Either update the
comment to state "NeighborWeights.xyzw maps to C, D, L, LD" to match the
existing code, or if the original intent was x→C, y→D, z→LD, w→L then swap the
uses of NeighborWeights.z and NeighborWeights.w in the neighborColor expression
(symbols: NeighborWeights, neighborColor, colors[4], colors[7], colors[6],
colors[3]) so the code matches the original comment.
---
Nitpick comments:
In `@package/Shaders/ISTemporalAA.hlsl`:
- Around line 292-296: The loop computing allTransparent repeatedly calls
alphaTex.Sample for each uvs[i], causing redundant fetches; integrate alpha
sampling into the existing neighborhood sampling pass (the earlier loop that
samples neighbors into uvs[]/neighborhood samples) by sampling alphaTex once per
neighbor there and storing the results (e.g., an alphaValues[] or alongside the
existing sampled data) and then use those stored values in the allTransparent
test (the allTransparent variable and the alphaTex.Sample calls) so the separate
8-sample loop is removed and texture fetches are not duplicated.
- Line 42: The constant buffer entry "JitterAndRes : packoffset(c1)" is declared
but never used; either remove it or document/consume it: if it's
reserved/populated by the engine leave the declaration but add a clear comment
above "JitterAndRes" stating its purpose (e.g., contains frame jitter in .xy and
render resolution in .zw), otherwise update the ISTemporalAA shader to actually
read JitterAndRes.xy for jitter and JitterAndRes.zw for resolution where
jitter/resolution are applied; ensure the comment or usage references the exact
symbol "JitterAndRes : packoffset(c1)" so future maintainers understand why it
exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2dfb96e9-41f2-4a65-a2f0-aec3ccf20573
📒 Files selected for processing (1)
package/Shaders/ISTemporalAA.hlsl
alandtse
changed the title
|
✅ A pre-release build is available for this PR: |
alandtse
force-pushed
the
ISTemporalAA_VR
branch
from
ef2fd58 to
f7235f6
Compare
alandtse
force-pushed
the
ISTemporalAA_VR
branch
from
f7235f6 to
3c133a7
Compare
alandtse
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
package/Shaders/Common/VR.hlsli (1)
96-133: Consider aligning the PR title/scope with the VR-focused changes.Given the added VR utility and test coverage, a clearer title would be something like
refactor(vr): add applyvelocitytouv helper and tests.
If this change tracks an issue, consider addingAddresses #<id>in the PR description.As per coding guidelines, "Conventional Commit Titles... if the existing title does not describe the code changes" and "Issue References... Suggest adding appropriate GitHub keywords."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/VR.hlsli` around lines 96 - 133, Update the PR title and description to reflect the VR-focused change: use a conventional commit-style title like "refactor(vr): add ApplyVelocityToUV helper and tests" and, if this change fixes or tracks an issue, add an "Addresses #<id>" line in the PR body; mention the key symbols changed/added (ApplyVelocityToUV and Stereo::GetEyeIndexFromTexCoord in VR.hlsli) and note that new tests were added so reviewers immediately understand scope.package/Shaders/Tests/TestVR.hlsl (1)
339-362: Add edge assertions for the new boundary semantics.This test covers happy-path +
x > 1OOB, but it misses two explicit behaviors inApplyVelocityToUV: VRx < 0(clamped, not OOB) and inclusiveyboundaries (y <= 0/y >= 1=> OOB).🧪 Suggested test additions
[numthreads(1, 1, 1)] void TestApplyVelocityToUV() { float2 velocity = float2(0.1, 0.0); bool oob; @@ // In VR, out of bounds is clamped (mono x < 0 maps to 0 -> stereo 0, mono x > 1 saturates to 1 -> stereo 0.5 for left) ASSERT(IsTrue, abs(resultOob.x - 0.5) < kEps); + + // VR-specific: mono x < 0 is clamped and does NOT set OOB + float2 resultNegX = Stereo::ApplyVelocityToUV(float2(0.75, 0.5), float2(-1.0, 0.0), oob); + ASSERT(IsFalse, oob); + ASSERT(IsTrue, abs(resultNegX.x - 0.5) < kEps); + + // Inclusive Y boundaries should report OOB + Stereo::ApplyVelocityToUV(float2(0.25, 0.5), float2(0.0, -0.5), oob); // y == 0 + ASSERT(IsTrue, oob); + Stereo::ApplyVelocityToUV(float2(0.25, 0.5), float2(0.0, 0.5), oob); // y == 1 + ASSERT(IsTrue, oob); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Tests/TestVR.hlsl` around lines 339 - 362, The test TestApplyVelocityToUV is missing assertions for the clamped-left-edge and inclusive-y-boundary semantics of Stereo::ApplyVelocityToUV: add a case where mono x + velocity < 0 (e.g., input float2(0.05, 0.5) with velocity float2(-0.2,0)) and assert oob is false and the returned x equals the left-eye clamp (0.0 mono -> stereo 0 or stereo 0.0/0.5 depending on mapping) and add two cases for y boundaries (e.g., input y <= 0 and y >= 1) that assert oob is true for both and that the returned coordinates are appropriately clamped/saturated; reference the existing TestApplyVelocityToUV function and Stereo::ApplyVelocityToUV when adding these assertions.package/Shaders/Common/DisplayMapping.hlsli (1)
84-89: Pick an explicit policy for negative PQ inputs.
pow(abs(linearCol), ...)avoids NaNs, but it also makes-xencode identically to+xwhileConvertGameToPQ()still accepts signed values viaGammaToLinearSafe(). Either clamp negatives at the API boundary or preserve sign through encode/decode so the helper stays predictable.🔧 One simple option if negatives should be invalid
float3 LinearToPQ(float3 linearCol, const float maxPqValue) { linearCol /= maxPqValue; - float3 colToPow = pow(abs(linearCol), PQ_constant_N); + float3 colToPow = pow(max(linearCol, 0.0.xxx), PQ_constant_N); float3 numerator = PQ_constant_C1 + PQ_constant_C2 * colToPow; float3 denominator = 1.0 + PQ_constant_C3 * colToPow; float3 pq = pow(numerator / denominator, PQ_constant_M);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/DisplayMapping.hlsli` around lines 84 - 89, The PQ encode currently uses pow(abs(linearCol), ...) which hides sign and makes negative inputs encode identically to positive ones; choose and implement an explicit policy: either clamp negatives to zero at the API boundary (e.g. in ConvertGameToPQ() / GammaToLinearSafe() before calling the helper) or preserve sign through encoding by recording sign(linearCol) and applying it to the encoded result (so DisplayMapping helper uses sign(linearCol) * pow(abs(linearCol), ...)); update the helper and any corresponding decode path to match the chosen policy so behavior is predictable and reversible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/Shaders/Common/DisplayMapping.hlsli`:
- Around line 84-89: The PQ encode currently uses pow(abs(linearCol), ...) which
hides sign and makes negative inputs encode identically to positive ones; choose
and implement an explicit policy: either clamp negatives to zero at the API
boundary (e.g. in ConvertGameToPQ() / GammaToLinearSafe() before calling the
helper) or preserve sign through encoding by recording sign(linearCol) and
applying it to the encoded result (so DisplayMapping helper uses sign(linearCol)
* pow(abs(linearCol), ...)); update the helper and any corresponding decode path
to match the chosen policy so behavior is predictable and reversible.
In `@package/Shaders/Common/VR.hlsli`:
- Around line 96-133: Update the PR title and description to reflect the
VR-focused change: use a conventional commit-style title like "refactor(vr): add
ApplyVelocityToUV helper and tests" and, if this change fixes or tracks an
issue, add an "Addresses #<id>" line in the PR body; mention the key symbols
changed/added (ApplyVelocityToUV and Stereo::GetEyeIndexFromTexCoord in
VR.hlsli) and note that new tests were added so reviewers immediately understand
scope.
In `@package/Shaders/Tests/TestVR.hlsl`:
- Around line 339-362: The test TestApplyVelocityToUV is missing assertions for
the clamped-left-edge and inclusive-y-boundary semantics of
Stereo::ApplyVelocityToUV: add a case where mono x + velocity < 0 (e.g., input
float2(0.05, 0.5) with velocity float2(-0.2,0)) and assert oob is false and the
returned x equals the left-eye clamp (0.0 mono -> stereo 0 or stereo 0.0/0.5
depending on mapping) and add two cases for y boundaries (e.g., input y <= 0 and
y >= 1) that assert oob is true for both and that the returned coordinates are
appropriately clamped/saturated; reference the existing TestApplyVelocityToUV
function and Stereo::ApplyVelocityToUV when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d90d2d5-c1ae-4ace-97f1-cd0042b0cb1c
📒 Files selected for processing (5)
package/Shaders/Common/Color.hlslipackage/Shaders/Common/DisplayMapping.hlslipackage/Shaders/Common/VR.hlslipackage/Shaders/ISTemporalAA.hlslpackage/Shaders/Tests/TestVR.hlsl
alandtse
changed the title
2 participants
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests