fix(VR): screen space shadows desync

[alandtse]

Summary by CodeRabbit

• New Features

  • VR stereo shadow synchronization pass with a user-facing toggle to enable/disable it.

• Improvements

  • Streamlined shadow sampling for more predictable performance.
  • VR-specific default shadow settings adjusted for better VR visual quality.
  • Automatic setup/teardown of VR stereo shadow resources to avoid manual management.

[coderabbitai]

📝 Walkthrough

Walkthrough

Removes dynamic sampling controls and related CB fields from screen-space shadow shaders, unrolls sampling loops, adds a new VR StereoSync compute shader, integrates stereo sync and resource management into C++ rendering flow, and updates sample-count handling and VR gating.

Changes

Cohort / File(s)	Summary
Shader parameter & loops features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl, features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli	Removed DynamicSampleCount, DynamicReadCount, and padding from PerFrame/DispatchParameters; removed parameter assignments; converted dynamic break-based loops to fully unrolled fixed-iteration loops.
New VR stereo sync shader features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl	Added compute shader implementing depth-weighted blur, hash-based rotation, reprojection to the other eye, depth/edge validation, and occlusion-aware stereo blending for per-eye shadow synchronization.
C++ integration & API changes src/Features/ScreenSpaceShadows.cpp, src/Features/ScreenSpaceShadows.h	Added DrawStereoSync(), InvalidateRaymarchShaders(), VR resources (stereoSyncCB, stereoSyncCS, stereoSyncCopyTex), changed GetScaledSampleCount() signature/logic and sample-count lifecycle, wired stereo dispatch into Prepass/DrawShadows, and enabled VR support/gating.

Sequence Diagram(s)

sequenceDiagram
    participant CPU as CPU
    participant RayCS as RaymarchCS
    participant StereoCS as StereoSyncCS
    participant RT as RenderTargets

    CPU->>RayCS: Dispatch per-eye raymarch
    RayCS->>RT: Write SrcShadowTexture & SrcDepthTexture

    CPU->>StereoCS: Dispatch stereo sync (per-eye)
    StereoCS->>RT: Read SrcDepthTexture, SrcShadowTexture
    StereoCS->>StereoCS: Depth-weighted blur & dtid hash rotate
    StereoCS->>StereoCS: Reproject sample positions to other eye
    StereoCS->>StereoCS: Validate reprojection (depth/edge/occluder checks)
    StereoCS->>StereoCS: Finalize stereo blend (occlusion-aware)
    StereoCS->>RT: Write OutShadowTexture

    CPU->>CPU: Continue rendering pipeline

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Possibly related PRs

• feat: drs based upscaling #1443: Touches and previously added DynamicSampleCount/DynamicReadCount fields that are removed here.
• fix(screenspace shadows): fix some aliasing #1487: Overlaps with sampling-control and loop-unrolling changes affecting the same shader code paths.
• fix(VR): SSGI discrepancies #1926: Implements VR stereo-sync/reprojection features similar to the new StereoSyncCS shader.

Suggested reviewers

• davo0411
• jiayev

Poem

🐰
I hop between two eyes so bright,
Unrolling shadows into light,
Blur, reproject, then softly blend,
Two views become one at the end,
Hooray — VR shadows take flight!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(VR): screen space shadows desync' clearly and specifically describes the main objective of this pull request, which adds VR stereo synchronization features to address desynchronization issues in screen-space shadows.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/Features/ScreenSpaceShadows.cpp (1)

48-51: Incomplete resource cleanup in ClearShaderCache.

Only stereoSyncCS is released here, but stereoSyncCB and stereoSyncCopyTex are also VR-specific resources. If the shader cache clear is meant to release all shader-related resources, consider adding cleanup for these as well, or document why they should persist.

♻️ Suggested addition for consistent cleanup

 	if (stereoSyncCS) {
 		stereoSyncCS->Release();
 		stereoSyncCS = nullptr;
 	}
+	// Note: stereoSyncCB and stereoSyncCopyTex are intentionally retained
+	// as they are GPU resources that persist across shader recompilations

Or if full cleanup is intended:

 	if (stereoSyncCS) {
 		stereoSyncCS->Release();
 		stereoSyncCS = nullptr;
 	}
