feat: improve directional shadow model#1767
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughRemoved the EffectShadows permutation bit and repacked nearby flags; added EffectRadius to permutation CB and set it from geometry; centralized and refactored shadow sampling/lighting into ShadowSampling; added a shadow downsample compute shader and integrated it into Deferred; updated water shader APIs and removed terrain/cloud include gates. Changes
Sequence Diagram(s)sequenceDiagram
participant CPU as Renderer (CPU)
participant Deferred as Deferred (CPU)
participant GPU as GPU
participant CS as DownsampleCS
participant PS as PixelShader
CPU->>Deferred: CopyShadowData(shadowView)
Deferred->>GPU: Bind shadowView SRV + shadowCopy UAV, set downsampleShadowCS
GPU->>CS: Dispatch downsampleShadowCS
CS->>GPU: Write shadowCopyTexture (UAV)
Deferred->>CPU: Expose shadowCopySRV (preferred)
CPU->>GPU: Submit draw calls (bind SRV array)
PS->>GPU: ShadowSampling::GetWorldShadow / GetEffectShadow / ExtractLighting calls
GPU-->>PS: Return dirColor + ambientColor + shadow factor
PS-->>GPU: Shade pixels using shadowed lighting
GPU-->>CPU: Present final frame
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Line 85: Cascade selection is using a different position than the shadow
lookup: replace the current cascadeIndex computation that uses positionWS +
viewDirection * (sampleOffset.x + sampleOffset.y) with a selection based on the
exact sample location used by the lookup. Compute the sample position (e.g.
samplePos = positionWS + sampleOffset) and call GetShadowDepth(samplePos,
eyeIndex) when comparing against sD.EndSplitDistances.x to set cascadeIndex so
the same samplePos is used for both cascade selection and the subsequent shadow
lookup.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 179-206: The ExtractLighting function can produce NaNs because the
ternary operator at the scale calculation evaluates both branches and does an
unguarded division by totalLight; change the scale computation to use a guarded,
component-wise denominator (similar to the llDirLightMult guard) — e.g. compute
a safe denom = max(totalLight, float3(epsilon)) and set scale = inputColor /
denom so zero components in totalLight won't produce NaN; update references to
totalLight/scale (and keep epsilon small, e.g. 1e-5) in ExtractLighting
(ambientColorAmb, dirLightColorDir, totalLight, scale).
In `@package/Shaders/DownsampleShadowCS.hlsl`:
- Around line 6-37: The 2x2 InputTexture.Load calls in main can read
out-of-bounds when dimensions or dispatch don't match; before performing the
four loads, call InputTexture.GetDimensions(w,h,slices) (or the appropriate
method for the input SRV), compute clamped sample coordinates for pixCoord +
uint2(0/1,0/1) by min(x, w-1) and min(y, h-1) and use those clamped coords for
the Load calls so every thread hits the barrier without early-return; keep
g_scratchDepths usage, GroupMemoryBarrierWithGroupSync, the later 2x2
shared-memory reduction and OutputTexture.GetDimensions/assign logic unchanged.
In `@src/Deferred.cpp`:
- Around line 216-307: The Dispatch uses a single fullShadowSize (from width)
for both X and Y, which breaks non-square shadow maps; change it to compute
fullWidth = shadowCopyWidth * 4 and fullHeight = shadowCopyHeight * 4 and call
context->Dispatch((fullWidth + 15) >> 4, (fullHeight + 15) >> 4,
shadowCopyArraySize) instead of the current single-dimension dispatch (locate
the Dispatch call and the fullShadowSize calculation in the downsample shadow
compute shader block where shadowCopyWidth/shadowCopyHeight and
shadowCopyArraySize are used); no other state changes are required.
🧹 Nitpick comments (2)
package/Shaders/DownsampleShadowCS.hlsl (1)
1-2: Run hlslkit register scan for the new CS bindings.New t0/u0 bindings can silently collide with existing CS resources once integrated. Please run hlslkit buffer scanning and compile this shader to confirm no register conflicts.
As per coding guidelines: “Applies to **/*.hlsl : Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts” and “Validate shaders with targeted testing using hlslkit compilation during development.”
package/Shaders/Common/ShadowSampling.hlsli (1)
7-11: Validate new includes for register conflicts.The added Color/IBL includes can introduce new resource bindings. Please run hlslkit buffer scanning and compile this shader to ensure no register collisions with existing bindings.
As per coding guidelines: “Applies to **/*.hlsl : Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts” and “Validate shaders with targeted testing using hlslkit compilation during development.”
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package/Shaders/Water.hlsl`:
- Line 1196: Remove the dead local variable by deleting the assignment to
waterColor (the float3 waterColor = diffuseOutput.refractionDiffuseColor;
declaration) since the symbol waterColor is never used; search for "waterColor"
and remove that declaration so no unused variable remains in the shader code
(ensure no other code references waterColor before deletion).
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package/Shaders/Water.hlsl`:
- Around line 1161-1163: The call to SphericalHarmonics::FuncProductIntegral is
passing a scalar (skylighting from Unproject) but the function expects two sh2
coefficients; change the second argument to the SH coefficient variable used for
diffuse (skylightingSH) so the call becomes FuncProductIntegral(skylightingSH,
specularLobe) (update the reference in the skylightingSpecular assignment),
ensuring specularLobe (sh2) and skylightingSH (sh2) are passed to match the
FuncProductIntegral signature and keep the subsequent saturate call.
In `@src/Deferred.cpp`:
- Around line 229-231: The code computes newWidth/newHeight as srcDesc.Width/4
and srcDesc.Height/4 which can become 0 for small textures and cause
CreateTexture2D to fail; update the calculation (where newWidth/newHeight are
set) to clamp each result to a minimum of 1 (e.g., compute the division and then
if result is 0 set to 1) before passing them into CreateTexture2D so malformed
or tiny shadow textures don't trigger DirectX errors.
🧹 Nitpick comments (3)
src/Deferred.cpp (1)
214-223: Verify the downsample path across SE/AE/VR variants.This block depends on the shadow SRV layout and array sizing; please confirm it behaves correctly in SE/AE/VR. If any version-specific engine offsets are needed for shadow resources, prefer
REL::RelocateMember()for runtime compatibility. As per coding guidelines: "Warn about SE/AE/VR compatibility issues and suggestREL::RelocateMember()patterns for runtime compatibility".package/Shaders/Common/ShadowSampling.hlsli (1)
7-11: Run hlslkit register scan for the new includes.Adding
Color.hlsliand optional IBL headers can introduce new bindings in certain permutations; please run hlslkit buffer scanning and compile key variants to catch register conflicts early. As per coding guidelines: "Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts" and "Validate shaders with targeted testing using hlslkit compilation during development".package/Shaders/Water.hlsl (1)
1131-1151: Run hlslkit register/compile checks for the new shadow + skylighting path.The new ShadowSampling/Skylighting wiring can change resource usage across permutations; please run hlslkit buffer scanning and compile key variants to catch register conflicts. As per coding guidelines: "Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts" and "Validate shaders with targeted testing using hlslkit compilation during development".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package/Shaders/Water.hlsl`:
- Around line 893-897: The SSR path currently lerps two linearized colors (using
Color::IrradianceToLinear on reflectionColor and finalSsrReflectionColor) and
returns the linear result, causing a mismatch with the non-SSR gamma-space
return; after the lerp in the SSR branch (where reflectionColor is assigned)
convert the lerped linear color back to the expected gamma/irradiance space
using the inverse helper (e.g., Color::LinearToIrradiance or the project’s
equivalent) before returning reflectionColor so both SSR and non-SSR paths
return the same color space.
🧹 Nitpick comments (2)
package/Shaders/Common/ShadowSampling.hlsli (1)
270-271: Hardcodedlerp(..., 0.0)makes the second argument unreachable.Both lerps use a blend factor of
0.0, meaningdirLightColorDirandambientColorDirare computed but never used. Either:
- Remove the dead code and assign directly:
dirColor = dirLightColorAmb;- Parameterize the blend factor if this is intended for future tuning.
- dirColor = lerp(dirLightColorAmb, dirLightColorDir, 0.0); - ambientColor = lerp(ambientColorAmb, ambientColorDir, 0.0); + dirColor = dirLightColorAmb; + ambientColor = ambientColorAmb;package/Shaders/Water.hlsl (1)
1120-1124: Clarify wetnessOcclusion intent when SKYLIGHTING is not defined.Line 1123 passes
inWorld(bool) aswetnessOcclusion(float). While HLSL implicitly converts this, the semantic meaning differs:
true→1.0= full raindrop effects (no occlusion)false→0.0= no raindrop effectsIf the intent is "enable raindrops when in world", this works. If the intent is "use a default occlusion value", consider using
1.0orfloat(inWorld)explicitly for clarity.`#if` defined(SKYLIGHTING) WaterNormalData waterData = GetWaterNormal(input, distanceFactor, depthControl.z, viewDirection, depth, eyeIndex, wetnessOcclusion); `#else` - WaterNormalData waterData = GetWaterNormal(input, distanceFactor, depthControl.z, viewDirection, depth, eyeIndex, inWorld); + WaterNormalData waterData = GetWaterNormal(input, distanceFactor, depthControl.z, viewDirection, depth, eyeIndex, float(inWorld)); `#endif`
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 44-56: The cloud shadow sampling in GetWorldShadow omits the
world-space offset used for dynamic origin shifts; update the call to
CloudShadows::GetCloudShadowMult to pass positionWS + offset (matching
TerrainShadows::GetTerrainShadow) so cloud shadows are computed with the same
camera-position adjustment, leaving the surrounding SharedData checks and
sampler usage unchanged.
🧹 Nitpick comments (2)
package/Shaders/Common/ShadowSampling.hlsli (2)
1-3: Consider adding a scope to the conventional commit title and link an issue if applicable.The PR title is close to conventional format but lacks a scope. A scoped option like
feat(shadows): simplify effect shadow samplingwould align with the repo convention, and if there’s a tracking issue, adding “Implements #…” or “Related to #…” would help with traceability.
As per coding guidelines: “Provide suggestions for Conventional Commit Titles… [and] Issue References.”
267-272: Consider simplifying constant lerp outputs to avoid extra ops.
lerp(..., 0.0)always returns the first operand, soambientColorDiris computed but unused. If the blend factor is intended to remain 0, you can simplify and drop the extra math.♻️ Optional simplification
- float3 ambientColorDir = max(0.0, inputColor - dirLightColorDir); - - dirColor = lerp(dirLightColorAmb, dirLightColorDir, 0.0); - ambientColor = lerp(ambientColorAmb, ambientColorDir, 0.0); + dirColor = dirLightColorAmb; + ambientColor = ambientColorAmb;
doodlum
changed the title
alandtse
changed the title
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.
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.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.