feat(weather): per-feature settings rework, main feature lock#1704
Conversation
📝 WalkthroughWalkthroughThis PR introduces weather-aware UI controls and per-weather feature settings management. It adds Util::WeatherUI helpers that grey out when weather overrides apply, enables editor navigation to specific feature settings via OpenWeatherFeatureSetting(), implements per-weather feature settings persistence in WeatherWidget with toggle state, and updates WeatherManager to respect feature-level enablement flags. Changes
Sequence DiagramssequenceDiagram
participant User
participant UI as Util::WeatherUI<br/>(Control)
participant EditorWindow
participant WeatherWidget
participant WeatherManager
User->>UI: Interact with weather-controlled slider/checkbox
UI->>UI: Check IsWeatherControlled()
alt Is Weather Controlled
UI->>EditorWindow: OpenWeatherFeatureSetting()
EditorWindow->>WeatherWidget: Locate & focus widget
WeatherWidget->>WeatherWidget: NavigateToFeatureSetting()
WeatherWidget->>User: Display feature settings UI
else Not Controlled
UI->>User: Allow edit & return value
end
sequenceDiagram
participant User
participant WeatherWidget as WeatherWidget<br/>(Settings UI)
participant WeatherManager
participant Registry as Weather<br/>Variable Registry
User->>WeatherWidget: Toggle per-weather override
User->>WeatherWidget: Edit feature setting
WeatherWidget->>WeatherManager: SaveWeatherSettings()
WeatherManager->>WeatherManager: Check __enabled flag
alt Weather Currently Active
WeatherManager->>Registry: Apply feature override
Registry->>User: Live update
end
WeatherWidget->>User: Confirm save
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
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: 1
🤖 Fix all issues with AI agents
In @src/WeatherEditor/Weather/WeatherWidget.cpp:
- Around line 1635-1642: The Json type conversion calls using
currentValue.get<T>() in the WeatherWidget UI code (e.g., inside the
dynamic_cast to WeatherVariables::WeatherVariable<bool> and the other branches
for float/int/string) can throw nlohmann::json::type_error; wrap those
retrievals in a try/catch or replace them with currentValue.value("<key>",
<default>) to provide a safe default and avoid exceptions, then use the
retrieved safe value for ImGui::Checkbox/Slider/TextInput and only write back to
featureJson[varName] when the UI changes (preserve modified flag behavior).
Ensure you update the bool branch
(dynamic_cast<WeatherVariables::WeatherVariable<bool>>), as well as the
corresponding float/int/string branches referenced in the review, to use the
safe retrieval pattern and handle exceptions consistently.
🧹 Nitpick comments (5)
docs/weather-system-docs/WeatherVariableRegistration.md (1)
113-115: Consider using weather-aware Checkbox in the example for consistency.The example shows
ImGui::Checkboxfor the "Enable Effect" setting with a comment saying it's "not weather-controlled in this example." However, sinceUtil::WeatherUI::Checkbox()is listed as available (line 121), it would be clearer to either:
- Use
Util::WeatherUI::Checkboxto demonstrate consistency, or- Add a brief note explaining when to use the regular ImGui version vs. the weather-aware version
This helps users understand the distinction better.
src/Utils/UI.cpp (2)
1373-1375: Remove unused variable declarations.Lines 1373-1374 declare
weatherManagerandcurrentWeathersbut they're not used until lines 1391-1407 inside the click/hover handlers. These declarations are redundant here and can be removed since the same variables are re-declared in the relevant blocks below.♻️ Suggested fix
if (isControlled) { - auto* weatherManager = WeatherManager::GetSingleton(); - auto currentWeathers = weatherManager->GetCurrentWeathers(); - // Make it look like a clickable button when weather-controlled ImGui::PushStyleColor(ImGuiCol_FrameBg, ImVec4(0.3f, 0.3f, 0.4f, 0.8f));
1424-1579: Consider extracting common weather-override UI logic.The tooltip rendering (lines 1458-1470, 1509-1521, 1560-1572) and click-to-edit handling (lines 1445-1456, 1496-1507, 1547-1558) are duplicated across all four control functions with identical logic.
Extracting these into private helpers would reduce duplication and simplify maintenance:
♻️ Suggested helper functions
namespace WeatherUI { namespace detail { void ShowWeatherOverrideTooltip() { auto* weatherManager = WeatherManager::GetSingleton(); auto currentWeathers = weatherManager->GetCurrentWeathers(); ImGui::PushTextWrapPos(ImGui::GetFontSize() * 35.0f); ImGui::TextColored(ImVec4(1.0f, 0.8f, 0.2f, 1.0f), "Weather Override Active"); ImGui::TextWrapped("This setting is controlled by the current weather (%s).", currentWeathers.currentWeather ? currentWeathers.currentWeather->GetFormEditorID() : "Unknown"); ImGui::Separator(); ImGui::TextColored(ImVec4(0.6f, 0.9f, 0.6f, 1.0f), "Click to open Weather Editor"); ImGui::PopTextWrapPos(); } void HandleClickToEdit(Feature* feature, const char* settingName) { auto* weatherManager = WeatherManager::GetSingleton(); auto* editorWindow = EditorWindow::GetSingleton(); auto currentWeathers = weatherManager->GetCurrentWeathers(); if (currentWeathers.currentWeather && editorWindow) { editorWindow->OpenWeatherFeatureSetting( currentWeathers.currentWeather, feature->GetShortName(), settingName); } } } }src/WeatherEditor/EditorWindow.cpp (1)
1111-1114: UseSetOpen(true)for consistency.The rest of the codebase uses
SetOpen(true)to open widgets (e.g., lines 186, 249, 542). Direct member access toopenmay bypass any side effects implemented inSetOpen().Suggested fix
- // Open the widget if it's not already open - if (!weatherWidget->open) { - weatherWidget->open = true; - } + // Open the widget if it's not already open + if (!weatherWidget->IsOpen()) { + weatherWidget->SetOpen(true); + }src/WeatherEditor/Weather/WeatherWidget.cpp (1)
1759-1767:pendingSettingHighlightis set but never used.The
settingNameparameter is stored inpendingSettingHighlight(line 1763), but it's never read inDrawFeatureSettings- onlypendingFeatureNavigationis used (line 1537). The clearing at lines 1753-1755 clears both, butpendingSettingHighlighthas no effect.Either implement the highlighting for the specific setting, or simplify the method signature if setting-level navigation isn't needed.
#!/bin/bash # Verify pendingSettingHighlight is not used elsewhere rg -n "pendingSettingHighlight" --type=cpp
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/weather-system-docs/WeatherVariableRegistration.mdsrc/Feature.cppsrc/Utils/UI.cppsrc/Utils/UI.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherManager.cpp
🧰 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/WeatherManager.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/Feature.cppsrc/Utils/UI.hsrc/WeatherEditor/EditorWindow.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/EditorWindow.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/WeatherManager.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/Feature.cppsrc/Utils/UI.hsrc/WeatherEditor/EditorWindow.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/EditorWindow.h
**/*
⚙️ 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/WeatherManager.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/Feature.cppsrc/Utils/UI.hdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherEditor/EditorWindow.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/EditorWindow.h
🧠 Learnings (5)
📓 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.
📚 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} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/WeatherEditor/Weather/WeatherWidget.hsrc/Feature.cppsrc/WeatherEditor/Weather/WeatherWidget.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/Feature.cpp
📚 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/Feature.cppsrc/Utils/UI.hdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherEditor/EditorWindow.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/EditorWindow.h
📚 Learning: 2025-06-17T09:27:49.594Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 1157
File: src/Feature.cpp:42-49
Timestamp: 2025-06-17T09:27:49.594Z
Learning: In src/Feature.cpp, when an obsolete feature's INI file is deleted, the feature should be silently disabled without surfacing any issues to the user. This is the intended behavior because a deleted INI file for an obsolete feature indicates that the user has properly cleaned up the obsolete feature.
Applied to files:
src/Feature.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 (11)
src/WeatherEditor/EditorWindow.h (1)
164-166: LGTM!The
OpenWeatherFeatureSettingdeclaration is well-designed with appropriate parameter types. Usingconst std::string&for string parameters avoids unnecessary copies.src/Utils/UI.h (1)
200-238: LGTM! Well-designed weather-aware UI API.The
WeatherUInamespace provides a clean, consistent API that:
- Mirrors ImGui conventions (const char* labels, bool returns)
- Documents behavior clearly with doxygen-style comments
- Maintains symmetry across control types (SliderFloat, Checkbox, ColorEdit3, ColorEdit4)
The
IsWeatherControlledutility function is a nice addition for cases where callers need to check override status without rendering a control.src/Utils/UI.cpp (3)
1337-1366: LGTM!The
IsWeatherControlledimplementation has proper null checks and a clear logical flow: validate inputs → check feature support → verify current weather → load and check settings.
1383-1384: VerifyImGuiSliderFlags_ReadOnlyavailability.
ImGuiSliderFlags_ReadOnlywas introduced in ImGui 1.87. The explicit casts suggest awareness of potential compatibility concerns. Confirm this flag is supported by the ImGui version used in the project.#!/bin/bash # Check ImGui version used in the project rg -n "IMGUI_VERSION" --type=h | head -5 # Also check if ImGuiSliderFlags_ReadOnly is defined rg -n "ImGuiSliderFlags_ReadOnly" --type=h
1445-1456: This pattern is correct and will detect clicks properly.
ImGui::IsItemClicked()is called afterImGui::PopItemFlag()(line 1441), so the disabled flag is no longer active when the click is checked. The disabled flag controls the visual appearance during rendering (line 1438), then is properly removed before input handling. This is the intended design and works correctly.The
SliderFloatcomparison is not applicable—ImGuiSliderFlags_ReadOnlyis a slider-specific flag for input behavior, not a replacement for the general item flag pattern used here.src/Feature.cpp (1)
39-40: Remove unused includes on lines 39-40.The includes for
WeatherManager.handWeatherVariableRegistry.hare not used anywhere in this file. Remove them or add their usage to keep the code clean.⛔ Skipped due to 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: ThePagi Repo: doodlum/skyrim-community-shaders PR: 1369 File: src/Features/SnowCover.cpp:277-293 Timestamp: 2025-08-05T18:22:40.578Z Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.src/WeatherEditor/Weather/WeatherWidget.h (1)
126-130: LGTM!The new navigation state members and
NavigateToFeatureSettingmethod are cleanly added. The public placement is appropriate since the method needs to be called fromEditorWindow::OpenWeatherFeatureSetting.src/WeatherManager.cpp (1)
212-228: LGTM!The per-feature enablement gate logic is correct:
- Defaults to
falseif__enabledis missing, preventing accidental activation- Returns early when disabled, avoiding unnecessary processing
- Properly filters the internal
__enabledflag from the output JSONsrc/WeatherEditor/Weather/WeatherWidget.cpp (2)
600-628: LGTM - immediate feature settings application logic is correct.The logic correctly:
- Saves settings for all features via
WeatherManager- Checks if the current weather matches before applying
- Respects the
__enabledflag- Filters out
__enabledbefore applying to the registryUsing
emptyWeatherwithlerpFactor = 1.0fprovides instant feedback during editing, which is the expected UX.
1744-1746: Good UX - guidance when overrides are disabled.The greyed-out guidance text clearly informs users that they need to enable overrides to customize settings. This prevents confusion about why controls aren't visible.
src/WeatherEditor/EditorWindow.cpp (1)
1096-1125:ImGui::SetWindowFocusis called before the window is created in the ImGui context.At line 1121,
ImGui::SetWindowFocusis invoked immediately after opening the widget. However, the widget's ImGui window is not created untilDrawWidget()is called duringRenderUI()(which happens later in the draw sequence). The window will not exist in ImGui's context whenSetWindowFocusis executed, so the focus will not take effect until the next frame when the window has been rendered.While the navigation to the feature/setting is set up correctly and will work when the window renders, the window focus may not be applied as intended on the same frame.
| if (auto* boolVar = dynamic_cast<WeatherVariables::WeatherVariable<bool>*>(var.get())) { | ||
| bool value = currentValue.get<bool>(); | ||
|
|
||
| if (ImGui::Checkbox(varDisplayName.c_str(), &value)) { | ||
| featureJson[varName] = value; | ||
| modified = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing exception handling for JSON type conversion.
The .get<T>() calls (lines 1636, 1657, 1681, 1703) can throw nlohmann::json::type_error if the stored value doesn't match the expected type. This could happen if settings were saved with an older version or corrupted.
Consider wrapping the value retrieval in try-catch or using value() with a default, consistent with the pattern used elsewhere (e.g., line 1544 uses .value("__enabled", false)).
Example fix for float variable
} else if (auto* floatVar = dynamic_cast<WeatherVariables::FloatVariable*>(var.get())) {
- float value = currentValue.get<float>();
+ float value = currentValue.is_number() ? currentValue.get<float>() : floatVar->GetMin();
float minVal = floatVar->GetMin();🤖 Prompt for AI Agents
In @src/WeatherEditor/Weather/WeatherWidget.cpp around lines 1635 - 1642, The
Json type conversion calls using currentValue.get<T>() in the WeatherWidget UI
code (e.g., inside the dynamic_cast to WeatherVariables::WeatherVariable<bool>
and the other branches for float/int/string) can throw
nlohmann::json::type_error; wrap those retrievals in a try/catch or replace them
with currentValue.value("<key>", <default>) to provide a safe default and avoid
exceptions, then use the retrieved safe value for
ImGui::Checkbox/Slider/TextInput and only write back to featureJson[varName]
when the UI changes (preserve modified flag behavior). Ensure you update the
bool branch (dynamic_cast<WeatherVariables::WeatherVariable<bool>>), as well as
the corresponding float/int/string branches referenced in the review, to use the
safe retrieval pattern and handle exceptions consistently.
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.