+	if (stereoSyncCB) {
+		delete stereoSyncCB;
+		stereoSyncCB = nullptr;
+	}
+	if (stereoSyncCopyTex) {
+		delete stereoSyncCopyTex;
+		stereoSyncCopyTex = nullptr;
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/ScreenSpaceShadows.cpp` around lines 48 - 51, The
ClearShaderCache block currently only releases stereoSyncCS; update it to also
release and null out the other VR-related shader resources (stereoSyncCB and
stereoSyncCopyTex) to avoid leaks or dangling pointers—i.e., check each of
stereoSyncCB and stereoSyncCopyTex like stereoSyncCS, call Release() if
non-nullptr, and set them to nullptr; ensure this change is applied inside the
same ClearShaderCache/cleanup routine where stereoSyncCS is handled so all VR
shader resources are consistently cleaned up.

🤖 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/Features/ScreenSpaceShadows.h`:
- Around line 82-85: Add a destructor for the ScreenSpaceShadows class that
deletes the heap-allocated VR wrapper objects allocated in SetupResources():
delete stereoSyncCB and delete stereoSyncCopyTex (and set them to nullptr). Keep
stereoSyncCS release logic in ClearShaderCache() unchanged; the destructor
should only free the ConstantBuffer* stereoSyncCB and Texture2D*
stereoSyncCopyTex to avoid leaks and ensure pointers are nulled.

---

Nitpick comments:
In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 48-51: The ClearShaderCache block currently only releases
stereoSyncCS; update it to also release and null out the other VR-related shader
resources (stereoSyncCB and stereoSyncCopyTex) to avoid leaks or dangling
pointers—i.e., check each of stereoSyncCB and stereoSyncCopyTex like
stereoSyncCS, call Release() if non-nullptr, and set them to nullptr; ensure
this change is applied inside the same ClearShaderCache/cleanup routine where
stereoSyncCS is handled so all VR shader resources are consistently cleaned up.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a653c74 and d6e7ccb.

📒 Files selected for processing (6)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl
• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/ScreenSpaceShadows.hlsli
• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/bend_sss_gpu.hlsli
• src/Features/ScreenSpaceShadows.cpp
• src/Features/ScreenSpaceShadows.h

💤 Files with no reviewable changes (1)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/RaymarchCS.hlsl

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Features/ScreenSpaceShadows.cpp (1)

79-99: ⚠️ Potential issue | 🟠 Major

Avoid dynamic-resolution-driven shader recompilation churn.

GetScaledSampleCount() now depends on dynamic render size, and Lines 97-99 invalidate shaders whenever that integer changes. With dynamic resolution, this can cause frequent runtime recompiles/stutter on the render thread.

Suggested mitigation

 uint ScreenSpaceShadows::GetScaledSampleCount()
 {
 	float2 renderSize = Util::ConvertToDynamic(globals::state->screenSize);
@@
-	uint scaledSampleCount = static_cast<uint>(std::round(bendSettings.SampleCount * 60 * areaScale));
+	uint scaledSampleCount = static_cast<uint>(std::round(bendSettings.SampleCount * 60 * areaScale));
+	// Quantize/clamp to reduce recompilation churn from small DRS oscillations.
+	scaledSampleCount = std::clamp(((scaledSampleCount + 7u) / 8u) * 8u, 8u, 256u);
 
 	return scaledSampleCount;
 }

As per coding guidelines, “Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance”.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/ScreenSpaceShadows.cpp` around lines 79 - 99,
GetScaledSampleCount currently returns a value tied directly to dynamic
renderSize and GetComputeRaymarch invalidates/recompiles shaders whenever that
integer changes (via lastCompiledSampleCount and InvalidateRaymarchShaders),
which causes churn; modify the logic so scaled sample count is coarse-grained or
debounced before triggering recompilation — e.g., quantize the output of
GetScaledSampleCount into steps (or apply a percent/hysteresis threshold or
require the value to be stable for N frames) based on bendSettings.SampleCount
so that only meaningful changes update lastCompiledSampleCount and call
InvalidateRaymarchShaders in GetComputeRaymarch.

🤖 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/Features/ScreenSpaceShadows.cpp`:
- Around line 274-291: The compute pass binds globals::state->sharedDataCB->CB()
to CS constant buffer slot 5 but only clears slot 1 on cleanup, risking state
leakage; after the Dispatch that uses stereoSyncCS, unbind slot 5 by setting a
nullptr constant buffer at slot 5 (similar to the existing
CSSetConstantBuffers(1,1,&cbPtr) clear) so both slot 1 and slot 5 are cleared
before resetting shader/resources (use the same cbPtr nullptr and call
context->CSSetConstantBuffers(5,1,&cbPtr) before
context->CSSetShader(nullptr,...)).
- Around line 70-77: ClearShaderCache() currently releases stereoSyncCS but
doesn't recompile it, causing DrawStereoSync() to silently return when shaders
are cleared in VR; after releasing stereoSyncCS in
ScreenSpaceShadows::ClearShaderCache(), call CompileComputeShaders() (same
pattern used by ScreenSpaceGI::ClearShaderCache() and
Skylighting::ClearShaderCache()) to recreate the compute shader, or
alternatively implement a lazy getter (e.g., GetComputeStereoSync()) that
recompiles on demand; update ClearShaderCache() to call CompileComputeShaders()
immediately after setting stereoSyncCS to nullptr so DrawStereoSync() will have
a valid shader.

---

Outside diff comments:
In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 79-99: GetScaledSampleCount currently returns a value tied
directly to dynamic renderSize and GetComputeRaymarch invalidates/recompiles
shaders whenever that integer changes (via lastCompiledSampleCount and
InvalidateRaymarchShaders), which causes churn; modify the logic so scaled
sample count is coarse-grained or debounced before triggering recompilation —
e.g., quantize the output of GetScaledSampleCount into steps (or apply a
percent/hysteresis threshold or require the value to be stable for N frames)
based on bendSettings.SampleCount so that only meaningful changes update
lastCompiledSampleCount and call InvalidateRaymarchShaders in
GetComputeRaymarch.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6e7ccb and 6e7f06a.

📒 Files selected for processing (3)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
• src/Features/ScreenSpaceShadows.cpp
• src/Features/ScreenSpaceShadows.h

🚧 Files skipped from review as they are similar to previous changes (1)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl

[coderabbitai]

🧹 Nitpick comments (2)

features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl (1)

26-28: Consider documenting the tuning rationale for bilateral blur constants.

These constants control the blur behavior but lack documentation on how they were tuned or their expected visual impact. A brief comment would help future maintainers adjust them if needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@features/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
around lines 26 - 28, Add brief explanatory comments above the constants
kDepthSigma, kMaxBlend, and kEdgeDepthThreshold in StereoSyncCS.hlsl describing
what each controls (e.g., depth weight falloff for bilateral blur, maximum blend
strength between original and blurred pixels, and the depth difference threshold
that prevents blending across edges), the units/range expected (suggested
working ranges like sigma ~0.005–0.05, max blend 0–1, threshold ~0.01–0.1), and
the visual impact of increasing/decreasing each value to help future tuning and
debugging; keep each explanation one short sentence referencing the constant
name so maintainers can quickly adjust those values.

src/Features/ScreenSpaceShadows.cpp (1)

347-402: Consider removing duplicate shader compilation in SetupResources().

stereoSyncCS is compiled in both SetupResources() (line 353) and lazily in DrawStereoSync() (line 258). Since DrawStereoSync() handles null gracefully, the compilation in SetupResources() is redundant and increases startup time. Consider removing it or documenting the intentional pre-compilation.

Proposed change - remove redundant compilation

 	if (globals::game::isVR) {
 		stereoSyncCB = new ConstantBuffer(ConstantBufferDesc<StereoSyncCB>());
-		stereoSyncCS = reinterpret_cast<ID3D11ComputeShader*>(Util::CompileShader(L"Data\\Shaders\\ScreenSpaceShadows\\StereoSyncCS.hlsl", { { "VR", "" }, { "FRAMEBUFFER", "" } }, "cs_5_0"));
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/ScreenSpaceShadows.cpp` around lines 347 - 402, Remove the
redundant eager shader compile in SetupResources: in the VR branch of
ScreenSpaceShadows::SetupResources delete the stereoSyncCS =
reinterpret_cast<ID3D11ComputeShader*>(Util::CompileShader(...)) call and leave
only the stereoSyncCB allocation (stereoSyncCB = new
ConstantBuffer(ConstantBufferDesc<StereoSyncCB>())); DrawStereoSync already
lazy-compiles stereoSyncCS when needed and handles null, so rely on that to
avoid duplicated work (or if you prefer eager compile, instead remove the lazy
compile from DrawStereoSync and document the choice).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@features/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl:
- Around line 26-28: Add brief explanatory comments above the constants
kDepthSigma, kMaxBlend, and kEdgeDepthThreshold in StereoSyncCS.hlsl describing
what each controls (e.g., depth weight falloff for bilateral blur, maximum blend
strength between original and blurred pixels, and the depth difference threshold
that prevents blending across edges), the units/range expected (suggested
working ranges like sigma ~0.005–0.05, max blend 0–1, threshold ~0.01–0.1), and
the visual impact of increasing/decreasing each value to help future tuning and
debugging; keep each explanation one short sentence referencing the constant
name so maintainers can quickly adjust those values.

In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 347-402: Remove the redundant eager shader compile in
SetupResources: in the VR branch of ScreenSpaceShadows::SetupResources delete
the stereoSyncCS =
reinterpret_cast<ID3D11ComputeShader*>(Util::CompileShader(...)) call and leave
only the stereoSyncCB allocation (stereoSyncCB = new
ConstantBuffer(ConstantBufferDesc<StereoSyncCB>())); DrawStereoSync already
lazy-compiles stereoSyncCS when needed and handles null, so rely on that to
avoid duplicated work (or if you prefer eager compile, instead remove the lazy
compile from DrawStereoSync and document the choice).

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e7f06a and 82416e6.

📒 Files selected for processing (2)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
• src/Features/ScreenSpaceShadows.cpp

[coderabbitai]

♻️ Duplicate comments (1)

src/Features/ScreenSpaceShadows.cpp (1)

287-304: ⚠️ Potential issue | 🟡 Minor

Unbind CS constant buffer slot 5 during stereo-sync cleanup.

Line 289 binds CS slot 5, but cleanup (Lines 301-304) only clears slot 1. Please clear slot 5 too to avoid compute-state leakage into later passes.

Proposed fix

 	cbPtr = nullptr;
 	context->CSSetShaderResources(0, 2, srvs);
 	context->CSSetUnorderedAccessViews(0, 1, uavs, nullptr);
 	context->CSSetConstantBuffers(1, 1, &cbPtr);
+	context->CSSetConstantBuffers(5, 1, &cbPtr);
 	context->CSSetShader(nullptr, nullptr, 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.”

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/ScreenSpaceShadows.cpp` around lines 287 - 304, The cleanup
misses unbinding the compute shader constant buffer bound to slot 5
(sharedDataBuf); update the cleanup sequence after Dispatch in
ScreenSpaceShadows.cpp to also clear CS slot 5 by calling
context->CSSetConstantBuffers(5, 1, &cbPtr) (using the same cbPtr = nullptr
pattern used for slot 1) so both slot 1 and slot 5 are unbound before resetting
shader resources/UAVs and clearing the CS with context->CSSetShader(nullptr,
nullptr, 0).

🧹 Nitpick comments (1)

src/Features/ScreenSpaceShadows.cpp (1)

116-123: Decouple right-eye shader recompilation from call order.

At Line 118, raymarchRightCS only recompiles when null. This works with current call order, but it couples correctness to GetComputeRaymarch() being called first after sample-count changes.

Proposed refactor

 ID3D11ComputeShader* ScreenSpaceShadows::GetComputeRaymarchRight()
 {
+	uint scaledSampleCount = GetScaledSampleCount();
+	if (scaledSampleCount != lastCompiledSampleCount) {
+		lastCompiledSampleCount = scaledSampleCount;
+		InvalidateRaymarchShaders();
+	}
+
 	if (!raymarchRightCS) {
-		uint scaledSampleCount = GetScaledSampleCount();
 		raymarchRightCS = (ID3D11ComputeShader*)Util::CompileShader(L"Data\\Shaders\\ScreenSpaceShadows\\RaymarchCS.hlsl", { { "SAMPLE_COUNT", std::format("{}", scaledSampleCount).c_str() }, { "RIGHT", "" } }, "cs_5_0");
 	}
 	return raymarchRightCS;
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Features/ScreenSpaceShadows.cpp` around lines 116 - 123,
GetComputeRaymarchRight currently only compiles a shader when raymarchRightCS is
null, which couples correctness to call order with GetComputeRaymarch; update
GetComputeRaymarchRight to track the last-used scaled sample count (e.g. a
member like raymarchSampleCount or raymarchRightSampleCount) and compare it to
the current GetScaledSampleCount() on each call, and if different (or
raymarchRightCS is null) release/replace raymarchRightCS by recompiling with the
current scaledSampleCount and the "RIGHT" define; this mirrors the logic used by
GetComputeRaymarch so right-eye shader recompilation is independent of call
order.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 287-304: The cleanup misses unbinding the compute shader constant
buffer bound to slot 5 (sharedDataBuf); update the cleanup sequence after
Dispatch in ScreenSpaceShadows.cpp to also clear CS slot 5 by calling
context->CSSetConstantBuffers(5, 1, &cbPtr) (using the same cbPtr = nullptr
pattern used for slot 1) so both slot 1 and slot 5 are unbound before resetting
shader resources/UAVs and clearing the CS with context->CSSetShader(nullptr,
nullptr, 0).

---

Nitpick comments:
In `@src/Features/ScreenSpaceShadows.cpp`:
- Around line 116-123: GetComputeRaymarchRight currently only compiles a shader
when raymarchRightCS is null, which couples correctness to call order with
GetComputeRaymarch; update GetComputeRaymarchRight to track the last-used scaled
sample count (e.g. a member like raymarchSampleCount or
raymarchRightSampleCount) and compare it to the current GetScaledSampleCount()
on each call, and if different (or raymarchRightCS is null) release/replace
raymarchRightCS by recompiling with the current scaledSampleCount and the
"RIGHT" define; this mirrors the logic used by GetComputeRaymarch so right-eye
shader recompilation is independent of call order.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 82416e6 and 459f354.

📒 Files selected for processing (2)

• features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
• src/Features/ScreenSpaceShadows.cpp