feat(volumetric-shadows)!: replace VSM with PCF froxel grid#2400
Closed
doodlum wants to merge 1 commit into
Closed
feat(volumetric-shadows)!: replace VSM with PCF froxel grid#2400doodlum wants to merge 1 commit into
doodlum wants to merge 1 commit into
Conversation
Rebuild the Volumetric Shadows feature around a view-space froxel grid of PCF-filtered directional shadow visibility instead of a 2D downsampled VSM moments texture. Each frame, BuildShadowFroxelCS reconstructs camera-relative world positions for every voxel via screen UV + exponential view-Z slicing, selects and blends the cascade(s) for that view depth, then samples the directional shadow array with a 5-tap cross PCF kernel using the hardware comparison sampler. Consumers (effects, decals, transparents) drop the cascade selection and shadow projection math entirely and just do a trilinear lookup into the 3D texture at slot t18 — replacing GetVSMShadow3D/2D with the new GetShadow3D/2D helpers in VolumetricShadows.hlsli. PCF removes the light-bleeding artefacts that VSM is prone to and gives a smoother falloff than Chebyshev's inequality on quantised moments. Inspired by jiayev's volumetric-fog branch, adapted to pre-filter only the directional shadow channel. BREAKING CHANGE: VolumetricShadows.hlsli now exposes Texture3D<float> SharedShadowMap at register t18 (was Texture2D<float2>), and the helper API is renamed from GetVSMShadow3D/2D to GetShadow3D/2D.
Contributor
📝 WalkthroughWalkthroughThis PR refactors volumetric shadowing from a Variance Shadow Maps (VSM) cascade-based approach to a pre-filtered 3D froxel-grid approach. The new pipeline builds a directional shadow froxel grid via a single compute shader, removes the prior downsample/blur pipeline, and updates all sampling APIs to use trilinear froxel lookups instead of variance moment filtering. ChangesVolumetric Shadows Froxel Grid Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.21.0)OpenGrep fatal error (exit code 2): [00.20][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/VolumetricShadows.cpp 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
features/Volumetric Shadows/Shaders/VolumetricShadows/BuildShadowFroxelCS.hlsl (1)
17-39: 💤 Low value
DirectionalShadowLightDatais duplicated fromShadowSampling.hlsli.This struct definition is identical to the one in
ShadowSampling.hlsli. If the layout ever changes, both must be updated in sync. Consider extracting to a shared header or adding a comment noting the dependency.🤖 Prompt for 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. In `@features/Volumetric` Shadows/Shaders/VolumetricShadows/BuildShadowFroxelCS.hlsl around lines 17 - 39, The struct DirectionalShadowLightData in BuildShadowFroxelCS.hlsl is duplicated from ShadowSampling.hlsli; remove the duplicate definition and instead include or import the shared definition (or create a new shared header) so both shaders reference the single canonical DirectionalShadowLightData; update BuildShadowFroxelCS.hlsl to reference the shared header (or add a clear comment linking to ShadowSampling.hlsli) to prevent drifting layouts between DirectionalShadowLightData definitions.src/Features/VolumetricShadows.cpp (2)
173-189: 💤 Low valueConstant buffer slots 5 and 12/13 are not cleared in cleanup.
Lines 173-183 clean up CB slot 1, SRV slots 0 and 98, samplers 0-1, and UAV 0, but constant buffer slots 5 (sharedDataBuf), 12 (perFrameCB), and 13 (vrPerFrameCB) are left bound. While this may be intentional if these buffers are expected to persist, it's inconsistent with the thorough cleanup of other bindings.
🤖 Prompt for 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. In `@src/Features/VolumetricShadows.cpp` around lines 173 - 189, The cleanup misses unbinding constant buffers at slots 5, 12 and 13 (sharedDataBuf, perFrameCB, vrPerFrameCB); update the CS cleanup to also call CSSetConstantBuffers for those slots using the existing nullCBs array (e.g. call context->CSSetConstantBuffers(5,1,nullCBs); context->CSSetConstantBuffers(12,1,nullCBs); context->CSSetConstantBuffers(13,1,nullCBs);) ensuring the nulling occurs before SetSharedShadowMapSRV/shadowView release and reusing the nullCBs variable already defined in this scope.
196-210: 💤 Low valueDrawSettings message may be misleading for VR users.
Line 199 says "(per eye)" but
kFroxelGridWidthis the mono eye width (160). In VR,froxelWidthis doubled to 320 total (side-by-side), so the actual per-eye resolution is still 160. The display at line 208 showsfroxelWidthwhich is 320 in VR, potentially confusing users. Consider clarifying:ImGui::TextWrapped( - "Builds a %ux%ux%u view-space froxel grid (per eye) of PCF-filtered directional shadow visibility.\n" + "Builds a %ux%ux%u view-space froxel grid of PCF-filtered directional shadow visibility.\n" "Consumers sample the grid via the shared shadow map texture at slot t%u.",🤖 Prompt for 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. In `@src/Features/VolumetricShadows.cpp` around lines 196 - 210, The UI text is misleading in VR because DrawSettings prints "(per eye)" using kFroxelGridWidth but then shows froxelWidth which is the total (doubled) width in VR; update DrawSettings to clearly state per-eye vs total dimensions by using kFroxelGridWidth/kFroxelGridHeight/kFroxelGridDepth when describing the per-eye froxel grid (the first ImGui::TextWrapped) and either display the resource line using per-eye values (kFroxelGridWidth, kFroxelGridHeight, kFroxelGridDepth) or show both per-eye and total (e.g., "per-eye: %ux%ux%u, total: %ux%ux%u") so users aren’t confused—modify the code around VolumetricShadows::DrawSettings, replacing the ambiguous use of froxelWidth/froxelHeight/froxelDepth with explicit per-eye and/or total indicators.features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (1)
21-47: 💤 Low value
LinearSamplermust be bound by consumers before callingSampleShadowFroxel.Line 46 uses
LinearSamplerwhich is not declared in this file. Consumers must ensure it's bound. This appears to be an existing pattern in the codebase, but consider adding a comment noting this dependency for clarity.🤖 Prompt for 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. In `@features/Volumetric` Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli around lines 21 - 47, SampleShadowFroxel calls SharedShadowMap.SampleLevel with LinearSampler (used on line where SampleShadowFroxel returns), but LinearSampler is not declared in this file and must be bound by callers; add a brief comment immediately above the SampleShadowFroxel function (referencing SampleShadowFroxel, LinearSampler and SharedShadowMap) stating that LinearSampler must be bound by consumers before calling this function and describing expected sampler state (e.g., linear filtering, wrap/ clamp as required), so callers know the dependency and correct binding.src/Features/VolumetricShadows.h (1)
66-67: 💤 Low valueStale transient pointer may be observed if
BuildShadowFroxelis never called.The
shadowViewmember is described as "transient" but initialized tonullptrand only set/cleared insideBuildShadowFroxel. If another code path reads this member before the first dispatch, it will see stale data. Consider documenting the expected lifecycle or making it a local variable in the dispatch function since it's only used transiently.🤖 Prompt for 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. In `@src/Features/VolumetricShadows.h` around lines 66 - 67, The transient member ID3D11ShaderResourceView* shadowView should not be a persistent class field because it can be observed stale before BuildShadowFroxel runs; move shadowView out of the class and make it a local variable inside the dispatch/shadow-building function (e.g., inside BuildShadowFroxel or the dispatch method that calls it) where it's allocated/used and cleared, or if it must remain a member, explicitly initialize/clear it in the constructor and every code path that may read it and add a clear lifecycle comment; ensure you remove all reads/writes to the old member and update call sites to use the new local variable so no other code can observe a stale pointer.
🤖 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 `@src/Features/VolumetricShadows.cpp`:
- Around line 59-92: The created froxel resources (shadowFroxelTexture,
shadowFroxelSRV, shadowFroxelUAV) from CreateFroxelResources and the
samplers/configCB allocated in SetupResources are not released; add a cleanup
path by implementing either a VolumetricShadows::~VolumetricShadows() destructor
or a ClearResources() method that safely Releases each COM pointer
(shadowFroxelUAV, shadowFroxelSRV, shadowFroxelTexture, comparisonSampler,
linearSampler) and deletes/zeroes configCB, setting pointers to nullptr after
release so unloading the feature won’t leak resources. Ensure the
destructor/ClearResources is called when the feature is torn down.
---
Nitpick comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/BuildShadowFroxelCS.hlsl:
- Around line 17-39: The struct DirectionalShadowLightData in
BuildShadowFroxelCS.hlsl is duplicated from ShadowSampling.hlsli; remove the
duplicate definition and instead include or import the shared definition (or
create a new shared header) so both shaders reference the single canonical
DirectionalShadowLightData; update BuildShadowFroxelCS.hlsl to reference the
shared header (or add a clear comment linking to ShadowSampling.hlsli) to
prevent drifting layouts between DirectionalShadowLightData definitions.
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 21-47: SampleShadowFroxel calls SharedShadowMap.SampleLevel with
LinearSampler (used on line where SampleShadowFroxel returns), but LinearSampler
is not declared in this file and must be bound by callers; add a brief comment
immediately above the SampleShadowFroxel function (referencing
SampleShadowFroxel, LinearSampler and SharedShadowMap) stating that
LinearSampler must be bound by consumers before calling this function and
describing expected sampler state (e.g., linear filtering, wrap/ clamp as
required), so callers know the dependency and correct binding.
In `@src/Features/VolumetricShadows.cpp`:
- Around line 173-189: The cleanup misses unbinding constant buffers at slots 5,
12 and 13 (sharedDataBuf, perFrameCB, vrPerFrameCB); update the CS cleanup to
also call CSSetConstantBuffers for those slots using the existing nullCBs array
(e.g. call context->CSSetConstantBuffers(5,1,nullCBs);
context->CSSetConstantBuffers(12,1,nullCBs);
context->CSSetConstantBuffers(13,1,nullCBs);) ensuring the nulling occurs before
SetSharedShadowMapSRV/shadowView release and reusing the nullCBs variable
already defined in this scope.
- Around line 196-210: The UI text is misleading in VR because DrawSettings
prints "(per eye)" using kFroxelGridWidth but then shows froxelWidth which is
the total (doubled) width in VR; update DrawSettings to clearly state per-eye vs
total dimensions by using kFroxelGridWidth/kFroxelGridHeight/kFroxelGridDepth
when describing the per-eye froxel grid (the first ImGui::TextWrapped) and
either display the resource line using per-eye values (kFroxelGridWidth,
kFroxelGridHeight, kFroxelGridDepth) or show both per-eye and total (e.g.,
"per-eye: %ux%ux%u, total: %ux%ux%u") so users aren’t confused—modify the code
around VolumetricShadows::DrawSettings, replacing the ambiguous use of
froxelWidth/froxelHeight/froxelDepth with explicit per-eye and/or total
indicators.
In `@src/Features/VolumetricShadows.h`:
- Around line 66-67: The transient member ID3D11ShaderResourceView* shadowView
should not be a persistent class field because it can be observed stale before
BuildShadowFroxel runs; move shadowView out of the class and make it a local
variable inside the dispatch/shadow-building function (e.g., inside
BuildShadowFroxel or the dispatch method that calls it) where it's
allocated/used and cleared, or if it must remain a member, explicitly
initialize/clear it in the constructor and every code path that may read it and
add a clear lifecycle comment; ensure you remove all reads/writes to the old
member and update call sites to use the new local variable so no other code can
observe a stale pointer.
🪄 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 Plus
Run ID: b13e86c4-aa83-4446-8c67-342a146a0d05
📒 Files selected for processing (9)
features/Volumetric Shadows/Shaders/Features/VolumetricShadows.inifeatures/Volumetric Shadows/Shaders/VolumetricShadows/BlurShadowCS.hlslfeatures/Volumetric Shadows/Shaders/VolumetricShadows/BuildShadowFroxelCS.hlslfeatures/Volumetric Shadows/Shaders/VolumetricShadows/DownsampleShadowCS.hlslfeatures/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlslipackage/Shaders/Common/ShadowSampling.hlslisrc/Features/VolumetricShadows.cppsrc/Features/VolumetricShadows.hsrc/State.cpp
💤 Files with no reviewable changes (2)
- features/Volumetric Shadows/Shaders/VolumetricShadows/BlurShadowCS.hlsl
- features/Volumetric Shadows/Shaders/VolumetricShadows/DownsampleShadowCS.hlsl
Comment on lines
+59
to
92
| void VolumetricShadows::CreateFroxelResources() | ||
| { | ||
| auto device = globals::d3d::device; | ||
|
|
||
| froxelWidth = kFroxelGridWidth * (REL::Module::IsVR() ? 2u : 1u); | ||
| froxelHeight = kFroxelGridHeight; | ||
| froxelDepth = kFroxelGridDepth; | ||
|
|
||
| D3D11_TEXTURE3D_DESC texDesc{}; | ||
| texDesc.Width = froxelWidth; | ||
| texDesc.Height = froxelHeight; | ||
| texDesc.Depth = froxelDepth; | ||
| texDesc.MipLevels = 1; | ||
| texDesc.Format = DXGI_FORMAT_R8_UNORM; | ||
| texDesc.Usage = D3D11_USAGE_DEFAULT; | ||
| texDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS; | ||
|
|
||
| DX::ThrowIfFailed(device->CreateTexture3D(&texDesc, nullptr, &shadowFroxelTexture)); | ||
| Util::SetResourceName(shadowFroxelTexture, "VolumetricShadows::ShadowFroxel"); | ||
|
|
||
| D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc{}; | ||
| srvDesc.Format = texDesc.Format; | ||
| srvDesc.ViewDimension = D3D11_SRV_DIMENSION_TEXTURE3D; | ||
| srvDesc.Texture3D.MipLevels = 1; | ||
| DX::ThrowIfFailed(device->CreateShaderResourceView(shadowFroxelTexture, &srvDesc, &shadowFroxelSRV)); | ||
| Util::SetResourceName(shadowFroxelSRV, "VolumetricShadows::ShadowFroxel SRV"); | ||
|
|
||
| D3D11_UNORDERED_ACCESS_VIEW_DESC uavDesc{}; | ||
| uavDesc.Format = texDesc.Format; | ||
| uavDesc.ViewDimension = D3D11_UAV_DIMENSION_TEXTURE3D; | ||
| uavDesc.Texture3D.WSize = froxelDepth; | ||
| DX::ThrowIfFailed(device->CreateUnorderedAccessView(shadowFroxelTexture, &uavDesc, &shadowFroxelUAV)); | ||
| Util::SetResourceName(shadowFroxelUAV, "VolumetricShadows::ShadowFroxel UAV"); | ||
| } |
Contributor
There was a problem hiding this comment.
Missing resource cleanup for froxel texture, SRV, UAV, samplers, and constant buffer.
The CreateFroxelResources function allocates shadowFroxelTexture, shadowFroxelSRV, and shadowFroxelUAV, but there's no destructor or cleanup method to release these COM objects. Similarly, linearSampler, comparisonSampler, and configCB allocated in SetupResources are never released. This will cause resource leaks when the feature is unloaded or the game exits.
Consider adding a destructor or ClearResources method:
🛠️ Suggested cleanup pattern
// In header, add destructor:
~VolumetricShadows();
// In cpp:
VolumetricShadows::~VolumetricShadows()
{
if (shadowFroxelUAV) { shadowFroxelUAV->Release(); shadowFroxelUAV = nullptr; }
if (shadowFroxelSRV) { shadowFroxelSRV->Release(); shadowFroxelSRV = nullptr; }
if (shadowFroxelTexture) { shadowFroxelTexture->Release(); shadowFroxelTexture = nullptr; }
if (comparisonSampler) { comparisonSampler->Release(); comparisonSampler = nullptr; }
if (linearSampler) { linearSampler->Release(); linearSampler = nullptr; }
delete configCB; configCB = nullptr;
}🤖 Prompt for 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.
In `@src/Features/VolumetricShadows.cpp` around lines 59 - 92, The created froxel
resources (shadowFroxelTexture, shadowFroxelSRV, shadowFroxelUAV) from
CreateFroxelResources and the samplers/configCB allocated in SetupResources are
not released; add a cleanup path by implementing either a
VolumetricShadows::~VolumetricShadows() destructor or a ClearResources() method
that safely Releases each COM pointer (shadowFroxelUAV, shadowFroxelSRV,
shadowFroxelTexture, comparisonSampler, linearSampler) and deletes/zeroes
configCB, setting pointers to nullptr after release so unloading the feature
won’t leak resources. Ensure the destructor/ClearResources is called when the
feature is torn down.
Collaborator
|
what makes this PR breaking? |
|
✅ A pre-release build is available for this PR: |
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
3 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.
Summary
Rebuild the Volumetric Shadows feature around a view-space froxel grid of PCF-filtered directional shadow visibility, replacing the 2D downsampled VSM moments texture it previously produced.
Each frame,
BuildShadowFroxelCSreconstructs camera-relative world positions for every voxel via screen UV + exponential view-Z slicing, picks and blends the cascade(s) for that view depth, then samples Skyrim's directional cascade array with a 5-tap cross PCF kernel using the hardware comparison sampler. The result is aTexture3D<float>of visibility values bound att18for consumers.Consumers drop the cascade selection and shadow projection math entirely —
Get3DFilteredShadow/GetLightingShadowinCommon/ShadowSampling.hlslinow call newVolumetricShadows::GetShadow3D/GetShadow2Dhelpers that just do a trilinear 3D lookup. PCF removes the light-bleeding artefacts inherent to VSM and gives a smoother falloff than Chebyshev's inequality on quantised moments.Inspired by the froxel grid in jiayev/skyrim-community-shaders
volumetric-fog,` adapted to pre-filter only the directional shadow channel.What changed
src/Features/VolumetricShadows.{h,cpp}— Texture3D + comparison sampler + single build CS.CopyShadowLightDatarenamed toBuildShadowFroxel(the old name was misleading after the refactor).features/Volumetric Shadows/Shaders/VolumetricShadows/BuildShadowFroxelCS.hlsl(new) — the per-frame build pass.features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli— rewritten to project a world position into the grid andSampleLevelit.features/Volumetric Shadows/Shaders/VolumetricShadows/{DownsampleShadowCS,BlurShadowCS}.hlsl— deleted (no longer needed; PCF replaces the gaussian-blurred VSM pipeline).package/Shaders/Common/ShadowSampling.hlsli— updated to call the new helper names.features/Volumetric Shadows/Shaders/Features/VolumetricShadows.ini— version bumped 2-0-1 → 3-0-0 since the shared resource type changed.Grid configuration
R8_UNORM(~1 MB mono, ~2 MB VR). Trilinear filter smooths the 256-level quantisation.[16, EndSplitDistances.y]— concentrates resolution where shadows matter most.SharedData::CameraDataformula; unprojection composesCameraViewInverse · CameraProjUnjitteredInverseto avoid jitter flicker.BREAKING CHANGE
VolumetricShadows.hlslinow exposesTexture3D<float> SharedShadowMap : register(t18)(wasTexture2D<float2>). The helper API is renamedGetVSMShadow3D/2D→GetShadow3D/2D. Any out-of-tree shader that included the old.hlslidirectly will need to update its calls.Test plan
ALL,SE,VRpresets and verifyBuildShadowFroxelCS.hlslvalidates withhlslkit-compile.HasDirectionalShadows()returns false → froxel SRV is null and consumers fall back to lit (no crash, no black shadows).VolumetricShadows::ShadowFroxel,VolumetricShadows::ShadowFroxel SRV/UAV, and sampler names are present.Generated by Claude Code
Summary by CodeRabbit