feat(skylighting): fast soft shadows#1741
Conversation
📝 WalkthroughWalkthroughAdds per‑probe shadow visibility: a compute pass writes 3D visibility arrays and bitfields; Skylighting sampling APIs and call-sites accept a ShadowVisibilityProbeArray and return shadowVisibility; lighting context gains detailed/soft shadow fields; shader flag bits reindexed and an effect hook removed. Changes
Sequence Diagram(s)sequenceDiagram
participant CS as UpdateProbesCS (Compute)
participant SM as SharedShadowMap
participant PA as ProbeArrays (ProbeArray + ShadowVisArray)
participant PS as Pixel Shader / Lighting
Note over CS,SM: per-probe compute dispatch
CS->>SM: sample shadow cascades (Get2DFilteredShadow / GetShadowDepth)
SM-->>CS: filtered visibility
CS->>PA: read probe SH + prior shadowVis / bitfields
CS->>CS: accumulate per-probe shadowSum, update bitfields and noise
CS->>PA: write outProbeArray & outShadowVisibilityArray (UAVs)
Note over PS,PA: per-pixel shading
PS->>PA: Skylighting::sample(probeArray, ShadowVisibilityProbeArray, blueNoise, screenPos, positionMS, normalWS, out shadowVisibility)
PA-->>PS: SH + shadowVisibility
PS->>PS: CreateDirectContext(detailedShadow, softShadow)
PS->>PS: combine dirColor + ambientColor applying skylightingShadowVisibility
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
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.
doodlum
changed the title
|
@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/Effect.hlsl`:
- Around line 550-603: The code initializes color from DLightColor directly, so
the later linear-lighting correction (llDirLightMult / Color::DirectionalLight
usage) never affects color; update the color initialization to use the same
corrected directional-term calculation as dirLightColor (i.e., apply
SharedData::linearLightingSettings correction and Color::DirectionalLight) or
precompute a corrected directional source and use that for color; refer to
symbols DLightColor, color, llDirLightMult, dirLightColor,
SharedData::linearLightingSettings and Color::DirectionalLight to locate and
apply the fix.
🧹 Nitpick comments (2)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (2)
41-54: Single-sample shadow lookup may be intentional but naming is misleading.The function
Get2DFilteredShadowCascadesuggests PCF filtering but only takes a single shadow sample. This may be intentional for probe updates (accumulated over frames), but the name could mislead future maintainers.Also, the hardcoded depth bias (
compareValue -= 0.0001) may need tuning based on shadow map resolution or depth range.Consider renaming to
GetSingleShadowSampleCascadeor similar to clarify the single-sample behavior, or add a brief comment explaining the temporal accumulation strategy.
128-156: Shadow visibility accumulation logic follows established probe update pattern.The temporal accumulation with inverse-confidence blending mirrors the occlusion probe logic above, ensuring consistent behavior. The
accumFramescap at 60 provides bounded memory for the visibility history.Minor formatting: Line 153 is missing a space before the brace.
🔧 Minor formatting fix
- } else if (!isValid){ + } else if (!isValid) {
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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Water.hlsl (1)
763-775: Compute skylighting shadow visibility even when wetness effects are off.
sunColoris now multiplied bywaterData.skylightingShadowVisibility, but that value is only populated inside the WETNESS_EFFECTS block. If WETNESS_EFFECTS is disabled, the modulation never takes effect. Consider sampling skylighting underSKYLIGHTINGunconditionally and reusing the SH result for wetness when enabled.✅ Suggested fix (single sample, reused for wetness)
# if defined(WETNESS_EFFECTS) // Wetness Effects Debug System: // DEBUG_WETNESS_EFFECTS Color Legend: // - BRIGHT MAGENTA: Ripples, BRIGHT GREEN: Splashes, CYAN: Both effects const bool inWorld = (Permutation::ExtraShaderDescriptor & Permutation::ExtraFlags::InWorld); -# if defined(SKYLIGHTING) -# if defined(VR) - float3 positionMSSkylight = input.WPosition.xyz + FrameBuffer::CameraPosAdjust[eyeIndex].xyz - FrameBuffer::CameraPosAdjust[0].xyz; -# else - float3 positionMSSkylight = input.WPosition.xyz; -# endif - sh2 skylightingSH = Skylighting::sample(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, Skylighting::stbn_vec3_2Dx1D_128x128x64, input.HPosition.xy, positionMSSkylight, float3(0, 0, 1), result.skylightingShadowVisibility); - float skylighting = SphericalHarmonics::Unproject(skylightingSH, float3(0, 0, 1)); - - float wetnessOcclusion = inWorld ? pow(saturate(skylighting), 2) : 0; -# else - float wetnessOcclusion = inWorld; -# endif +# endif + +# if defined(SKYLIGHTING) +# if defined(VR) + float3 positionMSSkylight = input.WPosition.xyz + FrameBuffer::CameraPosAdjust[eyeIndex].xyz - FrameBuffer::CameraPosAdjust[0].xyz; +# else + float3 positionMSSkylight = input.WPosition.xyz; +# endif + sh2 skylightingSH = Skylighting::sample(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, Skylighting::stbn_vec3_2Dx1D_128x128x64, input.HPosition.xy, positionMSSkylight, float3(0, 0, 1), result.skylightingShadowVisibility); +# endif + +# if defined(WETNESS_EFFECTS) +# if defined(SKYLIGHTING) + float skylighting = SphericalHarmonics::Unproject(skylightingSH, float3(0, 0, 1)); + float wetnessOcclusion = inWorld ? pow(saturate(skylighting), 2) : 0; +# else + float wetnessOcclusion = inWorld; +# endifAlso applies to: 1217-1222
🤖 Fix all issues with AI agents
In `@package/Shaders/Effect.hlsl`:
- Around line 565-567: The statement in the fragment using
SharedData::InInterior calls ShadowSampling::GetWorldShadow(...) and ends with a
double semicolon; remove the extra semicolon after the call so the line reads a
single-terminated statement. Locate the block referencing
SharedData::InInterior, worldPosition, FrameBuffer::CameraPosAdjust[eyeIndex],
and eyeIndex and delete the trailing extra ';' following the GetWorldShadow(...)
call.
In `@src/Features/Skylighting.cpp`:
- Around line 279-284: Guard against null deferred resources before building or
binding the SRV array and before dispatch: check globals::deferred is non-null
and that deferred->shadowView and deferred->perShadow are valid (perShadow->srv)
and texOcclusion is valid; if any are missing, skip the SRV creation/binding and
the subsequent dispatch call to avoid a crash. Update the code around the srvs
construction (the std::array<ID3D11ShaderResourceView*,3> srvs = { ... } block)
to early-return or bypass binding/dispatch when globals::deferred,
deferred->shadowView, or deferred->perShadow->srv (or texOcclusion->srv) are
null, and ensure any resources used later in the function are only accessed when
these checks pass.
♻️ Duplicate comments (1)
package/Shaders/Effect.hlsl (1)
550-554: Apply linear-lighting correction to the directional term.The
colorvariable is initialized fromDLightColor.xyzdirectly, whiledirLightColoris computed with linear-lighting correction but never used to updatecolor. This means the correction is computed but discarded.💡 Suggested fix
- float3 color = DLightColor.xyz * Color::EffectLightingMult(); - float llDirLightMult = (SharedData::linearLightingSettings.enableLinearLighting && !SharedData::linearLightingSettings.isDirLightLinear) ? SharedData::linearLightingSettings.dirLightMult : 1.0f; - float3 dirLightColor = Color::DirectionalLight(SharedData::DirLightColor.xyz / max(llDirLightMult, 1e-5), SharedData::linearLightingSettings.isDirLightLinear) * llDirLightMult * Color::EffectLightingMult(); + float3 color = Color::DirectionalLight(DLightColor.xyz / max(llDirLightMult, 1e-5), SharedData::linearLightingSettings.isDirLightLinear) * llDirLightMult * Color::EffectLightingMult();
🧹 Nitpick comments (1)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (1)
172-173: Unused variablesviewDirectionanduv.These variables are computed but never used in the subsequent shadow visibility logic. Consider removing them if they're not needed.
🧹 Remove unused variables
- float3 viewDirection = FrameBuffer::WorldToView(-normalize(cellCentreMS), false); - float2 uv = FrameBuffer::ViewToUV(viewDirection, false); - bool validShadow; uint hasShadowVisibility = Get2DFilteredShadow(cellCentreMS, 0, validShadow) > 0.5;
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 `@features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl`:
- Line 13: Fix the typo in the comment for the variable StartSplitDistances in
UpdateProbesCS.hlsl: change "cascade start ditances int xyz, 4 int z" to
"cascade start distances int xyz, 4 int z" (or similarly correct phrasing) so
the comment reads clearly and spells "distances" correctly; locate the comment
next to the float4 StartSplitDistances declaration and update only the comment
text.
- Around line 169-171: The code reads outShadowVisibilityBitShiftArray[dtid]
into shadowVisibilityBitShift unguarded, risking an uninitialized UAV read;
modify UpdateProbesCS so the read is conditional on the same validity check used
for outShadowVisibilityBitArray (e.g. check isValid before reading
outShadowVisibilityBitShiftArray[dtid]) or supply a safe default (zero) when the
cell is not valid; ensure any subsequent use (cellCentreMS +=
noise3D[shadowVisibilityBitShift] * Skylighting::CELL_SIZE * 0.5) only executes
after the validity/initialization guard so dtid/shadowVisibilityBitShift cannot
be stale.
- Line 84: The cascade 1 shadow sampling uses the wrong threshold variable
(shadowMapThreshold) when calling SharedShadowMap.SampleCmpLevelZero for
cascade1ShadowVisibility; update the call to use the cascade 1 specific
threshold sD.AlphaTestRef.z (instead of shadowMapThreshold) so that
comparisonSampler, float3(cascade1PositionLS.xy, 1), and cascade1PositionLS.z -
sD.AlphaTestRef.z are used, matching the cascade determination logic.
♻️ Duplicate comments (2)
package/Shaders/Effect.hlsl (2)
550-554: Directional term still bypasses linear-lighting correction.Line 550 initializes
colorfromDLightColor, so the corrected directional term computed in Lines 552–553 doesn’t influence the final directional contribution.
565-567: Extra semicolon after world-shadow multiply.Line 566 ends with a double semicolon.
🧹 Fix
- color *= ShadowSampling::GetWorldShadow(worldPosition.xyz, FrameBuffer::CameraPosAdjust[eyeIndex].xyz, eyeIndex);; + color *= ShadowSampling::GetWorldShadow(worldPosition.xyz, FrameBuffer::CameraPosAdjust[eyeIndex].xyz, eyeIndex);
🧹 Nitpick comments (3)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (3)
4-4: Unused includeRandom.hlsli.This include doesn't appear to be used—the
noise3Darray on lines 134-167 is hardcoded. Consider either removing the include or replacing the hardcoded array with a procedural noise function fromRandom.hlsli.
55-55: Magic number2.5should be documented.The cascade count threshold check
2.5 < sD.EndSplitDistances.wis unclear. Consider using a named constant or adding a comment explaining what this value represents (likely checking if cascade count > 2).
134-167: Consider extracting the noise array to a shared location or generating procedurally.This hardcoded 32-element noise array could be:
- Moved to a shared header if reused elsewhere
- Generated procedurally using the already-included
Random.hlsli- Stored as a constant buffer if it needs to be configurable
The current approach works but adds code bulk.
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
doodlum
changed the title
doodlum
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 59-68: The code multiplies ambientColor (an out parameter not yet
initialized) by DALCAmount inside the IBL block; change that to scale
ambientColorAmb instead by replacing the ambientColor *=
SharedData::iblSettings.DALCAmount; with ambientColorAmb *=
SharedData::iblSettings.DALCAmount so the diffuse IBL contribution
(ambientColorAmb) is properly scaled; update only the statement in the IBL
branch where SharedData::iblSettings.DALCAmount is applied and preserve
subsequent uses of ambientColorAmb and Color::IrradianceToGamma.
- Around line 55-63: The ExtractLighting function signature lacks a default for
the skylightingDiffuse parameter causing calls with three args to fail; update
the function declaration/definition for ExtractLighting to give
skylightingDiffuse a default value (e.g., skylightingDiffuse = 1.0f) so callers
that omit the argument (e.g., Effect.hlsl:562, Water.hlsl:1160) compile; ensure
you change the signature in the ExtractLighting definition/any forward
declarations so the default applies everywhere.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/Water.hlsl (1)
1087-1169: Gate skylighting sampling to InWorld contexts to match the pattern used in Lighting.hlsl.
inWorldis computed but only used for wetness occlusion;Skylighting::sampleFast()anddirShadoware sampled unconditionally. Lighting.hlsl gates skylighting sampling explicitly (inWorld ? Skylighting::sample(...) : float4(sqrt(4.0 * Math::PI), 0, 0, 0)), but Water.hlsl applies all skylighting values regardless of context. WhilesampleFast()has internal safety for interiors, the proper pattern is to skip sampling entirely in non-world contexts. Gate the skylighting block oninWorldor reset skylighting values to 1.0 when not in world.
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 59-68: The line that does a self-referential gamma addition should
replace the additive operation with an assignment: locate the ambientColorAmb
accumulation in the IBL block (uses SharedData::iblSettings,
ImageBasedLighting::GetIBLColor and ambientColorAmb) and change the gamma
conversion call so that Color::IrradianceToGamma is assigned back to
ambientColorAmb instead of being added to it, i.e. stop doing ambientColorAmb +=
Color::IrradianceToGamma(ambientColorAmb) and use an assignment to avoid
doubling the value.
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: 1
🤖 Fix all issues with AI agents
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 76-79: The ternary uses a vector condition (totalLight > 0.0)
which is invalid in HLSL 2021/SM6; change the assignment to compute scale
per-component by using select(totalLight > 0.0, inputColor / totalLight, 1.0) or
explicit per-component ternaries (scale.x = totalLight.x > 0.0 ? inputColor.x /
totalLight.x : 1.0, etc.) so that totalLight, inputColor and scale are evaluated
component-wise and compile under DXC.
🧹 Nitpick comments (1)
package/Shaders/Common/ShadowSampling.hlsli (1)
55-56: Consider adding an issue reference for this feature PR.If there’s a tracking issue, add “Implements #…”, “Addresses #…”, or “Related to #…” in the PR description. As per coding guidelines, ...
| // Calculate total expected lighting and find scale to match input | ||
| float3 totalLight = ambientColorAmb + dirLightColorDir; | ||
| float3 scale = totalLight > 0.0 ? inputColor / totalLight : 1.0; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find build/compilation configuration files
find . -type f \( -name "*.cmake" -o -name "CMakeLists.txt" -o -name "*.props" -o -name "*.vcxproj" \) | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 348
🏁 Script executed:
# Search for similar vector-conditional ternary patterns in HLSL files
rg -t hlsl ">\s*0\s*\?\s*.*\s*:\s*1" | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 104
🌐 Web query:
HLSL ternary operator vector condition bool3 float3 swizzle component-wise selection DXC FXC support
💡 Result:
-
Component-wise (vector) selection in HLSL is now intended to be done with
select(), not?:. HLSL 2021 makes?:short-circuiting and therefore requires the condition to be scalar, so code likebool3 c; float3 r = c ? a : b;is an error under HLSL 2021 rules. Useselect(c, a, b)for per-component selection. [1][2] -
DXC: When compiling in an HLSL 2021 mode,
?:with non-scalar conditions is rejected andselect()is the replacement. The Clang/LLVM HLSL frontend explicitly describesselect(vector<bool,N>, ...)as the “ternary operator for vectors.” [1][3] -
FXC: FXC predates HLSL 2021; the “vector ternary” behavior is legacy/non-short-circuit semantics (both sides evaluated), and HLSL 2021’s change is specifically called out as a migration issue (fixed via
select()). [1][2][4]
Practical patterns
// Component-wise choose (bool3 -> float3)
float3 r = select(cond3, a3, b3); // HLSL 2021-safe [1][2]
// If you want a *single* scalar decision:
float3 r2 = (any(cond3) ? a3 : b3); // scalar condition [2]
// Swizzles are fine as long as sizes match:
float2 rg = select(cond3.xy, a3.xy, b3.xy); // per-component on xySources: [1] HLSL 2021 Migration Guide, [2] Announcing HLSL 2021, [3] LLVM/Clang HLSL select overloads commit thread, [4] discussion of HLSL ternary non-short-circuit/vector semantics.
🏁 Script executed:
# Check CMakeLists.txt for shader compilation settings
cat CMakeLists.txt | grep -A 10 -B 5 -i "hlsl\|shader\|dxc\|fxc" | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 1585
🏁 Script executed:
# Search for vector-conditional ternary patterns in hlsl/hlsli files (using rg with different approach)
rg ">\s*0\s*\?" --type-list hlsl:.hlsl,.hlsli -A 2 | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 465
🏁 Script executed:
# Search for 'select' function usage in HLSL files
rg "select\s*\(" --type-add 'hlsl:*.{hlsl,hlsli}' -A 1 -B 1 | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Check for shader compilation configuration in cmake files
cat cmake/DetectGraphicsTools.cmake | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 3415
🏁 Script executed:
# Look at shader test configuration
cat tests/shaders/CMakeLists.txtRepository: doodlum/skyrim-community-shaders
Length of output: 4951
🏁 Script executed:
# Search for shader compilation settings in cmake files
rg "add_shader|dxc|fxc|hlsl" cmake/ -i | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 849
🏁 Script executed:
# Look for vector-conditional patterns in all hlsl/hlsli files directly
fd -e hlsl -e hlsli -x grep -l "> 0.0 ?" {} \;Repository: doodlum/skyrim-community-shaders
Length of output: 121
🏁 Script executed:
# Look at the specific file mentioned in the review
cat package/Shaders/Common/ShadowSampling.hlsli | sed -n '70,85p'Repository: doodlum/skyrim-community-shaders
Length of output: 805
🏁 Script executed:
# Look for shader compilation configuration in ShaderTestFramework or local cmake
rg "shader_model|hlsl_version|std_" cmake/ -iRepository: doodlum/skyrim-community-shaders
Length of output: 109
🏁 Script executed:
# Search for DXC configuration or compiler flags
rg "Shader Model|HLSL.*2021|select\(" . -i | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 214
🏁 Script executed:
# Check if there are any select() calls in the entire codebase
fd -e hlsl -e hlsli -x grep -H "select\s*(" {} \;Repository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Look at the broader context of the ShadowSampling.hlsli file around line 78
cat package/Shaders/Common/ShadowSampling.hlsli | head -100 | tail -50Repository: doodlum/skyrim-community-shaders
Length of output: 2251
Use component-wise selection for vector condition in ternary operator.
Line 78 uses a vector condition (totalLight > 0.0) in a ternary operator, which is not supported in HLSL 2021 and will fail compilation under DXC (Shader Model 6.0+) when short-circuit semantics are enforced. Use the select() function for per-component selection:
float3 scale = select(totalLight > 0.0, inputColor / totalLight, 1.0);Alternatively, explicit per-component guards also work:
float3 scale;
scale.x = totalLight.x > 0.0 ? inputColor.x / totalLight.x : 1.0;
scale.y = totalLight.y > 0.0 ? inputColor.y / totalLight.y : 1.0;
scale.z = totalLight.z > 0.0 ? inputColor.z / totalLight.z : 1.0;🤖 Prompt for AI Agents
In `@package/Shaders/Common/ShadowSampling.hlsli` around lines 76 - 79, The
ternary uses a vector condition (totalLight > 0.0) which is invalid in HLSL
2021/SM6; change the assignment to compute scale per-component by using
select(totalLight > 0.0, inputColor / totalLight, 1.0) or explicit per-component
ternaries (scale.x = totalLight.x > 0.0 ? inputColor.x / totalLight.x : 1.0,
etc.) so that totalLight, inputColor and scale are evaluated component-wise and
compile under DXC.
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: 1
🤖 Fix all issues with AI agents
In `@src/Features/Skylighting.cpp`:
- Around line 318-324: The srvs array in Skylighting.cpp contains 4 SRVs
(texProbeArray->srv, stbn_vec3_2Dx1D_128x128x64,
texShadowVisibilityBitArray->srv, texShadowVisibility->srv) but
PSSetShaderResources is only binding 3 slots; update the call to
context->PSSetShaderResources to bind 4 resources (starting at slot 50) so
texShadowVisibility is bound (ensure the srvs ordering matches the expected
shader slots t50..t53 and that texShadowVisibility is the fourth entry).
♻️ Duplicate comments (3)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (2)
13-13: Typo in comment: "ditances" → "distances".Proposed fix
- float4 StartSplitDistances; // cascade start ditances int xyz, 4 int z + float4 StartSplitDistances; // cascade start distances in xyz, 4 in z
160-162: Potential uninitialized UAV read forshadowVisibilityBitShift.Unlike
shadowVisibilityBitsat line 123,outShadowVisibilityBitShiftArray[dtid]is read unconditionally. On the first frame or for newly valid cells, this may read uninitialized data, causing incorrect noise offset and shadow sampling position.Proposed fix
- uint shadowVisibilityBitShift = outShadowVisibilityBitShiftArray[dtid]; + uint shadowVisibilityBitShift = isValid ? outShadowVisibilityBitShiftArray[dtid] : 0;src/Features/Skylighting.cpp (1)
279-284: Guard against missing deferred resources before dispatch.
globals::deferred(or its SRVs) can be null early or in unsupported configs, which would crash here. Add a null guard and skip the dispatch/binding when unavailable. As per coding guidelines, this should fail gracefully.Proposed fix
auto deferred = globals::deferred; + if (!deferred || !deferred->shadowView || !deferred->perShadow || !deferred->perShadow->srv) { + return; + } std::array<ID3D11ShaderResourceView*, 3> srvs = { texOcclusion->srv.get(), deferred->shadowView, deferred->perShadow->srv.get() };
🧹 Nitpick comments (2)
features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (2)
209-215: Variableishadows outer loop variable — rename for clarity.The inner loop at line 209 declares
uint iwhich shadows the outer loop'sint ifrom line 150. While HLSL's scoping rules make this technically valid with[unroll], it reduces readability and could lead to maintenance bugs.Proposed fix
- for(uint i = 0; i < 32; i++){ - float3 bitCellCentreMS = cellCentreMS + noise3D[i] * Skylighting::CELL_SIZE; + for(uint sampleIdx = 0; sampleIdx < 32; sampleIdx++){ + float3 bitCellCentreMS = cellCentreMS + noise3D[sampleIdx] * Skylighting::CELL_SIZE; float weight = rcp(distance(positionMSAdjusted, bitCellCentreMS) + 1e-10); - tempShadowSum += float((shadowBits >> i) & 1u) * weight; + tempShadowSum += float((shadowBits >> sampleIdx) & 1u) * weight; tempShadowWeight += weight; }
169-202: Consider extracting sharednoise3Darray to a common header.This static array is duplicated in
UpdateProbesCS.hlsl. Extracting it to a shared header (e.g.,SkylightingCommon.hlsli) would reduce maintenance burden and ensure consistency.
| ID3D11ShaderResourceView* srvs[4] = { | ||
| texProbeArray->srv.get(), | ||
| stbn_vec3_2Dx1D_128x128x64.get(), | ||
| texShadowVisibilityBitArray->srv.get(), | ||
| texShadowVisibility->srv.get() | ||
| }; | ||
| context->PSSetShaderResources(50, 3, srvs); |
There was a problem hiding this comment.
PS shader resource binding count mismatch — missing texShadowVisibility binding.
The srvs array has 4 elements but PSSetShaderResources only binds 3. This means texShadowVisibility (expected at t53 per Skylighting.hlsli) won't be bound, causing the PS shader to sample undefined data.
Proposed fix
ID3D11ShaderResourceView* srvs[4] = {
texProbeArray->srv.get(),
stbn_vec3_2Dx1D_128x128x64.get(),
texShadowVisibilityBitArray->srv.get(),
texShadowVisibility->srv.get()
};
- context->PSSetShaderResources(50, 3, srvs);
+ context->PSSetShaderResources(50, 4, srvs);🤖 Prompt for AI Agents
In `@src/Features/Skylighting.cpp` around lines 318 - 324, The srvs array in
Skylighting.cpp contains 4 SRVs (texProbeArray->srv, stbn_vec3_2Dx1D_128x128x64,
texShadowVisibilityBitArray->srv, texShadowVisibility->srv) but
PSSetShaderResources is only binding 3 slots; update the call to
context->PSSetShaderResources to bind 4 resources (starting at slot 50) so
texShadowVisibility is bound (ensure the srvs ordering matches the expected
shader slots t50..t53 and that texShadowVisibility is the fourth entry).
There was a problem hiding this comment.
fix the ai comment, unmatched resource setup so t53 is unpopulated, PS will call junk data (per rabbitai)
There was a problem hiding this comment.
@davo0411, I'll fix the shader resource binding count mismatch.
Task: Fix PS shader resource binding count
- Update PSSetShaderResources call in Skylighting.cpp to bind all 4 resources
- Ensure texShadowVisibility is properly bound at register t53
🧠 Learnings used
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-24T13:06:22.620Z
Learning: Applies to **/*.hlsl : Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
minor cleanup needed, looks good otherwise, shader code solid & i like that it cleans up effect shadows system and removes it
| float3(0.214, 0.934, 0.372), | ||
| float3(0.578, -0.762, -0.614), | ||
| float3(-0.469, 0.381, 0.947) | ||
| }; |
There was a problem hiding this comment.
is this ever used elsewhere? does it belong in one of the shader utility files instead?
| wsum += w; | ||
| } | ||
|
|
||
| shadowVisibility = shadowSum * rcp(wsum + 1e-10); |
There was a problem hiding this comment.
why not reuse rcpWsum function from earlier? i.e make function declared higher and reuse for all of these
| float3(0.214, 0.934, 0.372), | ||
| float3(0.578, -0.762, -0.614), | ||
| float3(-0.469, 0.381, 0.947) | ||
| }; |
There was a problem hiding this comment.
duplicate Noise3D. pull it somewhere else and call it in both places
| else | ||
| { | ||
| context.coatLightColor = context.lightColor; | ||
| } |
There was a problem hiding this comment.
does this break interlayer parallax? i am unfamiliar with "InterlayerParallax"
| ID3D11ShaderResourceView* srvs[4] = { | ||
| texProbeArray->srv.get(), | ||
| stbn_vec3_2Dx1D_128x128x64.get(), | ||
| texShadowVisibilityBitArray->srv.get(), | ||
| texShadowVisibility->srv.get() | ||
| }; | ||
| context->PSSetShaderResources(50, 3, srvs); |
There was a problem hiding this comment.
fix the ai comment, unmatched resource setup so t53 is unpopulated, PS will call junk data (per rabbitai)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package/Shaders/Water.hlsl (1)
1021-1037:noiseis undefined in GetWaterDiffuseColor after the signature change.This block still uses
noise, but the parameter was removed, causing a compile error. Define it locally (or re‑add the parameter).🩹 Proposed fix (local noise)
# if defined(SKYLIGHTING) + float noise = Random::InterleavedGradientNoise(input.HPosition.xy, SharedData::FrameCount); float3 skylightingPosition = lerp(input.WPosition.xyz, refractionWorldPosition.xyz, noise);src/State.h (1)
151-157: Fix naming inconsistency in HLSL:IsReflections(C++) should beInReflection(HLSL).The bit positions between C++
ExtraShaderDescriptorsand HLSLExtraFlagsare correctly aligned. However, there's a naming mismatch at bit 1: the C++ enum declaresIsReflectionswhile the HLSL header usesInReflection. Update the C++ enum to match the HLSL naming convention for consistency across the codebase, or vice versa. The cache versioning mechanism already handles invalidation on version changes, and SE/AE/VR compatibility uses proper runtime detection patterns.
🤖 Fix all issues with AI agents
In `@package/Shaders/Water.hlsl`:
- Around line 1167-1168: The Skylighting::sampleFast call in Water.hlsl is
missing the new ShadowVisibilityBitArray parameter required by the updated
signature; update the call to include the shadow-bit array (e.g., pass
Skylighting::ShadowVisibilityBitArray or the appropriate
ShadowVisibilityBitArray symbol used elsewhere) so the call becomes
Skylighting::sampleFast(SharedData::skylightingSettings,
Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray,
Skylighting::ShadowVisibilityBitArray, positionMSSkylight, dirShadow) ensuring
the correct identifier for the bit array matches the rest of the codebase.
♻️ Duplicate comments (1)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (1)
160-167: Guard the bit‑shift UAV read to avoid uninitialized data.
outShadowVisibilityBitShiftArray[dtid]is read even whenisValidis false, which can pull garbage on first use. Please gate it like the bit array.🩹 Proposed fix
- uint shadowVisibilityBitShift = outShadowVisibilityBitShiftArray[dtid]; + uint shadowVisibilityBitShift = isValid ? outShadowVisibilityBitShiftArray[dtid] : 0;
🧹 Nitpick comments (4)
features/Skylighting/Shaders/Skylighting/UpdateProbesCS.hlsl (1)
25-33: New SRV/UAV bindings — run hlslkit register scan + compile.With
SharedShadowMap/SharedShadowDataand new visibility UAVs (u2–u4), please verify there are no register collisions across compute permutations by running hlslkit buffer scanning and compiling this shader.features/Skylighting/Shaders/Skylighting/Skylighting.hlsli (1)
12-15: New t52/t53 bindings — verify register layout with hlslkit.Please run hlslkit buffer scanning + targeted compilation to ensure the new
ShadowVisibilityBitArray/ShadowVisibilityProbeArraybindings don’t collide with existing PS permutations.package/Shaders/Lighting.hlsl (1)
2294-2299: Skylighting visibility bindings — validate resource registers.This shader now depends on the new skylighting visibility textures; please run hlslkit scanning/compilation to confirm no register conflicts across deferred/non‑deferred permutations.
package/Shaders/Water.hlsl (1)
441-443: New skylighting bindings — run hlslkit register scan + compile.With the added visibility textures in Skylighting, please verify register layouts for this shader’s permutations using hlslkit scanning and compilation.
| sh2 skylightingSH = Skylighting::sampleFast(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, positionMSSkylight, dirShadow); | ||
| float skylighting = SphericalHarmonics::Unproject(skylightingSH, float3(0, 0, 1)); |
There was a problem hiding this comment.
Skylighting::sampleFast call is missing the shadow‑bit array (compile break).
The new signature requires ShadowVisibilityBitArray. Without it, this call won’t compile.
🩹 Proposed fix
- sh2 skylightingSH = Skylighting::sampleFast(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, positionMSSkylight, dirShadow);
+ sh2 skylightingSH = Skylighting::sampleFast(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, Skylighting::ShadowVisibilityBitArray, positionMSSkylight, dirShadow);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sh2 skylightingSH = Skylighting::sampleFast(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, positionMSSkylight, dirShadow); | |
| float skylighting = SphericalHarmonics::Unproject(skylightingSH, float3(0, 0, 1)); | |
| sh2 skylightingSH = Skylighting::sampleFast(SharedData::skylightingSettings, Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray, Skylighting::ShadowVisibilityBitArray, positionMSSkylight, dirShadow); | |
| float skylighting = SphericalHarmonics::Unproject(skylightingSH, float3(0, 0, 1)); |
🤖 Prompt for AI Agents
In `@package/Shaders/Water.hlsl` around lines 1167 - 1168, The
Skylighting::sampleFast call in Water.hlsl is missing the new
ShadowVisibilityBitArray parameter required by the updated signature; update the
call to include the shadow-bit array (e.g., pass
Skylighting::ShadowVisibilityBitArray or the appropriate
ShadowVisibilityBitArray symbol used elsewhere) so the call becomes
Skylighting::sampleFast(SharedData::skylightingSettings,
Skylighting::SkylightingProbeArray, Skylighting::ShadowVisibilityProbeArray,
Skylighting::ShadowVisibilityBitArray, positionMSSkylight, dirShadow) ensuring
the correct identifier for the bit array matches the rest of the codebase.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.