fix(weather-editor): reinit weathers for record changes#2174
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWidgets that reference forms consumed only at load (precipitation and reference effect) now require manual Apply; Apply paths call explicit weather reinitialization ( Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Editor UI
participant Widget as Widget (Precip/RefEffect/Weather)
participant Engine as Game Weather Engine
User->>UI: Edit precipitation / reference effect
UI->>Widget: DrawWidget() (widget reports RequiresManualApply)
Widget-->>UI: Show Apply button & manual-apply warning
User->>UI: Click Apply
UI->>Widget: ApplyChanges()
Widget->>Engine: Update runtime particle/ref data
Widget->>Engine: ForceWeatherReinit(currentWeather)
Engine-->>Widget: Weather reinitialized
Widget-->>UI: Apply complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
234-328:⚠️ Potential issue | 🟡 MinorAdd
pendingReinit = trueto ImageSpace and VolumetricLighting form-picker and inherit handlers.ImageSpace and VolumetricLighting form references are assigned in
SetWeatherValues()identically to PrecipitationData and ReferenceEffect (all read at weather load time per theForceWeatherReinitcomment), but only the latter two setpendingReinit = truewhen changed. Changes toimageSpaceRefs[i](lines 246–249, 259–260) andvolumetricLightingRefs[i](lines 294–297, 307–308) will not propagate until the weather is manually re-forced.Extend
pendingReinit = trueto all four form-picker sites and all four inherit handlers to ensure consistency with precipitationData and referenceEffect behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 234 - 328, When ImageSpace or VolumetricLighting refs change they must flag a weather reinit; add pendingReinit = true in the same places PrecipitationData and ReferenceEffect do: inside the inherit-handler branches where settings.imageSpaceRefs[i] and settings.volumetricLightingRefs[i] are set from parent (the blocks that compare and assign from parentWidget->settings.imageSpaceRefs[i] / parentWidget->settings.volumetricLightingRefs[i]) and immediately after the WeatherUtils::DrawFormPickerCached calls that return true for ImageSpace and VolumetricLighting (the form-picker change handlers). This ensures pendingReinit is set whenever imageSpaceRefs or volumetricLightingRefs are changed (symbols to locate: settings.imageSpaceRefs, settings.volumetricLightingRefs, pendingReinit, parentWidget, WeatherUtils::DrawFormPickerCached, and SetWeatherValues/ForceWeatherReinit mentioned in the comment).
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
498-505: Prefer routing the reinit throughpendingReinit+ApplyChanges()for consistency.
LoadSettingscurrently doespendingReinit = false; ApplyChanges(); Widget::ForceWeatherReinit(weather);. Combined with the conditional reinit insideApplyChanges(L1666‑1669), this is functionally equivalent to just settingpendingReinit = trueand lettingApplyChangeshandle the reinit. Same pattern duplication inRevertChangesat L1697‑1699.Keeping the reinit decision in one place makes the lifecycle easier to reason about and avoids the risk of the two paths diverging in the future.
♻️ Consolidate to the pendingReinit pattern
originalSettings = settings; - pendingReinit = false; + pendingReinit = true; ApplyChanges(); - Widget::ForceWeatherReinit(weather); }And at L1697‑1699:
settings = vanillaSettings; - pendingReinit = false; + pendingReinit = true; ApplyChanges(); - Widget::ForceWeatherReinit(weather); }Note: this also interacts with the
ForceWeatherReinitbehavior concern raised inWidget.cpp— if that helper ends up gated onsky->currentWeather == weather, this simplification still holds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 498 - 505, The explicit call to Widget::ForceWeatherReinit(weather) in LoadSettings (and the same in RevertChanges) duplicates reinit logic; instead set pendingReinit = true and call ApplyChanges() so the reinit flows through the single-path in ApplyChanges (which already checks pendingReinit), and remove the direct Widget::ForceWeatherReinit(weather) calls; update LoadFeatureSettings/LoadSettings and RevertChanges to assign pendingReinit = true, preserve originalSettings assignment, then call ApplyChanges() only.
🤖 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/WeatherEditor/Widget.cpp`:
- Around line 279-292: The method ForceWeatherReinit(RE::TESWeather* weather)
unconditionally force-switches the sky to the passed weather without checking if
the passed weather matches the current sky weather, unlike
ForceCurrentWeatherReinit() which guards on sky->currentWeather being non-null.
To fix this, add a nullptr check for the weather parameter and add a condition
to only call sky->ForceWeather(weather, true) if the passed weather is non-null
and matches sky->currentWeather. This prevents unintended force-switching side
effects and aligns with existing usage patterns that gate ForceWeather calls.
---
Outside diff comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 234-328: When ImageSpace or VolumetricLighting refs change they
must flag a weather reinit; add pendingReinit = true in the same places
PrecipitationData and ReferenceEffect do: inside the inherit-handler branches
where settings.imageSpaceRefs[i] and settings.volumetricLightingRefs[i] are set
from parent (the blocks that compare and assign from
parentWidget->settings.imageSpaceRefs[i] /
parentWidget->settings.volumetricLightingRefs[i]) and immediately after the
WeatherUtils::DrawFormPickerCached calls that return true for ImageSpace and
VolumetricLighting (the form-picker change handlers). This ensures pendingReinit
is set whenever imageSpaceRefs or volumetricLightingRefs are changed (symbols to
locate: settings.imageSpaceRefs, settings.volumetricLightingRefs, pendingReinit,
parentWidget, WeatherUtils::DrawFormPickerCached, and
SetWeatherValues/ForceWeatherReinit mentioned in the comment).
---
Nitpick comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 498-505: The explicit call to Widget::ForceWeatherReinit(weather)
in LoadSettings (and the same in RevertChanges) duplicates reinit logic; instead
set pendingReinit = true and call ApplyChanges() so the reinit flows through the
single-path in ApplyChanges (which already checks pendingReinit), and remove the
direct Widget::ForceWeatherReinit(weather) calls; update
LoadFeatureSettings/LoadSettings and RevertChanges to assign pendingReinit =
true, preserve originalSettings assignment, then call ApplyChanges() only.
🪄 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
Run ID: b8257574-e941-412a-b5cd-3e89568a8d87
📒 Files selected for processing (8)
src/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/WeatherEditor/Widget.cpp (1)
279-282:⚠️ Potential issue | 🟠 MajorAvoid force-switching non-active weather during reinit.
ForceWeatherReinit(weather)still callsForceWeather(weather, true)for any non-null widget weather, so applying/reverting an inactive weather can switch the live sky to that weather as a side effect. Please gate this to the active weather, or confirm that force-switching edited inactive weather is intentional. This duplicates the earlier finding on this helper.🔒 Proposed guard
void Widget::ForceWeatherReinit(RE::TESWeather* weather) { - if (weather && globals::game::sky) - globals::game::sky->ForceWeather(weather, true); + auto* sky = globals::game::sky; + if (!weather || !sky || sky->currentWeather != weather) + return; + + sky->ForceWeather(weather, true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Widget.cpp` around lines 279 - 282, Widget::ForceWeatherReinit currently force-applies any non-null weather and can unintentionally switch the live sky; change it to first get the sky's active/current weather from globals::game::sky (e.g., via its currentWeather/GetActiveWeather accessor) and only call globals::game::sky->ForceWeather(weather, true) when the passed-in weather equals the active weather, otherwise skip (or add a clear comment if force-switching inactive weather is intentionally desired and should remain). Ensure you modify Widget::ForceWeatherReinit and document the intended behavior.
🤖 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/WeatherEditor/Widget.cpp`:
- Around line 458-463: The warning text and tooltip currently appear regardless
of the showApply parameter, which can mislead users if the Apply button is
hidden; update the conditional that renders the warning (the block using
RequiresManualApply(), editorWindow->settings.autoApplyChanges, and menu where
ImGui::TextColored and ImGui::SetTooltip are called) to also check the showApply
flag so the "(Changes require manual apply)" text and the "use the Apply button"
tooltip are only shown when showApply is true.
---
Duplicate comments:
In `@src/WeatherEditor/Widget.cpp`:
- Around line 279-282: Widget::ForceWeatherReinit currently force-applies any
non-null weather and can unintentionally switch the live sky; change it to first
get the sky's active/current weather from globals::game::sky (e.g., via its
currentWeather/GetActiveWeather accessor) and only call
globals::game::sky->ForceWeather(weather, true) when the passed-in weather
equals the active weather, otherwise skip (or add a clear comment if
force-switching inactive weather is intentionally desired and should remain).
Ensure you modify Widget::ForceWeatherReinit and document the intended behavior.
🪄 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
Run ID: cd9ba5c5-648f-4e24-afbf-5f6e47950a77
📒 Files selected for processing (2)
src/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Widget.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WeatherEditor/Weather/WeatherWidget.cpp
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
1498-1546:⚠️ Potential issue | 🟠 Major
InheritAllFromParentcopies record references but never setspendingReinit.Lines 1499-1504 copy
imageSpaceRefs,volumetricLightingRefs,precipitationData, andreferenceEffectfrom the parent, and line 1545 then callsApplyChanges()(when auto-apply is enabled). HoweverpendingReinitis not set, soApplyChanges()will skipWidget::ForceWeatherReinit(weather)and the inherited precipitation/visual-effect/imagespace/volumetric-lighting records will not propagate live — exactly the case this PR was written to fix. The per-record inherit checkboxes inDrawWidget()correctly setpendingReinit = truealongsiderecordChanged; "Inherit All" should do the same.🔧 Proposed fix
// Copy records (form references) + bool recordsChanged = false; for (size_t i = 0; i < ColorTimes::kTotal; i++) { + if (settings.imageSpaceRefs[i] != parentWidget->settings.imageSpaceRefs[i] || + settings.volumetricLightingRefs[i] != parentWidget->settings.volumetricLightingRefs[i]) + recordsChanged = true; settings.imageSpaceRefs[i] = parentWidget->settings.imageSpaceRefs[i]; settings.volumetricLightingRefs[i] = parentWidget->settings.volumetricLightingRefs[i]; } + if (settings.precipitationData != parentWidget->settings.precipitationData || + settings.referenceEffect != parentWidget->settings.referenceEffect) + recordsChanged = true; settings.precipitationData = parentWidget->settings.precipitationData; settings.referenceEffect = parentWidget->settings.referenceEffect; @@ // Apply the changes + if (recordsChanged) { + pendingReinit = true; + } if (EditorWindow::GetSingleton()->settings.autoApplyChanges) { ApplyChanges(); }If a pendingReinit when records are unchanged is acceptable, the diff can be simplified to unconditionally setting
pendingReinit = true;before theApplyChanges()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 1498 - 1546, In InheritAllFromParent, after copying parent records (imageSpaceRefs, volumetricLightingRefs, precipitationData, referenceEffect) ensure the widget marks itself for a full reinit so those live resources propagate: set pendingReinit = true (the same flag set by individual per-record changes in DrawWidget) before calling ApplyChanges() (so Widget::ForceWeatherReinit(weather) runs); update the code path around the ApplyChanges() call in InheritAllFromParent accordingly.
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
227-422: Minor: consolidate therecordChanged/pendingReinitdual assignment.The pattern
recordChanged = true; pendingReinit = true;is repeated at 8 call sites (lines 248-249, 261-262, 298-299, 311-312, 343-344, 356-357, 386-387, 399-400). Since every record change in this tab semantically requires a reinit, one of the two flags is redundant — either drop the separaterecordChangedlocal and gate auto-apply onpendingReinitdirectly, or wrap the two writes in a small helper lambda. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 227 - 422, Replace the repeated "recordChanged = true; pendingReinit = true;" by consolidating to a single change call: either remove the local recordChanged and gate the auto-apply on pendingReinit only, or introduce a small helper (e.g. a lambda named SetPendingReinit or MarkChanged) at the top of WeatherWidget::[the drawing function] that sets both pendingReinit and recordChanged once; then replace all eight duplicate sites to call that helper and update the final auto-apply check to use pendingReinit (or the helper-updated flag) together with EditorWindow::GetSingleton()->settings.autoApplyChanges and ApplyChanges().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 1498-1546: In InheritAllFromParent, after copying parent records
(imageSpaceRefs, volumetricLightingRefs, precipitationData, referenceEffect)
ensure the widget marks itself for a full reinit so those live resources
propagate: set pendingReinit = true (the same flag set by individual per-record
changes in DrawWidget) before calling ApplyChanges() (so
Widget::ForceWeatherReinit(weather) runs); update the code path around the
ApplyChanges() call in InheritAllFromParent accordingly.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 227-422: Replace the repeated "recordChanged = true; pendingReinit
= true;" by consolidating to a single change call: either remove the local
recordChanged and gate the auto-apply on pendingReinit only, or introduce a
small helper (e.g. a lambda named SetPendingReinit or MarkChanged) at the top of
WeatherWidget::[the drawing function] that sets both pendingReinit and
recordChanged once; then replace all eight duplicate sites to call that helper
and update the final auto-apply check to use pendingReinit (or the
helper-updated flag) together with
EditorWindow::GetSingleton()->settings.autoApplyChanges and ApplyChanges().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a53fd528-7a90-41e3-840c-b8edc48470f6
📒 Files selected for processing (1)
src/WeatherEditor/Weather/WeatherWidget.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
411-413: PersistentpendingReinitmay defer reinit into an unrelated edit.Unlike the previous local
recordChanged,pendingReinitis a member flag that persists across frames. IfautoApplyChangesis off when a record is edited (or records are edited outside the Records tab, e.g., viaInheritAllFromParent/LoadSettings/RevertChangespaths that set it), the flag remains set until the nextApplyChanges()call. That next call may come from an unrelated change (a Fog/DALC slider in another tab), at which pointWidget::ForceWeatherReinitwill fire while the user is just moving a slider.ForceWeatherReinitdoes guard onsky->currentWeather == weather, so this is bounded — no harm, just a reinit attributed to the wrong interaction.If you want to keep reinits tightly coupled to the record-change interaction, consider either (a) consuming the flag without gating on
autoApplyChanges(always callForceWeatherReinitonce the flag is set, separate fromSetWeatherValues), or (b) only settingpendingReinitat the momentApplyChanges()is about to be invoked. Not a blocker — current behavior is safe due to the guard insideWidget::ForceWeatherReinit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` around lines 411 - 413, The member flag pendingReinit persists across frames and can trigger a reinit from an unrelated UI action when ApplyChanges() is later invoked; fix by either: (A) consume the flag immediately after the record-edit path by calling Widget::ForceWeatherReinit(...) once pendingReinit is set (clear pendingReinit afterward) so reinit is tied to the edit rather than waiting on EditorWindow::GetSingleton()->settings.autoApplyChanges, or (B) stop setting pendingReinit during the edit and instead set it only at the moment ApplyChanges() is about to run (so pendingReinit is transient and only exists for the intended ApplyChanges() call); update the logic around SetWeatherValues / ApplyChanges / ForceWeatherReinit to implement the chosen approach and ensure pendingReinit is cleared after ForceWeatherReinit is invoked.
🤖 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/Weather/WeatherWidget.cpp`:
- Around line 411-413: The member flag pendingReinit persists across frames and
can trigger a reinit from an unrelated UI action when ApplyChanges() is later
invoked; fix by either: (A) consume the flag immediately after the record-edit
path by calling Widget::ForceWeatherReinit(...) once pendingReinit is set (clear
pendingReinit afterward) so reinit is tied to the edit rather than waiting on
EditorWindow::GetSingleton()->settings.autoApplyChanges, or (B) stop setting
pendingReinit during the edit and instead set it only at the moment
ApplyChanges() is about to run (so pendingReinit is transient and only exists
for the intended ApplyChanges() call); update the logic around SetWeatherValues
/ ApplyChanges / ForceWeatherReinit to implement the chosen approach and ensure
pendingReinit is cleared after ForceWeatherReinit is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 121c9a38-dff8-4f17-9ac9-cbbee7983ae5
📒 Files selected for processing (1)
src/WeatherEditor/Weather/WeatherWidget.cpp
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
alandtse
changed the title
…aders#2174) (cherry picked from commit e0d4460)
2 participants
When a weathers with records that are tied to them, like precipitation, only read which precipitation record it should play when the weather initialized. Changes to these values wouldn't propagate until the weather was re-initialized, either through forcing the weather or by having it reappear naturally.
This PR introduces some new helpers and logic to re-init weathers when these records are changed so that the changes are actually viewable live. Fixes the issue where changes related to precipitation and reference effects were not applying during edits.
Adds some extra guards to prevent weathers being reforced every frame when for example a slider is used.
Summary by CodeRabbit
New Features
Bug Fixes