perf(volumetric shadows): optimise data and fix cascades

[doodlum]

Summary by CodeRabbit

• Refactor
  • Volumetric shadow pipeline reworked to remove the GPU per-geometry copy stage and instead supply directional shadow parameters via a single shader-readable buffer from CPU.
• Bug Fixes
  • Improved shadow projection, depth sampling, VR handling, cascade boundaries and fade math for more accurate, stable volumetric shadows.
• Chores
  • Simplified shader inputs and reduced redundant bindings for clearer, more efficient runtime behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces a GPU compute-shader per-geometry copy with a CPU-populated directional-shadow structured buffer and SRV (t98); removes the CopyShadowDataCS shader and per-geometry buffer, and updates shaders and call sites to consume DirectionalShadowLightData via FrameBuffer::GetShadowDepth.

Changes

Cohort / File(s)	Summary
Compute Shader Removal features/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlsl	Deleted compute shader, PerGeometry layout, constant buffers, and main entry that populated RWStructuredBuffer<PerGeometry>.
Volumetric Shadows Shader Updates features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli	Removed ShadowData/SharedShadowData; GetVSMShadow3D/GetVSMShadow2D now read from DirectionalShadowLights[0], use FrameBuffer::GetShadowDepth, add camera-position adjustments, switch to float4x4 shadow proj multiplication, change fade math, and update cascade bounds comparisons.
Common Shader Utilities package/Shaders/Common/FrameBuffer.hlsli, package/Shaders/Common/ShadowSampling.hlsli	Added FrameBuffer::GetShadowDepth(float3,uint) and new DirectionalShadowLightData struct plus StructuredBuffer<DirectionalShadowLightData> DirectionalShadowLights : register(t98).
Deferred Shadow Management src/Deferred.h, src/Deferred.cpp	Added C++ DirectionalShadowLightData struct, created directionalShadowLights dynamic structured buffer and SRV, implemented CopyShadowLightData() and SetShadowCascadeParameters() to populate ShadowProj/InvShadowProj and split distances, and bind SRV to slot t98.
VolumetricShadows Refactor src/Features/VolumetricShadows.h, src/Features/VolumetricShadows.cpp	Removed C++ PerGeometry struct, perShadow buffer and copyShadowCS; renamed CopyShadowData() → CopyShadowLightData() and removed compute-dispatch and associated UAV/SRV usage.
Call Site Updates src/State.cpp	Replaced calls to CopyShadowData() with CopyShadowLightData() in the shadowmask rendering path.

Sequence Diagram(s)

sequenceDiagram
    participant CPU as rgba(70,130,180,0.5) CPU / Deferred
    participant GPU as rgba(34,139,34,0.5) GPU (StructuredBuffer t98)
    participant Shader as rgba(218,165,32,0.5) Volumetric Shader
    participant DepthUtil as rgba(147,112,219,0.5) FrameBuffer::GetShadowDepth

    CPU->>CPU: Render shadow maps
    CPU->>CPU: CopyShadowLightData()\nPopulate DirectionalShadowLightData
    CPU->>GPU: Map/Memcpy/Unmap structured buffer\nBind SRV to t98
    Shader->>GPU: Read DirectionalShadowLights[0]
    Shader->>DepthUtil: GetShadowDepth(positionWS, eyeIndex)
    DepthUtil-->>Shader: NDC depth for cascade checks

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat: volumetric shadows #1874: Modifies the volumetric shadow pipeline and per-geometry copy flow that this PR replaces.

Suggested reviewers

• alandtse
• davo0411
• soda3000

Poem

🐰 I hopped through code and packed the light,
One buffer fed from CPU at night.
Cascades arranged in tidy rows,
No shader copy, just tidy shows —
Shadows snug in matrices bright.

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: replacing a compute shader-based per-geometry shadow data copy with a direct structured buffer approach for volumetric shadows, which optimizes performance and fixes cascade parameter handling.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/directional-shadow-data

---

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: 2

