feat: unlock shadow limit#1941
Open
doodlum wants to merge 243 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR overhauls shadow handling: it moves shadow-data assembly from a GPU compute pass to CPU-populated structured buffers, adds a ShadowCasterManager for slot scheduling/budgeting, extends shader shadow-sampling APIs (PCF/PCSS), and wires directional/point shadow uploads and debug/visualization into the renderer and LightLimitFix feature. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App / Game
participant SCM as ShadowCasterManager
participant Deferred as Deferred Renderer
participant GPU as GPU Shaders (HLSL)
App->>SCM: ForEachShadowLight() / assign slots
activate SCM
SCM-->>App: shadowMapIndex per light
deactivate SCM
App->>Deferred: EarlyPrepass()
activate Deferred
Deferred->>Deferred: Build DirectionalShadowData (CPU)
Deferred->>GPU: Bind StructuredBuffer t98/t99 and ShadowMaps t101
deactivate Deferred
App->>GPU: Draw frame (Lighting)
activate GPU
GPU->>GPU: ShadowSampling::GetLightingShadow(DirectionalShadows[0], ...)
GPU->>GPU: GetShadowLightShadow(shadowMapIndex, ...)
GPU-->>App: Shaded pixels with PCF/PCSS and LLF debug data
deactivate GPU
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
cyberrumor
added a commit
to cyberrumor/based-lighting-configs
that referenced
this pull request
Mar 8, 2026
With this PR: - community-shaders/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
This comment was marked as resolved.
This comment was marked as resolved.
alandtse
force-pushed
the
shadow-limit-fix
branch
2 times, most recently
from
efc619d to
ed692a5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
alandtse
force-pushed
the
shadow-limit-fix
branch
from
ed692a5 to
e6ba2b7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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
alandtse
force-pushed
the
shadow-limit-fix
branch
from
b1089cd to
5f27c69
Compare
Contributor
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
alandtse
force-pushed
the
shadow-limit-fix
branch
from
5f27c69 to
d373e24
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/Features/VolumetricShadows.cpp (1)
276-279:⚠️ Potential issue | 🟠 MajorBind null when the current shadow SRV is missing.
If Line 83 returns
nullptr, this still rebinds the previousshadowCopySRV, so slot18samples stale shadow data from an earlier frame/scene instead of disabling the VSM path for this frame.Suggested fix
- ID3D11ShaderResourceView* srv = shadowCopySRV ? shadowCopySRV : shadowView; + ID3D11ShaderResourceView* srv = shadowView ? (shadowCopySRV ? shadowCopySRV : shadowView) : 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 logic uses ID3D11ShaderResourceView* srv = shadowCopySRV ? shadowCopySRV : shadowView; which can rebind a stale shadowCopySRV when both are null; change it so that srv is explicitly nullptr if neither shadowCopySRV nor shadowView is valid and then call context->PSSetShaderResources(18, 1, &srv) with that nullptr to unbind slot 18; update the code path around shadowCopySRV, shadowView and the PSSetShaderResources call to ensure slot 18 is explicitly cleared when no current SRV exists (use a local ID3D11ShaderResourceView* nullSrv = nullptr and pass its address).
🧹 Nitpick comments (3)
src/Features/LightLimitFix.h (1)
228-228: Partial array initialization.
uint clusterSize[3] = { 16 }initializes only the first element; the remaining two elements are zero-initialized. If the intent is{16, 16, 16}, make it explicit. If{16, 0, 0}is intended, a comment would clarify.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/LightLimitFix.h` at line 228, The array clusterSize defined as "uint clusterSize[3] = { 16 }" only initializes the first element and leaves the other two as zero; update the initializer to explicitly set all three entries (e.g., change the initializer to {16, 16, 16} if that's intended) or keep {16, 0, 0} but add a clarifying comment next to clusterSize to document the intended shape so future readers know whether all three dimensions should be 16 or zeros are deliberate.features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (1)
120-122: Fade curve difference between 2D and 3D paths.
GetVSMShadow3Dusespow(fade * fade, 8)(line 120), whileGetVSMShadow2Dusespow(fade, 8)(line 174). The 3D path effectively appliespow(fade, 16)— a much sharper falloff. If this is intentional for volumetric vs surface shadowing, consider adding a comment; otherwise align them.🤖 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 120 - 122, GetVSMShadow3D uses pow(fade * fade, 8) producing an effective pow(fade,16) which differs from GetVSMShadow2D's pow(fade,8); either align the 3D path by replacing pow(fade * fade, 8) with pow(fade, 8) (and update fadeFactor/surfaceShadow usage accordingly) or add a brief comment in VolumetricShadows.hlsli next to GetVSMShadow3D/fadeFactor explaining the intentional stronger falloff for volumetric shadows so reviewers know the discrepancy is deliberate.package/Shaders/Common/ShadowSampling.hlsli (1)
380-387: Magic sentinel values for slot state could use named constants.
ShadowParam.y == 0(unwritten),< 0(suppressed),> 0(valid) are checked inline. Consider extracting these as named constants for clarity, though the comments document the semantics adequately.🤖 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 380 - 387, Replace the inline magic checks against shadow.ShadowParam.y with well-named constants to clarify slot states: define symbolic names for the unwritten sentinel (== 0), the suppressed sentinel (< 0) and the valid-positive case (> 0) and use those constants in the comparisons where ShadowParam.y is tested; update the nearby comment to reference the constants (and their meanings) so future readers see intent instead of raw numbers. Ensure the constants are visible to the same shader scope (e.g., top of ShadowSampling.hlsli or shared header) and keep the comparison logic in the same function using the new names.
🤖 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 153-154: GetVSMShadow2D is incorrectly using
mul(transpose(shadowProj), float4(position,1)) while GetVSMShadow3D and other
shadow code (in ShadowSampling.hlsli) consistently use mul(shadowProj,
float4(...,1)) for the column_major ShadowProj matrix; update GetVSMShadow2D to
remove transpose and use mul(shadowProj, float4(position, 1)) (and the analogous
change at the other occurrence) so both 2D and 3D paths use the same matrix
multiplication convention with ShadowProj/primaryCascade.
In `@package/Shaders/Lighting.hlsl`:
- Around line 2433-2435: The SharedShadowMap register t80 declared in
Common/ShadowSampling.hlsli collides with TexLandDisplacement0Sampler (t80–t85)
used when LANDSCAPE and TRUE_PBR are defined; update the shader declarations so
they don't overlap: either wrap the SharedShadowMap (and any related Shared*
resources) register assignment in a conditional (`#if` !defined(LANDSCAPE) ||
!defined(TRUE_PBR)) or pick a nonconflicting register when both LANDSCAPE and
TRUE_PBR are defined (e.g., move SharedShadowMap to a free register and update
its declaration in Common/ShadowSampling.hlsli and any call sites like
ShadowSampling::GetLightingShadow), ensuring all references in Lighting.hlsl and
ShadowSampling.hlsli use the matching symbol names (SharedShadowMap,
ShadowSampling::GetLightingShadow) so permutations compile without register
collisions.
In `@src/Features/LightLimitFix.cpp`:
- Around line 206-208: LightLimitFix::LoadSettings currently assigns the
incoming json directly ("settings = o_json") which throws on malformed or
type-mismatched user config; change LoadSettings to validate and merge instead:
keep the existing defaults in the member "settings", check for presence and
correct types on keys in "o_json" (use contains and is_* checks), only overwrite
validated fields, and wrap the parse/merge in a try/catch to log errors and
leave defaults intact on failure so startup does not crash.
In `@src/Features/LightLimitFix.h`:
- Around line 134-135: previousEnableLightsVisualisation and
currentEnableLightsVisualisation are being initialized from
settings.EnableLightsVisualisation before Settings settings is declared, causing
a forward-reference to uninitialized data; fix by ensuring settings is
initialized before those members—either move the declaration of Settings
settings so it appears before
previousEnableLightsVisualisation/currentEnableLightsVisualisation, or stop
using member initializer expressions and instead set
previousEnableLightsVisualisation and currentEnableLightsVisualisation from
settings.EnableLightsVisualisation inside the class constructor (use Settings
settings as the source and assign to
previousEnableLightsVisualisation/currentEnableLightsVisualisation there).
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 40-51: ForEachShadowLight currently assumes accum is
null-terminated and can read out of bounds; update the loop to check the array
length via accum.size() (or its equivalent) before accessing accum[idx] and stop
when idx >= accum.size(); also handle the case where accum[idx] is nullptr by
breaking or continuing as intended, and ensure idx increments by
light->shadowMapCount only after confirming light is non-null—make these changes
in the ForEachShadowLight template to prevent OOB reads.
In `@src/Features/VolumetricShadows.cpp`:
- Around line 77-79: Update the two outdated comments that refer to slot 19 to
reflect the actual PS register bindings: change mentions of "t19" / "slot 19" to
"t98/t99" or "slots 98 and 99" as appropriate; specifically update the comment
near the top that references Deferred::CopyShadowData() and the later comment
referencing the shadow data structured buffer so they state that
Deferred::CopyShadowData() binds the directional shadow structured buffer to PS
slot 98 and the cascade texture to PS slot 99 (matching ShadowSampling.hlsli and
the DirectionalShadows/DirectionalShadowCascades declarations).
---
Duplicate comments:
In `@src/Features/VolumetricShadows.cpp`:
- Around line 276-279: The current binding logic uses ID3D11ShaderResourceView*
srv = shadowCopySRV ? shadowCopySRV : shadowView; which can rebind a stale
shadowCopySRV when both are null; change it so that srv is explicitly nullptr if
neither shadowCopySRV nor shadowView is valid and then call
context->PSSetShaderResources(18, 1, &srv) with that nullptr to unbind slot 18;
update the code path around shadowCopySRV, shadowView and the
PSSetShaderResources call to ensure slot 18 is explicitly cleared when no
current SRV exists (use a local ID3D11ShaderResourceView* nullSrv = nullptr and
pass its address).
---
Nitpick comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 120-122: GetVSMShadow3D uses pow(fade * fade, 8) producing an
effective pow(fade,16) which differs from GetVSMShadow2D's pow(fade,8); either
align the 3D path by replacing pow(fade * fade, 8) with pow(fade, 8) (and update
fadeFactor/surfaceShadow usage accordingly) or add a brief comment in
VolumetricShadows.hlsli next to GetVSMShadow3D/fadeFactor explaining the
intentional stronger falloff for volumetric shadows so reviewers know the
discrepancy is deliberate.
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 380-387: Replace the inline magic checks against
shadow.ShadowParam.y with well-named constants to clarify slot states: define
symbolic names for the unwritten sentinel (== 0), the suppressed sentinel (< 0)
and the valid-positive case (> 0) and use those constants in the comparisons
where ShadowParam.y is tested; update the nearby comment to reference the
constants (and their meanings) so future readers see intent instead of raw
numbers. Ensure the constants are visible to the same shader scope (e.g., top of
ShadowSampling.hlsli or shared header) and keep the comparison logic in the same
function using the new names.
In `@src/Features/LightLimitFix.h`:
- Line 228: The array clusterSize defined as "uint clusterSize[3] = { 16 }" only
initializes the first element and leaves the other two as zero; update the
initializer to explicitly set all three entries (e.g., change the initializer to
{16, 16, 16} if that's intended) or keep {16, 0, 0} but add a clarifying comment
next to clusterSize to document the intended shape so future readers know
whether all three dimensions should be 16 or zeros are deliberate.
🪄 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: 60805a5b-faf6-43f6-80a4-ce9ed0678bdd
📒 Files selected for processing (27)
.github/configs/shader-validation-vr.yamlCMakeLists.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 (6)
- src/State.cpp
- vcpkg.json
- extern/CommonLibSSE-NG
- src/XSEPlugin.cpp
- .github/configs/shader-validation-vr.yaml
- src/Menu/FeatureListRenderer.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- CMakeLists.txt
- src/Features/RenderDoc.cpp
- src/Globals.cpp
- src/Deferred.cpp
- src/Deferred.h
- src/Features/LightLimitFix/ShadowRenderer.cpp
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/Features/LightLimitFix/ShadowCasterManager.h (3)
41-50:⚠️ Potential issue | 🟡 MinorBound
ForEachShadowLight()byaccum.size().This helper only stops on a null sentinel. If
shadowLightsAccumis truncated or malformed,accum[idx]is read out of bounds before the break condition runs.🤖 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 41 - 50, ForEachShadowLight reads accum[idx] until a null sentinel and can index past the array if accum is malformed; change the loop in ForEachShadowLight to check idx against accum.size() (use size_t) before accessing accum[idx], e.g., loop while idx < accum.size() and inside break if accum[idx] is null, then call fn and advance idx by accum[idx]->shadowMapCount; ensure shadowMapCount is validated (>=1) before using it to avoid infinite loops or zero-advance.
145-157:⚠️ Potential issue | 🟠 MajorReserve the sun slot when sizing and clamping the pool.
ShadowLightCountexcludes the directional sun, butGetShadowSlot()reserves slot0for it whileLightContainer::Sizeis documented as onlyShadowLightCount + ConvertedShadowSlots. IfInit()allocates or clamps from that total directly, the pool is one slot short before the renderer cap is even considered. Verify that the effective size isShadowLightCount + ConvertedShadowSlots + 1and that malformed JSON is clamped before any allocation path. 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."#!/bin/bash set -euo pipefail hdr="$(fd '^ShadowCasterManager\.h$' src)" cpp="$(fd '^ShadowCasterManager\.cpp$' src)" printf 'Header contract\n' sed -n '145,157p' "$hdr" sed -n '288,289p' "$hdr" sed -n '393,408p' "$hdr" printf '\nInit / allocation paths\n' rg -n -C4 'Init\s*\(|Install\s*\(|ShadowLightCount|ConvertedShadowSlots|FindFreeIndex|GetShadowSlot|new\s+LightEntry|resize\(|\bSize\b' "$cpp"Also applies to: 288-289, 393-396, 405-408
🤖 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 145 - 157, The pool sizing and clamping logic must reserve the sun slot: ensure all allocation and clamp code (Init(), Install(), any place that builds or resizes LightContainer::Size, and code path that reads ShadowLightCount or ConvertedShadowSlots) computes effectiveSize = ShadowLightCount + ConvertedShadowSlots + 1 (the extra +1 reserves slot 0 used by GetShadowSlot()); validate and clamp malformed JSON values for ShadowLightCount/ConvertedShadowSlots before any allocation or new LightEntry/reserve/resize calls so you never allocate one slot too few, and update any places that compare against the renderer cap or call FindFreeIndex to use effectiveSize instead of just ShadowLightCount + ConvertedShadowSlots.
106-119:⚠️ Potential issue | 🟠 Major
RedrawBudgetFormulastill needs formula-scoped validation.The header documents
RedrawBudgetFormulaas frame-scoped, butValidate/SetParam/GetParamexpose one untyped symbol surface. Unless the implementation filters identifiers by formula kind, users can still referencelight*variables here and get stale per-light state. Verify that the parser rejects light-scoped symbols for the budget formula; if it does not, this is still a correctness bug. 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."#!/bin/bash set -euo pipefail hdr="$(fd '^ShadowCasterManager\.h$' src)" cpp="$(fd '^ShadowCasterManager\.cpp$' src)" printf 'Header API\n' sed -n '106,120p' "$hdr" printf '\nFormulaHelper implementation\n' rg -n -C4 'FormulaHelper::(Validate|SetParam|GetParam|Reparse)|register_symbol|symbol_table|kFormulaParam_' "$cpp" printf '\nFormula call sites\n' rg -n -C4 'RedrawBudgetFormula|ScoreFormula|RedrawIntervalFormula|Validate\s*\(|Reparse\s*\(' src/Features/LightLimitFixAlso applies to: 200-202
package/Shaders/Common/ShadowSampling.hlsli (2)
21-21:⚠️ Potential issue | 🔴 CriticalGuard or move
SharedShadowMap.This still binds to
t80, which overlaps the TRUE_PBR landscape SRV range whenLighting.hlslpulls this header in.🤖 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, The Texture2D declaration SharedShadowMap currently binds to register(t80) which conflicts with the TRUE_PBR landscape SRV range when included by Lighting.hlsl; fix by either moving the binding to a non-conflicting register or removing the hardcoded register and guarding the declaration with a macro so callers (e.g., Lighting.hlsl) can supply a safe register; update the SharedShadowMap declaration in ShadowSampling.hlsli to use a guarded/nullable binding (or a different unique t-slot) to avoid overlapping the TRUE_PBR SRV range.
45-46:⚠️ Potential issue | 🔴 CriticalMove the shadow-light SRVs out of
t100.The GRASS_COLLISION permutation includes both this header and
GrassCollision.hlsli, soShadows : register(t100)still collides with that SRV block.🤖 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 SRV bindings for Shadows and ShadowMaps collide with the GrassCollision SRV at t100; update the StructuredBuffer<ShadowData> Shadows and Texture2DArray<float> ShadowMaps register declarations to use unused registers (e.g., move from register(t100)/t101 to a non-conflicting pair like register(t110)/t111 or another free range) and ensure any shader references to Shadows and ShadowMaps are adjusted accordingly so the permutation no longer has SRV register collisions.
🤖 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-159: The fade window should be computed from the far-cascade
span instead of the full shadow range: compute cascadeSpan =
shadow.EndSplitDistances.y - shadow.StartSplitDistances.y, set FadeWindow =
cascadeSpan * 0.1 (or a named fraction), compute fadeStart =
shadow.EndSplitDistances.y - FadeWindow, and then compute fade =
saturate((shadowMapDepth - fadeStart) / FadeWindow); update uses of FadeWindow,
fadeStart and the final saturate call (referencing FadeWindow, fadeStart,
shadow.EndSplitDistances.y, shadow.StartSplitDistances.y, and shadowMapDepth)
accordingly.
---
Duplicate comments:
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Line 21: The Texture2D declaration SharedShadowMap currently binds to
register(t80) which conflicts with the TRUE_PBR landscape SRV range when
included by Lighting.hlsl; fix by either moving the binding to a non-conflicting
register or removing the hardcoded register and guarding the declaration with a
macro so callers (e.g., Lighting.hlsl) can supply a safe register; update the
SharedShadowMap declaration in ShadowSampling.hlsli to use a guarded/nullable
binding (or a different unique t-slot) to avoid overlapping the TRUE_PBR SRV
range.
- Around line 45-46: The SRV bindings for Shadows and ShadowMaps collide with
the GrassCollision SRV at t100; update the StructuredBuffer<ShadowData> Shadows
and Texture2DArray<float> ShadowMaps register declarations to use unused
registers (e.g., move from register(t100)/t101 to a non-conflicting pair like
register(t110)/t111 or another free range) and ensure any shader references to
Shadows and ShadowMaps are adjusted accordingly so the permutation no longer has
SRV register collisions.
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 41-50: ForEachShadowLight reads accum[idx] until a null sentinel
and can index past the array if accum is malformed; change the loop in
ForEachShadowLight to check idx against accum.size() (use size_t) before
accessing accum[idx], e.g., loop while idx < accum.size() and inside break if
accum[idx] is null, then call fn and advance idx by accum[idx]->shadowMapCount;
ensure shadowMapCount is validated (>=1) before using it to avoid infinite loops
or zero-advance.
- Around line 145-157: The pool sizing and clamping logic must reserve the sun
slot: ensure all allocation and clamp code (Init(), Install(), any place that
builds or resizes LightContainer::Size, and code path that reads
ShadowLightCount or ConvertedShadowSlots) computes effectiveSize =
ShadowLightCount + ConvertedShadowSlots + 1 (the extra +1 reserves slot 0 used
by GetShadowSlot()); validate and clamp malformed JSON values for
ShadowLightCount/ConvertedShadowSlots before any allocation or new
LightEntry/reserve/resize calls so you never allocate one slot too few, and
update any places that compare against the renderer cap or call FindFreeIndex to
use effectiveSize instead of just ShadowLightCount + ConvertedShadowSlots.
🪄 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: aed07b78-0e5c-46b3-9fa0-8ea8b5c1aa40
📒 Files selected for processing (4)
package/Shaders/Common/ShadowSampling.hlslipackage/Shaders/Lighting.hlslsrc/Deferred.cppsrc/Features/LightLimitFix/ShadowCasterManager.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Deferred.cpp
- package/Shaders/Lighting.hlsl
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli (1)
153-166:⚠️ Potential issue | 🟠 MajorDrop the secondary-cascade transpose.
The primary 2D path here and the other directional shadow paths all use
mul(shadowProj, ...). Transposing only the secondary cascade changes the matrix convention at the blend seam, so the secondary sample won't line up with the primary one.Suggested fix
- positionLS = mul(transpose(shadowProj), float4(position, 1)).xyz; + positionLS = mul(shadowProj, float4(position, 1)).xyz;Run this to compare the remaining
ShadowProjmultiplications. Expected result: this is the onlytranspose(shadowProj)left in the directional paths.#!/bin/bash rg -n -C2 'transpose\(shadowProj\)|mul\(shadowProj,\s*float4\(' \ "features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli" \ "package/Shaders/Common/ShadowSampling.hlsli"🤖 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 secondary-cascade path incorrectly applies transpose(shadowProj) when computing positionLS, causing a matrix-convention mismatch vs the primary path and misaligned samples; change the secondary cascade multiplication to use mul(shadowProj, float4(position, 1)) (same as the primary) so shadowProj, positionLS, SampleVSMCascade2D, primaryCascade and secondaryCascade use consistent matrix multiplication and remove the transpose call.package/Shaders/Common/ShadowSampling.hlsli (1)
49-50:⚠️ Potential issue | 🔴 Critical
Shadowsstill collides with grass collision att100.
RunGrass.hlslpulls in both this header andfeatures/Grass Collision/Shaders/GrassCollision/GrassCollision.hlslifor theGRASS_COLLISIONpermutation, and that header already bindsCollisionatt100. LeavingShadowsatt100makes that combined variant impossible to bind correctly. Move the LLF shadow SRV pair to a non-conflicting range and keep the runtime bindings aligned.Run this to confirm the binding collision and include path. Expected result: both headers declare
t100, andRunGrass.hlslincludes both.#!/bin/bash rg -n -C2 'register\(t100\)' \ "package/Shaders/Common/ShadowSampling.hlsli" \ "features/Grass Collision/Shaders/GrassCollision/GrassCollision.hlsli" rg -n -C3 'GrassCollision\.hlsli|ShadowSampling\.hlsli' \ "package/Shaders/RunGrass.hlsl"🤖 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 49 - 50, The ShadowSampling.hlsli file binds StructuredBuffer<ShadowData> Shadows and Texture2DArray<float> ShadowMaps to register(t100/t101) which collides with Collision in GrassCollision.hlsli; change the LLF shadow SRV pair to a non-conflicting register range (e.g., move Shadows and ShadowMaps from t100/t101 to an unused pair such as t110/t111) and update any runtime binding tables or descriptor indices that reference these symbols so the new registers are used at runtime; ensure RunGrass.hlsl variant that includes both ShadowSampling.hlsli and GrassCollision.hlsli compiles with the new non-conflicting registers.src/Features/LightLimitFix/ShadowCasterManager.h (1)
57-91:⚠️ Potential issue | 🟠 MajorBudget formulas need their own symbol whitelist.
RedrawBudgetFormulais documented as frame-scoped, butValidate()/SetParam()/GetParam()have no formula kind and expose the full parameter set. That lets light-scoped symbols validate here and then depend on whatever light values were last written into the shared table at budget-evaluation time. Please validate/bind budget formulas against a budget-only symbol set.Run this to confirm whether budget validation/evaluation is formula-type aware. Expected result: the current implementation uses the same
FormulaHelperentry points for all three formulas and does not pass a budget-specific symbol set.#!/bin/bash rg -n -C3 'FormulaHelper::Validate|FormulaHelper::SetParam|FormulaHelper::GetParam|RedrawBudgetFormula|ScoreFormula|RedrawIntervalFormula' \ "src/Features/LightLimitFix/ShadowCasterManager.cpp" \ "src/Features/LightLimitFix/ShadowCasterManager.h"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: 117-120, 201-203
🤖 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 57 - 91, The budget formulas (RedrawBudgetFormula/ScoreFormula/RedrawIntervalFormula) are being validated and bound against the full FormulaParams enum via FormulaHelper::Validate/SetParam/GetParam, allowing light-scoped symbols to leak into frame-scoped budget evaluation; add a budget-only symbol whitelist and make formula validation/evaluation formula-type aware by changing the formula API to accept a symbol set or formula kind. Update RedrawBudgetFormula (and ScoreFormula/RedrawIntervalFormula) to call FormulaHelper::Validate/SetParam/GetParam with a new budgetSymbols set (a reduced subset of FormulaParams containing only frame-scoped/budget-safe names), or overload the FormulaHelper methods to accept an enum FormulaKind and internally switch to the correct whitelist; ensure Validate() signature and any callers in ShadowCasterManager.cpp use the budget-specific set so light-scoped params cannot be validated for budget formulas.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@features/Volumetric`
Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlsli:
- Around line 153-166: The secondary-cascade path incorrectly applies
transpose(shadowProj) when computing positionLS, causing a matrix-convention
mismatch vs the primary path and misaligned samples; change the secondary
cascade multiplication to use mul(shadowProj, float4(position, 1)) (same as the
primary) so shadowProj, positionLS, SampleVSMCascade2D, primaryCascade and
secondaryCascade use consistent matrix multiplication and remove the transpose
call.
In `@package/Shaders/Common/ShadowSampling.hlsli`:
- Around line 49-50: The ShadowSampling.hlsli file binds
StructuredBuffer<ShadowData> Shadows and Texture2DArray<float> ShadowMaps to
register(t100/t101) which collides with Collision in GrassCollision.hlsli;
change the LLF shadow SRV pair to a non-conflicting register range (e.g., move
Shadows and ShadowMaps from t100/t101 to an unused pair such as t110/t111) and
update any runtime binding tables or descriptor indices that reference these
symbols so the new registers are used at runtime; ensure RunGrass.hlsl variant
that includes both ShadowSampling.hlsli and GrassCollision.hlsli compiles with
the new non-conflicting registers.
In `@src/Features/LightLimitFix/ShadowCasterManager.h`:
- Around line 57-91: The budget formulas
(RedrawBudgetFormula/ScoreFormula/RedrawIntervalFormula) are being validated and
bound against the full FormulaParams enum via
FormulaHelper::Validate/SetParam/GetParam, allowing light-scoped symbols to leak
into frame-scoped budget evaluation; add a budget-only symbol whitelist and make
formula validation/evaluation formula-type aware by changing the formula API to
accept a symbol set or formula kind. Update RedrawBudgetFormula (and
ScoreFormula/RedrawIntervalFormula) to call
FormulaHelper::Validate/SetParam/GetParam with a new budgetSymbols set (a
reduced subset of FormulaParams containing only frame-scoped/budget-safe names),
or overload the FormulaHelper methods to accept an enum FormulaKind and
internally switch to the correct whitelist; ensure Validate() signature and any
callers in ShadowCasterManager.cpp use the budget-specific set so light-scoped
params cannot be validated for budget formulas.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56ee20a2-87b1-4948-b085-4e715f0e4228
📒 Files selected for processing (5)
features/Volumetric Shadows/Shaders/VolumetricShadows/VolumetricShadows.hlslipackage/Shaders/Common/ShadowSampling.hlslisrc/Features/LightLimitFix.hsrc/Features/LightLimitFix/ShadowCasterManager.cppsrc/Features/LightLimitFix/ShadowCasterManager.h
alandtse
force-pushed
the
shadow-limit-fix
branch
2 times, most recently
from
e81b0e2 to
89a8dfa
Compare
soda3000
reviewed
Apr 5, 2026
Contributor
soda3000
left a comment
soda3000
left a comment
There was a problem hiding this comment.
The scope of this specific PR is huge, so I recommend trying to break it down into smaller self-contained PRs if possible.
alandtse
force-pushed
the
shadow-limit-fix
branch
from
be6fef2 to
7d2fdf6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
alandtse
force-pushed
the
shadow-limit-fix
branch
2 times, most recently
from
124c1ff to
0243c7c
Compare
slf0.tracy capture diagnosis (MaxRedrawPerFrame=3): user reported
visible shadow artifacts at the start of the capture when redraw
budget was very low. Root cause:
- Scheduler chose ~37 lights but rendered only 3 per frame.
- The 34 non-redrawn chosen lights flowed through the
GameSetShadowCasterSlot loop (line 2437) which inserts each
light into the engine's shadow caster array and sets
descriptor.shadowmapIndex = slot.
- The cluster shader then samples kSHADOWMAPS[slot] for those
lights.
- For a freshly-chosen light (LastDrawnFrame < 0), the depth
content at its slot has never been rendered for THAT light:
it is either cleared (init state) or, post-Option A,
the EVICTED previous occupant's shadow.
- Cluster reads stale depth -> projects a wrong shadow shape
through the new light = the artifact the user saw.
Fix: in the GameSetShadowCasterSlot loop, skip point-light slots
where LastDrawnFrame < 0. The light remains a chosen pool member
waiting for its first render turn, but we do not claim it as a
shadow caster this frame. Cluster lighting still illuminates it
via the normal-light path with no false shadow. Once it wins a
redraw turn (next frame or whenever budget permits),
LastDrawnFrame goes >= 0 and it joins the shadow set normally
with byte-correct depth content.
The check explicitly excludes:
- Sun slot (i == 0 with Sun=true): cascade pipeline, not
kSHADOWMAPS, never has stale-slot problem.
- Converted-slot range (i >= PointLightEnd): converted lights
don't sample kSHADOWMAPS via the slot path; they participate
through the s_normalConvert non-shadow pipeline.
Tracy diag: new counter scm.first_render_skips emitted as TracyPlot
so future captures can correlate fix activity with MaxRedrawPerFrame
settings. Expected behaviour:
MaxRedraw=3 -> high first_render_skips, especially during
camera motion that introduces new chosen lights.
MaxRedraw=16 -> first_render_skips near 0 in stable scenes
(budget catches up to new lights within a few
frames).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sons Step 1 of the SCM architectural cleanup. UpdateCamera() conflates three physically distinct fail conditions into one boolean: - Light outside camera frustum (off-screen) - Light past per-light shadow-LOD fade end distance - Engine-internal soft fails (rare) Today's c.invalidCamera collapses all three and routes them to the same ConvertLight action. The frustum case is wasted work (light contributes nothing visible either way; drop is correct). The LOD case is genuinely the convert case (light still illuminates via cluster, ConvertLight + lodDimmer reset is correct). This commit classifies but does not yet change behaviour. After the UpdateCamera() failure path we read the engine's side-band flags to recover the sub-reason: frustrumCull != 0 -> c.invalidFrustum lodDimmer == 0.0f -> c.invalidLod Both can be set simultaneously (a light off-screen AND distant); action selection in a future commit will treat frustum-out as terminal regardless of the LOD bit. Tracy diag: three new per-frame plots emitted alongside the existing breakdown so the next capture quantifies what fraction of the 71/frame invalidCamera load is each sub-reason: scm.candidates.invalid_frustum scm.candidates.invalid_lod scm.candidates.invalid_other Expected behaviour: invalid_frustum + invalid_lod >= invalid_camera (a light can carry both bits). Behaviour-impacting differentiation follows as the next commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 2 of the architectural cleanup, re-introduced WITHOUT step 3 (activeLights snapshot) to isolate any crash risk. The earlier combined commit (b80c6e099) crashed deterministically with rbx = 0x3B4711C7 across two runs; the snapshot iteration was the likely culprit, not the action-loop change. This commit lands only the action-loop change so any remaining crash bisects to one location. slf2.tracy diagnosis: 100% of invalidCamera fails in the user's scene are frustum-culls (71.19/frame avg = 238989 of 238989). LOD-faded is essentially 0 (0.03/frame). Step 2 captures the entire 0.8 ms/ frame ConvertLight cost; step 3 has nothing left to optimise once step 2 is in. Behaviour: when c.invalidFrustum is set, route to a `continue` -- no engine call. The engine's UpdateCamera does a sphere/cone-vs- frustum test on the light's bounding volume; failure means the entire illumination volume is provably outside the camera frustum, so no on-screen pixel can be lit by this light. ConvertLight would be wasted work (insert into activeLights for cluster lighting which will reject it anyway -- no cluster cell intersects the light's bounds when the bounds are outside the frustum). Portal-culled and LOD-faded paths unchanged: portal still DisableLight (cluster has no portal awareness), LOD still ConvertLight + lodDimmer reset (light is still visible, should illuminate as non-shadow). New counter scm.dropped_frustum tracks the drop; next capture should show converted.invalid plummet to ~0 (only LOD path remains, ~0.03/frame). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…render
Industry-standard "cached shadow maps" (UE5, CryEngine, Frostbite all
ship this): a shadow map only needs re-rendering when (a) the light
moved/rotated/resized or (b) at least one caster in its influence volume
moved or changed. Otherwise the previous shadow map is byte-identical
to what a fresh render would produce -- keep the slot and free the
budget for lights that actually changed.
Mechanism: at each scoring pass, compute a 64-bit hash of
- light pose (world.translate + world.rotate) and radius
- for each caster in BSLight::geomList: pointer identity (set
membership) and worldBound (engine-maintained, tracks rigid motion
AND BSDynamicTriShape vertex updates)
If the hash matches what was recorded at the previous successful redraw,
add 1e15 to the entry's RedrawScore -- it falls to the bottom of the
priority queue. Defensive bias rather than hard skip: if literally
nothing else needs work the entry still wins, which guards against
hashing bugs producing permanent staleness.
Hash is computed in HashCombine using boost-style golden-ratio mixing.
No cryptographic strength needed -- only that distinct shadow scenes
map to distinct hashes with very high probability. ~50us/frame for 100
candidates with ~30 casters each.
References: UE5 Cached Shadow Maps documentation; Bouchard, "Shadow
Mapping for Hi-Fi Games" (GDC 2017); Frostbite movable-light caching
in their lighting talks.
Subsumes the now-removed fade-delta flicker priority (uncommitted
da06b5713): NiLight::fade changes don't affect shadow content, so they
no longer trigger needless redraws. Position/radius animation
(kPulse etc.) is caught directly by the light pose component of the
hash.
LightEntry gains lastGeomHash (the committed cache key) and
pendingGeomHash (this frame's computed value, promoted to lastGeomHash
only when RedrawFrame=true is actually set). The promote-on-render
ordering makes the cache key always reflect what's in the slot, not
what we observed -- so a hash check on frame N+1 correctly compares
against the frame-N rendered state, even if N+1's scheduling decides
not to redraw.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Retry of the slf5 wrong-shadow fix from a different angle, on top of the rollback (38a246cd4). The original attempt (5da6245c4, bisect- confirmed root of crashes) tried to fix it at every eviction site by converting `e.Light = nullptr` to `e.Clear()`. Two of those sites previously had bare `continue` with no LightEntry write at all -- adding Clear() created new high-frequency slot-eviction events that exposed a downstream assumption and produced the recurring CTDs. This retry attacks the same bug at the OPPOSITE end of the lifecycle: the slot-acquire site. There is exactly one place in the code where a new light takes ownership of a free slot (s_lights.Lights[idx].Light = c.light, in AddNewlyChosen). Adding e.Clear() immediately before that assignment guarantees the new occupant starts with pristine metadata regardless of what state the previous owner left the slot in -- without touching any eviction site, so the high-churn behaviour that caused the CTDs is avoided. The sun slot already does this correctly (Clear() before assign at line 1995, unchanged in this commit). Expected effect: the slf5 wrong-shadow artifact at low MaxRedrawPerFrame is fixed -- new occupants of recently-vacated slots correctly trip the first_render_skips gate via LastDrawnFrame=-1, and the cluster pipeline drops them from the shadow set until they actually render. No CTDs because eviction-side behaviour is byte-identical to 401c4ee7f / 38a246cd4 (the bisect-confirmed good state). If this still CTDs, the bug is more subtle than slot-metadata inheritance and we'll need a fresh diagnosis. If it does NOT CTD and the wrong-shadow artifact is gone, this single-line fix replaces the entire 5da6245c4 -> 6d5166839 commit chain that was rolled back. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tighten verbose comments around the slot lifecycle hash, the cached-
shadow reuse path, and the c.invalid scheduling branch. Drop historical
commentary ("History note", "Pre-reclaim", "Previously this loop") in
favour of describing only the current behaviour and the regression case
each comment guards against.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Defends against a real hazard observed in third-party VR crash reports on the v1.5.1 release: during EnableLight or GameSetShadowCasterSlot, the engine can free the BSShadowLight via re-entrant scene mutation callbacks WITHOUT nulling our cached pointer. Inside the engine call, `*GetAccumLightSlot() += light->shadowMapCount` reads the freed shadowMapCount value (garbage from tbbmalloc-reused memory), corrupting the global accumLightSlot counter; a subsequent indexed array access at `[base + corrupted*8]` AVs. The existing post-call bare null check passes for the freed-but- non-null case. isUsableLight rejects it via two cheap defensive checks: - isAliveNow: the light is still in activeShadowLights - isVtableValid: the first 8 bytes of the object are non-zero Neither check is bulletproof (a tbbmalloc-reused block can survive both), but together they reject the common case the bare null check misses. Applied at both engine-call sites: - post-EnableLight (chosen+RedrawFrame branch) - post-GameSetShadowCasterSlot (cached-shadow re-insertion loop) Pure defensive addition; no behavioural change for the happy path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Below 4 the per-slot redraw rotation outpaces TAA's temporal blend. The shader's worldPositionWS reconstruction (input.WorldPosition.xyz + CameraPosAdjust) carries per-frame camera-relative jitter that drifts the cluster-shadow sample UV by ~0.18 game units between consecutive frames. When the resulting UV crosses an occluder silhouette in the slot's cached depth content, the verdict flips between "lit" and "shadowed" — a 9x brightness flicker on the nearest light's contribution in a static scene. Repro: brazier scene with ~6 torches at MaxRedrawPerFrame=1, ShadowLightCount=127. The dominant Torch -32Y at ~45 units from the shaded pixel produced (0.0, 0.0, 0.0) "shadowed" or (2.12, 1.22, 0.50) "lit" depending on which side of the boundary the per-frame jitter landed; other torches at ~219 units contributed an order of magnitude less either way. Verified via RenderDoc DebugPixel trace on two consecutive captures (2013 states each, identical PSO and SRVs, same cluster light list, identical shadow atlas content per slot MD5, zero projection-vs-atlas drift per SCM-TL diagnostic logging). 4 is the smallest value that keeps each slot refreshing fast enough for TAA to absorb the jitter. Higher MaxRedrawPerFrame values (default 16) were never affected and remain unchanged. The ImGui slider's lower bound moves to kMinMaxRedrawPerFrame; saved configs from earlier versions where 1..3 were valid choices are silently raised on load. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This reverts commit cb78895.
Replace the two-system scheduler (per-frame importance ranking +
cross-frame slot retention) with a single score-driven model that
matches modern shadow-atlas architectures (Frostbite, Northlight,
UE5 VSM): one score determines who gets atlas slots, the slot pool
is a dumb container that follows the chosen set.
Expose lightframessincerender to exprtk: the slot's age since its
last actual render, sentinel 1e6 when unassigned or never rendered.
Default ScoreFormula replaces (1 + lightchosenlastframe * 0.3) with
the continuous decay (1 + max(0, 1 - lightframessincerender / 8) * 0.4):
peak bonus 0.4 at age=0, linear decay to zero stickiness over 8
frames. lightchosenlastframe remains available for custom formulas.
Delete:
- c.invalid slot-retention block (~30 lines)
- c.excess slot-retention block (~30 lines)
- LRU eviction Phase 1/2/3 (~65 lines), reverted to the simple
"drop entries no longer chosen" loop that existed pre-2d8c62d47
- retained_invalid / retained_excess / lru_evictions counters and
their TracyPlot emissions
The deleted retention machinery was added by 2d8c62d to suppress
rank-drift churn on flickering torches. The same suppression now
lives in the score formula's recency term -- a continuous decay
beats the previous boolean step because it preserves rank-drift
stickiness while letting genuinely-demoted lights (slider lowered,
camera moved away, light truly stopped contributing) decay out
of chosen status and reach the c.excess Convert path the same
frame instead of hoarding their slot indefinitely.
Add convertOrDisable lambda inside ScheduleShadowCasters: a single
decision point for "this light won't shadow this frame -- Convert
to keep diffuse via cluster pipeline, or Disable to drop it
entirely?". Both c.invalid and c.excess call it. Spots always
Disable (engine has no NiSpotLight equivalent; converting bleeds
light through walls behind the cone). Omnis/hemis Convert when
ConvertExcessToNormal is on or pin-convert is set. The allowConvert
veto handles invalidPortal -- cluster lighting has no portal-graph
awareness, so portal-occluded omnis must Disable rather than Convert.
c.invalid also tightens the top-of-branch usability gate from
isAliveNow to isUsableLight (membership + vtable) to match c.excess.
Both branches fan into virtually-dispatched ReturnShadowmaps via
either ConvertLight or DisableLight, so a freed-but-canonical
pointer must be skipped for either path -- the previous asymmetry
left a small unsafe-call window in c.invalid's Disable branch.
User-visible behaviour change: lowering ShadowLightCount mid-session
now immediately converts the demoted lights to non-shadow cluster
lights, rather than retaining their atlas slots until eviction.
Distant lights that drop in importance similarly convert on the
same frame their score falls past the chosen threshold.
Net diff: -150 lines of slot-cache policy, +20 lines of helper +
formula variable plumbing. The chosen/excess/invalid classification
becomes the single policy authority; the slot pool is reduced to a
mechanical container with no policy of its own.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: distant omni shadow lights appear disabled and never convert. Cause: the c.invalidFrustum fast-path (b98f6fb) routed `frustrumCull != 0` candidates to a bare `continue` with no engine call, on the theory that "bounding-sphere outside frustum = no visible pixel can be lit, so converting is wasted work." Ghidra-verified UpdateCamera semantics (BSShadowParabolicLight_UpdateCamera in 1.6.640 / 1.6.1170 / VR) show that assumption is wrong: frustrumCull = 0xff is set by EITHER of two conditions, conflated into a single flag: (1) BSMultiBoundSphere::WithinFrustum (BSMultiBoundShape vfunc 0x29 -- Ghidra's prior "Func41" label was the decimal-formatted index, per CommonLibSSE-NG). Sphere(niLight.pos, Radius.x) vs camera frustum. Geometric cull; radius matches what the cluster builder reads, so failure here means no visible pixel can be lit. (2) Shadow-distance LOD: ((camDist^2 - radius^2) * camera.LodAdjust) > ShadowDistanceSquared_Current, gated on the BSShadowLight lodFade flag. ShadowDistanceSquared_Current is fShadowDistance^2 (8000^2 outdoors, 3000^2 indoors by default). This is NOT a visibility test -- it's "skip per-light shadow rendering at this distance". A light past shadow distance can still be in the camera frustum and illuminating visible pixels via cluster lighting. Because the engine collapsed both into the same field, our drop fast-path silently removed distant-but-visible lights from every cluster lighting source: - Not in shadowLightsAccum (engine didn't render them this frame) - Not in activeLights (still typed as BSShadowLight, lives in activeShadowLights) - Not in s_normalConvert (we didn't call ConvertLight) User-visible symptom: distant torches/sconces vanish entirely instead of contributing diffuse light. Fix: drop the c.invalidFrustum fast-path. Let frustum-out candidates flow through convertOrDisable like LOD-faded ones. Omnis convert, spots disable (still no NiSpotLight equivalent), portal-occluded disable (allowConvert=false in the helper). The cluster builder's own `(color * fade) > 1e-4 && radius > 1e-4` filter is the authoritative "does this light contribute" gate -- measured against the actual cluster lighting math, not against an engine-internal cull bound that may not match. Genuinely-out-of-range lights are still filtered, just on the right metric. Perf: the ~0.5 ms/frame ConvertLight cost the optimization originally saved comes back on heavy outdoor scenes (~70 calls/frame in earlier captures). The cost of visually-correct distant lighting. Drop the scm.dropped_frustum Tracy counter since the path no longer exists. invalidFrustum is still tagged in the validation pass for the candidate-breakdown counter so per-frame triage data is preserved. Companion Ghidra renames applied to the three open binaries: fShadowDistance, fShadowDistance_Interior ShadowDistance_Current, ShadowDistanceSquared_Current BSShadowParabolicLight_UpdateCamera (vfunc 16) BSMultiBoundSphere_WithinFrustum (vfunc 0x29) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User-reported regression on shadow-limit-fix: complex grass outside goes completely black, disappears when SLF is disabled. Cause: a slot in Shadows[] whose owning light has empty shadowmapDescriptors (cleared by Hook_DisableColorMask -> ReturnShadowmaps between ScheduleShadowCasters and the next CopyShadowLightData) keeps ShadowProj at its default zero matrix but still receives ShadowParam.y = range (non-zero) in CopyShadowLightData. The grass per-light shadow loop (RunGrass.hlsl) -- and Lighting.hlsl's equivalent -- branch on ShadowParam.y semantics: > 0 -> valid radius; sample kSHADOWMAPS via ShadowProj at slot. == 0 -> safe sentinel; shader returns 1.0 (fully lit, no shadow). < 0 -> suppression sentinel; shader returns 0.0 (fully dark). With ShadowProj = zero matrix but ShadowParam.y > 0, the shader does `mul(zero, worldPos) = 0`, depth=0, the depth comparison says fully shadowed, and any shadow-flagged light touching a grass pixel zeroes the diffuse contribution. Multiple overlapping shadow lights -> grass pitch black. Upstream/dev doesn't hit this because it samples the engine's pre-rendered 4-channel screen-space mask (`shadowColor[light.shadowLightIndex]`), which the engine always populates. Our branch switched grass to LightLimitFix::GetShadowLightShadow (kSHADOWMAPS array via per-light projection) to support >4 shadow lights -- that path requires ShadowProj to be valid OR ShadowParam.y to be the 0 sentinel. Fix: have SetShadowParameters return bool, and gate the ShadowParam.y assignment on success. When the projection wasn't written (descriptors empty), force ShadowParam.y = 0 so the shader's safe sentinel fires and the light contributes no shadow rather than contributing full shadow. Suppression keeps its -1 sentinel. Symmetric correctness: ShadowProj=0 must never pair with ShadowParam.y>0 in the SRV. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…odDimmer Two correctness gaps surfaced by the promotion-path audit: 1. Hook install was boot-time gated on ConvertExcessToNormal || PromoteNormalToShadow. If the user had both flags false at boot and toggled ConvertExcessToNormal on at runtime, Hook_IsShadowLight (vtable slot 3), Hook_ConvertLights_Remove, and Hook_ConvertLights_SetLight were never installed -- so entries added to s_normalConvert at runtime were invisible to the engine (IsShadowLight kept returning true), and cell-unload reconciliation through Hook_ConvertLights_Remove was bypassed. Conversion silently did nothing. Install all four conversion-related hooks unconditionally when SCM is enabled. Their behaviour is already gated by s_settings.* checks and by container membership (empty containers -> hooks are no-ops), so installing them with conversion settings off costs only the vtable dispatch indirection. 2. The lodDimmer=1 override for converted lights at LightLimitFix.cpp's ForEachConvertedLight injection point fired unconditionally every frame. UpdateCamera's shadow-distance LOD cull (the second condition that conflates into frustrumCull -- see ShadowCasterManager.cpp's Ghidra-verified comment) hard-zeroes lodDimmer; the override was needed for cluster contribution. But if the engine ever sets a smooth fade value between 0 and 1, we previously snapped it to full intensity, producing distant always-full-bright converted lights that ignored the engine's intended gradual fade. Restore lodDimmer only when fully zeroed (`if (lodDimmer == 0.0f)`). Smooth fades are preserved. Functionally equivalent to the prior code when the engine only writes 0 or 1; safer when it writes intermediate values. The c.invalid first-frame restore in ShadowCasterManager.cpp gets the same conditional for symmetry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per-frame count log was firing on any plCount or slot-usage delta of
1, producing ~10 lines/sec in busy outdoor scenes (~25k lines in a
39-minute session log -- dwarfed every other signal).
Two filters now:
- Significance: only emit when plCount moves by >= 4 from the last
logged value, OR when unshadowedLights changes (rarer, more
interesting since it indicates slot pressure).
- Rate: floor at 1 line/sec via std::chrono::steady_clock.
Both gates must pass. First call always logs (sentinel -1 baseline)
so the user sees an early baseline value.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Enabled flag is now a boot-time gate only -- toggling at runtime
stages a setting for the next launch but has no immediate effect, the
same way ShadowLightCount and the atlas texture size are handled. The
existing `if (!settings.Enabled) return;` at the top of Install()
(before all 24 engine-modification call sites and before
RegisterSceneTransitionEvents) is the single gate; when disabled at
boot, the exe is left completely vanilla -- no REL::safe_write,
install_context_hook, write_vfunc, or detour_thunk fires.
Why restart-only:
Three consecutive crash logs (2026-05-17 20:01:12, 20:31:12, plus a
third with the soft-disable in place) all bottomed out at the same
engine AV. Ghidra-verified frame chain for 20:31:12:
NiCamera::CalculateAndDrawShadowCasterLights
-> CalculateActiveShadowCasterLights (vanilla scheduler --
what disable would
route to)
-> BSShadowDirectionalLight::sub_SE100818_AE107602 (sun shadow)
-> FUN_1414bf320 (BSCullingProcess inner)
-> BSCullingProcess::sub (stores r14 = vfunc bool = 1)
-> FUN_1414f50d0
-> BSBatchRenderer::sub_SE100843_AE107633 (AV at +0x54,
`mov rax, [r14+0x48]`)
The AV is in the engine's vanilla scheduler itself. SCM's boot-time
patches (kSHADOWMAPS sized to ShadowLightCount via REL::safe_write,
Hook_CreateNormalDepthBuffer / Hook_CreateReadOnlyDepthBuffer
redirecting depth-buffer storage, Hook_DisableColorMask replacing the
engine's DrawColorMask CALL with our per-light ReturnShadowmaps
sweep) leave the engine state incompatible with vanilla traversal.
We tried three intermediate strategies before concluding this:
1. Immediate ResetSession from Update() on the toggle's
Enabled→false edge. Crashed mid-frame -- Update() runs from
Prepass() which is render-thread but not a quiescent point for
the engine's own shadow bookkeeping.
2. Defer ResetSession to a pending flag consumed at the top of the
next CalculateActiveShadowCasterLights call (the "between
frames" point). Crashed again -- the engine's
activeShadowLights still contained ~50 lights we'd been
managing, with shadowmapDescriptors already zeroed by
Hook_DisableColorMask for the current frame; flipping
Hook_IsShadowLight's verdict back to "yes shadow" mid-walk left
the engine dispatching vfuncs on half-state objects.
3. Soft-disable: flag flip only, no container drain, let cell
change drain via SceneTransitionEventHandler. Still crashed --
the vanilla scheduler itself trips on our atlas/depth/color-mask
patches, regardless of how we manage our internal containers.
The deep engine hooking is not reversible at runtime; restart is the
only safe path. Tooltip and the restart-required label reflect this
in both toggle directions.
Removes the runtime gating from this saga:
- Per-frame Hook_CalculateActiveShadowCasters::thunk's
`if (!Enabled) func()` route.
- Hook_OverwriteShadowMapIndex's `if (!Enabled) return;` early-out.
- Update()'s per-flag container clears for ConvertExcessToNormal
and PromoteNormalToShadow true->false transitions.
- s_pendingDisableReset atomic flag and its consumer.
ResetSession remains, called only from SceneTransitionEventHandler
on LoadingMenu (the engine's natural between-cell quiescent point).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The label compared `settings.Enabled != s_settings.Enabled` (staged vs applied). The moment the user clicked Save Settings, Update() did `s_settings = capped` and both became equal, hiding the label even though the change wouldn't take effect until the next launch. Capture the boot-time Enabled value once in Install() into a dedicated `s_bootEnabled`. Compare against that instead -- the boot value is immutable for the session, so the label persists from the moment the user toggles until they actually restart. Label position is unchanged (before the `if (!settings.Enabled) ImGui::BeginDisabled()` block) so ImGui doesn't grey it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # src/Features/LightLimitFix.h
alandtse
force-pushed
the
shadow-limit-fix
branch
from
e7c5731 to
bac0948
Compare
Refine three comments in ShadowCasterManager.cpp to reflect Ghidra- verified engine behavior: - Hook_OverwriteShadowMapIndex only fires for point/spot lights; sun cascades enter RenderCascade with renderTarget already set to 2/3/4 by BSShadowDirectionalLight::RenderShadowmaps, bypassing the kNONE block our hook lives inside. - LightContainer::FindFreeIndex notes that sunOff=1 is a bookkeeping reservation, not a kSHADOWMAPS slot consumer (sun writes target 2 kSHADOWMAPS_ESRAM, not target 4). Calls out the FindLight/ FindFreeIndex range-match invariant as the regression risk. - MaxShadowAccumIterationBound names iNumSplits:Display as the source of the cascade count (INI-capped at 3 by ShadowmapRasterizerFix); keeps 4 as a defensive upper bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Net -109 lines (199 removed, 90 added). Removed text falls into three buckets: - Restated-the-code: assignments / clears / smoothing whose intent is obvious from the line being commented. - Pre-fix history: notes about how the prior split-phase scheduler / earlier boot-time gating / the now-removed C++ DRS controller used to behave, only relevant to readers who didn't have the current code. - Tutorial prose: multi-paragraph explanations of EWMA smoothing, per-importance-value tables, descriptor-creation step traces, etc. Kept the why-not-what content (regression invariants, ordering rules, ESRAM-bank-vs-shadow-map distinction, vtable-hook bypass rationale). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Conflicts resolved (HEAD wins each time, reasoning below): - LightLimitFix.cpp #include "Utils/ExternalEmittance.h": already present in HEAD at a different position; dropped the duplicate insertion from origin. - ShadowCasterManager.h ForEachShadowLight bound: HEAD uses MaxShadowAccumIterationBound() (setting-derived); origin reverted to accum.capacity(). The engine writes via SetShadowCasterLightArrayEntry which bypasses BSTArray::push_back, so _capacity stays at the initial preallocation and using it as the bound silently caps SLF at vanilla shadow counts. Kept HEAD. - ShadowCasterManager.cpp sun-render block: HEAD has the sun Render() call wrapped in Tracy zones; origin duplicated the point-light loop inside the sun if-branch (would only run when Sun=true, and the point loop already exists below). Kept HEAD's structure -- preserves the sun fix from 597eb1c and avoids the duplicate loop. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
input.WorldPosition in Effect.hlsl is camera-relative (the existing fog and view-space transforms confirm this -- fog explicitly takes CameraPosAdjust as a separate argument). GetShadowLightShadow's ShadowProj is a world-space projection matrix, so passing camera- relative coords yields shadow lookups offset by -CameraPosAdjust per eye. Visible in VR (different offset per eye) and third-person. Lighting.hlsl and RunGrass.hlsl already compute the world-space position explicitly before calling GetShadowLightShadow; Effect.hlsl now matches that pattern. Caught by Copilot PR review on #35. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per repo standard (every D3D11 resource must be named via SetResourceName), pass "LLF::ShadowLights" to the Buffer constructor when creating the per-frame shadow-light SBV.
Two issues caught by CodeRabbit on PR #35: - Deferred::CopyShadowLightData was gated on ShadowCasterManager::GetInstalledSlotCount() != 0, which is the point/spot kSHADOWMAPS slice count. The sun lives in a separate target (kSHADOWMAPS_ESRAM), so the guard was incorrectly skipping sun directional shadow upload (t98/t99) whenever the point-shadow pool was empty. Remove the guard. - LightLimitFix::Settings was missing EnableLightsVisualisation and LightsVisualisationMode from the NLOHMANN serialization macro, so those debug-overlay toggles reset on every restart. Add them to the macro. The LLFDEBUG permutation toggle at startup is a deeper plumbing change deferred for a follow-up. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Debug viz toggles (EnableLightsVisualisation, LightsVisualisationMode) are runtime-only — they were intentionally not in the NLOHMANN macro. Also drop the Deferred.cpp commentary about why CopyShadowLightData has no early-return; that comment was describing absent code.
Phase 1 of restoring focus shadow rendering. The previous behavior applied three byte patches to disable the engine's focus shadow path entirely (rationale in the original Intellightent code: "we don't use them anyway and they would overwrite the extra depth buffers"). The "we don't use them" part is a consequence of the LLF shader path sampling only cascades; the "overwrite" part is the real slot conflict at kSHADOWMAPS[4..7]. Replace the patches with per-frame dynamic slot reservation: - Read FocusShadowActors.size (RID 527703) at the top of ScheduleShadowCasters. Renamed the relocation accessor from GetSunInt1 to GetFocusShadowActorCount with proper documentation (Ghidra label was already FocusShadowActors.size). - Track s_focusShadowSlots (0..4). When non-zero, slots [kFocusShadowBaseSlotIndex .. +s_focusShadowSlots) are excluded from the point-light pool. Engine writes focus shadows there as designed. - Eject any point light occupying a freshly-claimed focus slot at scheduler entry; the candidate loop reassigns it to a free slot or routes it through the existing excess (convert/disable) path. - Effective scheduler budget shrinks by s_focusShadowSlots so the lowest-rank candidates that no longer fit become excess naturally. - FindFreeIndex treats the full focus-reservable range [4..8) as last-resort (two-pass allocation), so an actor appearance rarely needs to evict a previously-chosen point light. Cost: 0..4 point shadow slots, only when focus actors are active (player in sun-lit area, dialog targets, combat). Zero cost in unlit interiors / caves where the engine never claims focus slots. Caveat: the LLF shader path (GetDirectionalShadow) still samples only kSHADOWMAPS_ESRAM cascades, so the focus-shadow visual contribution requires Phase 2 (focus sampling in LightLimitFix.hlsli, mirroring Utility.hlsl's TexFocusShadowMap path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two refinements to the focus-shadow slot management:
1. Backwards-fill fallback. FindFreeIndex's first pass already
skips the entire focus-reservable range [4..8). The fallback
pass now iterates 7→6→5→4 so the slot LEAST likely to be
claimed next as focus actor count grows fills first. Player
in slot 4 is always claimed when focus count >= 1, so a point
light at slot 4 is the most likely future eviction; slot 7
only claims when all four actors are tracked (rare).
2. Focus rows in the shadow caster table. Adds up to four
read-only "Focus" rows when s_focusShadowSlots > 0. Each
shows:
- Synthetic key for table uniqueness (no Light* pointer)
- "eng" indicator in Mode column (cannot pin/suppress)
- Type column tinted blue with "Focus" label + tooltip
- Address shows "focus[N]" instead of a raw pointer
Lets users see why slots 4..4+N appear missing from the
point-light pool and which slots specifically the engine
has claimed this frame.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2 of focus shadow restoration. After Phase 1 stopped trampling kSHADOWMAPS slots 4-7, the engine renders focus actor shadows there again, but LightLimitFix::GetDirectionalShadow was sampling only the cascade depth and ignoring the focus content -- net visual still "player shadow missing in sun-lit scenes". Plumbing: - Deferred::DirectionalShadowLightData gains FocusShadowProj[4] + FocusShadowCount (struct now 544 bytes, still 16-aligned). - Deferred::SetShadowCascadeParameters populates the focus matrices from sun->focusShadowmapDescriptors[i].lightTransform when isEnabled, mirroring the cascade matrix path. - ShadowSampling.hlsli's DirectionalShadowLightData struct extended to match (column_major float4x4 FocusShadowProj[4] + uint count). - LightLimitFix.hlsli's GetDirectionalShadow loops up to FocusShadowCount focus matrices, projects worldPositionWS through each, gates on UV/depth in [0,1] (pixels outside the actor's projection aren't covered by that focus shadow), PCF-samples ShadowMaps slice (4 + i), and folds the result into the cascade via min() so any occluding focus actor wins. The t101 ShadowMaps binding (kSHADOWMAPS, 8 slices) already covered slices 4-7; no new bindings needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release comment
Current state.
CS from this branch.
coc intellightest(note this test esp will break blackreach so not for playthrough). Grab from release assets.Working generally.
Other ways to generate shadows:
Defaults to 16 shadow lights and uses an auto budget system to try to maintain your target fps (but always allows 4 shadow casters to be drawn). If shadows aren't updating every frame, you may be exhausting the budget.
Summary by CodeRabbit
New Features
Other