Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverts deferred composite from a compute dispatch to a fullscreen VS+PS draw, refactors GBuffer normal encode/decode to an analytic reconstruction with signature/type changes, updates related tests, adds a D3D11 state-backup utility, updates Deferred pipeline/shader cache and bindings, and shifts a few texture register indices. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Context as D3D11 DeviceContext
participant State as Util::D3DStateBackup
participant Pipeline as Graphics Pipeline (VS/PS)
participant GPU as GPU
App->>State: Backup(context)
State->>Context: Get* state & capture pointers
Context-->>State: Return captured state
App->>Context: CopySubresourceRegion(kMAIN → kMAIN_COPY)
Context->>GPU: Perform texture copy
App->>Context: Bind compositeBlend/Depth/RS states
App->>Context: Set VS (DeferredCompositeVS) and PS (DeferredCompositePS)
App->>Context: Bind SRVs (mainCopy.SRV, GBuffers, SSGI...)
App->>Context: Set RTV(s) and optional stencil DS state
App->>GPU: Draw(3) fullscreen triangle
GPU->>GPU: VS -> PS -> Output Merge (SV_Target0, SV_Target1)
App->>State: Restore(context)
State->>Context: Reapply saved bindings & Release()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
No actionable suggestions for changed features. |
alandtse
reviewed
Apr 19, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/Common/GBuffer.hlsli`:
- Around line 14-21: The DecodeNormal function's pole/sign convention is
inverted: for enc=(0.5,0.5) (EncodeNormal(0,0,1)) the current n.z = f/2 - 1
yields -1; change the z reconstruction in DecodeNormal to the matching sign
convention (set n.z = 1.0 - f/2.0) and return a normalized normal (normalize(n))
to ensure unit-length output; update the DecodeNormal(half2 enc) implementation
accordingly so it matches EncodeNormal and the up-normal tests.
In `@package/Shaders/Tests/TestGBuffer.hlsl`:
- Around line 8-23: The test currently assumes both exact pole normals roundtrip
through GBuffer::EncodeNormal/GBuffer::DecodeNormal, but the chosen encoding
only preserves one pole; update the TestGBuffer.hlsl tests to stop expecting
both exact poles to decode to themselves: adjust the testNormals or the ASSERT
checks so the pole that the implementation supports (the pole that DecodeNormal
returns for xy==0) is asserted exact (or within existing 0.05h tolerance) and
replace the opposite pole with a near-pole sample (e.g., tiny nonzero x or y)
and assert it decodes within tolerance; apply the same change for the similar
cases around lines 48-56 so tests no longer require symmetric pole roundtrip but
still validate near-pole behavior for both poles.
In `@src/Deferred.cpp`:
- Around line 415-423: The composite fullscreen draw leaves HS/DS (hull/domain
shaders) possibly bound which can leave the device in a tessellation state
incompatible with Draw(3, 0); extend the D3DStateBackup class to capture and
restore hull and domain shaders (HS/DS) alongside VS/PS/GS, then before the
composite draw (where GetCompositeVS/GetCompositePS(interior) are set and GS is
explicitly unbound) explicitly unbind HS and DS (set them to nullptr) so the IA
primitive topology D3D11_PRIMITIVE_TOPOLOGY_TRIANGLELIST is valid; ensure the
backup/restore paths handle nulls gracefully and do not leak references.
In `@src/Utils/D3DStateBackup.h`:
- Around line 7-12: D3DStateBackup currently stores AddRef'd COM pointers via
Backup() but is trivially copyable and lacks a destructor, leading to double
Release or leaks; make D3DStateBackup non-copyable (delete copy constructor and
copy assignment), implement a move constructor and move assignment to transfer
ownership, add a destructor that releases any held COM pointers, and ensure
Restore() and Backup() manage reference counts consistently (e.g., call AddRef
when capturing and Release in destructor/Restore), updating any helper members
(kNumSRVSlots, kNumSamplerSlots, kNumCBSlots) usage accordingly so captured
arrays are safely cleared on move and release on destruction.
🪄 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: 4972dc2f-2b07-4172-a36d-2755a0b3b393
📒 Files selected for processing (8)
package/Shaders/Common/GBuffer.hlslipackage/Shaders/DeferredCompositePS.hlslpackage/Shaders/DeferredCompositeVS.hlslpackage/Shaders/Tests/TestGBuffer.hlslsrc/Deferred.cppsrc/Deferred.hsrc/Features/VRStereoOptimizations.hsrc/Utils/D3DStateBackup.h
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/Shaders/Tests/TestGBuffer.hlsl (1)
16-23: Keep the tests proving encoded range and decoded direction.The angled-normal test now only checks unit length, so a decoder returning the wrong unit vector would pass. Also keep
[0,1]assertions forEncodeNormal, since these values are stored through unorm GBuffer channels.🧪 Proposed test strengthening
for (int i = 0; i < 6; i++) { half3 original = normalize(testNormals[i]); half2 encoded = GBuffer::EncodeNormal(original); half3 decoded = GBuffer::DecodeNormal(encoded); + ASSERT(IsTrue, encoded.x >= 0.0h && encoded.x <= 1.0h); + ASSERT(IsTrue, encoded.y >= 0.0h && encoded.y <= 1.0h); ASSERT(IsTrue, abs(decoded.x - original.x) < 0.05h); ASSERT(IsTrue, abs(decoded.y - original.y) < 0.05h); ASSERT(IsTrue, abs(decoded.z - original.z) < 0.05h); } @@ for (int i = 0; i < 4; i++) { half3 original = testNormals[i]; half2 encoded = GBuffer::EncodeNormal(original); half3 decoded = GBuffer::DecodeNormal(encoded); + ASSERT(IsTrue, encoded.x >= 0.0h && encoded.x <= 1.0h); + ASSERT(IsTrue, encoded.y >= 0.0h && encoded.y <= 1.0h); + ASSERT(IsTrue, dot(decoded, original) > 0.95h); half length = sqrt(decoded.x * decoded.x + decoded.y * decoded.y + decoded.z * decoded.z); ASSERT(IsTrue, abs(length - 1.0h) < 0.05h); } @@ half2 encoded = GBuffer::EncodeNormal(equatorNormal); half3 decoded = GBuffer::DecodeNormal(encoded); + ASSERT(IsTrue, encoded.x >= 0.0h && encoded.x <= 1.0h); + ASSERT(IsTrue, encoded.y >= 0.0h && encoded.y <= 1.0h); ASSERT(IsTrue, abs(decoded.x - 1.0h) < 0.01h);Also applies to: 36-42, 47-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Tests/TestGBuffer.hlsl` around lines 16 - 23, The tests in TestGBuffer.hlsl for the angled-normal cases currently only verify unit length and therefore would accept wrong directions; update the test loops (the blocks around the GBuffer::EncodeNormal/GBuffer::DecodeNormal usages — repeated at the other sites mentioned) to also assert that the encoded half2 is within [0,1] (since it will be stored in unorm channels) and that the decoded half3 matches the original direction component-wise (e.g., abs(decoded.x - original.x) < 0.05h, same for y and z) in addition to the unit-length check; apply these extra assertions to the other test blocks (the ranges referenced in the comment) so both encoded-range and decoded-direction are validated.
🤖 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/Tests/TestGBuffer.hlsl`:
- Around line 16-23: The tests in TestGBuffer.hlsl for the angled-normal cases
currently only verify unit length and therefore would accept wrong directions;
update the test loops (the blocks around the
GBuffer::EncodeNormal/GBuffer::DecodeNormal usages — repeated at the other sites
mentioned) to also assert that the encoded half2 is within [0,1] (since it will
be stored in unorm channels) and that the decoded half3 matches the original
direction component-wise (e.g., abs(decoded.x - original.x) < 0.05h, same for y
and z) in addition to the unit-length check; apply these extra assertions to the
other test blocks (the ranges referenced in the comment) so both encoded-range
and decoded-direction are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 043980eb-29b9-4999-ae71-a4b90df99263
📒 Files selected for processing (4)
features/IBL/Shaders/IBL/IBL.hlslipackage/Shaders/DeferredCompositePS.hlslpackage/Shaders/Tests/TestGBuffer.hlslsrc/Deferred.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Deferred.cpp
Collaborator
|
@coderabbitai please confirm this does not break any existing functionality like stereo reprojection. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package/Shaders/Common/GBuffer.hlsli (1)
7-21:⚠️ Potential issue | 🟠 MajorHandle the exact
+Zpole explicitly.
EncodeNormal(float3(0, 0, 1))still returnsfloat2(0.5, 0.5), andDecodeNormal(float2(0.5, 0.5))reconstructsfloat3(0, 0, -1). Near-pole tests avoid this, but an exact +Z normal would still flip.🐛 One possible fix while keeping the current center-as-`-Z` convention
float2 EncodeNormal(float3 n) { + if (n.z > 0.999999 && dot(n.xy, n.xy) < 1e-12) { + return float2(1.0, 0.5); + } n.z = max(0.001, sqrt(8.0 - 8.0 * n.z)); n.xy /= n.z; return n.xy + 0.5; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/GBuffer.hlsli` around lines 7 - 21, The encoding/decoding pair flips the exact +Z pole; update EncodeNormal and DecodeNormal to special-case the +Z pole: in EncodeNormal(float3 n) detect if n.z is >= 1.0 - EPS (or very close to 1.0) and return float2(0.5,0.5) directly, and in DecodeNormal(float2 enc) detect when enc is within EPS of float2(0.5,0.5) and return float3(0,0,1) (otherwise keep the existing math); use a small epsilon (e.g. 1e-6) for the comparisons to avoid floating-point equality issues.
🤖 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/Tests/TestGBuffer.hlsl`:
- Around line 30-43: The test currently only verifies decoded normal magnitude;
update the test loop that iterates testNormals and calls GBuffer::EncodeNormal /
GBuffer::DecodeNormal to also assert direction correctness by comparing the
decoded normal to the original (e.g., require dot(decoded, original) near 1.0 or
check per-component closeness) in addition to the existing length check; use the
same ASSERT/IsTrue mechanism to fail when abs(dot(decoded, original) - 1.0)
exceeds a small threshold (or when component differences exceed a small
epsilon).
---
Duplicate comments:
In `@package/Shaders/Common/GBuffer.hlsli`:
- Around line 7-21: The encoding/decoding pair flips the exact +Z pole; update
EncodeNormal and DecodeNormal to special-case the +Z pole: in
EncodeNormal(float3 n) detect if n.z is >= 1.0 - EPS (or very close to 1.0) and
return float2(0.5,0.5) directly, and in DecodeNormal(float2 enc) detect when enc
is within EPS of float2(0.5,0.5) and return float3(0,0,1) (otherwise keep the
existing math); use a small epsilon (e.g. 1e-6) for the comparisons to avoid
floating-point equality issues.
🪄 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: 1cc64991-851c-47ff-bfa8-8ae2a6100834
📒 Files selected for processing (3)
package/Shaders/Common/GBuffer.hlslipackage/Shaders/Tests/TestGBuffer.hlslsrc/Deferred.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Deferred.cpp
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 13636 🏁 Script executed: Length of output: 237 🏁 Script executed: Length of output: 1653 🏁 Script executed: Length of output: 18201 🏁 Script executed: Length of output: 19867
✅ Stereo Reprojection — Core Pipeline IntactThe heart of the stereo optimization pipeline —
✅ Eye-1 Skip — Replaced by Stencil (Equivalent Behavior)The old compute path checked
This is functionally equivalent and is actually a cleaner mechanism — hardware stencil culling avoids divergent branching in the shader. ✅ Motion Vectors — Not BrokenMotion vectors are written by the geometry shaders themselves (
✅ VR-Specific Paths in DeferredCompositePS — PreservedThe new pixel shader retains all VR-specific logic:
✅ GBuffer Normal Encoding Change — Self-ConsistentNeither
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Features/ScreenSpaceGI.cpp (1)
776-776: LGTM — the three SRV bindings (radiance disocc, GI, blur) are migrated consistently toglobals::deferred->normalRoughnessRT. Notedeferredis already aliased locally at line 726; consider usingrts[deferred->normalRoughnessRT].SRVfor consistency with the adjacentrts[deferred->forwardRenderTargets[0]].SRVusage, but this is purely stylistic.Also applies to: 829-829, 861-861
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/ScreenSpaceGI.cpp` at line 776, Replace usages of globals::deferred in SRV lookups with the local alias deferred for consistency: change expressions like rts[globals::deferred->normalRoughnessRT].SRV to rts[deferred->normalRoughnessRT].SRV (and likewise for the other two occurrences where radiance disocc, GI, blur SRVs were migrated) so they match the adjacent rts[deferred->forwardRenderTargets[0]].SRV style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Features/ScreenSpaceGI.cpp`:
- Line 776: Replace usages of globals::deferred in SRV lookups with the local
alias deferred for consistency: change expressions like
rts[globals::deferred->normalRoughnessRT].SRV to
rts[deferred->normalRoughnessRT].SRV (and likewise for the other two occurrences
where radiance disocc, GI, blur SRVs were migrated) so they match the adjacent
rts[deferred->forwardRenderTargets[0]].SRV style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 21ff6cd3-d93d-4e28-b9f3-8efd2b6da741
📒 Files selected for processing (4)
src/Deferred.cppsrc/Deferred.hsrc/Features/ScreenSpaceGI.cppsrc/Features/SubsurfaceScattering.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Deferred.h
- src/Deferred.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Deferred.cpp (1)
418-426:⚠️ Potential issue | 🟠 MajorClear tessellation stages before the fullscreen draw.
GSis cleared, but staleHS/DSbindings can still leak into this triangle-list fullscreen draw. Extend the state backup to capture/restore HS/DS and explicitly unbind them beforeDraw(3, 0). As per coding guidelines, “Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations.”#!/bin/bash # Verify whether Deferred.cpp clears only GS and whether D3DStateBackup currently captures HS/DS. # Expected fixed result: Deferred draw clears HSSetShader/DSSetShader, and backup/restore handles HS/DS. rg -n -C3 'GSSetShader|HSSetShader|DSSetShader|class D3DStateBackup|struct D3DStateBackup|Backup\(.*context|Restore\(.*context' src/Deferred.cpp src/Utils/D3DStateBackup.h🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 418 - 426, The fullscreen composite draw currently clears GS but may leak Hull/Domain shaders; update D3DStateBackup to capture and restore HS/DS (add storage of the hull and domain shader interfaces and include them in Backup/Restore) and, in the draw path before context->Draw(3, 0), explicitly unbind hull/domain shaders via context->HSSetShader(nullptr, nullptr, 0) and context->DSSetShader(nullptr, nullptr, 0); ensure the changes reference D3DStateBackup, HSSetShader, DSSetShader, GetCompositeVS/GetCompositePS and maintain existing GS clearing logic so HS/DS are both saved/restored and explicitly cleared for the triangle-list fullscreen pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Deferred.cpp`:
- Around line 375-405: The normalRoughness texture is currently bound as both an
SRV (t2) and an RTV (normalRoughness.RTV) which is illegal in D3D11; create a
copy like mainCopy (e.g., normalRoughnessCopy or normalRoughnessCopy.SRV) before
the draw and sample from that SRV in the SRV array passed to
PSSetShaderResources (replace normalRoughness.SRV with the copy's SRV), while
leaving normalRoughness.RTV bound as the render target; mirror the existing
mainCopy copy-before-sample logic used elsewhere (see mainCopy,
PSSetShaderResources and OMSetRenderTargets) so DeferredCompositePS.hlsl reads
from the copied SRV not the active RTV.
---
Duplicate comments:
In `@src/Deferred.cpp`:
- Around line 418-426: The fullscreen composite draw currently clears GS but may
leak Hull/Domain shaders; update D3DStateBackup to capture and restore HS/DS
(add storage of the hull and domain shader interfaces and include them in
Backup/Restore) and, in the draw path before context->Draw(3, 0), explicitly
unbind hull/domain shaders via context->HSSetShader(nullptr, nullptr, 0) and
context->DSSetShader(nullptr, nullptr, 0); ensure the changes reference
D3DStateBackup, HSSetShader, DSSetShader, GetCompositeVS/GetCompositePS and
maintain existing GS clearing logic so HS/DS are both saved/restored and
explicitly cleared for the triangle-list fullscreen pass.
🪄 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: 5b68cd7a-149a-45f5-a7b3-0d7b10b51698
📒 Files selected for processing (2)
package/Shaders/DeferredCompositePS.hlslsrc/Deferred.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- package/Shaders/DeferredCompositePS.hlsl
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
doodlum
changed the title
|
✅ A pre-release build is available for this PR: |
Merged
This was referenced Apr 21, 2026
alandtse
added a commit
to alandtse/open-shaders
that referenced
this pull request
Apr 29, 2026
This reverts commit 7f64e55.
This was referenced Apr 29, 2026
alandtse
added a commit
that referenced
this pull request
Apr 30, 2026
6 tasks
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
2 participants
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.
VRStereoOptimizations should now run much faster. This pass should run much faster on all hardware. However I have not tested this yet.
Summary by CodeRabbit