perf: optimise shadows#2147
Conversation
📝 WalkthroughWalkthroughRefactored shadow visibility filtering in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Utility.hlsl (1)
620-644:⚠️ Potential issue | 🟠 MajorPB path incorrectly clamps upper half of shadow samples — use
SampleShadowPCFinstead.In the
RENDER_SHADOWMASKPBcase,shadowMapUvis computed across the full[0,1]range (line 628:lightDirection.xy / lightDirection.z * 0.5 + 0.5), which matches the layout of a single-paraboloid shadow map. However, line 638 callsSampleDualParaboloidShadowPCF(..., lowerHalf=false), which internally applies:uv.y = min(uv.y, 0.5);This caps all samples to
y ≤ 0.5, collapsing the filter footprint onto a horizontal line whenevershadowMapUv.y > 0.5. The effect is visible as incorrect shadowing in the upper hemisphere of the single-paraboloid map—a horizontal seam or band where filtering breaks down.By contrast, line 636 (SHADOWFILTER==1) samples the full unclamped range, so SHADOWFILTER==3 will produce visibly different shadows.
Fix: Use
SampleShadowPCFinstead, which applies no y-clamping and is designed for single-paraboloid layouts. Alternatively, pre-remapshadowMapUv.yto[0, 0.5]before the call, mirroring theRENDER_SHADOWMASKDPBpattern on line 656.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Utility.hlsl` around lines 620 - 644, In the RENDER_SHADOWMASKPB branch you must stop using SampleDualParaboloidShadowPCF (which clamps uv.y) and call SampleShadowPCF instead so the full shadowMapUv range is sampled; locate the call to SampleDualParaboloidShadowPCF in the block that computes shadowMapUv and shadowVisibility and replace it with SampleShadowPCF passing the same sampler/params (shadowMapUv, EndSplitDistances.x, shadowMapCompareValue, rotationMatrix, ShadowSampleParam.z, false) or otherwise mirror the RENDER_SHADOWMASKDPB pattern (remapping uv.y to [0,0.5]) if you prefer clamping — ensure shadowVisibility is set from SampleShadowPCF rather than SampleDualParaboloidShadowPCF.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package/Shaders/Utility.hlsl`:
- Around line 620-644: In the RENDER_SHADOWMASKPB branch you must stop using
SampleDualParaboloidShadowPCF (which clamps uv.y) and call SampleShadowPCF
instead so the full shadowMapUv range is sampled; locate the call to
SampleDualParaboloidShadowPCF in the block that computes shadowMapUv and
shadowVisibility and replace it with SampleShadowPCF passing the same
sampler/params (shadowMapUv, EndSplitDistances.x, shadowMapCompareValue,
rotationMatrix, ShadowSampleParam.z, false) or otherwise mirror the
RENDER_SHADOWMASKDPB pattern (remapping uv.y to [0,0.5]) if you prefer clamping
— ensure shadowVisibility is set from SampleShadowPCF rather than
SampleDualParaboloidShadowPCF.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3105647f-f37d-4c8d-9429-be58f1c721e1
📒 Files selected for processing (1)
package/Shaders/Utility.hlsl
|
✅ A pre-release build is available for this PR: |
(cherry picked from commit 31862bd)
Summary by CodeRabbit