feat(interior sun): force single shadow cascade option #1589
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note
|
| Cohort / File(s) | Summary |
|---|---|
Header: InteriorSun declarations src/Features/InteriorSun.h |
Adds ForceSingleShadowCascade to InteriorSun::Settings; introduces BSShadowDirectionalLight_SetFrameCamera hook struct with thunk declaration and relocation; adds savedActivePlanes storage. |
Feature implementation & UI src/Features/InteriorSun.cpp |
Implements BSShadowDirectionalLight_SetFrameCamera::thunk; patches SetFrameCamera via vtable hook; updates settings serialization and UI (checkbox + tooltip) and raises Interior Shadow Distance minimum to 3000.0f; adjusts room/portal inclusion logic and preserves/restores frustum culling around new logic. |
Deferred pipeline change src/Deferred.cpp |
Includes InteriorSun.h; in Deferred::CopyShadowData() maps per-shadow buffer, reads global gShadowDistance, and overrides EndSplitDistances/StartSplitDistances when interior-sun single-cascade mode is active, then unmaps buffer. |
Shadow culling fix src/EngineFixes/ShadowmapCascadeCullingFix.cpp |
Conditionalizes frustum overlap/far-face adjustments for interior-sun mode: disables frustum culling when interior-sun single-cascade is active; otherwise keeps prior overlap adjustment with per-corner logic. |
Sequence Diagram
sequenceDiagram
participant Light as BSShadowDirectionalLight
participant Hook as SetFrameCamera thunk
participant Deferred as Deferred::CopyShadowData
participant Buffer as perShadow Buffer
Note over Light,Hook: Runtime hook installed (vtable thunk)
Light->>Hook: SetFrameCamera(camera)
alt ForceSingleShadowCascade enabled & interior sun
Hook->>Light: override split distances (collapse cascades)
end
Hook-->>Light: return
Light->>Deferred: shadow pipeline continues
Deferred->>Buffer: dispatch compute shader (copy)
Deferred->>Buffer: map()
alt ForceSingleShadowCascade enabled & interior sun
Deferred->>Buffer: read gShadowDistance -> maxDistance
Deferred->>Buffer: write EndSplitDistances = (maxDistance, maxDistance, maxDistance, w)
Deferred->>Buffer: write StartSplitDistances = (0.0, maxDistance, maxDistance, w)
end
Deferred->>Buffer: unmap()
Deferred-->>Light: bind shadow resources and continue
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
- Review points:
- Correctness and safety of the BSShadowDirectionalLight::SetFrameCamera thunk (split calculations, frustum plane preservation/restoration).
- Buffer mapping/unmapping and relocation-based
gShadowDistancedereference in Deferred::CopyShadowData. - Interaction between the hook, deferred override, and ShadowmapCascadeCullingFix (ensure no conflicting/duplicated overrides).
- Serialization/UI change for
ForceSingleShadowCascadeand updated slider min value.
Possibly related PRs
- feat: add Interior Sun Shadows #1076 — touches interior-sun shadow handling and gShadowDistance usage; closely related to split-distance logic used here.
- fix: using unused cell flag to toggle ISS #1082 — modifies interior detection/IsInteriorWithSun logic that this change depends on for enabling single-cascade behavior.
- refactor(interior sun): rename interior sun shadows #1387 — refactors/exposes InteriorSun APIs and state that this PR extends with the ForceSingleShadowCascade setting.
Suggested reviewers
- alandtse
- doodlum
Poem
🐰 In dim-lit halls where sunbeams play,
I nudge the shadows one bright way.
One perfect cascade, crisp and clear,
A rabbit cheers — the dark draws near. ✨
Pre-merge checks and finishing touches
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately summarizes the main change: adding a ForceSingleShadowCascade option to the Interior Sun feature to enable forcing a single shadow cascade. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
Tip
📝 Customizable high-level summaries are now available in beta!
You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
- Provide your own instructions using the
high_level_summary_instructionssetting. - Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
- Use
high_level_summary_in_walkthroughto move the summary from the description to the walkthrough section.
Example instruction:
"Divide the high-level summary into five sections:
- 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
- 📓 References — List relevant issues, discussions, documentation, or related PRs.
- 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
- 📊 Contributor Summary — Include a Markdown table showing contributions:
| Contributor | Lines Added | Lines Removed | Files Changed |- ✔️ Additional Notes — Add any extra reviewer context.
Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.
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 @coderabbitai help to get the list of available commands and usage tips.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
If it's a fix, should it be optional? Just because mods work around the problem, if there is something broken, why allow it to exist? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Deferred.cpp(2 hunks)src/Features/InteriorSun.cpp(4 hunks)src/Features/InteriorSun.h(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/InteriorSun.hsrc/Deferred.cppsrc/Features/InteriorSun.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/InteriorSun.hsrc/Deferred.cppsrc/Features/InteriorSun.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
src/Features/InteriorSun.hsrc/Deferred.cpp
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Deferred.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Deferred.cpp
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
src/Deferred.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
src/Deferred.cpp
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
src/Deferred.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
src/Deferred.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
This is fair, I agree with that too. Just forcing max shadow res in interiors may be bad on perf for lower end hardware was the main reason not to force it. |
|
✅ A pre-release build is available for this PR: |
|
You said minimal performance impact so I assumed that was correct. Did you measure the change? |
|
has bugs will revisit soon |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
will get feedback from dlizzio & iso but this works well on my end. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/Features/InteriorSun.h (1)
100-102: Global savedActivePlanes storage may be fragile under re‑entrancyStoring culling state in the feature‑wide
savedActivePlanesworks ifBSShadowDirectionalLight_SetFrameCamera::thunkis strictly non‑reentrant and invoked for a single directional light on a single render thread, but:
- Any re‑entrant call to the thunk (or future multi‑light use) would overwrite
savedActivePlaneswhile an earlier call is still active, breaking restoration.- The vector’s type and
std::vectorusage require that all necessary headers are available through this header (either directly or viaShaderCache.h), otherwise consumers that includeInteriorSun.hin isolation could fail to compile.If there’s any chance of re‑entrancy or multiple lights, consider making the saved state local to the thunk (e.g., captured in a small RAII helper) or keyed per light instead of using a single shared member. Also verify that
<vector>and the declaration ofREX::EnumSetare available from headers included before this one.As per coding guidelines.
src/Features/InteriorSun.cpp (2)
20-27: UI for ForceSingleShadowCascade and updated shadow distance rangeThe new “Force Single Shadow Cascade” checkbox and tooltip clearly describe the trade‑offs, and raising the
Interior Shadow Distanceslider’s minimum to 3000f matches the comment about visual issues at lower values.One behavioral note: if users have existing configs with
InteriorShadowDistance < 3000, that value will still be used until the slider is touched. If you want to enforce the new minimum immediately on load, you could clampsettings.InteriorShadowDistanceafterLoadSettings.
193-210: Single‑cascade room/portal inclusion: behavior vs comments and perfThe new logic in
PopulateReplacementJobArraysdoes:if (settings.ForceSingleShadowCascade) { // Include EVERYTHING - no distance checks at all for maximum coverage addedSet.insert(object.get()); replacementJobArrays[count++ % jobArraySize].push_back(object); } else { if (IsInSunDirectionAndWithinShadowDistance(object, lightDir, playerPos)) { ... } }Observations:
- The comment above says “include ALL rooms/portals within shadow distance regardless of sun direction,” but the implementation actually includes all rooms/portals with no distance check. The inline comment below reflects that, so the two comments contradict each other.
- Including everything may be fine for typical interior sizes, but it removes the distance‑based coarse culling you previously had, which could be noticeable in very large or heavily connected interiors.
Consider either:
- Aligning the comments with the current “no distance checks” behavior, or
- Restoring a distance check (while dropping the direction check) to better match the “within shadow distance” intent, e.g. by factoring out a helper that only checks distance and using it in the single‑cascade branch.
This would keep intent clear and help manage worst‑case performance.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/EngineFixes/ShadowmapCascadeCullingFix.cpp(2 hunks)src/Features/InteriorSun.cpp(5 hunks)src/Features/InteriorSun.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/InteriorSun.cppsrc/EngineFixes/ShadowmapCascadeCullingFix.cppsrc/Features/InteriorSun.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/InteriorSun.cppsrc/EngineFixes/ShadowmapCascadeCullingFix.cppsrc/Features/InteriorSun.h
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
src/Features/InteriorSun.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
src/EngineFixes/ShadowmapCascadeCullingFix.cpp (2)
32-41: Cascade overlap adjustment remains correct and localizedThe refactored overlap fix still computes
splitOverlap = *gfSplitOverlap * 2.0fand pulls eachfarFacecorner back along the normalizedfar-neardirection. Doing this inside afor (i < 4)loop keeps behavior equivalent to the prior fix while making the intent clearer and tightly scoped to the four frustum corners.No functional issues spotted here.
16-26: File not found in repositoryThe file
src/EngineFixes/ShadowmapCascadeCullingFix.cppand the related code referenced in this review comment could not be located in the cloned repository. Extensive searches for:
- File paths and directories
- Code identifiers (
ForceSingleShadowCascade,activePlanes,BSShadowDirectionalLight_SetFrameCamera)- Related shadow/cascade/culling keywords
all returned no results. The repository contains only feature directories (Dynamic Cubemaps, Grass Collision, IBL, etc.) with no C++ source files.
Please verify:
- Whether this review comment applies to the correct repository/branch
- Whether the file is being added as new code in this PR
- If the file should exist elsewhere in the codebase
src/Features/InteriorSun.h (2)
31-36: Settings extension looks consistentAdding
ForceSingleShadowCascadewith a default oftruealongsideForceDoubleSidedRenderingandInteriorShadowDistanceis consistent with how the feature is exposed and serialized inInteriorSun.cpp. This keeps existing configs compatible while enabling the new behavior by default.
60-64: SetFrameCamera thunk declaration aligns with usageDeclaring
BSShadowDirectionalLight_SetFrameCamerawith athunkandREL::Relocationmatches the existing pattern for other hooks in this feature (e.g.,BSBatchRenderer_RenderPassImmediately). This provides a clear place to concentrate SetFrameCamera-related logic.src/Features/InteriorSun.cpp (3)
6-11: Settings serialization updated appropriatelyIncluding
ForceSingleShadowCascadein theNLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTmacro keeps JSON load/save in sync withInteriorSun::Settings. With the in‑struct default set totrue, older configs that lack this field will automatically get the intended default.
79-81: SetFrameCamera vfunc hook follows existing runtime patternsHooking
BSShadowDirectionalLight::SetFrameCameraviastl::write_vfunc<0x10, BSShadowDirectionalLight_SetFrameCamera>(RE::VTABLE_BSShadowDirectionalLight[0])is consistent with how other engine vfuncs are patched in this project and respects the runtime abstraction throughRE::VTABLE_*. Just ensure index0x10is correct for all supported runtimes (SE/AE/VR) and updated if upstream vtables change.As per coding guidelines.
237-242: SetShadowDistance wrapper remains a thin, compatible forwarder
InteriorSun::SetShadowDistanceis still just a relocated trampoline to the engine’s implementation. The change here doesn’t alter behavior and keeps SE/AE relocation viaREL::RelocationIDintact.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Dawntic
left a comment
Dawntic
left a comment
There was a problem hiding this comment.
BSShadowLight has a 'shadow map count' member, setting that to 1 when player is inside might get the game to do some of the math by itself, though I don't know if that will help the issue at all.
| } | ||
| } | ||
|
|
||
| if (cascadeCamera) { |
| frustum.fRight = centerX + newHalfWidth; | ||
| frustum.fTop = centerY + newHalfHeight; | ||
| frustum.fBottom = centerY - newHalfHeight; | ||
|
|
There was a problem hiding this comment.
Does the Z axis need to be scaled too?
|
|
||
| // Update the camera with modified frustum | ||
| RE::NiUpdateData updateData; | ||
| cascadeCamera->Update(updateData); |
There was a problem hiding this comment.
Should this scaling also be applied to culling camera frustums?
| } | ||
| } | ||
|
|
||
| // AFTER SetFrameCamera calculates splits, override them for interior sun if enabled |
There was a problem hiding this comment.
These values get updated at the beginning of the cascade loop so changes here will only propagate to subsequent reads e.g. shader buffer updates.
src/Deferred.cpp
Outdated
| auto* shadowData = static_cast<PerGeometry*>(mapped.pData); | ||
|
|
||
| // Get shadow distance from game global variable | ||
| static float* gShadowDistance = reinterpret_cast<float*>(REL::RelocationID(528314, 415263).address()); |
There was a problem hiding this comment.
I think shaders get passed distance in ndc units, might be worth double checking that the global here matches.
3 participants
Adds an optional "ForceSingleShadowCascade" button to Interior Sun Shadows that sets the first cascade for the shadow maps to be the max distance and disables the additional cascades, essentially making the resolution for the interior shadows maximum for the entire distance. This removes previous issues of bleed, light leaking, etc, and also increases the quality of the shadows dramatically with minimal performance degradation. Also makes VL actually visible where it should be, whereas previously the cascades hid this.
Removes fulstrum culling when ForceSingleShadowCascade=true which caused dramatic light leaks and missing lights in certain viewing angles.
Streamlines gInteriorShadowDistance and gShadowDistance usage to stop bad ini values breaking the effect. Also means the slider in the UI actually controls everything it is meant to.
Logic behind keeping as toggle is for backward compatibility, some mods may be designed around shadow cascades, or users can opt to disable it for performance.
Increase to minimum shadow distance interior to 3000 units, as 1000 was awful in any room bigger than a shop would cause dramatic visual artifacts, VL bleed/explosions, etc. Fixed description for this, the quality is NOT better with lower distance (due to cascade fix now).
Disabled/Enabled.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes