feat: volumetric shadows#1851
Conversation
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.
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.
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/State.h (1)
152-158:⚠️ Potential issue | 🟡 MinorVerify shader-cache compatibility for the GrassSphereNormal bit shift.
Changing descriptor bit positions can make existing cached permutation keys ambiguous; please ensure the shader cache is invalidated or version-bumped to avoid mismatched lookups.
package/Shaders/Common/LightingEval.hlsli (1)
168-175:⚠️ Potential issue | 🟠 MajorWetness lighting applies detailed shadow twice in non-InterlayerParallax PBR path.
In
CreateDirectLightingContext, whenInterlayerParallaxis not enabled,context.coatLightColoris set tocontext.lightColor * detailedShadow. Then inEvaluateWetnessLightingline 172, the PBR path multiplies bydetailedShadowagain, squaring the shadow and over-darkening wet surfaces. The non-PBR path correctly applies shadow only once.Either remove the pre-multiplication of
detailedShadowfromcoatLightColorconstruction in the else branch, or avoid the multiplication inEvaluateWetnessLightingwhen using the pre-shadowed value.🐛 Suggested fix
- const float3 lightColor = context.coatLightColor * context.detailedShadow; + const float3 lightColor = context.coatLightColor;
🤖 Fix all issues with AI agents
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 91-97: Guard the distance fade denominator to prevent
divide-by-zero: when computing fade = saturate(distSq / sD.ShadowLightParam.z)
(and the equivalent 2D-path fade), replace the raw denominator with a clamped
value such as max(small_epsilon, sD.ShadowLightParam.z) so fade cannot be NaN;
then use that safe fade to compute sampleCount and rcpSampleCount. Update both
the 3D-path code around the midPosition/distSq calculation and the corresponding
2D-path fade calculation so both use the same clamped denominator symbol to
ensure sampleCount and rcpSampleCount are always valid.
In `@package/Shaders/Common/LightingEval.hlsli`:
- Around line 111-126: The specular term in the non-PBR lighting path uses
unshadowed context.lightColor (causing highlights in shadowed areas); update the
specular computation in the block that assigns lightingOutput.specular (which
calls VanillaSpecular(context, material.Shininess, uv)) to use the
already-computed diffuseLightColor (which includes context.detailedShadow)
instead of context.lightColor so specular respects detailedShadow like diffuse
and other specular paths.
In `@package/Shaders/Water.hlsl`:
- Line 1250: GetSunColor is declared with four parameters but the call uses only
two, causing a compile error; update the call at the site where "float3 sunColor
= GetSunColor(normal, viewDirection) * surfaceShadow;" to pass the missing
worldPosition and eyeIndex arguments (i.e. call GetSunColor(normal,
viewDirection, worldPosition, eyeIndex)), using the existing world position
variable in scope (e.g., worldPosition/WorldPosition) and the current eyeIndex
variable. Ensure the identifiers you pass match the names available in this
shader scope so the function signature GetSunColor(float3 normal, float3
viewDirection, float3 worldPosition, uint eyeIndex) is satisfied.
- Around line 880-882: ssrReflectionColorBlurred is incorrectly sampled from
RawSSRReflectionTex (a copy-paste bug) so the lerp with ssrReflectionColorRaw is
a no-op; change the sample for ssrReflectionColorBlurred to use the blurred SSR
texture and sampler (SSRReflectionTex with SSRReflectionSampler — the t10/s10
blurred resources) still using ssrReflectionUvDR, then keep
ssrReflectionColorRaw sampled from RawSSRReflectionTex/RawSSRReflectionSampler
and the lerp with ssrAmount * 0.7 unchanged.
In `@src/Features/VolumetricShadows.cpp`:
- Around line 346-349: The hook in VolumetricShadows::PostPostLoad uses
REL::RelocationID(100458, 107175) but passes a VR offset of 0x00, which will
crash in VR; update the REL::Relocate call to use the correct VR offset
(matching the other hooks for RelocationID(100458, 107175)) or wrap the
stl::write_thunk_call<CreateDepthStencil_VolumetricLighting>(...) invocation
with a VR guard (if (!REL::Module::IsVR()) ...) so the hook is not applied in VR
builds.
In `@src/Features/VolumetricShadows.h`:
- Around line 47-76: Add proper resource cleanup by implementing
ClearShaderCache() to Release() all COM pointers (copyShadowCS,
downsampleShadowMip0CS, downsampleShadowMip1CS, blurShadowHorizontalCS,
blurShadowVerticalCS, shadowCopyTexture, shadowCopySRV, shadowCopyMip0SRV,
shadowCopyMip1SRV, shadowCopyMip0UAV, shadowCopyMip1UAV, shadowBlurTempTexture,
shadowBlurTempMip0SRV, shadowBlurTempMip1SRV, shadowBlurTempMip0UAV,
shadowBlurTempMip1UAV, linearSampler) and delete/release the perShadow Buffer,
setting each pointer to nullptr and shadowCopyWidth/shadowCopyHeight to 0 after
release; make Release calls safe (check non-null before Release and then null
out). Add a class destructor (~VolumetricShadows) that calls ClearShaderCache()
so resources are freed on feature unload. Ensure ClearShaderCache() is
idempotent and can be called multiple times without UB.
In `@src/Hooks.cpp`:
- Around line 169-173: The thunk function currently dereferences
pass->geometry->worldBound.radius which can crash when pass or pass->geometry is
null; update static void thunk(RE::BSShader* shader, RE::BSRenderPass* pass,
uint32_t renderFlags) to check that pass and pass->geometry are non-null before
reading worldBound.radius and only assign
globals::state->permutationData.EffectRadius when safe, otherwise set it to a
safe default (e.g., 0.0f) after calling func(shader, pass, renderFlags).
- Around line 337-339: RE::INIPrefSettingCollection::GetSingleton() and its
GetSetting("iNumFocusShadow:Display") can return null; update the render hook in
Hooks.cpp so you null-check the singleton and the returned setting before
writing to setting->data.i. Specifically, call
RE::INIPrefSettingCollection::GetSingleton(), if non-null call
GetSetting("iNumFocusShadow:Display"), and only assign to the setting's data.i
when the setting pointer is non-null; leave the subsequent func() call
unchanged.
🧹 Nitpick comments (7)
features/Volumetric Shadows/Shaders/VolumetricShadows/DownsampleShadowCS.hlsl (1)
18-22: No fallback if neitherDOWNSAMPLE_SHADOW_MIP0norDOWNSAMPLE_SHADOW_MIP1is defined.If this shader is compiled without either define,
CASCADEis never declared and all references to it will fail. Consider adding a#elsewith an#errordirective to catch misconfiguration at compile time.💡 Suggested safeguard
`#if` defined(DOWNSAMPLE_SHADOW_MIP0) static const uint CASCADE = 1; `#elif` defined(DOWNSAMPLE_SHADOW_MIP1) static const uint CASCADE = 0; +#else +#error "Must define DOWNSAMPLE_SHADOW_MIP0 or DOWNSAMPLE_SHADOW_MIP1" `#endif`package/Shaders/Lighting.hlsl (1)
2398-2440: Shadow integration looks solid; run hlslkit register scan/compile.Given the new world + volumetric shadow path, please run hlslkit buffer scanning and compilation to confirm no register binding conflicts across permutations.
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."
features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (2)
9-28: Confirm t18/t19 bindings are conflict-free.New SharedShadowMap/SharedShadowData bindings should be verified against other shader resources; recommend hlslkit buffer scan + compile.
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."
30-45: Prefer a#definefor this scalar constant.Project convention favors
#definefor simple scalar constants.♻️ Suggested refactor
-static const float VSM_MIN_VARIANCE = 0.00001; +#define VSM_MIN_VARIANCE 0.00001Based on learnings: "In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use
#define(e.g.,#defineNTHREADS 128), while more complex constants use static const within namespaces."features/Volumetric Shadows/Shaders/VolumetricShadows/BlurShadowCS.hlsl (1)
5-24: Run hlslkit register scan/compile for the new bindings.t0/u0 bindings + shared cache should be validated against the broader shader set to avoid 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."
package/Shaders/Effect.hlsl (1)
602-631: 8-sample world-shadow loop is expensive; consider quality gating.This adds 8×
GetWorldShadowper pixel in a conditional path; consider tying sampleCount to a quality setting, distance cutoff, or a cheaper fallback.As per coding guidelines: "Always consider GPU workload and user experience when implementing graphics features."
package/Shaders/Common/ShadowSampling.hlsli (1)
71-77:surfaceShadowrelies on loop iteration order — fragile and unclear.
surfaceShadowis overwritten every iteration, keeping only the last sample (neareststartPosition). This works for the WATER/UNDERWATER paths wherestartPosition == positionWS, but for theEFFECTpath,startPositionis behind the effect position, sosurfaceShadowends up ~256 units away from the actual surface. Adding a brief comment explaining the intent (or sampling the surface explicitly) would improve clarity.Suggestion: add a comment or sample the surface explicitly
float worldShadow = 0.0; for(uint i = 0; i < sampleCount; i++){ float t = (float(i) + noise) * rcpSampleCount; float3 sampledPositionWS = lerp(endPosition, startPosition, t); float worldShadowSample = ShadowSampling::GetWorldShadow(sampledPositionWS, FrameBuffer::CameraPosAdjust[eyeIndex].xyz, eyeIndex); - surfaceShadow = worldShadowSample; + surfaceShadow = worldShadowSample; // last iteration ≈ startPosition (surface for WATER/UNDERWATER) worldShadow += worldShadowSample; }
| void VolumetricShadows::PostPostLoad() | ||
| { | ||
| stl::write_thunk_call<CreateDepthStencil_VolumetricLighting>(REL::RelocationID(100458, 107175).address() + REL::Relocate(0x9DC, 0x9DC, 0x00)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the VolumetricShadows.cpp file around the flagged lines
wc -l src/Features/VolumetricShadows.cppRepository: doodlum/skyrim-community-shaders
Length of output: 114
🏁 Script executed:
# Read the context around lines 346-349
sed -n '330,360p' src/Features/VolumetricShadows.cppRepository: doodlum/skyrim-community-shaders
Length of output: 761
🏁 Script executed:
# Search for other write_thunk_call patterns to understand the common VR offset handling
rg "write_thunk_call" --type cpp -B 2 -A 2 | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 7259
🏁 Script executed:
# Search for other REL::Relocate patterns with three parameters to see how VR offsets are typically set
rg "REL::Relocate\([^,]+,[^,]+," --type cpp | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 4627
🏁 Script executed:
# Search for any VR-specific handling in VolumetricShadows files
rg "VolumetricShadows|VolumetricLighting" --type cpp -B 3 -A 3 | grep -i "vr\|is_vr\|module"Repository: doodlum/skyrim-community-shaders
Length of output: 1443
🏁 Script executed:
# Search for other instances where 0x00 is used as a third parameter (VR offset)
rg "REL::Relocate\([^,]+,[^,]+, 0x00\)" --type cppRepository: doodlum/skyrim-community-shaders
Length of output: 254
🏁 Script executed:
# Check if there's a pattern of intentional VR gating or if 0x00 is ever used legitimately
rg "Relocate.*0x00" --type cpp -B 2 -A 2 | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 438
🏁 Script executed:
# Look for related depth stencil or volumetric shadow handling in VR context
rg "CreateDepthStencil|VolumetricShadow" --type cpp -B 5 -A 5 | head -80Repository: doodlum/skyrim-community-shaders
Length of output: 3364
🏁 Script executed:
# Check if VolumetricShadows has any VR-specific handling or comments
cat src/Features/VolumetricShadows.cpp | head -60Repository: doodlum/skyrim-community-shaders
Length of output: 2351
🏁 Script executed:
# Check the header file for VolumetricShadows to understand the feature better
find . -name "VolumetricShadows.h" -type fRepository: doodlum/skyrim-community-shaders
Length of output: 110
🏁 Script executed:
# Search for any TODO/FIXME comments or related issues around VR depth stencil
rg "VolumetricLighting|depth.*stencil" --type cpp -i | grep -i "vr\|bug\|fix\|todo" | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 528
🏁 Script executed:
# Check the VolumetricShadows header to understand VR compatibility
cat src/Features/VolumetricShadows.h | head -80Repository: doodlum/skyrim-community-shaders
Length of output: 3195
🏁 Script executed:
# Search for any recent commits or comments about this specific hook
rg "CreateDepthStencil_VolumetricLighting|0x9DC" --type cpp -B 5 -A 5Repository: doodlum/skyrim-community-shaders
Length of output: 1491
🏁 Script executed:
# Check if VolumetricShadows feature is explicitly disabled in VR anywhere
rg "VolumetricShadows" src/Features/VR.cppRepository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Look for the pattern of VR exclusion in other features
rg "if.*IsVR.*write_thunk_call" --type cpp -B 2 -A 2 | head -40Repository: doodlum/skyrim-community-shaders
Length of output: 58
VR relocation offset is missing for the depth-stencil hook.
All other hooks for RelocationID(100458, 107175) provide proper VR offsets; using 0x00 will cause crashes in VR. Either supply the correct VR offset or gate this hook in VR using if (!REL::Module::IsVR()) to avoid runtime failures.
🐛 Suggested fix (gate in VR)
- stl::write_thunk_call<CreateDepthStencil_VolumetricLighting>(REL::RelocationID(100458, 107175).address() + REL::Relocate(0x9DC, 0x9DC, 0x00));
+ if (!REL::Module::IsVR()) {
+ stl::write_thunk_call<CreateDepthStencil_VolumetricLighting>(
+ REL::RelocationID(100458, 107175).address() + REL::Relocate(0x9DC, 0x9DC, 0x00));
+ }🤖 Prompt for AI Agents
In `@src/Features/VolumetricShadows.cpp` around lines 346 - 349, The hook in
VolumetricShadows::PostPostLoad uses REL::RelocationID(100458, 107175) but
passes a VR offset of 0x00, which will crash in VR; update the REL::Relocate
call to use the correct VR offset (matching the other hooks for
RelocationID(100458, 107175)) or wrap the
stl::write_thunk_call<CreateDepthStencil_VolumetricLighting>(...) invocation
with a VR guard (if (!REL::Module::IsVR()) ...) so the hook is not applied in VR
builds.
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: 3
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)
934-995:⚠️ Potential issue | 🟠 MajorGuard
refractedViewDirectionwhen depth is zeroIn REFRACTIONS permutations without DEPTH,
refractionWorldPositionremains at 0 and flips the direction. Falling back toviewDirectionavoids inverted shadow rays.Proposed fix
- output.refractedViewDirection = normalize(refractionWorldPosition.xyz - input.WPosition.xyz); + float3 refractedDir = viewDirection; + if (depth > 0.0) + refractedDir = normalize(refractionWorldPosition.xyz - input.WPosition.xyz); + output.refractedViewDirection = refractedDir;
🤖 Fix all issues with AI agents
In `@package/Shaders/Effect.hlsl`:
- Around line 623-641: The shadow accumulator is incorrectly initialized to 1.0
which biases the averaged world-shadow result; change the initialization of the
local variable shadow to 0.0 so the loop that calls
ShadowSampling::GetWorldShadow (inside the inWorld && !SharedData::InInterior
branch) correctly sums samples and the subsequent multiply by rcpSampleCount
produces a proper average; leave the rest of the logic (the noisyIndex/tSample
computation, samplePositionWS adjustment, and use of
FrameBuffer::CameraPosAdjust[eyeIndex] and eyeIndex) unchanged.
- Around line 560-573: Get the directional shadow value from
ShadowSampling::Get3DFilteredShadow instead of leaving dirShadow at 1.0: change
the call in the inWorld && !SharedData::InInterior branch so that dirShadow is
assigned from ShadowSampling::Get3DFilteredShadow(worldPosition.xyz,
viewDirection, screenPosition, eyeIndex, unusedSurfaceShadow) (keeping
unusedSurfaceShadow as the last param), so dirShadow is used for dirColor and
shadowVariance calculations; ensure the function's return value replaces the
hardcoded 1.0 initialization of dirShadow and only call/assign when the inWorld
&& !SharedData::InInterior condition is true.
In `@package/Shaders/Water.hlsl`:
- Around line 873-883: The blurred SSR texture is currently sampled using
RawSSRReflectionSampler which bypasses intended filtering; in the SSR sampling
block (look for SSRReflectionTex, RawSSRReflectionTex, RawSSRReflectionSampler,
SSRReflectionSampler and the ssrReflectionColorBlurred variable) replace the
sampler used to sample SSRReflectionTex with SSRReflectionSampler so the blurred
texture uses the correct filtered sampler, leaving RawSSRReflectionTex sampled
with RawSSRReflectionSampler for the raw/nearest read and keeping subsequent
lerp and max logic unchanged.
🧹 Nitpick comments (4)
package/Shaders/Common/LightingEval.hlsli (1)
11-14: PR metadata: add an issue reference if one existsIf there’s a tracking issue for this feature, consider adding “Implements #…” (or similar) to the PR description.
As per coding guidelines: “Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords: ‘Fixes
#123’ / ‘Closes#123’ / ‘Implements#123’ / ‘Addresses#123’ / ‘Related to#123’.”package/Shaders/Effect.hlsl (1)
836-839: Run hlslkit register scan + targeted compile for the new shadow pathThe GrayscaleToAlpha + Soft/Falloff path now hits
GetLightingShadow; please run hlslkit buffer scanning and compile the key permutations to catch register conflicts early.As per coding guidelines: “/*.hlsl: Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts; Validate shaders with targeted testing using hlslkit compilation during development.”
package/Shaders/Lighting.hlsl (1)
2416-2434: Please validate volumetric‑shadow permutations with hlslkitThe new volumetric shadow path adds permutations around
VOLUMETRIC_SHADOWSand ShadowDir/DefShadow. A buffer scan + compile check should help catch any register conflicts early.As per coding guidelines: “/*.hlsl: Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts; Validate shaders with targeted testing using hlslkit compilation during development.”
package/Shaders/Water.hlsl (1)
836-889: Run hlslkit register scan/compile for updated water SSR + skylighting pathsThis shader now mixes SSR, dynamic cubemaps, and skylighting-driven shadowing; please run hlslkit buffer scans and compile the relevant permutations to catch register conflicts.
As per coding guidelines: “/*.hlsl: Highlight GPU register conflicts and recommend hlslkit buffer scanning to identify buffer conflicts; Validate shaders with targeted testing using hlslkit compilation during development.”
3 participants
Summary by CodeRabbit
New Features
Improvements
Compatibility