chore(ibl): add per weather settings#1703
Conversation
📝 WalkthroughWalkthroughThis PR integrates IBL settings (EnableDiffuseIBL, DiffuseIBLScale, IBLSaturation, DALCAmount, FogAmount) with a weather variable registration system by replacing direct ImGui calls with weather-aware UI hooks and introducing a RegisterWeatherVariables() method that binds these settings to weather-driven behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant IBL as IBL Feature
participant WReg as GlobalWeatherRegistry
participant WeatherUI as WeatherUI System
IBL->>WReg: RegisterWeatherVariables()
WReg->>WReg: Register EnableDiffuseIBL (bool)
WReg->>WReg: Register DiffuseIBLScale (float, 0.0-10.0)
WReg->>WReg: Register IBLSaturation (float, 0.0-2.0)
WReg->>WReg: Register DALCAmount (float, 0.0-1.0)
WReg->>WReg: Register FogAmount (float, 0.0-1.0)
WeatherUI->>WReg: Query registered variables
WeatherUI->>IBL: Apply weather-driven UI hooks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/IBL.h (1)
43-52: Type mismatch:EnableDiffuseIBLisuintbut cast tobool*.The
EnableDiffuseIBLmember is declared asuintbut is cast tobool*in bothDrawSettings()andRegisterWeatherVariables(). While this may work in practice on little-endian systems, it's technically undefined behavior and could cause issues if the compiler makes assumptions aboutboolsize (typically 1 byte vs 4 bytes foruint).Consider changing the type to
boolif it's only used as a boolean, or use a proper conversion mechanism.Suggested fix
struct Settings { - uint EnableDiffuseIBL = 1; + bool EnableDiffuseIBL = true; uint PreserveFogLuminance = 0; uint UseStaticIBL = 1; uint EnableInterior = 0;Note: This would require updating the JSON serialization macro to handle the
booltype properly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/IBL.cppsrc/Features/IBL.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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/IBL.hsrc/Features/IBL.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/IBL.hsrc/Features/IBL.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Features/IBL.hsrc/Features/IBL.cpp
🧠 Learnings (3)
📓 Common learnings
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.
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.
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-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/Features/IBL.h
📚 Learning: 2025-06-24T07:17:36.604Z
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.
Applied to files:
src/Features/IBL.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
🔇 Additional comments (5)
src/Features/IBL.h (1)
36-36: LGTM on the method declaration.The
RegisterWeatherVariables()override follows the established pattern of other virtual methods in this class.src/Features/IBL.cpp (4)
7-7: LGTM on the include addition.The new include for
WeatherVariableRegistry.his necessary for the weather variable integration.
25-34: LGTM on weather UI integration.The transition from direct ImGui calls to
Util::WeatherUIwrappers is consistent across the weather-controlled settings. The ranges match those defined inRegisterWeatherVariables().
68-103: LGTM on float variable registrations.The float variables are registered with:
- Correct default values matching
Settingsstruct defaults- Consistent ranges with the UI sliders
- Clear descriptive names and tooltips
53-66: No null check needed.GetOrCreateFeatureRegistry()uses a "get or create" pattern that guarantees non-null return—it either retrieves an existing registry from the map or creates a new one if missing. The function never returnsnullptr. This is consistent with how the codebase uses it elsewhere without null checks (including the example documentation).Likely an incorrect or invalid review comment.
|
✅ A pre-release build is available for this PR: |
doodlum
changed the title
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.