feat(skylighting): add per-probe shadow skylighting #2498
Open
doodlum wants to merge 14 commits into
Open
Conversation
Augments the skylighting probe update with a second output: a 32-frame bitmask of sun shadow visibility per probe, accumulated from cascade shadow map samples with per-frame fixed-pattern offsets. The bitmask average becomes a smoothed shadow visibility value usable by other features without rerunning shadow taps. Adds two new 3D textures (texShadowBitmask R32_UINT, texShadowVisibility R16_FLOAT) and a new SRV slot t53 for sampling visibility from the pixel shader. Adds a Skylighting::CaptureShadowCascadeSRV() that grabs the active cascade SRV during the shadowmask render pass so the next probe update has it. Refactors the Skylighting::Sample API to take screen position for per-pixel jitter, and renames GetVertexSkylightingDiffuse to GetSkylightingDiffuse, which now takes a pre-sampled SH so callers can reuse it for both sampling and evaluation. Adds a new SampleShadowVisibility(positionMS, normalWS, screenPos) entry point. Updates all in-tree callers (DeferredCompositeCS, Lighting, RunGrass). Bumps Skylighting feature version 1-3-0 -> 1-4-0.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-probe directional shadow visibility sampling and accumulation, refactors skylighting diffuse to accept precomputed SH, wires visibility into deferred/direct lighting, updates probe compute shader and all relevant GPU resource allocation/bindings, and updates shader call sites to use the new API. ChangesSkylighting Shadow Visibility Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 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. |
![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: 3
🧹 Nitpick comments (1)
features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (1)
197-244: 💤 Low valueInconsistent epsilon constant for weight sum guard.
Sample()at line 135 usesEPSILON_WEIGHT_SUMfor the weight sum guard, butSampleShadowVisibility()at line 242 uses a hardcoded0.0001. For consistency and maintainability, consider using the same constant.♻️ Suggested fix
- float shadow = sum / max(wsum, 0.0001); + float shadow = sum / max(wsum, EPSILON_WEIGHT_SUM);🤖 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/Skylighting/Shaders/Skylighting/Skylighting.hlsli` around lines 197 - 244, SampleShadowVisibility() uses a hardcoded weight-guard value (0.0001) while Sample() uses EPSILON_WEIGHT_SUM; update SampleShadowVisibility to use the same EPSILON_WEIGHT_SUM constant to keep behavior consistent and maintainable: locate SampleShadowVisibility(float3 positionMS, float3 normalWS, float2 screenPosition) and replace the max(wsum, 0.0001) guard with max(wsum, EPSILON_WEIGHT_SUM) (ensure EPSILON_WEIGHT_SUM is visible in this translation unit or include the header/define where it’s declared so ShadowVisibilityProbeArray sampling and shadow = sum / max(wsum, EPSILON_WEIGHT_SUM) compile correctly).
🤖 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 `@features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl`:
- Around line 8-16: The HLSL StructuredBuffer struct DirectionalShadowLightData
now contains a float4 CascadeDepthParams that is not present in the CPU-side
struct used to populate DirectionalShadowLights, causing a layout mismatch; fix
this by making both sides byte-identical: either remove CascadeDepthParams from
the HLSL struct (DirectionalShadowLightData) or add a matching
CascadeDepthParams (float4) member to the CPU-side DirectionalShadowLightData
and populate it when building the directionalShadowLights buffer, ensuring
matching types, order, and alignment/packing rules so
StructuredBuffer<DirectionalShadowLightData> reads are safe.
- Around line 108-111: The cascade selection logic casts a saturated float
cascadeSelect to uint (cascadeIndex = uint(cascadeSelect)), which yields 0 for
any value <1.0 and thus collapses most probes to cascade 0; change this to an
explicit threshold-based selection so cascadeIndex becomes 1 when cascadeSelect
crosses the intended boundary (for example use a comparison like cascadeSelect
>= 0.5 or cascadeSelect > 0.0 depending on desired behavior) and set
cascadeIndex accordingly (e.g., cascadeIndex = (cascadeSelect >= 0.5) ? 1u : 0u)
so both cascades can be chosen correctly.
In `@src/Features/Skylighting.cpp`:
- Around line 241-245: The code unconditionally binds shadowCascadeSRV into the
srvs array (CS SRV t1) causing undefined shadow sampling when shadowCascadeSRV
is null; modify the logic around the srvs construction/dispatch that uses
shadowCascadeSRV (the array named srvs and the subsequent compute dispatch that
runs UpdateProbesCS.hlsl) to check if shadowCascadeSRV is non-null before
binding and dispatching; if it is null, either remove/replace that entry with a
safe fallback SRV (e.g., a default empty shadow SRV) or skip the
shadow-dependent dispatch path and update probe visibility with the fallback
behavior so frames before first capture use deterministic visibility; ensure the
branch references shadowCascadeSRV and the compute dispatch that consumes SRV t1
(UpdateProbesCS.hlsl) so the null case is handled explicitly.
---
Nitpick comments:
In `@features/Skylighting/Shaders/Skylighting/Skylighting.hlsli`:
- Around line 197-244: SampleShadowVisibility() uses a hardcoded weight-guard
value (0.0001) while Sample() uses EPSILON_WEIGHT_SUM; update
SampleShadowVisibility to use the same EPSILON_WEIGHT_SUM constant to keep
behavior consistent and maintainable: locate SampleShadowVisibility(float3
positionMS, float3 normalWS, float2 screenPosition) and replace the max(wsum,
0.0001) guard with max(wsum, EPSILON_WEIGHT_SUM) (ensure EPSILON_WEIGHT_SUM is
visible in this translation unit or include the header/define where it’s
declared so ShadowVisibilityProbeArray sampling and shadow = sum / max(wsum,
EPSILON_WEIGHT_SUM) compile correctly).
🪄 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: 4718e9b6-133c-4811-8b37-823efeeae145
📒 Files selected for processing (8)
features/Skylighting/Shaders/Skylighting/Skylighting.hlslifeatures/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlslpackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Features/Skylighting.cppsrc/Features/Skylighting.hsrc/State.cpp
- Remove CascadeDepthParams from HLSL struct to match CPU-side layout - Fix cascade selection collapsing to cascade 0 by using threshold compare - Guard null shadowCascadeSRV before compute dispatch - Use EPSILON_WEIGHT_SUM constant in SampleShadowVisibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fold SampleShadowVisibility into Sample via a conditional out parameter, eliminating the duplicate trilinear loop. In PSHADER contexts the shadow visibility probe is read in the same 8-cell pass as the SH probe, halving texture fetches and shared setup (jitter, UVW, bounds checks). Compute shader callers (SKYLIGHTING_PROBE_REGISTER) are unaffected. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ghting Gate shadow visibility sampling behind SKYLIGHTING_SHADOW_VIS, defined only for RIM_LIGHTING, SOFT_LIGHTING, LOAD_SOFT_LIGHTING, and BACK_LIGHTING permutations. Other Lighting.hlsl permutations and RunGrass.hlsl skip the shadow visibility texture fetch entirely. Apply visibility to material.rimSoftLightColor and backLightColor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
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)
features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (1)
164-164:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPotential NaN when
evalNormalpoints straight down.When
evalNormal.z ≤ 0andevalNormal.xy ≈ (0,0)(e.g., a ceiling normal of(0,0,-1)), the vector becomes(0,0,0)after themaxclamp, andnormalize()returns NaN.🛡️ Proposed fix to handle degenerate normals
- float3 biasedNormal = normalize(float3(evalNormal.xy, max(0.0, evalNormal.z))); + float3 candidateNormal = float3(evalNormal.xy, max(0.0, evalNormal.z)); + float3 biasedNormal = dot(candidateNormal, candidateNormal) > 1e-6 + ? normalize(candidateNormal) + : float3(0, 0, 1);🤖 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/Skylighting/Shaders/Skylighting/Skylighting.hlsli` at line 164, The biasedNormal calculation can produce a zero vector and NaNs when evalNormal.z ≤ 0 and evalNormal.xy ≈ (0,0); replace the direct normalize of float3(evalNormal.xy, max(0.0, evalNormal.z)) with a guarded path: form v = float3(evalNormal.xy, max(0.0, evalNormal.z)), test its length (or length squared) against a small epsilon (e.g. 1e-6), and if it is near-zero set biasedNormal to a safe fallback normal (e.g. float3(0,0,1) normalized), otherwise set biasedNormal = normalize(v); reference symbols: biasedNormal, evalNormal, normalize.
🧹 Nitpick comments (1)
features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (1)
200-200: 💤 Low valueDead code:
cellCentreMSis unused.This variable is computed but never referenced in
SampleNoBias(unlikeSamplewhere it's used for tangent weighting).♻️ Suggested removal
- float3 cellCentreMS = (cellID + 0.5 - ARRAY_DIM / 2) * CELL_SIZE; - float3 trilinearWeights = 1 - abs(offset - trilinearPos);🤖 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/Skylighting/Shaders/Skylighting/Skylighting.hlsli` at line 200, The variable cellCentreMS computed in Skylighting.hlsli at SampleNoBias is dead code — remove the unused declaration to clean up the shader; specifically delete the line that defines cellCentreMS in the SampleNoBias function (the analogous use remains in Sample for tangent weighting), and verify no other references to cellCentreMS exist in SampleNoBias after removal.
🤖 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.
Outside diff comments:
In `@features/Skylighting/Shaders/Skylighting/Skylighting.hlsli`:
- Line 164: The biasedNormal calculation can produce a zero vector and NaNs when
evalNormal.z ≤ 0 and evalNormal.xy ≈ (0,0); replace the direct normalize of
float3(evalNormal.xy, max(0.0, evalNormal.z)) with a guarded path: form v =
float3(evalNormal.xy, max(0.0, evalNormal.z)), test its length (or length
squared) against a small epsilon (e.g. 1e-6), and if it is near-zero set
biasedNormal to a safe fallback normal (e.g. float3(0,0,1) normalized),
otherwise set biasedNormal = normalize(v); reference symbols: biasedNormal,
evalNormal, normalize.
---
Nitpick comments:
In `@features/Skylighting/Shaders/Skylighting/Skylighting.hlsli`:
- Line 200: The variable cellCentreMS computed in Skylighting.hlsli at
SampleNoBias is dead code — remove the unused declaration to clean up the
shader; specifically delete the line that defines cellCentreMS in the
SampleNoBias function (the analogous use remains in Sample for tangent
weighting), and verify no other references to cellCentreMS exist in SampleNoBias
after removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a1b8362-0f08-49a0-97d1-758732024296
📒 Files selected for processing (3)
features/Skylighting/Shaders/Skylighting/Skylighting.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlsl
🚧 Files skipped from review as they are similar to previous changes (2)
- package/Shaders/Lighting.hlsl
- package/Shaders/RunGrass.hlsl
Handle zero-length biasedNormal in GetSkylightingDiffuse when evalNormal points straight down. Remove unused cellCentreMS in SampleNoBias. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Halves VRAM for the shadow visibility volume (values are [0,1]). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace dirSoftShadow with skylightingShadowVisibility when SKYLIGHTING_SHADOW_VIS is defined, so probe shadow visibility flows naturally through the existing softShadow path to drive rim, soft, back, and coat lighting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s include DynamicCubemaps.hlsli transitively includes Skylighting.hlsli, and the include guard prevented re-inclusion when SKYLIGHTING_SHADOW_VIS was defined later. Move the define before all feature includes so the 4-param Sample overload is compiled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Skip VSM dirSoftShadow assignment when SKYLIGHTING_SHADOW_VIS handles it - Gate VSM fallback (dirVSMDetailedShadow) on !DEFERRED - Remove per-pixel pcg3d jitter blur from Sample - Remove screenPosition parameter from Sample and all call sites - Remove HasDirectionalShadows/InInterior guard from UpdateProbesCS - Restore original comments (Receiver normal bias, irradiance probes URL) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl`:
- Around line 96-127: The code uses the raw 8-bit accumFrames to derive bitIndex
and validBits which can wrap to 0 (divide-by-zero) — fix by clamping/deriving a
separate 1..32 history count and a 0..31 ring index: compute a safe historyCount
= max(accumFrames, 1u) (or saturate to 32 with historyCount = min(historyCount,
32u)), derive bitIndex as (accumFrames == 0 ? 0u : (accumFrames - 1u) % 32u) or
as a separate ringIndex = accumFrames % 32u, then use validBits =
min(historyCount, 32u) before computing float(countbits(bitmask)) /
float(validBits) and for the fade calculation; update references to accumFrames,
bitIndex, validBits, and outShadowBitmask/texAccumFramesArray accordingly to
avoid wrap-induced zero.
In `@package/Shaders/Lighting.hlsl`:
- Around line 2417-2431: Skylighting's interior paths are using a stale exterior
ShadowVisibilityProbeArray because Skylighting::Prepass may early-exit before
binding slot t53 but the shader still calls Skylighting::Sample(...,
skylightingShadowVisibility) and assigns dirSoftShadow from it; update the
shader so that before calling Skylighting::Sample or assigning dirSoftShadow you
either gate the call behind the same exterior condition used in
Skylighting::Prepass or explicitly set skylightingShadowVisibility to 1.0 when
the Prepass/skylight probe binding was skipped (ensure this change touches the
skylightingShadowVisibility declaration and both sampling sites used by
Skylighting::Sample and the dirSoftShadow assignment; same fix should be applied
at the other occurrence noted around lines 2651-2653).
In `@package/Shaders/RunGrass.hlsl`:
- Around line 562-563: Skylighting::Sample is conditionally declared to take an
extra out float shadowVisibility when SKYLIGHTING_SHADOW_VIS is defined, so
update both Skylighting::Sample call sites in RunGrass (the two places where you
currently call Skylighting::Sample(positionMSSkylight, normal)) to be guarded:
when SKYLIGHTING_SHADOW_VIS is defined declare a local float shadowVisibility;
call Skylighting::Sample(positionMSSkylight, normal, shadowVisibility) and pass
shadowVisibility into any subsequent logic as needed (e.g., keep calling
Skylighting::GetSkylightingDiffuse as before), otherwise keep the existing 2-arg
call; ensure the conditional uses `#if/`#else/#endif around the call to match the
Skylighting.hlsli signature.
🪄 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: b549e83f-960b-4ab8-9e8d-65506f775533
📒 Files selected for processing (5)
features/Skylighting/Shaders/Skylighting/Skylighting.hlslifeatures/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlslpackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Features/Skylighting.cpp
accumFrames is R8_UINT (0-255). On wrap to 0, bitIndex underflows and validBits becomes 0 causing NaN. Clamp shadow frame count with max(accumFrames, 1u) for the bitmask logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✅ 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
2 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.
Augments the skylighting probe update with a second output: a 32-frame bitmask of sun shadow visibility per probe, accumulated from cascade shadow map samples with per-frame fixed-pattern offsets. The bitmask average becomes a smoothed shadow visibility value usable by other features without rerunning shadow taps.
Adds two new 3D textures (texShadowBitmask R32_UINT, texShadowVisibility R16_FLOAT) and a new SRV slot t53 for sampling visibility from the pixel shader. Adds a Skylighting::CaptureShadowCascadeSRV() that grabs the active cascade SRV during the shadowmask render pass so the next probe update has it.
Refactors the Skylighting::Sample API to take screen position for per-pixel jitter, and renames GetVertexSkylightingDiffuse to GetSkylightingDiffuse, which now takes a pre-sampled SH so callers can reuse it for both sampling and evaluation. Adds a new SampleShadowVisibility(positionMS, normalWS, screenPos) entry point. Updates all in-tree callers (DeferredCompositeCS, Lighting, RunGrass).
Bumps Skylighting feature version 1-3-0 -> 1-4-0.
Summary by CodeRabbit
New Features
Refactor