🤖 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 600-609: In SetShadowCascadeParameters, the loop iterates over
lightData.shadowmapDescriptors.size() but writes into dd.ShadowProj and
dd.InvShadowProj which only have two entries, causing potential out-of-bounds
writes; fix by clamping the iteration count to the fixed cascade array size
(e.g., compute count = std::min(lightData.shadowmapDescriptors.size(),
size_t(2)) or use the known constant for the cascade count) and iterate up to
that count when populating dd.ShadowProj[i] and dd.InvShadowProj[i].

In `@src/Features/VolumetricShadows.cpp`:
- Around line 273-274: The current binding logic always binds shadowCopySRV even
when there is no source shadowView this frame, causing stale VSM data; change
the logic around shadowCopySRV/shadowView so that if shadowView is null you bind
nullptr instead of shadowCopySRV. Concretely, in the block that computes srv
before calling context->PSSetShaderResources(18, 1, &srv), set srv to nullptr
when shadowView is null; otherwise choose shadowCopySRV if available, else
shadowView. Keep using the existing context->PSSetShaderResources call and
ensure no leftover SRV is rebound when there is no source this frame.

🪄 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: 35a677fa-0d5d-4094-9544-4b187916eb37

📥 Commits

Reviewing files that changed from the base of the PR and between 025a91e and 5b6dbc6.

📒 Files selected for processing (9)

• features/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlsl
• features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli
• package/Shaders/Common/FrameBuffer.hlsli
• package/Shaders/Common/ShadowSampling.hlsli
• src/Deferred.cpp
• src/Deferred.h
• src/Features/VolumetricShadows.cpp
• src/Features/VolumetricShadows.h
• src/State.cpp

💤 Files with no reviewable changes (1)

• features/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlsl

[github-actions]

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

[coderabbitai]

🧹 Nitpick comments (1)

src/Deferred.cpp (1)

598-609: Make the clamp type-agnostic so std::min deduction can’t bite later.

Small nit: std::min(lightData.shadowmapDescriptors.size(), static_cast<uint32_t>(std::size(dd.ShadowProj))) relies on shadowmapDescriptors.size() returning exactly uint32_t. If CLib ever changes to size_t (or this template is instantiated with a container whose size() is size_t), template argument deduction fails to compile. Prefer an explicit template argument or a common-type cast.

Proposed refactor

-	const auto count = std::min(lightData.shadowmapDescriptors.size(), static_cast<uint32_t>(std::size(dd.ShadowProj)));
-	for (uint32_t i = 0; i < count; i++) {
+	const size_t count = std::min<size_t>(lightData.shadowmapDescriptors.size(), std::size(dd.ShadowProj));
+	for (size_t i = 0; i < count; i++) {

🤖 Prompt for AI Agents

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

In `@src/Deferred.cpp` around lines 598 - 609, The std::min call in
Deferred::SetShadowCascadeParameters uses mixed integer types which can break
template deduction; make it type-agnostic by using a fixed common type (e.g.,
std::size_t) for the comparison and loop: compute count as
std::min<std::size_t>(lightData.shadowmapDescriptors.size(),
std::size(dd.ShadowProj)) and change the loop index to std::size_t (and cast to
uint32_t where needed for APIs expecting that type), so the comparison and
iteration no longer depend on container-specific size() return types.

🤖 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/Deferred.cpp`:
- Around line 598-609: The std::min call in Deferred::SetShadowCascadeParameters
uses mixed integer types which can break template deduction; make it
type-agnostic by using a fixed common type (e.g., std::size_t) for the
comparison and loop: compute count as
std::min<std::size_t>(lightData.shadowmapDescriptors.size(),
std::size(dd.ShadowProj)) and change the loop index to std::size_t (and cast to
uint32_t where needed for APIs expecting that type), so the comparison and
iteration no longer depend on container-specific size() return types.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 12f5627f-c6f9-45a6-ae51-310b030b3934

📥 Commits

Reviewing files that changed from the base of the PR and between a734481 and 315713f.

📒 Files selected for processing (1)

• src/Deferred.cpp