refactor(interior sun): rename interior sun shadows#1387
Conversation
WalkthroughRenames InteriorSunShadows to InteriorSun across codebase, updates includes/usages, and adds an interior-specific shadow distance setting with setter and UI handling. Adjusts consumers (VolumetricLighting, Hooks, TruePBR) to new API/flags. Updates globals/feature registry accordingly. In VR path, bumps minimum Address Library version to 0.189.0. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Settings UI
participant IS as InteriorSun
participant Game as Game Engine (relocation)
User->>UI: Adjust Interior Shadow Distance
UI->>IS: DrawSettings() update value
IS->>IS: SetShadowDistance(inInterior)
IS->>Game: Apply shadow distance (relocation)
Game-->>IS: Distance applied
sequenceDiagram
participant Engine as Engine
participant IS as InteriorSun
participant VL as VolumetricLighting
Engine->>IS: GetWorldSpace::thunk()
IS-->>Engine: enableInteriorSun / disableInteriorSun
VL->>IS: IsInteriorWithSun(cell)
IS-->>VL: bool
VL->>VL: inInteriorWithSun = result
VL-->>Engine: Use interior lighting path if enabled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
alandtse
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/Features/VolumetricLighting.cpp (1)
218-225: Bug: inInteriorWithSun not updated on interior-cell changesEarlyPrepass returns early when currentlyInInterior == inInterior, so moving between two interior cells (one with sun, one without) won’t update inInteriorWithSun or re-run SetupVL. Track the interior cell pointer and update when it changes.
Apply this minimal diff to update when the interior cell changes:
- if (initialised && currentlyInInterior == inInterior) - return; + static RE::TESObjectCELL* lastInteriorCell = nullptr; + const bool interiorCellChanged = (interiorCell != lastInteriorCell); + if (initialised && currentlyInInterior == inInterior && !interiorCellChanged) + return; initialised = true; inInterior = currentlyInInterior; - inInteriorWithSun = InteriorSun::IsInteriorWithSun(interiorCell); + lastInteriorCell = interiorCell; + inInteriorWithSun = InteriorSun::IsInteriorWithSun(interiorCell); SetupVL();This ensures VL toggles correctly when entering/leaving interiors and when switching interior cells with different sun visibility.
🧹 Nitpick comments (1)
src/Features/VolumetricLighting.cpp (1)
3-3: Nit: include path consistencyConsider using a consistent include path with other files (e.g., "Features/InteriorSun.h") to keep includes uniform across the codebase. Current path is valid given the file’s location, this is just for consistency.
-#include "InteriorSun.h" +#include "Features/InteriorSun.h"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/Feature.cpp(2 hunks)src/Features/InteriorSun.cpp(7 hunks)src/Features/InteriorSun.h(5 hunks)src/Features/VolumetricLighting.cpp(2 hunks)src/Features/VolumetricLighting.h(1 hunks)src/Globals.cpp(2 hunks)src/Globals.h(2 hunks)src/Hooks.cpp(2 hunks)src/TruePBR.cpp(2 hunks)src/XSEPlugin.cpp(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#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/Globals.h
🔇 Additional comments (15)
src/XSEPlugin.cpp (1)
156-158: Couldn’t locate any calls toIsVRAddressLibraryAtLeastVersionor theXSEPlugin.cppfile in this branch—please verify that the VR Address Library version check exists and that its return value is handled appropriately (i.e., surface errors and disable VR features on failure as suggested).src/Features/VolumetricLighting.h (1)
131-131: Rename acknowledgment: field is consistent with new APIThe rename to inInteriorWithSun is clear and initialized correctly. Matches usage in the .cpp.
src/Globals.cpp (2)
22-22: Header rename to InteriorSun is consistentInclude updated to Features/InteriorSun.h aligns with the PR-wide rename.
66-66: Global feature object renamed and type-updated correctlyglobals::features now exposes InteriorSun interiorSun{}; consistent with headers and consumers.
src/Globals.h (2)
12-12: Forward declaration updated to InteriorSunMatches implementation and consumers.
67-67: Extern renamed to interiorSun with new typeExtern symbol matches Globals.cpp and downstream usage.
src/Feature.cpp (2)
13-13: Header include renamed to InteriorSun.hConsistent with new feature name.
223-223: InteriorSun rename complete; no leftover references foundRan
rg -n "InteriorSunShadows|interiorSunShadows|Features/InteriorSunShadows.h"and confirmed there are no remaining references to the old symbols. All registry entries have been updated correctly.src/TruePBR.cpp (2)
6-6: Header rename to InteriorSun.hAligned with global rename.
678-678: Lighting flags gated on InteriorSun stateThe consolidated check against interiorSun.loaded && interiorSun.isInteriorWithSun before adding ShadowDir/DefShadow is appropriate and matches the feature’s intent.
src/Hooks.cpp (2)
12-12: Header include updatedSwitched to Features/InteriorSun.h; consistent with project-wide rename.
903-905: VerifyInteriorSun::UpdateRasterStateCullModesignature
Please confirm that theInteriorSuntype defines a method matchingvoid UpdateRasterStateCullMode(/*pass type*/, /*technique type*/);and that its name/signature aligns with this call site.
• File:
src/Hooks.cpp
• Lines: 903–905src/Features/VolumetricLighting.cpp (1)
231-234: Interior VL gating correctly respects InteriorSun stateUsing settings.InteriorEnabled && inInteriorWithSun for both VR and non-VR paths is correct and aligns with the feature behavior.
src/Features/InteriorSun.h (1)
4-98: LGTM! Well-structured refactoring and feature addition.The renaming from
InteriorSunShadowstoInteriorSunis consistently applied, and the new interior shadow distance feature follows the existing architectural patterns. The default value of 5000 forInteriorShadowDistanceappears reasonable for typical interior spaces.src/Features/InteriorSun.cpp (1)
64-66: Confirm interior shadow distance patchThe relocation in
src/Features/InteriorSun.cpp(around lines 64–66) currently updates onlygShadowDistance, yet the new feature introducesgInteriorShadowDistance. Please verify that the game’sSetShadowDistanceimplementation (called at line 21 and defined at lines 206–211) correctly switches betweengShadowDistanceandgInteriorShadowDistancefor interior cells. If it does not, the patch should be updated to referencegInteriorShadowDistancewhen inside interiors.
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit