fix(weather-editor): weather data on game load#2268
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWeather editor now caches discovery of widget ChangesWeather editor + Widget loading
Sequence Diagram(s)sequenceDiagram
participant Prepass as WeatherEditor::Prepass()
participant Checker as WeatherEditor::HasWidgetJsonFiles()
participant Window as EditorWindow
participant Loader as WeatherEditor::EnsureDataLoaded()
participant Factory as WidgetFactory::PopulateWidgets()
participant Widget as Widget::Load(showNotification)
Prepass->>Window: EditorWindow::CanBeOpen()?
Prepass->>Checker: HasWidgetJsonFiles()?
alt ShouldPreloadEditorResources() true
Prepass->>Loader: EnsureDataLoaded()
Loader-->>Prepass: resources initialized
end
Prepass->>Factory: PopulateWidgets()
Factory->>Widget: widget->Load(false)
Widget-->>Factory: initialized (notifications suppressed)
Factory-->>Prepass: widgets ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). Comment |
|
No actionable suggestions for changed features. |
…b.com/SkrubbySkrubInAShrub/skyrim-community-shaders-main into fix/weather-editor-load-on-game-start
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WeatherEditor/Widget.cpp (1)
93-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
showNotificationis not consistently honored in error branches.
Load(false)suppresses success/no-file messages, but open/invalid/parse/unexpected-error paths still show notifications. In preload flows, a batch of bad files can still spam toasts.Suggested fix
void Widget::Load(bool showNotification) { + auto notify = [&](const std::string& message, const ImVec4& color, float duration = 3.0f) { + if (showNotification) { + EditorWindow::GetSingleton()->ShowNotification(message, color, duration); + } + }; + std::string filePath = GetSaveFilePath(); @@ - EditorWindow::GetSingleton()->ShowNotification( - std::format("Failed to open file for {}", GetEditorID()), - ImVec4(1.0f, 0.5f, 0.0f, 1.0f), - 3.0f); + notify(std::format("Failed to open file for {}", GetEditorID()), + ImVec4(1.0f, 0.5f, 0.0f, 1.0f)); @@ - EditorWindow::GetSingleton()->ShowNotification( - std::format("Invalid file for {} - resetting to vanilla", GetEditorID()), - ImVec4(1.0f, 0.5f, 0.0f, 1.0f), - 3.0f); + notify(std::format("Invalid file for {} - resetting to vanilla", GetEditorID()), + ImVec4(1.0f, 0.5f, 0.0f, 1.0f)); @@ - EditorWindow::GetSingleton()->ShowNotification( - std::format("Parse error for {} - resetting to vanilla", GetEditorID()), - ImVec4(1.0f, 0.0f, 0.0f, 1.0f), - 3.0f); + notify(std::format("Parse error for {} - resetting to vanilla", GetEditorID()), + ImVec4(1.0f, 0.0f, 0.0f, 1.0f)); @@ - EditorWindow::GetSingleton()->ShowNotification( - std::format("Error loading {} - resetting to vanilla", GetEditorID()), - ImVec4(1.0f, 0.0f, 0.0f, 1.0f), - 3.0f); + notify(std::format("Error loading {} - resetting to vanilla", GetEditorID()), + ImVec4(1.0f, 0.0f, 0.0f, 1.0f));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Widget.cpp` around lines 93 - 145, The error branches currently always call EditorWindow::GetSingleton()->ShowNotification (open-fail, null JSON, parse_error catch, std::exception catch) even when showNotification is false; update each place that calls ShowNotification (in the open failure block, after js.is_null(), inside the parse_error catch, and inside the std::exception catch) to only invoke ShowNotification when showNotification is true, leaving settingsFile.close(), logger calls, js = json(), and LoadSettings() behavior unchanged; refer to the symbols settingsFile, js, showNotification, GetEditorID(), and LoadSettings() to locate and modify the calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/WeatherEditor.cpp`:
- Around line 96-98: The three static flags s_dataAvailable,
s_resourcesInitialized, and s_weathersLoaded are never cleared so the prepass
(if (s_dataAvailable && !s_resourcesInitialized && EditorWindow::CanBeOpen()) {
EnsureDataLoaded(); }) only runs once; add a Reset() method (or logic) that
clears those flags and call it on game transitions or when
EditorWindow::CanBeOpen() transitions from false→true so EnsureDataLoaded() can
run again—implement Reset() to set s_dataAvailable = s_resourcesInitialized =
s_weathersLoaded = false and invoke it from the appropriate lifecycle hook (or
inside the code path that detects EditorWindow::CanBeOpen() becoming true) so
the preload cycle is retriggered.
---
Outside diff comments:
In `@src/WeatherEditor/Widget.cpp`:
- Around line 93-145: The error branches currently always call
EditorWindow::GetSingleton()->ShowNotification (open-fail, null JSON,
parse_error catch, std::exception catch) even when showNotification is false;
update each place that calls ShowNotification (in the open failure block, after
js.is_null(), inside the parse_error catch, and inside the std::exception catch)
to only invoke ShowNotification when showNotification is true, leaving
settingsFile.close(), logger calls, js = json(), and LoadSettings() behavior
unchanged; refer to the symbols settingsFile, js, showNotification,
GetEditorID(), and LoadSettings() to locate and modify the calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f051050-59a1-4c43-b3cf-7e4eae27f444
📒 Files selected for processing (4)
src/Features/WeatherEditor.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/WeatherEditor/Widget.cpp (1)
73-73: Add an issue-closing reference in the PR description.Since this is a bug fix, consider adding a keyword like
Fixes #<issue>/Closes #<issue>for traceability and auto-close behavior.As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords: 'Fixes
#123' or 'Closes#123' for bug fixes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Widget.cpp` at line 73, The PR is missing an issue-closing reference for this bug fix; update the PR description to include a GitHub keyword such as "Fixes #<issue>" or "Closes #<issue>" so the issue auto-closes when merged — mention the related function/class (Widget::Load) in the PR description or body to provide context and ensure traceability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/WeatherEditor/Widget.cpp`:
- Line 73: The PR is missing an issue-closing reference for this bug fix; update
the PR description to include a GitHub keyword such as "Fixes #<issue>" or
"Closes #<issue>" so the issue auto-closes when merged — mention the related
function/class (Widget::Load) in the PR description or body to provide context
and ensure traceability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07569dc5-4c37-495c-b3ef-f475a5b6353c
📒 Files selected for processing (1)
src/WeatherEditor/Widget.cpp
|
This needs to only happen if there are overrides detected. The weather load is slow and if a user doesn't need it, should be gated. Please confirm. |
Sure, I'll add a check for when overrides are detected. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/WeatherEditor.cpp`:
- Around line 39-55: The code sets s_checkedWidgetJsonFiles = true too early and
treats filesystem errors from std::filesystem::is_directory as "no folder",
which permanently suppresses future scans; update WeatherEditor.cpp so that
s_checkedWidgetJsonFiles is only set after a successful directory probe or after
all folders were scanned without encountering I/O errors, and treat
std::error_code failures from std::filesystem::is_directory and
directory_iterator as real errors (log and abort the scan but do not flip
s_checkedWidgetJsonFiles to true). Specifically, adjust the loop over
Widget::kSaveFolderNames and checks around
std::filesystem::is_directory(widgetSettingsPath, ec) and the directory_iterator
to only set s_hasWidgetJsonFiles/s_checkedWidgetJsonFiles after a confirmed
successful inspection (use the existing logger::warn with ec.message() on
failure) so a transient I/O/path error does not permanently mark the check as
completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4dc4140d-6a6e-46d1-9739-676c0f07f105
📒 Files selected for processing (5)
src/Features/WeatherEditor.cppsrc/Features/WeatherEditor.hsrc/Menu.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.h
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
AI-assisted review (Claude Sonnet 4.6) — please verify before acting on this.
The overall gating is correct — EnsureDataLoaded() is only reached when HasWidgetJsonFiles() returns true, and s_resourcesInitialized makes the whole check a one-shot after that. No data loads for users without override files.
One bug in HasWidgetJsonFiles(): the three filesystem error paths return false without setting s_checkedWidgetJsonFiles = true, so on any filesystem error the function retries the full directory scan every Prepass() frame rather than caching the negative result.
// src/Features/WeatherEditor.cpp – all three error exits should be `continue` not `return false`
if (ec) {
logger::warn("[WeatherEditor] Failed to inspect widget settings path '{}': {}", ...);
- return false;
+ continue; // skip this folder, check the rest; cache result at end of loop
}Same for the entryEc check (line ~55) and the post-loop iterator ec check (line ~64). With continue in all three places, errors are tolerated per-folder/per-entry, the remaining folders are still checked, and s_checkedWidgetJsonFiles = true at line 70 correctly caches the final answer regardless of errors.
alandtse
changed the title
Replace `return false` with `continue` in the three filesystem error paths so the scan tolerates per-folder/per-entry errors and still caches the final result via `s_checkedWidgetJsonFiles = true` at the end of the loop. Previously, any filesystem error caused Prepass() to re-scan all widget folders every frame. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Nevermind, handled it directly so we can merge |
Adapted for cs-1.4.11-PL-VR: keep saved editor/weather override loading and active-cell gating while preloading Weather Editor resources only when saved widget JSON exists.
Adapted for cs-1.4.11-PL-VR: keep saved editor/weather override loading and active-cell gating while preloading Weather Editor resources only when saved widget JSON exists.
Adapted for cs-1.4.11-PL-VR: keep saved editor/weather override loading and active-cell gating while preloading Weather Editor resources only when saved widget JSON exists.
2 participants
fixes bug introduced in 790044b that caused weather editor data to be loaded only after the weather editor menu was opened. This leads to no overrides being applied until the menu is opened.
Summary by CodeRabbit
Improvements
Chores