Conversation
📝 WalkthroughWalkthroughThis pull request centralizes shadow handling: adds CPU-side shadow scheduling and per-light shadow data, moves directional cascade data into Deferred uploads, replaces shadow-mask sampling with per-light PCF/PCSS sampling, extends shader/shared-data layouts and UI/settings persistence, and updates build config to include ExprTk. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Deferred as Deferred Renderer
participant LLF as LightLimitFix
participant SCM as ShadowCasterManager
participant VSM as VolumetricShadows
participant Shaders as GPU Shaders
App->>Deferred: SetupResources()
Deferred->>Deferred: Determine shadowMapSlots from SRV
Deferred->>Deferred: Create perDirectionalShadow buffer (SRV t98)
App->>LLF: SetupShadowResources()
LLF->>LLF: Create PCF/compare sampler, allocate perShadows (t100/t101)
App->>SCM: Install(settings)
SCM->>SCM: Initialize scheduler/formula parsing
loop Per-frame
App->>Deferred: EarlyPrepasses()
Deferred->>Deferred: Render shadow maps
Deferred->>Deferred: CopyShadowData() → populate DirectionalShadowData, upload SRV t98, bind cascades t99
LLF->>SCM: Update(settings, shadowSceneNode)
SCM->>SCM: Score & schedule shadow-casting lights
LLF->>LLF: CopyPointShadowData() → build per-slot ShadowData, upload SRV t100/t101, bind ShadowSamplerCmp
VSM->>VSM: CopyShadowData() (VSM downsample/blur) → bind t18
App->>Shaders: Render scene
Shaders->>Shaders: GetLightingShadow(DirectionalShadowData)
Shaders->>Shaders: GetShadowLightShadow(ShadowData) per light → PCF/PCSS sampling
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.2.510)vcpkg.json2026-03-29 20:05:58,422 [MainThread ] [ERROR] Template file not found: vcpkg.json ... [truncated 2529 characters] ... ceback__) 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. |
With this PR: - doodlum/skyrim-community-shaders#1941 We're getting close to the shadow limit being increased. The only reason LPO doesn't already include shadow casting lights everywhere is because of the shadow limit. Add an option for shadow lights to the fomod installer, and add shadow light configs. This prepares LPO for the impending shadow light limit increase.
alandtse
force-pushed
the
shadow-limit-fix
branch
from
3f3eb91 to
952dae6
Compare
|
Hmmm, lightlimitfix.cppa and lightlimigfix.h had a full file conflict for some reason. Don't pull the latest with the merge commit. Need to figure out why. |
alandtse
force-pushed
the
shadow-limit-fix
branch
2 times, most recently
from
efc619d to
ed692a5
Compare
|
Ok I confirmed the llf changes will look like a full file replace unless we rebase on dev before squashing. I'll take a pass to do that when this is ready to merge. We'll lose all the individual commits, but will have a smaller diff. |
alandtse
force-pushed
the
shadow-limit-fix
branch
from
ed692a5 to
e6ba2b7
Compare
|
Ok I rebased it so it should be clean for the short term. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Globals.h (1)
221-221:⚠️ Potential issue | 🟡 MinorDuplicate declaration of
tes.
RE::TES* tesis already declared at line 215. This duplicate declaration at line 221 should be removed.🧹 Proposed fix
extern RE::Setting* shadowMaskQuarter; extern REL::Relocation<ID3D11Buffer**> perFrame; extern REL::Relocation<RE::BSGraphics::BSShaderAccumulator**> currentAccumulator; - extern RE::TES* tes;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Globals.h` at line 221, Remove the duplicate extern declaration of RE::TES* tes in Globals.h by deleting the second occurrence (the redundant line declaring extern RE::TES* tes) so only the original declaration remains; ensure no other code depends on two externs and rebuild to confirm no linker changes are necessary.features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (1)
153-166:⚠️ Potential issue | 🟠 MajorRemove transpose from
GetVSMShadow2D()matrix multiplication.Lines 154 and 166 use
mul(transpose(shadowProj), ...)whileGetVSMShadow3D()(lines 92, 108) andShadowSampling::GetLightingShadow()both usemul(shadowProj, ...)with the sameDirectionalShadowData.ShadowProj. This inconsistency causes incorrect light space transformation.Proposed fix
- float3 positionLS = mul(transpose(shadowProj), float4(position, 1)).xyz; + float3 positionLS = mul(shadowProj, float4(position, 1)).xyz;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Volumetric` Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli around lines 153 - 166, The matrix multiplication in GetVSMShadow2D uses mul(transpose(shadowProj), float4(position, 1)) for both primary and secondary cascades which is inconsistent with GetVSMShadow3D and ShadowSampling::GetLightingShadow; change both occurrences to use mul(shadowProj, float4(position, 1)) so the same DirectionalShadowData.ShadowProj convention is used (affects the calls around SampleVSMCascade2D, the secondary cascade block using secondaryCascade = 1 - primaryCascade, and the positionLS computation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 12-15: GetShadowDepth and the other shadow-depth code are
incorrectly hard-coding FrameBuffer::CameraPosAdjust[0]; update all uses to
index with the provided eyeIndex (e.g., FrameBuffer::CameraPosAdjust[eyeIndex])
so the camera-adjusted depth, cascade selection, and ray transforms use the
correct eye. Locate GetShadowDepth(float3 positionWS, uint eyeIndex = 0) and the
nearby functions that accept eyeIndex (the shadow depth / cascade selection path
around the other occurrences) and replace any CameraPosAdjust[0] accesses with
CameraPosAdjust[eyeIndex], keeping the rest of the logic intact.
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 45-46: The two SRV declarations conflict when GRASS_COLLISION is
enabled because ShadowSampling.hlsli declares StructuredBuffer<ShadowData>
Shadows : register(t100) while GrassCollision.hlsli declares Texture2D<float4>
Collision : register(t100); update the ShadowSampling.hlsli resource bindings to
avoid t100 (e.g., shift Shadows and ShadowMaps to unused consecutive SRV
registers such as t102/t103 or a dedicated shadow SRV range), and ensure
RunGrass.hlsl/any includes using those new registers; adjust any shader code
that references Shadows or ShadowMaps to the new register names so the
grass+shadow-light variant has no binding overlap.
- Line 21: SharedShadowMap currently unguarded binds to register(t80) colliding
with TRUE_PBR's TexLandDisplacement0Sampler; locate the declaration of
Texture2D<float2> SharedShadowMap in ShadowSampling.hlsli and either wrap it
with `#if` defined(VOLUMETRIC_SHADOWS) / `#endif` so it only exists when volumetric
shadows are enabled, or move its register to an unused slot (e.g., change
register(t80) to register(t98) or register(t99)) to avoid the conflict with
Lighting.hlsl's TexLandDisplacement0Sampler under TRUE_PBR.
In `@package/Shaders/Lighting.hlsl`:
- Around line 2627-2652: The debug code treats overflowed lights as unshadowed
because UpdateLights() strips LightFlags::Shadow; change the logic to detect
overflow by inspecting light.shadowMapIndex before depending on
light.lightFlags: first check if light.shadowMapIndex >=
SharedData::lightLimitFixSettings.ShadowMapSlots and increment
debugOverflowCount (and handle as overflow), else proceed to test
(light.lightFlags & LightLimitFix::LightFlags::Shadow) to increment
debugPLShadowCount / debugUnshadowedPLCount and use shadowMapIndex to classify
types; update the block around debugOverflowCount, debugPLShadowCount,
debugUnshadowedPLCount and the shadow-type classification that currently reads
Shadows[light.shadowMapIndex].ShadowParam.x so overflowed indices are never
dereferenced.
In `@src/Deferred.cpp`:
- Around line 118-131: The code leaves shadowMapSlots unchanged when the
kSHADOWMAPS SRV is null or not a Texture2DArray/has ArraySize==0, causing later
code to treat stale non-zero slots as valid; in SetupResources (Deferred.cpp)
after checking
renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV,
set shadowMapSlots = 0 on both failure branches (null SRV and the else branch
where desc.ViewDimension != D3D11_SRV_DIMENSION_TEXTURE2DARRAY or ArraySize ==
0) and update the corresponding logger messages to reflect the reset so later
code using shadowMapSlots no longer assumes an array exists.
- Around line 172-189: The perDirectionalShadow pointer is overwritten in
SetupResources without releasing the previous GPU Buffer, leaking DirectX
resources; before assigning a new Buffer instance in the block that constructs
sbDesc/srvDesc, release or delete the existing perDirectionalShadow (or
preferably switch perDirectionalShadow to a smart pointer like
std::unique_ptr/ComPtr) and null-check it, then create the new Buffer and call
CreateSRV; update usages of perDirectionalShadow accordingly (Buffer,
perDirectionalShadow, CreateSRV) to ensure no leaks and safe reinitialization
when render targets are recreated.
In `@src/Features/LightLimitFix.cpp`:
- Around line 598-614: The loop currently uses bufferIndex (per-light count) to
decide shadowing against globals::deferred->shadowMapSlots, which is wrong for
multi-slice lights; change the logic to track slicesUsed (start 0) and for each
light (shadowSceneNode->GetRuntimeData().shadowLightsAccum[mapIndex]) compute
canShadow = (slicesUsed + light->shadowMapCount) <=
(int)globals::deferred->shadowMapSlots, pass canShadow to addShadowLight instead
of bufferIndex check, then if canShadow do slicesUsed += light->shadowMapCount;
still increment mapIndex by light->shadowMapCount and bufferIndex only if you
need the original per-light index (or remove bufferIndex entirely).
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 64-86: FormulaHelper currently holds a raw pointer _ptr to a
dynamically allocated FormulaWrapper and is copyable by default, which causes
shallow copies and double-free; make FormulaHelper non-copyable by explicitly
deleting the copy constructor and copy assignment operator (e.g., declare
FormulaHelper(const FormulaHelper&) = delete; and FormulaHelper& operator=(const
FormulaHelper&) = delete;) so copies cannot be made; leave or implement move
operations only if ownership transfer is needed, but at minimum delete copy
operations in the FormulaHelper declaration to prevent double-delete.
- Around line 103-120: Validate and clamp the combined slot count when loading
or initializing settings: in LoadSettings() or Init() (not just Install()),
compute total = Settings::ShadowLightCount + Settings::ConvertedShadowSlots + 1
(for sun) and clamp it to the renderer's supported maximum before allocating the
LightContainer or related DX11 resources; update Settings::ShadowLightCount
and/or Settings::ConvertedShadowSlots (or derive effective counts) so subsequent
code uses the capped values, and ensure allocation failures degrade gracefully
(release any partially created DirectX11 resources and log an error) to prevent
crashes from malformed JSON.
In `@src/Features/LightLimitFix/ShadowRenderer.cpp`:
- Around line 25-38: SetupShadowResources currently recreates the D3D11 sampler
without releasing the previous COM object; before calling
globals::d3d::device->CreateSamplerState(&cmpDesc, &shadowCmpSampler) in
LightLimitFix::SetupShadowResources, check if shadowCmpSampler is non-null and
call shadowCmpSampler->Release() (and set it to nullptr) to avoid leaking the
old sampler, and ensure that on CreateSamplerState failure the member is left
null to prevent dangling references.
- Around line 102-129: The current guard uses "plCount < slots" to allow writes
into sd[depthSlot], but for dual-paraboloid lights (shadowMapCount==2) depthSlot
can index past sd; change the condition to validate the actual depthSlot range
before writing — e.g., compute depthSlot using light->GetVRRuntimeData() /
GetRuntimeData() as done, then ensure (depthSlot + light->shadowMapCount) <=
slots (or depthSlot < slots and depthSlot + light->shadowMapCount - 1 < slots)
before touching sd[depthSlot] and calling SetShadowParameters /
ShadowCasterManager::RecordSlot so you never perform out-of-bounds writes for
lights with shadowMapCount == 2 (references: sd, plCount, slots, depthSlot,
shadowMapCount, shadowAccum, ShadowCasterManager, GetRuntimeData,
GetVRRuntimeData, SetShadowParameters).
In `@src/Features/VolumetricShadows.cpp`:
- Around line 276-279: The current binding logic prefers shadowCopySRV and
therefore rebinds a previous-frame SRV when the current source (shadowView) is
missing; change the selection to only bind a valid current-frame SRV and bind
null otherwise. Specifically, in the block that sets PS slot 18 (using
shadowCopySRV, shadowView and context->PSSetShaderResources), choose shadowView
if present and bind nullptr when shadowView is null (do not fall back to
shadowCopySRV), so the VSM path is effectively disabled for this frame; ensure
you pass a null ID3D11ShaderResourceView* to PSSetShaderResources when no
current SRV is available.
---
Outside diff comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 153-166: The matrix multiplication in GetVSMShadow2D uses
mul(transpose(shadowProj), float4(position, 1)) for both primary and secondary
cascades which is inconsistent with GetVSMShadow3D and
ShadowSampling::GetLightingShadow; change both occurrences to use
mul(shadowProj, float4(position, 1)) so the same
DirectionalShadowData.ShadowProj convention is used (affects the calls around
SampleVSMCascade2D, the secondary cascade block using secondaryCascade = 1 -
primaryCascade, and the positionLS computation).
In `@src/Globals.h`:
- Line 221: Remove the duplicate extern declaration of RE::TES* tes in Globals.h
by deleting the second occurrence (the redundant line declaring extern RE::TES*
tes) so only the original declaration remains; ensure no other code depends on
two externs and rebuild to confirm no linker changes are necessary.
🪄 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
Run ID: ac942958-f423-4cdd-a37d-aebe012b1bdb
📒 Files selected for processing (26)
CMakeLists.txtextern/CommonLibSSE-NGfeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslifeatures/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlslifeatures/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlslfeatures/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlslipackage/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Deferred.cppsrc/Deferred.hsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LightLimitFix/ShadowCasterManager.cppsrc/Features/LightLimitFix/ShadowCasterManager.hsrc/Features/LightLimitFix/ShadowRenderer.cppsrc/Features/RenderDoc.cppsrc/Features/VolumetricShadows.cppsrc/Features/VolumetricShadows.hsrc/Globals.cppsrc/Globals.hsrc/Menu/FeatureListRenderer.cppsrc/State.cppsrc/XSEPlugin.cppvcpkg.json
💤 Files with no reviewable changes (3)
- features/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli
- src/Features/VolumetricShadows.h
- features/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlsl
| float GetShadowDepth(float3 positionWS, uint eyeIndex = 0) | ||
| { | ||
| float4 positionCS = mul(FrameBuffer::CameraViewProj[eyeIndex], float4(positionWS, 1)); | ||
| return positionCS.z / positionCS.w; | ||
| return length(positionWS - FrameBuffer::CameraPosAdjust[0].xyz); | ||
| } |
There was a problem hiding this comment.
Use eyeIndex consistently in the camera-adjusted depth path.
Lines 14, 65-66, and 69 hard-code CameraPosAdjust[0] even though both functions take eyeIndex. In VR, the right eye will transform the ray and choose cascades from the left-eye origin, which is a straightforward way to get stereo mismatch near split boundaries.
Proposed fix
float GetShadowDepth(float3 positionWS, uint eyeIndex = 0)
{
- return length(positionWS - FrameBuffer::CameraPosAdjust[0].xyz);
+ return length(positionWS - FrameBuffer::CameraPosAdjust[eyeIndex].xyz);
}
...
- startPosition += FrameBuffer::CameraPosAdjust[0].xyz;
- endPosition += FrameBuffer::CameraPosAdjust[0].xyz;
+ startPosition += FrameBuffer::CameraPosAdjust[eyeIndex].xyz;
+ endPosition += FrameBuffer::CameraPosAdjust[eyeIndex].xyz;
...
- float shadowMapDepth = length(midPosition - FrameBuffer::CameraPosAdjust[0].xyz);
+ float shadowMapDepth = length(midPosition - FrameBuffer::CameraPosAdjust[eyeIndex].xyz);Also applies to: 63-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli around lines 12 - 15,
GetShadowDepth and the other shadow-depth code are incorrectly hard-coding
FrameBuffer::CameraPosAdjust[0]; update all uses to index with the provided
eyeIndex (e.g., FrameBuffer::CameraPosAdjust[eyeIndex]) so the camera-adjusted
depth, cascade selection, and ray transforms use the correct eye. Locate
GetShadowDepth(float3 positionWS, uint eyeIndex = 0) and the nearby functions
that accept eyeIndex (the shadow depth / cascade selection path around the other
occurrences) and replace any CameraPosAdjust[0] accesses with
CameraPosAdjust[eyeIndex], keeping the rest of the logic intact.
| # include "IBL/IBL.hlsli" | ||
| #endif | ||
|
|
||
| Texture2D<float2> SharedShadowMap : register(t80); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the ShadowSampling.hlsli file around line 21
cat -n package/Shaders/Common/ShadowSampling.hlsli | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 1014
🏁 Script executed:
# Check the Lighting.hlsl file around lines 468-469
sed -n '460,480p' package/Shaders/Lighting.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 1243
🏁 Script executed:
# Verify if Lighting.hlsl includes ShadowSampling.hlsli
grep -n "ShadowSampling" package/Shaders/Lighting.hlsl | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 599
🏁 Script executed:
# Check what other files in the same directory declare registers at t80
rg "register\(t80\)" package/Shaders/ -nRepository: doodlum/skyrim-community-shaders
Length of output: 270
🏁 Script executed:
# Check the context around line 467-468 in Lighting.hlsl to see the guard
sed -n '465,475p' package/Shaders/Lighting.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 608
🏁 Script executed:
# Check if SharedShadowMap in ShadowSampling.hlsli is guarded
sed -n '15,30p' package/Shaders/Common/ShadowSampling.hlsli | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 545
🏁 Script executed:
# Check what compilation paths include both files and define TRUE_PBR
# First, find which files include Lighting.hlsl
rg "include.*Lighting.hlsl" package/Shaders/ -n | head -10Repository: doodlum/skyrim-community-shaders
Length of output: 638
🏁 Script executed:
# Check if there are any define directives that set TRUE_PBR in Lighting.hlsl
grep -n "define TRUE_PBR" package/Shaders/Lighting.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 58
🏁 Script executed:
# Search for where TRUE_PBR is defined in the codebase
rg "TRUE_PBR" package/Shaders/ -n | grep -i "define" | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 1273
🏁 Script executed:
# Check if SharedShadowMap is used anywhere or if it's guarded
rg "SharedShadowMap" package/Shaders/ -nRepository: doodlum/skyrim-community-shaders
Length of output: 173
🏁 Script executed:
# Let's also verify the exact include order and context in Lighting.hlsl
sed -n '920,935p' package/Shaders/Lighting.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 718
🏁 Script executed:
# Check if there are compilation unit guards around the TRUE_PBR block in Lighting.hlsl
sed -n '465,510p' package/Shaders/Lighting.hlsl | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 2118
🏁 Script executed:
# Double-check if SharedShadowMap appears in any recent PR context or is maybe intended for a future feature
git log --oneline --all -20 -- package/Shaders/Common/ShadowSampling.hlsliRepository: doodlum/skyrim-community-shaders
Length of output: 1039
🏁 Script executed:
# Check if there are any TODO or FIXME comments around the SharedShadowMap declaration
sed -n '15,30p' package/Shaders/Common/ShadowSampling.hlsli | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 545
🏁 Script executed:
# Check the actual commit that introduced SharedShadowMap to understand context
git show 4f1df36:package/Shaders/Common/ShadowSampling.hlsli | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 793
🏁 Script executed:
# Look for any guards or upcoming use of SharedShadowMap in feature branches or TODOs
rg "SharedShadowMap|VOLUMETRIC_SHADOWS" package/Shaders/ -A 2 -B 2Repository: doodlum/skyrim-community-shaders
Length of output: 1987
SharedShadowMap creates an unguarded register binding collision with Lighting.hlsl's TRUE_PBR texture at t80.
Line 21 declares SharedShadowMap : register(t80) without any guard condition. However, when Lighting.hlsl includes this header in a LANDSCAPE+TRUE_PBR permutation, it also declares TexLandDisplacement0Sampler : register(t80) (lines 467–469, guarded by #if defined(TRUE_PBR)), resulting in two SRVs binding to the same register in the same shader compilation unit.
SharedShadowMap is currently unused in the codebase. Since the VOLUMETRIC_SHADOWS feature is already conditionally included in this file, wrap the declaration: #if defined(VOLUMETRIC_SHADOWS) / #endif, or assign it to a different available slot such as t98 or t99.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Common/ShadowSampling.hlsli` at line 21, SharedShadowMap
currently unguarded binds to register(t80) colliding with TRUE_PBR's
TexLandDisplacement0Sampler; locate the declaration of Texture2D<float2>
SharedShadowMap in ShadowSampling.hlsli and either wrap it with `#if`
defined(VOLUMETRIC_SHADOWS) / `#endif` so it only exists when volumetric shadows
are enabled, or move its register to an unused slot (e.g., change register(t80)
to register(t98) or register(t99)) to avoid the conflict with Lighting.hlsl's
TexLandDisplacement0Sampler under TRUE_PBR.
| StructuredBuffer<ShadowData> Shadows : register(t100); | ||
| Texture2DArray<float> ShadowMaps : register(t101); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "ShadowSampling.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 119
🏁 Script executed:
fd -t f "GrassCollision.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 144
🏁 Script executed:
fd -t f "RunGrass.hlsl"Repository: doodlum/skyrim-community-shaders
Length of output: 105
🏁 Script executed:
cat -n package/Shaders/Common/ShadowSampling.hlsli | sed -n '40,50p'Repository: doodlum/skyrim-community-shaders
Length of output: 453
🏁 Script executed:
cat -n "features/Grass Collision/Shaders/GrassCollision/GrassCollision.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 7487
🏁 Script executed:
cat -n package/Shaders/RunGrass.hlsl | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 1466
🏁 Script executed:
grep -n "#include" package/Shaders/RunGrass.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 355
🏁 Script executed:
wc -l package/Shaders/RunGrass.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 110
🏁 Script executed:
grep -n "ShadowSampling\|GrassCollision" package/Shaders/RunGrass.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 1223
🏁 Script executed:
grep -r "ShadowSampling.hlsli" package/Shaders/ --include="*.hlsl" --include="*.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 586
🏁 Script executed:
grep -r "GrassCollision.hlsli" package/Shaders/ --include="*.hlsl" --include="*.hlsli"Repository: doodlum/skyrim-community-shaders
Length of output: 155
🏁 Script executed:
sed -n '125,140p' package/Shaders/RunGrass.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 332
🏁 Script executed:
sed -n '440,460p' package/Shaders/RunGrass.hlslRepository: doodlum/skyrim-community-shaders
Length of output: 585
The shadow-light SRV block at t100 conflicts with grass collision bindings in combined variants.
When GRASS_COLLISION is defined, RunGrass.hlsl includes both GrassCollision.hlsli (line 132) and ShadowSampling.hlsli (line 451). Both declare resources at t100:
GrassCollision.hlsliline 3:Texture2D<float4> Collision : register(t100);ShadowSampling.hlsliline 45:StructuredBuffer<ShadowData> Shadows : register(t100);
This creates a binding conflict for the grass + shadow-light variant, making it impossible to bind correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Common/ShadowSampling.hlsli` around lines 45 - 46, The two
SRV declarations conflict when GRASS_COLLISION is enabled because
ShadowSampling.hlsli declares StructuredBuffer<ShadowData> Shadows :
register(t100) while GrassCollision.hlsli declares Texture2D<float4> Collision :
register(t100); update the ShadowSampling.hlsli resource bindings to avoid t100
(e.g., shift Shadows and ShadowMaps to unused consecutive SRV registers such as
t102/t103 or a dedicated shadow SRV range), and ensure RunGrass.hlsl/any
includes using those new registers; adjust any shader code that references
Shadows or ShadowMaps to the new register names so the grass+shadow-light
variant has no binding overlap.
| // kSHADOWMAPS is created by BSShaderRenderTargets_Create before SetupResources() is called, | ||
| // so the SRV is available here. Re-runs on resolution change (render targets recreated). | ||
| if (auto* shadowMapsSRV = renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV) { | ||
| D3D11_SHADER_RESOURCE_VIEW_DESC desc{}; | ||
| shadowMapsSRV->GetDesc(&desc); | ||
| if (desc.ViewDimension == D3D11_SRV_DIMENSION_TEXTURE2DARRAY && desc.Texture2DArray.ArraySize > 0) { | ||
| shadowMapSlots = desc.Texture2DArray.ArraySize; | ||
| logger::info("[Deferred] kSHADOWMAPS ArraySize = {}, effective shadowMapSlots = {}", desc.Texture2DArray.ArraySize, shadowMapSlots); | ||
| } else { | ||
| logger::warn("[Deferred] kSHADOWMAPS SRV not a Texture2DArray or ArraySize=0; keeping shadowMapSlots = {}", shadowMapSlots); | ||
| } | ||
| } else { | ||
| logger::warn("[Deferred] kSHADOWMAPS depthSRV is null at SetupResources; keeping shadowMapSlots = {}", shadowMapSlots); | ||
| } |
There was a problem hiding this comment.
Reset shadowMapSlots on the SRV failure paths.
Keeping the previous non-zero slot count here means the rest of the frame still treats the shadow array as valid, so later code keeps allocating/uploading/binding against resources that were not actually discovered this setup pass.
Suggested fix
if (auto* shadowMapsSRV = renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV) {
D3D11_SHADER_RESOURCE_VIEW_DESC desc{};
shadowMapsSRV->GetDesc(&desc);
if (desc.ViewDimension == D3D11_SRV_DIMENSION_TEXTURE2DARRAY && desc.Texture2DArray.ArraySize > 0) {
shadowMapSlots = desc.Texture2DArray.ArraySize;
logger::info("[Deferred] kSHADOWMAPS ArraySize = {}, effective shadowMapSlots = {}", desc.Texture2DArray.ArraySize, shadowMapSlots);
} else {
- logger::warn("[Deferred] kSHADOWMAPS SRV not a Texture2DArray or ArraySize=0; keeping shadowMapSlots = {}", shadowMapSlots);
+ shadowMapSlots = 0;
+ logger::warn("[Deferred] kSHADOWMAPS SRV not a Texture2DArray or ArraySize=0; resetting shadowMapSlots to 0");
}
} else {
- logger::warn("[Deferred] kSHADOWMAPS depthSRV is null at SetupResources; keeping shadowMapSlots = {}", shadowMapSlots);
+ shadowMapSlots = 0;
+ logger::warn("[Deferred] kSHADOWMAPS depthSRV is null at SetupResources; resetting shadowMapSlots to 0");
}As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Deferred.cpp` around lines 118 - 131, The code leaves shadowMapSlots
unchanged when the kSHADOWMAPS SRV is null or not a Texture2DArray/has
ArraySize==0, causing later code to treat stale non-zero slots as valid; in
SetupResources (Deferred.cpp) after checking
renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV,
set shadowMapSlots = 0 on both failure branches (null SRV and the else branch
where desc.ViewDimension != D3D11_SRV_DIMENSION_TEXTURE2DARRAY or ArraySize ==
0) and update the corresponding logger messages to reflect the reset so later
code using shadowMapSlots no longer assumes an array exists.
| struct FormulaHelper | ||
| { | ||
| FormulaHelper(); | ||
| ~FormulaHelper(); | ||
|
|
||
| bool Parse(const std::string& input); | ||
| double Calculate(); | ||
|
|
||
| /// Re-parse with a new expression, replacing any previously compiled formula. | ||
| /// Returns true on success. On failure the old formula remains active. | ||
| bool Reparse(const std::string& input); | ||
|
|
||
| /// Compile `input` into a temporary expression and return true if it succeeds. | ||
| /// On failure, `errorOut` receives the first parser error message. | ||
| /// Does NOT affect the active formula. | ||
| static bool Validate(const std::string& input, std::string& errorOut); | ||
|
|
||
| static void SetParam(int32_t index, double value); | ||
| static double GetParam(int32_t index); | ||
|
|
||
| private: | ||
| void* _ptr; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ShadowCasterManager.h" | head -5Repository: doodlum/skyrim-community-shaders
Length of output: 126
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.h | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 4295
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.h | tail -n +60 | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 2479
🏁 Script executed:
find . -type f -name "ShadowCasterManager.cpp" | head -5Repository: doodlum/skyrim-community-shaders
Length of output: 128
🏁 Script executed:
rg -A 20 "FormulaHelper::~FormulaHelper|FormulaHelper::FormulaHelper\(\)" src/Features/LightLimitFix/Repository: doodlum/skyrim-community-shaders
Length of output: 1843
Make FormulaHelper non-copyable.
_ptr manages a dynamically allocated FormulaWrapper* that is deleted in the destructor. Without explicit deletion of copy/move operations, copying a FormulaHelper instance will shallow-copy the pointer, causing a double-delete when either object is destroyed.
Proposed fix
struct FormulaHelper
{
FormulaHelper();
~FormulaHelper();
+ FormulaHelper(const FormulaHelper&) = delete;
+ FormulaHelper& operator=(const FormulaHelper&) = delete;
+ FormulaHelper(FormulaHelper&&) = delete;
+ FormulaHelper& operator=(FormulaHelper&&) = delete;
bool Parse(const std::string& input);
double Calculate();📝 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.
| struct FormulaHelper | |
| { | |
| FormulaHelper(); | |
| ~FormulaHelper(); | |
| bool Parse(const std::string& input); | |
| double Calculate(); | |
| /// Re-parse with a new expression, replacing any previously compiled formula. | |
| /// Returns true on success. On failure the old formula remains active. | |
| bool Reparse(const std::string& input); | |
| /// Compile `input` into a temporary expression and return true if it succeeds. | |
| /// On failure, `errorOut` receives the first parser error message. | |
| /// Does NOT affect the active formula. | |
| static bool Validate(const std::string& input, std::string& errorOut); | |
| static void SetParam(int32_t index, double value); | |
| static double GetParam(int32_t index); | |
| private: | |
| void* _ptr; | |
| }; | |
| struct FormulaHelper | |
| { | |
| FormulaHelper(); | |
| ~FormulaHelper(); | |
| FormulaHelper(const FormulaHelper&) = delete; | |
| FormulaHelper& operator=(const FormulaHelper&) = delete; | |
| FormulaHelper(FormulaHelper&&) = delete; | |
| FormulaHelper& operator=(FormulaHelper&&) = delete; | |
| bool Parse(const std::string& input); | |
| double Calculate(); | |
| /// Re-parse with a new expression, replacing any previously compiled formula. | |
| /// Returns true on success. On failure the old formula remains active. | |
| bool Reparse(const std::string& input); | |
| /// Compile `input` into a temporary expression and return true if it succeeds. | |
| /// On failure, `errorOut` receives the first parser error message. | |
| /// Does NOT affect the active formula. | |
| static bool Validate(const std::string& input, std::string& errorOut); | |
| static void SetParam(int32_t index, double value); | |
| static double GetParam(int32_t index); | |
| private: | |
| void* _ptr; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/LightLimitFix/ShadowCasterManager.h` around lines 64 - 86,
FormulaHelper currently holds a raw pointer _ptr to a dynamically allocated
FormulaWrapper and is copyable by default, which causes shallow copies and
double-free; make FormulaHelper non-copyable by explicitly deleting the copy
constructor and copy assignment operator (e.g., declare FormulaHelper(const
FormulaHelper&) = delete; and FormulaHelper& operator=(const FormulaHelper&) =
delete;) so copies cannot be made; leave or implement move operations only if
ownership transfer is needed, but at minimum delete copy operations in the
FormulaHelper declaration to prevent double-delete.
| struct Settings | ||
| { | ||
| /// Enable the shadow caster scheduler entirely. Requires a game restart to take effect. | ||
| bool Enabled = true; | ||
|
|
||
| /// Number of simultaneous shadow-casting point/spot lights (NOT counting the directional sun). | ||
| /// 0 = scheduler active but selects no point lights (sun/directional unaffected). | ||
| /// 4 = vanilla point light count with intelligent selection replacing the game's default. | ||
| /// 5-64 = extended mode; depth buffer array is expanded beyond game's 8-slot limit | ||
| /// when this exceeds 8. | ||
| /// Higher values allow more lights to hold stale shadow maps between redraws at | ||
| /// the cost of startup memory. The redraw budget and interval formula control | ||
| /// per-frame GPU cost independently. | ||
| int32_t ShadowLightCount = 16; | ||
|
|
||
| /// Number of additional converted-light slots (lights treated as normal lights | ||
| /// for geometry but tracked alongside shadow casters when ConvertExcessToNormal is enabled). | ||
| int32_t ConvertedShadowSlots = 32; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the main files in the ShadowCasterManager feature
find . -path "*LightLimitFix*" -type f | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 627
🏁 Script executed:
# Search for LoadSettings and Init implementations in the ShadowCasterManager area
rg "LoadSettings|void Init" --type cpp -A 5 -B 2 | grep -A 10 -B 2 "ShadowCasterManager\|ShadowLightCount\|ConvertedShadowSlots"Repository: doodlum/skyrim-community-shaders
Length of output: 2971
🏁 Script executed:
# Look for the 64-slot limit mentioned in PR notes
rg "64|ShadowLightCount|ConvertedShadowSlots" --type cpp --type h | head -30Repository: doodlum/skyrim-community-shaders
Length of output: 2244
🏁 Script executed:
# Find LightContainer definition and Size method
rg "class LightContainer|struct LightContainer" --type h -A 20Repository: doodlum/skyrim-community-shaders
Length of output: 1974
🏁 Script executed:
# Check FormulaHelper definition
rg "class FormulaHelper|struct FormulaHelper" --type h -A 15Repository: doodlum/skyrim-community-shaders
Length of output: 1467
🏁 Script executed:
# Read the Init implementation to see validation logic
sed -n '1,500p' ./src/Features/LightLimitFix/ShadowCasterManager.cpp | grep -A 30 "void Init"Repository: doodlum/skyrim-community-shaders
Length of output: 758
🏁 Script executed:
# Search for 64-slot limit or similar constraints
rg "ShadowLightCount|ConvertedShadowSlots|64" ./src/Features/LightLimitFix/ --type cpp --type h -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 33412
🏁 Script executed:
# Check LightLimitFix.cpp for LoadSettings to see validation
rg "LoadSettings|SaveSettings" ./src/Features/LightLimitFix.cpp -A 10Repository: doodlum/skyrim-community-shaders
Length of output: 338
🏁 Script executed:
# Look at the full ShadowCasterManager.h around FormulaHelper to see copy/move semantics
sed -n '50,150p' ./src/Features/LightLimitFix/ShadowCasterManager.hRepository: doodlum/skyrim-community-shaders
Length of output: 4357
🏁 Script executed:
# Check if there's any validation of settings in the feature's LoadSettings
cat ./src/Features/LightLimitFix.h | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 2691
Validate the combined slot count in LoadSettings() or Init().
LoadSettings() applies no validation, and Install() only clamps ShadowLightCount. A user can set both ShadowLightCount and ConvertedShadowSlots to 64 each via JSON, bypassing the UI sliders' individual limits. The combined total (ShadowLightCount + ConvertedShadowSlots + 1 for sun) should be clamped to the renderer's supported maximum before the LightContainer is allocated.
Per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/LightLimitFix/ShadowCasterManager.h` around lines 103 - 120,
Validate and clamp the combined slot count when loading or initializing
settings: in LoadSettings() or Init() (not just Install()), compute total =
Settings::ShadowLightCount + Settings::ConvertedShadowSlots + 1 (for sun) and
clamp it to the renderer's supported maximum before allocating the
LightContainer or related DX11 resources; update Settings::ShadowLightCount
and/or Settings::ConvertedShadowSlots (or derive effective counts) so subsequent
code uses the capped values, and ensure allocation failures degrade gracefully
(release any partially created DirectX11 resources and log an error) to prevent
crashes from malformed JSON.
| void LightLimitFix::SetupShadowResources() | ||
| { | ||
| // PCF comparison sampler bound to s14 for point/spot shadow sampling. | ||
| D3D11_SAMPLER_DESC cmpDesc = {}; | ||
| cmpDesc.Filter = D3D11_FILTER_COMPARISON_MIN_MAG_LINEAR_MIP_POINT; | ||
| cmpDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP; | ||
| cmpDesc.AddressV = D3D11_TEXTURE_ADDRESS_CLAMP; | ||
| cmpDesc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP; | ||
| cmpDesc.MaxAnisotropy = 1; | ||
| cmpDesc.ComparisonFunc = D3D11_COMPARISON_LESS_EQUAL; | ||
| cmpDesc.MinLOD = 0; | ||
| cmpDesc.MaxLOD = D3D11_FLOAT32_MAX; | ||
| DX::ThrowIfFailed(globals::d3d::device->CreateSamplerState(&cmpDesc, &shadowCmpSampler)); | ||
| } |
There was a problem hiding this comment.
SetupShadowResources() leaks the previous comparison sampler on reset.
This recreates shadowCmpSampler every setup pass without releasing the old COM object first, so resource rebuilds leak a sampler each time.
Suggested fix
void LightLimitFix::SetupShadowResources()
{
// PCF comparison sampler bound to s14 for point/spot shadow sampling.
+ if (shadowCmpSampler) {
+ shadowCmpSampler->Release();
+ shadowCmpSampler = nullptr;
+ }
+
D3D11_SAMPLER_DESC cmpDesc = {};
cmpDesc.Filter = D3D11_FILTER_COMPARISON_MIN_MAG_LINEAR_MIP_POINT;
cmpDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP;📝 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.
| void LightLimitFix::SetupShadowResources() | |
| { | |
| // PCF comparison sampler bound to s14 for point/spot shadow sampling. | |
| D3D11_SAMPLER_DESC cmpDesc = {}; | |
| cmpDesc.Filter = D3D11_FILTER_COMPARISON_MIN_MAG_LINEAR_MIP_POINT; | |
| cmpDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.AddressV = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.MaxAnisotropy = 1; | |
| cmpDesc.ComparisonFunc = D3D11_COMPARISON_LESS_EQUAL; | |
| cmpDesc.MinLOD = 0; | |
| cmpDesc.MaxLOD = D3D11_FLOAT32_MAX; | |
| DX::ThrowIfFailed(globals::d3d::device->CreateSamplerState(&cmpDesc, &shadowCmpSampler)); | |
| } | |
| void LightLimitFix::SetupShadowResources() | |
| { | |
| // PCF comparison sampler bound to s14 for point/spot shadow sampling. | |
| if (shadowCmpSampler) { | |
| shadowCmpSampler->Release(); | |
| shadowCmpSampler = nullptr; | |
| } | |
| D3D11_SAMPLER_DESC cmpDesc = {}; | |
| cmpDesc.Filter = D3D11_FILTER_COMPARISON_MIN_MAG_LINEAR_MIP_POINT; | |
| cmpDesc.AddressU = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.AddressV = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.AddressW = D3D11_TEXTURE_ADDRESS_CLAMP; | |
| cmpDesc.MaxAnisotropy = 1; | |
| cmpDesc.ComparisonFunc = D3D11_COMPARISON_LESS_EQUAL; | |
| cmpDesc.MinLOD = 0; | |
| cmpDesc.MaxLOD = D3D11_FLOAT32_MAX; | |
| DX::ThrowIfFailed(globals::d3d::device->CreateSamplerState(&cmpDesc, &shadowCmpSampler)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/LightLimitFix/ShadowRenderer.cpp` around lines 25 - 38,
SetupShadowResources currently recreates the D3D11 sampler without releasing the
previous COM object; before calling
globals::d3d::device->CreateSamplerState(&cmpDesc, &shadowCmpSampler) in
LightLimitFix::SetupShadowResources, check if shadowCmpSampler is non-null and
call shadowCmpSampler->Release() (and set it to nullptr) to avoid leaking the
old sampler, and ensure that on CreateSamplerState failure the member is left
null to prevent dangling references.
| while (true) { | ||
| auto light = shadowAccum[mapIndex]; | ||
| if (!light) | ||
| break; | ||
|
|
||
| mapIndex += light->shadowMapCount; | ||
|
|
||
| if (plCount < slots) { | ||
| uint32_t depthSlot = globals::game::isVR ? | ||
| light->GetVRRuntimeData().shadowmapDescriptors[0].shadowmapIndex : | ||
| light->GetRuntimeData().shadowmapDescriptors[0].shadowmapIndex; | ||
|
|
||
| float shadowTypeF = light->GetIsParabolicLight() ? float(light->shadowMapCount == 2 ? 2 : 1) : 0.f; | ||
| sd[depthSlot].ShadowParam.x = shadowTypeF; | ||
|
|
||
| if (globals::game::isVR) | ||
| SetShadowParameters(light->GetVRRuntimeData(), sd[depthSlot]); | ||
| else | ||
| SetShadowParameters(light->GetRuntimeData(), sd[depthSlot]); | ||
|
|
||
| float range = light->light->GetLightRuntimeData().radius.x; | ||
| // -1.0 sentinel: shader returns 0.0 (fully dark) → light invisible. | ||
| // 0.0 means unwritten slot → shader returns 1.0 (fully lit, no shadow). | ||
| uintptr_t lightKey = reinterpret_cast<uintptr_t>(light); | ||
| sd[depthSlot].ShadowParam.y = ShadowCasterManager::IsSuppressed(lightKey) ? -1.0f : range; | ||
| ShadowCasterManager::RecordSlot(depthSlot, | ||
| { static_cast<uint32_t>(shadowTypeF), range, true, lightKey }); | ||
| } else { |
There was a problem hiding this comment.
The per-slot shadow buffer guard is wrong for dual-paraboloid lights.
sd is sized by shadow-map slices, but this branch is gated by plCount < slots, which only counts logical lights. When shadowMapCount == 2, depthSlot can already be outside sd while the guard still passes, causing an out-of-bounds CPU write at sd[depthSlot].
Suggested fix
- if (plCount < slots) {
- uint32_t depthSlot = globals::game::isVR ?
- light->GetVRRuntimeData().shadowmapDescriptors[0].shadowmapIndex :
- light->GetRuntimeData().shadowmapDescriptors[0].shadowmapIndex;
+ uint32_t depthSlot = globals::game::isVR ?
+ light->GetVRRuntimeData().shadowmapDescriptors[0].shadowmapIndex :
+ light->GetRuntimeData().shadowmapDescriptors[0].shadowmapIndex;
+ const uint32_t shadowMapCount = light->shadowMapCount;
+
+ if (depthSlot < slots && depthSlot + shadowMapCount <= slots) {
float shadowTypeF = light->GetIsParabolicLight() ? float(light->shadowMapCount == 2 ? 2 : 1) : 0.f;
sd[depthSlot].ShadowParam.x = shadowTypeF;
@@
ShadowCasterManager::RecordSlot(depthSlot,
{ static_cast<uint32_t>(shadowTypeF), range, true, lightKey });
} else {
unshadowedLights++;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/LightLimitFix/ShadowRenderer.cpp` around lines 102 - 129, The
current guard uses "plCount < slots" to allow writes into sd[depthSlot], but for
dual-paraboloid lights (shadowMapCount==2) depthSlot can index past sd; change
the condition to validate the actual depthSlot range before writing — e.g.,
compute depthSlot using light->GetVRRuntimeData() / GetRuntimeData() as done,
then ensure (depthSlot + light->shadowMapCount) <= slots (or depthSlot < slots
and depthSlot + light->shadowMapCount - 1 < slots) before touching sd[depthSlot]
and calling SetShadowParameters / ShadowCasterManager::RecordSlot so you never
perform out-of-bounds writes for lights with shadowMapCount == 2 (references:
sd, plCount, slots, depthSlot, shadowMapCount, shadowAccum, ShadowCasterManager,
GetRuntimeData, GetVRRuntimeData, SetShadowParameters).
| // Bind the VSM shadow texture to PS slot 18. | ||
| // The shadow data structured buffer (slot 19) is already bound by Deferred::CopyShadowData(). | ||
| ID3D11ShaderResourceView* srv = shadowCopySRV ? shadowCopySRV : shadowView; | ||
| context->PSSetShaderResources(18, 1, &srv); |
There was a problem hiding this comment.
Avoid rebinding last frame's VSM copy when the source SRV is missing.
If PSGetShaderResources(4, ...) returns nullptr, this still binds the old shadowCopySRV, so slot 18 keeps sampling stale shadow data from an earlier frame/scene instead of disabling the VSM path for this frame.
As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/VolumetricShadows.cpp` around lines 276 - 279, The current
binding logic prefers shadowCopySRV and therefore rebinds a previous-frame SRV
when the current source (shadowView) is missing; change the selection to only
bind a valid current-frame SRV and bind null otherwise. Specifically, in the
block that sets PS slot 18 (using shadowCopySRV, shadowView and
context->PSSetShaderResources), choose shadowView if present and bind nullptr
when shadowView is null (do not fall back to shadowCopySRV), so the VSM path is
effectively disabled for this frame; ensure you pass a null
ID3D11ShaderResourceView* to PSSetShaderResources when no current SRV is
available.
GatherRed has shadow acne artifacts.
Move shadow-specific implementations out of LightLimitFix.cpp into LightLimitFix/ShadowRenderer.cpp to keep the base file closer to upstream and reduce future merge conflict surface area: - SetupShadowResources() (extracted from SetupResources) - EarlyPrepass() / CopyPointShadowData() - BuildShadowSlotColorLegend() - DrawShadowSamplingSettings() (extracted from DrawSettings) - DrawOverlay() MAX_LIGHTS / CLUSTER_MAX_LIGHTS promoted to private static constexpr members of LightLimitFix so both translation units can share them. LightLimitFix.cpp drops from 1037 to 716 lines. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tegory constant Post-rebase fixups: three changes that were dropped during the full linear rebase due to commit skipping and merge-base differences. - LightLimitFix.cpp: restore VR cluster defines in SetupResources and ClearShaderCache (commit was skipped as already-upstream cherry-pick) - LightLimitFix.h: restore FeatureCategories::kLighting constant - ShadowRenderer.cpp: restore ThemeManager::Constants::OVERLAY_WINDOW_POSITION and Util.h/ThemeManager.h includes in DrawOverlay - Lighting.hlsl: use correct LinearToSrgb (renamed from TrueLinearToGamma by upstream c8fe392) with the updated condition from fix: fix shadow detection Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ComputeSlopeBias used ddx/ddy inside the LLF light loop. At a shadow light's radius boundary, adjacent quad pixels diverge — one takes `continue`, one enters shadow sampling — making derivatives undefined. This produced NaN bias, causing the shadow test to always pass (fully lit), creating a bright artifact line at every shadow light boundary. Replaced slope bias with ShadowBiasConst in SampleParaboloidShadow and GetSpotlightShadow; removed now-dead ComputeSlopeBias and ShadowBiasSlopeScale.
alandtse
force-pushed
the
shadow-limit-fix
branch
from
b1089cd to
5f27c69
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/Deferred.cpp (2)
172-189:⚠️ Potential issue | 🟠 MajorRelease the old
perDirectionalShadowbuffer before recreating it.
Deferred::SetupResources()can rerun when render targets are recreated. Line 187 overwrites the raw pointer without deleting the previousBuffer, so each reset leaks one GPU resource.Proposed fix
- perDirectionalShadow = new Buffer(sbDesc); + delete perDirectionalShadow; + perDirectionalShadow = nullptr; + perDirectionalShadow = new Buffer(sbDesc); perDirectionalShadow->CreateSRV(srvDesc);As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 172 - 189, Deferred::SetupResources currently overwrites the raw perDirectionalShadow pointer without releasing the previous GPU resource, leaking a Buffer; before assigning a new Buffer(sbDesc) call delete (or release) on the existing perDirectionalShadow if it's non-null and set it to nullptr, then recreate it and call perDirectionalShadow->CreateSRV(srvDesc); update error paths to leave a valid state if creation fails and ensure the Buffer destructor/release path frees the underlying SRV and D3D resources via the Buffer class.
118-131:⚠️ Potential issue | 🟠 MajorReset
shadowMapSlotswhenkSHADOWMAPSdiscovery fails.Keeping the previous value here lets later passes keep treating a stale shadow-array capacity as valid after a target recreation or setup failure.
Proposed fix
if (desc.ViewDimension == D3D11_SRV_DIMENSION_TEXTURE2DARRAY && desc.Texture2DArray.ArraySize > 0) { shadowMapSlots = desc.Texture2DArray.ArraySize; logger::info("[Deferred] kSHADOWMAPS ArraySize = {}, effective shadowMapSlots = {}", desc.Texture2DArray.ArraySize, shadowMapSlots); } else { - logger::warn("[Deferred] kSHADOWMAPS SRV not a Texture2DArray or ArraySize=0; keeping shadowMapSlots = {}", shadowMapSlots); + shadowMapSlots = 0; + logger::warn("[Deferred] kSHADOWMAPS SRV not a Texture2DArray or ArraySize=0; resetting shadowMapSlots to 0"); } } else { - logger::warn("[Deferred] kSHADOWMAPS depthSRV is null at SetupResources; keeping shadowMapSlots = {}", shadowMapSlots); + shadowMapSlots = 0; + logger::warn("[Deferred] kSHADOWMAPS depthSRV is null at SetupResources; resetting shadowMapSlots to 0"); }As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 118 - 131, When kSHADOWMAPS discovery fails in SetupResources (i.e., depthSRV is null or desc.ViewDimension != D3D11_SRV_DIMENSION_TEXTURE2DARRAY or desc.Texture2DArray.ArraySize == 0), reset shadowMapSlots to 0 instead of preserving the prior value; update the branches that currently log warnings (the else branches around renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV and the ViewDimension/ArraySize check) to assign shadowMapSlots = 0 before logging so downstream passes don’t use a stale capacity.features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (1)
12-15:⚠️ Potential issue | 🟠 MajorUse
eyeIndexconsistently in the camera-adjusted depth path.Lines 14, 65-66, and 69 hard-code
FrameBuffer::CameraPosAdjust[0]. In VR the right eye will pick cascades and ray endpoints from the left-eye origin, which is a direct stereo mismatch near split boundaries.Proposed fix
float GetShadowDepth(float3 positionWS, uint eyeIndex = 0) { - return length(positionWS - FrameBuffer::CameraPosAdjust[0].xyz); + return length(positionWS - FrameBuffer::CameraPosAdjust[eyeIndex].xyz); } @@ - startPosition += FrameBuffer::CameraPosAdjust[0].xyz; - endPosition += FrameBuffer::CameraPosAdjust[0].xyz; + startPosition += FrameBuffer::CameraPosAdjust[eyeIndex].xyz; + endPosition += FrameBuffer::CameraPosAdjust[eyeIndex].xyz; @@ - float shadowMapDepth = length(midPosition - FrameBuffer::CameraPosAdjust[0].xyz); + float shadowMapDepth = length(midPosition - FrameBuffer::CameraPosAdjust[eyeIndex].xyz);Also applies to: 63-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Volumetric` Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli around lines 12 - 15, GetShadowDepth and nearby camera-adjusted depth/cascade/ray-endpoint code use FrameBuffer::CameraPosAdjust[0] instead of the provided eyeIndex, causing stereo mismatch; update GetShadowDepth to index CameraPosAdjust with the eyeIndex parameter and change the other hard-coded CameraPosAdjust[0] accesses in the same block (cascade selection and ray endpoint calculations) to use the eyeIndex variable (or pass the correct eyeIndex through helpers) so each eye uses its own CameraPosAdjust entry.src/Features/LightLimitFix.cpp (1)
598-614:⚠️ Potential issue | 🟠 MajorTrack consumed shadow-map slices, not shadowed lights.
shadowMapSlotsis array-slice capacity. Comparing it tobufferIndexlets a multi-slice light (shadowMapCount == 2) keep its Shadow flag after the array is already full, so the shader can sample a stale or out-of-rangeshadowMapIndex.Proposed fix
- int bufferIndex = 0; - int mapIndex = 0; + uint32_t mapIndex = 0; while (true) { RE::BSShadowLight* light = shadowSceneNode->GetRuntimeData().shadowLightsAccum[mapIndex]; if (!light) break; - // Only set Shadow flag for lights with a valid written slot. - // Overflow lights still use addShadowLight for correct color/radius setup, - // but without the Shadow flag so the HLSL does not do a shadow map lookup - // with a stale or out-of-range shadowMapIndex. - addShadowLight(light, bufferIndex < (int)globals::deferred->shadowMapSlots); - - mapIndex += light->shadowMapCount; - bufferIndex++; + const uint32_t shadowMapCount = light->shadowMapCount; + const bool hasWrittenSlots = mapIndex + shadowMapCount <= globals::deferred->shadowMapSlots; + + // Only set Shadow for lights whose slices fit inside the written array. + addShadowLight(light, hasWrittenSlots); + mapIndex += shadowMapCount; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/LightLimitFix.cpp` around lines 598 - 614, The loop currently compares bufferIndex (counting lights) against globals::deferred->shadowMapSlots, which allows multi-slice lights (shadowMapCount>1) to be marked Shadow even when the slice capacity is exhausted; change the logic to track consumed shadow-map slices instead: introduce a consumedSlices (or reuse bufferIndex as slice counter) and for each RE::BSShadowLight* light from shadowSceneNode->GetRuntimeData().shadowLightsAccum use consumedSlices and light->shadowMapCount to decide the flag passed to addShadowLight (true only if consumedSlices + light->shadowMapCount <= (int)globals::deferred->shadowMapSlots), then increment consumedSlices by light->shadowMapCount; keep mapIndex += light->shadowMapCount to advance the source array.src/Features/VolumetricShadows.cpp (1)
276-279:⚠️ Potential issue | 🟠 MajorBind slot 18 only when this frame has a valid source shadow SRV.
Line 278 falls back to
shadowCopySRVwhenshadowViewis null, so a frame with no current source keeps sampling the previous frame’s blurred VSM texture instead of disabling the VSM path for that frame.Proposed fix
- ID3D11ShaderResourceView* srv = shadowCopySRV ? shadowCopySRV : shadowView; + ID3D11ShaderResourceView* srv = shadowView ? shadowCopySRV : nullptr; context->PSSetShaderResources(18, 1, &srv);As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VolumetricShadows.cpp` around lines 276 - 279, The current binding falls back to shadowCopySRV when shadowView is null causing stale VSM sampling; change the logic around the PSSetShaderResources call so you only bind a VSM SRV to slot 18 when the current-frame source SRV (shadowView) is valid — if shadowView is null, bind a null/empty SRV to slot 18 (do not use shadowCopySRV as fallback) while leaving the shadow data structured buffer binding (Deferred::CopyShadowData()) unchanged; update the code around the shadowView/shadowCopySRV variables and the context->PSSetShaderResources(18, 1, ...) call accordingly.package/Shaders/Common/ShadowSampling.hlsli (1)
45-46:⚠️ Potential issue | 🔴 Critical
Shadowsstill conflicts with the grass-collision SRV slot.
features/Grass Collision/Shaders/GrassCollision/GrassCollision.hlslialready bindsCollisiontot100. KeepingShadowsatt100makes the grass + shadow-light permutation invalid, so the shadow-light SRVs need a dedicated range and the matching CPU binds updated with it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Common/ShadowSampling.hlsli` around lines 45 - 46, The Shadow SRV binding for StructuredBuffer<ShadowData> named Shadows is conflicting with the grass collision SRV (Collision bound to t100); move the shadow SRV range away from t100 (e.g., choose an unused register range for Shadows and ShadowMaps instead of t100/t101) and update the HLSL declarations (Shadows and ShadowMaps) to the new registers, then update the matching CPU-side bind code that sets these SRVs so the new register indices are used; ensure you also update any permutation or binding table that assumes t100 to avoid the grass+shadow-light permutation failure (refer to Shadows, ShadowMaps, and Collision to locate all usages).src/Features/LightLimitFix/ShadowCasterManager.h (2)
116-120:⚠️ Potential issue | 🟠 MajorClamp the combined shadow-slot budget before allocation.
These two fields share the same renderer limit, so malformed JSON can still push
ShadowLightCount + ConvertedShadowSlots + 1past the supported slot count even if each field is individually "valid". Please cap the combined total in the load/init path before allocating the light container or DX11 resources. As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/LightLimitFix/ShadowCasterManager.h` around lines 116 - 120, Clamp the combined shadow-slot budget before any allocation: in the manager's load/init code where ShadowLightCount and ConvertedShadowSlots are parsed/validated, compute total = ShadowLightCount + ConvertedShadowSlots + 1, then cap total to the renderer's max slots and reduce ConvertedShadowSlots (or ShadowLightCount) accordingly so allocations for the light container and DX11 resources never request more than the supported slot count; ensure the capped values are used for subsequent resource creation and add a safe fallback path that logs the adjustment and avoids creating oversized DX11 resources.
64-67: 🛠️ Refactor suggestion | 🟠 MajorDelete
FormulaHelpercopy operations.
_ptris owning state torn down in~FormulaHelper(), so the compiler-generated copy members would shallow-copy it and double-delete on the first accidental copy. Please delete copy/move here unless you implement real transfer semantics.🛠️ Example fix
struct FormulaHelper { FormulaHelper(); ~FormulaHelper(); + FormulaHelper(const FormulaHelper&) = delete; + FormulaHelper& operator=(const FormulaHelper&) = delete; + FormulaHelper(FormulaHelper&&) = delete; + FormulaHelper& operator=(FormulaHelper&&) = delete; bool Parse(const std::string& input); double Calculate(); @@ private: void* _ptr; };Also applies to: 84-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/LightLimitFix/ShadowCasterManager.h` around lines 64 - 67, FormulaHelper owns `_ptr` and defines a destructor, so you must prevent implicit shallow copies; declare the copy constructor and copy-assignment operator as deleted (FormulaHelper(const FormulaHelper&) = delete; FormulaHelper& operator=(const FormulaHelper&) = delete;) and also delete the move constructor and move-assignment operator unless you implement transfer semantics (FormulaHelper(FormulaHelper&&) = delete; FormulaHelper& operator=(FormulaHelper&&) = delete;). Apply the same deletion to the other owning struct in this file that holds `_ptr`.
🧹 Nitpick comments (1)
package/Shaders/RunGrass.hlsl (1)
598-599: Compute the PCF rotation matrix lazily.Both pixel shader variants build this before they know whether the tile contains any shadowed point lights. In grass-heavy scenes that adds per-fragment work even when the clustered-light loop exits early or every clustered light is unshadowed. Compute it on the first
LightLimitFix::LightFlags::Shadowhit instead.Also applies to: 873-874
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/RunGrass.hlsl` around lines 598 - 599, Currently the rotationMatrix is computed unconditionally with ShadowSampling::GetPCFRotationMatrix(input.WorldPosition.xyz); move that call so it is computed lazily the first time a light with LightLimitFix::LightFlags::Shadow is encountered in the per-pixel clustered-light loop: introduce a local flag (e.g. bool pcfComputed) and a float2x2 pcfRotation local, then on the first shadow-flag hit call ShadowSampling::GetPCFRotationMatrix(input.WorldPosition.xyz) to populate pcfRotation and set pcfComputed, and use pcfRotation thereafter instead of the eagerly-created rotationMatrix; apply the same change in both pixel shader variants (also update the other occurrence around lines 873-874).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 156-157: The current fade uses the full range (fade =
saturate(shadowMapDepth / shadow.EndSplitDistances.y)), causing early fading;
change it to a narrow edge-only ramp: compute a small window near
shadow.EndSplitDistances.y (e.g., margin either a small constant or a fraction
of the cascade size using shadow.StartSplitDistances.y and
shadow.EndSplitDistances.y) and remap shadowMapDepth into that window using a
smooth ramp (smoothstep or equivalent) so fading only starts inside that narrow
boundary; update the calculation that produces fade (referencing fade,
shadowMapDepth, shadow.StartSplitDistances.y, and shadow.EndSplitDistances.y)
accordingly.
In `@src/Deferred.cpp`:
- Around line 580-589: The early return guarded by shadowMapSlots causes
directional shadow cascade upload to be skipped and leaves stale SRV bindings in
t98/t99; instead, stop gating the sun cascade upload on shadowMapSlots and gate
on the presence of sunShadowLight and the cascade SRVs (e.g., check
globals::game::smState->shadowSceneNode,
shadowSceneNode->GetRuntimeData().sunShadowDirLight and the ESram cascade SRVs),
and on any path where the cascade data is unavailable explicitly bind null to
shader slots t98 and t99 to clear previous-frame bindings before returning;
update the logic around shadowMapSlots, shadowSceneNode, sunShadowLight and the
code paths referenced (lines handling t98/t99 binding at the end of the block)
to implement these checks and null-bind clears.
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 76-82: The Validate/SetParam/GetParam trio currently shares a
global parameter table allowing light-scoped symbols in RedrawBudgetFormula;
update Validate(const std::string& input, std::string& errorOut) to enforce a
symbol whitelist when validating expressions for RedrawBudgetFormula (only allow
frametime, frametarget, stableframes, camera/environment params) and reject or
rewrite any light-scoped symbols (lightindex, lightintensity, etc.), or
alternatively split the backing parameter storage into separate tables per
formula type and have Validate check/associate the correct table; also ensure
SetParam and GetParam operate against the per-formula table (or respect the same
symbol restrictions) so runtime reads/writes cannot access stale light-scoped
values for RedrawBudgetFormula.
---
Duplicate comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 12-15: GetShadowDepth and nearby camera-adjusted
depth/cascade/ray-endpoint code use FrameBuffer::CameraPosAdjust[0] instead of
the provided eyeIndex, causing stereo mismatch; update GetShadowDepth to index
CameraPosAdjust with the eyeIndex parameter and change the other hard-coded
CameraPosAdjust[0] accesses in the same block (cascade selection and ray
endpoint calculations) to use the eyeIndex variable (or pass the correct
eyeIndex through helpers) so each eye uses its own CameraPosAdjust entry.
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 45-46: The Shadow SRV binding for StructuredBuffer<ShadowData>
named Shadows is conflicting with the grass collision SRV (Collision bound to
t100); move the shadow SRV range away from t100 (e.g., choose an unused register
range for Shadows and ShadowMaps instead of t100/t101) and update the HLSL
declarations (Shadows and ShadowMaps) to the new registers, then update the
matching CPU-side bind code that sets these SRVs so the new register indices are
used; ensure you also update any permutation or binding table that assumes t100
to avoid the grass+shadow-light permutation failure (refer to Shadows,
ShadowMaps, and Collision to locate all usages).
In `@src/Deferred.cpp`:
- Around line 172-189: Deferred::SetupResources currently overwrites the raw
perDirectionalShadow pointer without releasing the previous GPU resource,
leaking a Buffer; before assigning a new Buffer(sbDesc) call delete (or release)
on the existing perDirectionalShadow if it's non-null and set it to nullptr,
then recreate it and call perDirectionalShadow->CreateSRV(srvDesc); update error
paths to leave a valid state if creation fails and ensure the Buffer
destructor/release path frees the underlying SRV and D3D resources via the
Buffer class.
- Around line 118-131: When kSHADOWMAPS discovery fails in SetupResources (i.e.,
depthSRV is null or desc.ViewDimension != D3D11_SRV_DIMENSION_TEXTURE2DARRAY or
desc.Texture2DArray.ArraySize == 0), reset shadowMapSlots to 0 instead of
preserving the prior value; update the branches that currently log warnings (the
else branches around
renderer->GetDepthStencilData().depthStencils[RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS].depthSRV
and the ViewDimension/ArraySize check) to assign shadowMapSlots = 0 before
logging so downstream passes don’t use a stale capacity.
In `@src/Features/LightLimitFix.cpp`:
- Around line 598-614: The loop currently compares bufferIndex (counting lights)
against globals::deferred->shadowMapSlots, which allows multi-slice lights
(shadowMapCount>1) to be marked Shadow even when the slice capacity is
exhausted; change the logic to track consumed shadow-map slices instead:
introduce a consumedSlices (or reuse bufferIndex as slice counter) and for each
RE::BSShadowLight* light from
shadowSceneNode->GetRuntimeData().shadowLightsAccum use consumedSlices and
light->shadowMapCount to decide the flag passed to addShadowLight (true only if
consumedSlices + light->shadowMapCount <=
(int)globals::deferred->shadowMapSlots), then increment consumedSlices by
light->shadowMapCount; keep mapIndex += light->shadowMapCount to advance the
source array.
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 116-120: Clamp the combined shadow-slot budget before any
allocation: in the manager's load/init code where ShadowLightCount and
ConvertedShadowSlots are parsed/validated, compute total = ShadowLightCount +
ConvertedShadowSlots + 1, then cap total to the renderer's max slots and reduce
ConvertedShadowSlots (or ShadowLightCount) accordingly so allocations for the
light container and DX11 resources never request more than the supported slot
count; ensure the capped values are used for subsequent resource creation and
add a safe fallback path that logs the adjustment and avoids creating oversized
DX11 resources.
- Around line 64-67: FormulaHelper owns `_ptr` and defines a destructor, so you
must prevent implicit shallow copies; declare the copy constructor and
copy-assignment operator as deleted (FormulaHelper(const FormulaHelper&) =
delete; FormulaHelper& operator=(const FormulaHelper&) = delete;) and also
delete the move constructor and move-assignment operator unless you implement
transfer semantics (FormulaHelper(FormulaHelper&&) = delete; FormulaHelper&
operator=(FormulaHelper&&) = delete;). Apply the same deletion to the other
owning struct in this file that holds `_ptr`.
In `@src/Features/VolumetricShadows.cpp`:
- Around line 276-279: The current binding falls back to shadowCopySRV when
shadowView is null causing stale VSM sampling; change the logic around the
PSSetShaderResources call so you only bind a VSM SRV to slot 18 when the
current-frame source SRV (shadowView) is valid — if shadowView is null, bind a
null/empty SRV to slot 18 (do not use shadowCopySRV as fallback) while leaving
the shadow data structured buffer binding (Deferred::CopyShadowData())
unchanged; update the code around the shadowView/shadowCopySRV variables and the
context->PSSetShaderResources(18, 1, ...) call accordingly.
---
Nitpick comments:
In `@package/Shaders/RunGrass.hlsl`:
- Around line 598-599: Currently the rotationMatrix is computed unconditionally
with ShadowSampling::GetPCFRotationMatrix(input.WorldPosition.xyz); move that
call so it is computed lazily the first time a light with
LightLimitFix::LightFlags::Shadow is encountered in the per-pixel
clustered-light loop: introduce a local flag (e.g. bool pcfComputed) and a
float2x2 pcfRotation local, then on the first shadow-flag hit call
ShadowSampling::GetPCFRotationMatrix(input.WorldPosition.xyz) to populate
pcfRotation and set pcfComputed, and use pcfRotation thereafter instead of the
eagerly-created rotationMatrix; apply the same change in both pixel shader
variants (also update the other occurrence around lines 873-874).
🪄 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
Run ID: 49bb58e4-c1dd-470b-917f-2d1f850315d4
📒 Files selected for processing (26)
CMakeLists.txtextern/CommonLibSSE-NGfeatures/Light Limit Fix/Shaders/LightLimitFix/Common.hlslifeatures/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlslifeatures/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlslfeatures/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlslipackage/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Deferred.cppsrc/Deferred.hsrc/Features/LightLimitFix.cppsrc/Features/LightLimitFix.hsrc/Features/LightLimitFix/ShadowCasterManager.cppsrc/Features/LightLimitFix/ShadowCasterManager.hsrc/Features/LightLimitFix/ShadowRenderer.cppsrc/Features/RenderDoc.cppsrc/Features/VolumetricShadows.cppsrc/Features/VolumetricShadows.hsrc/Globals.cppsrc/Globals.hsrc/Menu/FeatureListRenderer.cppsrc/State.cppsrc/XSEPlugin.cppvcpkg.json
💤 Files with no reviewable changes (3)
- features/Light Limit Fix/Shaders/LightLimitFix/LightLimitFix.hlsli
- src/Features/VolumetricShadows.h
- features/Volumetric Shadows/Shaders/VolumetricShadows/CopyShadowDataCS.hlsl
✅ Files skipped from review due to trivial changes (7)
- extern/CommonLibSSE-NG
- vcpkg.json
- src/State.cpp
- src/Menu/FeatureListRenderer.cpp
- src/XSEPlugin.cpp
- src/Globals.h
- package/Shaders/Lighting.hlsl
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Features/RenderDoc.cpp
- features/Light Limit Fix/Shaders/LightLimitFix/Common.hlsli
- src/Globals.cpp
- package/Shaders/Common/SharedData.hlsli
- src/Features/LightLimitFix/ShadowRenderer.cpp
| // Fade shadows out toward the cascade boundary so they dissolve cleanly. | ||
| float fade = saturate(shadowMapDepth / shadow.EndSplitDistances.y); |
There was a problem hiding this comment.
This is a full-range fade, not a boundary fade.
fade = saturate(shadowMapDepth / shadow.EndSplitDistances.y) starts blending shadows back to fully lit immediately at the camera, so a point halfway through the far cascade is already 50% faded. If the intent is only to hide the far-edge cutoff, start the ramp in a narrow window near EndSplitDistances.y.
💡 Example fix
- float fade = saturate(shadowMapDepth / shadow.EndSplitDistances.y);
+ float fadeStart = shadow.EndSplitDistances.y * 0.9;
+ float fade = smoothstep(fadeStart, shadow.EndSplitDistances.y, shadowMapDepth);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/Common/ShadowSampling.hlsli` around lines 156 - 157, The
current fade uses the full range (fade = saturate(shadowMapDepth /
shadow.EndSplitDistances.y)), causing early fading; change it to a narrow
edge-only ramp: compute a small window near shadow.EndSplitDistances.y (e.g.,
margin either a small constant or a fraction of the cascade size using
shadow.StartSplitDistances.y and shadow.EndSplitDistances.y) and remap
shadowMapDepth into that window using a smooth ramp (smoothstep or equivalent)
so fading only starts inside that narrow boundary; update the calculation that
produces fade (referencing fade, shadowMapDepth, shadow.StartSplitDistances.y,
and shadow.EndSplitDistances.y) accordingly.
| // Dispatch the active filter mode for omnidirectional/paraboloid shadow maps. | ||
| // Mode 0: single-tap gather-based PCF with slope bias | ||
| // Mode 1: 8-tap spiral PCF with temporal rotation | ||
| // Mode 2: PCSS contact-hardened soft shadows | ||
| float SampleParaboloidShadow(uint shadowIndex, float2 sampleUV, float depth, float2x2 rotationMatrix) | ||
| { | ||
| uint mode = SharedData::lightLimitFixSettings.FilterMode; | ||
| float kernelRadius = PCFKernelShadowLight * SharedData::lightLimitFixSettings.KernelScale; | ||
|
|
||
| // Constant bias only — slope bias (ddx/ddy) is undefined inside a non-uniform loop | ||
| // where adjacent quad pixels may be at different iterations. | ||
| [branch] if (mode >= 1) | ||
| { | ||
| return PCFSpiral8(shadowIndex, sampleUV, depth - ShadowBiasConst, kernelRadius, rotationMatrix); | ||
| } | ||
| // Mode 0: single-tap with constant bias | ||
| return SampleShadowGather(shadowIndex, sampleUV, depth - ShadowBiasConst); |
There was a problem hiding this comment.
Mode 2 currently aliases mode 1 for paraboloid lights.
SampleParaboloidShadow() routes both FilterMode == 1 and FilterMode == 2 through PCFSpiral8(), even though this block documents mode 2 as PCSS. If that fallback is intentional, please make it explicit here/UI-side; otherwise mode 2 needs its own path for hemisphere/omni lights.
| if (shadowMapSlots == 0) | ||
| return; | ||
|
|
||
| auto* shadowSceneNode = globals::game::smState->shadowSceneNode[0]; | ||
| if (!shadowSceneNode) | ||
| return; | ||
|
|
||
| auto* sunShadowLight = shadowSceneNode->GetRuntimeData().sunShadowDirLight; | ||
| if (!sunShadowLight) | ||
| return; |
There was a problem hiding this comment.
Decouple directional-shadow upload from shadowMapSlots and clear stale bindings on skip paths.
shadowMapSlots is populated from RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS, but this pass uploads sun cascade data from RE::RENDER_TARGET_DEPTHSTENCIL::kSHADOWMAPS_ESRAM. Returning on Line 580 skips the directional upload for an unrelated resource state, and every return before Lines 605-613 leaves the previous frame’s t98/t99 bindings live. Gate on sunShadowLight / cascade SRV availability instead, and bind null to slots 98 and 99 when the data is unavailable.
As per coding guidelines, "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
Also applies to: 598-613
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Deferred.cpp` around lines 580 - 589, The early return guarded by
shadowMapSlots causes directional shadow cascade upload to be skipped and leaves
stale SRV bindings in t98/t99; instead, stop gating the sun cascade upload on
shadowMapSlots and gate on the presence of sunShadowLight and the cascade SRVs
(e.g., check globals::game::smState->shadowSceneNode,
shadowSceneNode->GetRuntimeData().sunShadowDirLight and the ESram cascade SRVs),
and on any path where the cascade data is unavailable explicitly bind null to
shader slots t98 and t99 to clear previous-frame bindings before returning;
update the logic around shadowMapSlots, shadowSceneNode, sunShadowLight and the
code paths referenced (lines handling t98/t99 binding at the end of the block)
to implement these checks and null-bind clears.
| /// Compile `input` into a temporary expression and return true if it succeeds. | ||
| /// On failure, `errorOut` receives the first parser error message. | ||
| /// Does NOT affect the active formula. | ||
| static bool Validate(const std::string& input, std::string& errorOut); | ||
|
|
||
| static void SetParam(int32_t index, double value); | ||
| static double GetParam(int32_t index); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "ShadowCasterManager.*" -type fRepository: doodlum/skyrim-community-shaders
Length of output: 179
🏁 Script executed:
cd src/Features/LightLimitFix && wc -l ShadowCasterManager.h ShadowCasterManager.cppRepository: doodlum/skyrim-community-shaders
Length of output: 148
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.h | head -100Repository: doodlum/skyrim-community-shaders
Length of output: 4295
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.cpp | head -150Repository: doodlum/skyrim-community-shaders
Length of output: 6571
🏁 Script executed:
rg -n "RedrawBudgetFormula|BudgetFormula" src/Features/LightLimitFix/ -A 3 -B 1Repository: doodlum/skyrim-community-shaders
Length of output: 2461
🏁 Script executed:
rg -n "SetParam|GetParam" src/Features/LightLimitFix/ | head -40Repository: doodlum/skyrim-community-shaders
Length of output: 5202
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.cpp | sed -n '150,350p'Repository: doodlum/skyrim-community-shaders
Length of output: 11220
🏁 Script executed:
rg -n "s_formulaRedrawBudget" src/Features/LightLimitFix/ShadowCasterManager.cpp | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 537
🏁 Script executed:
cat -n src/Features/LightLimitFix/ShadowCasterManager.cpp | sed -n '1200,1400p'Repository: doodlum/skyrim-community-shaders
Length of output: 8984
🏁 Script executed:
rg -n "Calculate\(\)" src/Features/LightLimitFix/ShadowCasterManager.cpp -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 782
Restrict formula symbol sets to enforce frame-scoped parameters in RedrawBudgetFormula.
The Validate(), SetParam(), and GetParam() static methods share a single backing parameter table with no per-formula-type restrictions. This allows users to enter RedrawBudgetFormula expressions using light-scoped symbols (lightindex, lightintensity, etc.), which contain stale values from the previous frame since the budget is evaluated before light parameters are updated. Implement symbol filtering in Validate() to restrict RedrawBudgetFormula to frame-scoped symbols (frametime, frametarget, stableframes, camera/environment params) and allow light-scoped symbols only in score/interval formulas, or split the parameter tables per formula type. This aligns with the coding guideline requirement for user input validation to prevent unexpected behavior from malformed configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/LightLimitFix/ShadowCasterManager.h` around lines 76 - 82, The
Validate/SetParam/GetParam trio currently shares a global parameter table
allowing light-scoped symbols in RedrawBudgetFormula; update Validate(const
std::string& input, std::string& errorOut) to enforce a symbol whitelist when
validating expressions for RedrawBudgetFormula (only allow frametime,
frametarget, stableframes, camera/environment params) and reject or rewrite any
light-scoped symbols (lightindex, lightintensity, etc.), or alternatively split
the backing parameter storage into separate tables per formula type and have
Validate check/associate the correct table; also ensure SetParam and GetParam
operate against the per-formula table (or respect the same symbol restrictions)
so runtime reads/writes cannot access stale light-scoped values for
RedrawBudgetFormula.
2 participants
Current state.
CS from this branch.
coc intellightestWorking generally.
Summary by CodeRabbit