feat(weather editor): integrate weather picker and rework feature menu#1817
Closed
SkrubbySkrubInAShrub wants to merge 23 commits into
Closed
feat(weather editor): integrate weather picker and rework feature menu#1817SkrubbySkrubInAShrub wants to merge 23 commits into
SkrubbySkrubInAShrub wants to merge 23 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughRemoved the WeatherPicker feature and its artifacts; introduced a new overlay-based WeatherEditor (headers, implementation, settings, UI, time controls, weather loading/filtering, colorized displays) and updated globals, menu wiring, docs, and UI category mapping accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User as "User / Menu"
participant Menu as "Menu"
participant Editor as "WeatherEditor (overlay)"
participant Game as "Game / RE::Sky / Engine"
rect rgba(200,200,255,0.5)
User->>Menu: Open Weather Details
Menu->>Editor: Invoke RenderWeatherDetailsWindow / show overlay
end
rect rgba(200,255,200,0.5)
Editor->>Editor: LoadAllWeathers / UpdateFilteredWeathers (DataLoaded/Prepass)
Editor->>Game: Query current sky/weather (RenderWeatherControls)
Game-->>Editor: Current sky/weather state
end
rect rgba(255,200,200,0.5)
User->>Editor: Interact (time controls, pick weather)
Editor->>Game: Apply time/weather changes (pause, accelerate, transition)
Game-->>Editor: Updated state / confirmation
end
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
SkrubbySkrubInAShrub
force-pushed
the
weather-picker-rework
branch
from
89fe470 to
fb0320b
Compare
![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: 3
🤖 Fix all issues with AI agents
In `@src/Features/WeatherEditor.cpp`:
- Around line 477-503: In WeatherEditor::DisplayPrecipitationInfo, the check for
particleTexture.textureName uses .c_str() which is never null so the "None"
branch never runs; change the condition to test if
particleTexture.textureName.empty() (or !empty()) to detect a missing texture
name, keeping the GET_INSTANCE_MEMBER(particleTexture,
weather->precipitationData) usage and preserving the same log messages and
tooltip behavior.
- Around line 80-124: Replace the hardcoded "Sleep/Wait Menu" string in
WeatherEditor::Prepass (the ui->IsMenuOpen call) with the appropriate named
constant: use RE::SleepWaitMenu::MENU_NAME if that symbol exists; if not, add a
module-level constant (e.g., constexpr auto kSleepWaitMenuName = "Sleep/Wait
Menu") and use that constant instead to match the project's MENU_NAME pattern
and avoid magic strings.
In `@src/Menu.cpp`:
- Around line 1069-1082: The current Menu::DrawWeatherDetailsWindow only checks
WeatherDetailsWindow.Enabled but must also ensure the weatherEditor feature
finished initialization; add a guard before calling RenderWeatherDetailsWindow
to return early if globals::features::weatherEditor is not initialized/loaded
(e.g. check a Loaded/Initialized flag or an IsLoaded()/IsInitialized() method
that is set in DataLoaded()), then proceed to set p_open and call
RenderWeatherDetailsWindow only when the feature is confirmed loaded.
🧹 Nitpick comments (2)
.claude/CLAUDE.md (1)
1-3: PR metadata: tighten conventional-commit scope + add issue refs (optional)The title is close to conventional commit format, but scopes typically avoid spaces and keep the title ≤50 chars. Consider something like
feat(weather-editor): integrate picker into editor. If the pause/wait freeze fix or time-control feature maps to a tracked issue, add aFixes/Addresses #…reference in the PR description.As per coding guidelines: “Format: type(scope): description” with a 50-character title limit and “Suggest adding appropriate GitHub keywords (Fixes/Closes/Addresses #…) for bug fixes or features.”
src/Features/WeatherEditor.h (1)
40-57: Add Doxygen comments for the new public helpers.Public API surface expanded (e.g.,
RenderWeatherDetailsWindow,DisplayWeatherInfo,RenderCoreWeatherDetails,RenderFeatureWeatherAnalysis,RenderWeatherControls,RenderWeatherInformationDisplay). Please add brief Doxygen comments for these new entry points.As per coding guidelines: “Include Doxygen comments for all public methods, especially graphics-related functions.”
|
✅ A pre-release build is available for this PR: |
added 2 commits
February 5, 2026 13:20
…er changes into WeatherEditor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/WeatherEditor.cpp`:
- Around line 247-277: DrawTimeControls currently uses local statics
(s_isTimePaused, s_savedTimeScale) causing desync with
EditorWindow::IsTimePaused() used in Prepass; replace the local static state by
centralizing pause state and saved timeScale into a single shared location
(e.g., add or use EditorWindow::IsTimePaused/SetTimePaused and a shared
EditorWindow::savedTimeScale float or a new TimeState struct) so both
DrawTimeControls and Prepass read/write the same flags; update DrawTimeControls
to call the centralized API to query/set pause and saved scale, preserve the
sleep/wait check logic (UI::IsMenuOpen("Sleep/Wait Menu")) and ensure restoring
time uses the shared savedTimeScale rather than a local default.
🧹 Nitpick comments (2)
src/Features/WeatherEditor.cpp (2)
390-405: Sanitize persisted window position values before use.
If config values are non‑finite or out-of-range, reset to defaults to avoid invalid coordinates and UI instability.As per coding guidelines:
**/*.{cpp,h,hpp}: Validate user input and prevent DirectX crashes from malformed configurations.
681-845: Optional: cache weather label strings to avoid per‑frame allocations.
weatherLabelsis rebuilt every frame; consider rebuilding only whens_filteredWeatherschanges.
SkrubbySkrubInAShrub
force-pushed
the
weather-picker-rework
branch
from
816361e to
878dd07
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Features/WeatherEditor.cpp`:
- Around line 780-786: The lock path can call editorWindow->LockWeather with a
null sky->currentWeather; update the ImGui button handler so it checks that sky
and sky->currentWeather are non-null before calling
editorWindow->LockWeather(sky->currentWeather), and if currentWeather is null
simply skip the LockWeather call (or show/disable the lock action) to avoid
breaking the Prepass() lock enforcement logic; locate this change around the
ImGui::Button(lockLabel) block that calls editorWindow->UnlockWeather() /
LockWeather(...) and add the null guard for sky->currentWeather.
- Around line 1194-1205: The DrawOverlay auto-enable logic forces
WeatherDetailsWindow.Enabled = true every frame when
WeatherDetailsWindow.ShowInOverlay and overlayVisible are true, so closing the
window is immediately undone; change this to only auto-enable once on the
transition when the overlay first becomes visible (track previous overlayVisible
state in WeatherEditor, e.g., a member like prevOverlayVisible) and call
WeatherDetailsWindow.Enabled = true only when overlayVisible is true and
prevOverlayVisible was false, otherwise do not overwrite Enabled; continue to
pass &WeatherDetailsWindow.Enabled to RenderWeatherDetailsWindow so the user can
close it and update prevOverlayVisible each frame.
🧹 Nitpick comments (1)
src/Features/WeatherEditor.cpp (1)
1083-1090: Redundant condition in flag check.The condition
flagNames.size() == 1 || (flagNames.size() == 1 && flagNames[0] == "None")has a redundant second clause. IfflagNames.size() == 1is true, the entire condition is already true.Did you intend to check for a single non-"None" flag? If so:
♻️ Suggested simplification
- if (flagNames.empty() || flagNames.size() == 1 || (flagNames.size() == 1 && flagNames[0] == "None")) { + if (flagNames.empty() || flagNames.size() == 1) {Or if the intent was to handle "None" specifically:
- if (flagNames.empty() || flagNames.size() == 1 || (flagNames.size() == 1 && flagNames[0] == "None")) { + if (flagNames.empty() || (flagNames.size() == 1 && flagNames[0] != "None")) {
added 2 commits
February 5, 2026 13:58
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Features/WeatherEditor.cpp`:
- Around line 242-322: The time-scale slider writes a min 0.1 value back into
calendar->timeScale->value while the game is paused (timeScale==0), causing
unintentional resumes; update DrawTimeControls so the SliderFloat for
s_timeScaleSlider is either disabled when the editorWindow reports
IsTimePaused() (or calendar->timeScale->value==0) or, if you must show a slider,
only display s_timeScaleSlider but guard the assignment so that when
calendar->timeScale->value==0 you do not write s_timeScaleSlider back into
calendar->timeScale->value; instead use editorWindow->savedTimeScale to show a
nonzero slider position and restore calendar->timeScale->value from
savedTimeScale only when ResumeTime()/PauseTime() logic actually unpauses.
🧹 Nitpick comments (1)
src/Features/WeatherEditor.cpp (1)
681-845: Consider caching weather labels to avoid per-frame allocations.Rebuilding
weatherLabelsevery frame can be costly with large mod lists. Cache and rebuild only whens_filteredWeatherschanges to keep the UI responsive.
As per coding guidelines: Always consider GPU workload and user experience when implementing graphics features.♻️ Suggested refactor
- // Weather Selection - now with colored text - std::vector<std::string> weatherLabels; - weatherLabels.reserve(s_filteredWeathers.size()); - for (const auto& weather : s_filteredWeathers) { - weatherLabels.push_back(Util::FormatWeather(weather)); - } + // Weather Selection - now with colored text + static std::vector<std::string> weatherLabels; + static uint32_t s_labelsFilter = 0; + if (s_labelsFilter != s_weatherFlagFilter || weatherLabels.size() != s_filteredWeathers.size()) { + weatherLabels.clear(); + weatherLabels.reserve(s_filteredWeathers.size()); + for (const auto& weather : s_filteredWeathers) { + weatherLabels.push_back(Util::FormatWeather(weather)); + } + s_labelsFilter = s_weatherFlagFilter; + }
Comment on lines
+242
to
+322
| void WeatherEditor::DrawTimeControls() | ||
| { | ||
| ImGui::Spacing(); | ||
| Util::DrawSectionHeader("Time Controls"); | ||
| ImGui::Spacing(); | ||
|
|
||
| auto calendar = RE::Calendar::GetSingleton(); | ||
| if (calendar && calendar->gameHour && calendar->timeScale) { | ||
| static constexpr float s_vanillaTimeScale = 20.0f; | ||
| auto editorWindow = EditorWindow::GetSingleton(); | ||
| bool isTimePaused = editorWindow->IsTimePaused(); | ||
|
|
||
| // Check if sleep/wait menu is open - don't sync pause state during wait | ||
| auto ui = RE::UI::GetSingleton(); | ||
| bool sleepWaitMenuOpen = ui && ui->IsMenuOpen(RE::SleepWaitMenu::MENU_NAME); | ||
|
|
||
| // Check if time is actually paused (timeScale is 0), but ignore during sleep/wait | ||
| if (!sleepWaitMenuOpen) { | ||
| if (calendar->timeScale->value == 0.0f && !isTimePaused) { | ||
| // External pause: keep a sane restore value for the shared state. | ||
| editorWindow->savedTimeScale = s_vanillaTimeScale; | ||
| } else if (calendar->timeScale->value > 0.0f && isTimePaused) { | ||
| // External resume: clear the shared pause state. | ||
| editorWindow->timePaused = false; | ||
| } | ||
| isTimePaused = editorWindow->IsTimePaused(); | ||
| } | ||
|
|
||
| if (ImGui::Button(isTimePaused ? "Resume Time" : "Pause Time", ImVec2(120, 0))) { | ||
| if (isTimePaused) { | ||
| editorWindow->ResumeTime(); | ||
| } else { | ||
| editorWindow->PauseTime(); | ||
| } | ||
| isTimePaused = editorWindow->IsTimePaused(); | ||
| } | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text("Pause or resume game time progression"); | ||
| } | ||
|
|
||
| ImGui::SameLine(); | ||
|
|
||
| ImGui::SliderFloat("Game Time", &calendar->gameHour->value, 0.0f, 23.99f, "%.2f"); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text("Adjust the current game time"); | ||
| } | ||
|
|
||
| // Timescale multiplier (exponential scale, vanilla at midpoint) | ||
| static float s_timeScaleSlider = s_vanillaTimeScale; | ||
|
|
||
| // Sync slider with actual value (handles external changes like reset button) | ||
| if (std::abs(calendar->timeScale->value - s_timeScaleSlider) > 0.01f) { | ||
| s_timeScaleSlider = calendar->timeScale->value; | ||
| } | ||
|
|
||
| if (ImGui::Button("Reset Speed", ImVec2(120, 0))) { | ||
| calendar->timeScale->value = s_vanillaTimeScale; | ||
| s_timeScaleSlider = s_vanillaTimeScale; | ||
| editorWindow->savedTimeScale = s_vanillaTimeScale; | ||
| } | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text("Reset time speed to vanilla (%.1fx)", s_vanillaTimeScale); | ||
| } | ||
|
|
||
| ImGui::SameLine(); | ||
|
|
||
| if (ImGui::SliderFloat("##TimeScale", &s_timeScaleSlider, 0.1f, 4000.0f, | ||
| s_timeScaleSlider == s_vanillaTimeScale ? "Vanilla Speed" : "", ImGuiSliderFlags_Logarithmic)) { | ||
| calendar->timeScale->value = s_timeScaleSlider; | ||
| } | ||
|
|
||
| ImGui::SameLine(); | ||
|
|
||
| ImGui::Text("%.1fx", calendar->timeScale->value); | ||
| if (auto _tt = Util::HoverTooltipWrapper()) { | ||
| ImGui::Text("Adjust how fast time passes (vanilla: %.1fx)", s_vanillaTimeScale); | ||
| } | ||
| } | ||
|
|
||
| ImGui::Spacing(); | ||
| } |
Contributor
There was a problem hiding this comment.
Pause can be unintentionally cleared by the time‑scale slider.
When timeScale is 0 (paused), the logarithmic slider’s min of 0.1 can clamp the value and then write 0.1 back, effectively resuming time or jittering the slider. Consider disabling the slider when paused or syncing it to the saved (non‑zero) scale without writing to the engine.
💡 Suggested fix
- // Sync slider with actual value (handles external changes like reset button)
- if (std::abs(calendar->timeScale->value - s_timeScaleSlider) > 0.01f) {
- s_timeScaleSlider = calendar->timeScale->value;
- }
+ // Sync slider with actual value (handles external changes like reset button)
+ if (isTimePaused) {
+ s_timeScaleSlider = std::max(editorWindow->savedTimeScale, 0.1f);
+ } else if (std::abs(calendar->timeScale->value - s_timeScaleSlider) > 0.01f) {
+ s_timeScaleSlider = calendar->timeScale->value;
+ }
- if (ImGui::SliderFloat("##TimeScale", &s_timeScaleSlider, 0.1f, 4000.0f,
- s_timeScaleSlider == s_vanillaTimeScale ? "Vanilla Speed" : "", ImGuiSliderFlags_Logarithmic)) {
- calendar->timeScale->value = s_timeScaleSlider;
- }
+ ImGui::BeginDisabled(isTimePaused);
+ if (ImGui::SliderFloat("##TimeScale", &s_timeScaleSlider, 0.1f, 4000.0f,
+ s_timeScaleSlider == s_vanillaTimeScale ? "Vanilla Speed" : "", ImGuiSliderFlags_Logarithmic)) {
+ calendar->timeScale->value = s_timeScaleSlider;
+ }
+ ImGui::EndDisabled();🤖 Prompt for AI Agents
In `@src/Features/WeatherEditor.cpp` around lines 242 - 322, The time-scale slider
writes a min 0.1 value back into calendar->timeScale->value while the game is
paused (timeScale==0), causing unintentional resumes; update DrawTimeControls so
the SliderFloat for s_timeScaleSlider is either disabled when the editorWindow
reports IsTimePaused() (or calendar->timeScale->value==0) or, if you must show a
slider, only display s_timeScaleSlider but guard the assignment so that when
calendar->timeScale->value==0 you do not write s_timeScaleSlider back into
calendar->timeScale->value; instead use editorWindow->savedTimeScale to show a
nonzero slider position and restore calendar->timeScale->value from
savedTimeScale only when ResumeTime()/PauseTime() logic actually unpauses.
Collaborator
Author
|
I want to do this properly so I'm going to split this PR up. |
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
2 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.
This is a major PR for after 1.4.8 is released, but I want to see what feedback I can get.
Summary by CodeRabbit
New Features
Chores
Documentation
UI