feat(weather-editor): per widget window data#2040
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughConsolidates widget window begin/setup into Widget::BeginWidgetWindow(), adds per-widget-type persistent sizing (JSON), introduces Widget::GetWidgetTypeName(), removes session open-widget persistence, and centralizes menu open/save/close behavior via WidgetFactory helpers. Multiple widget DrawWidget implementations updated to use BeginWidgetWindow(). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
|
No actionable suggestions for changed features. |
![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: 4
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
36-39: Structural inconsistency with other widgets.Similar to
LensFlareWidget.cpp, onlyDrawWidgetHeader()is gated byBeginWidgetWindow(), whileBeginScrollableContent()and the property drawing logic are always executed. In contrast,PrecipitationWidget.cppgates all content within theifblock. If both approaches are valid (e.g., the window content area renders correctly when collapsed), this is fine; otherwise, consider aligning the structure across widgets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp` around lines 36 - 39, The widget currently gates only DrawWidgetHeader() with BeginWidgetWindow() while calling BeginScrollableContent() and subsequent property drawing unconditionally, causing structural inconsistency with PrecipitationWidget.cpp (which wraps all content) and differing behavior from LensFlareWidget.cpp; update the ImageSpaceWidget.cpp flow so that BeginScrollableContent() and the property rendering logic are executed only when BeginWidgetWindow() returns true (i.e., move BeginScrollableContent() and all dependent UI draws inside the BeginWidgetWindow() block) or, if intentional, make a comment and mirror LensFlareWidget.cpp behavior for consistency by either gating only the header in all widgets or gating the entire content in all widgets (reference BeginWidgetWindow, DrawWidgetHeader, BeginScrollableContent).
🤖 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/EditorWindow.cpp`:
- Around line 1338-1340: The bug is that resetLayout is being cleared
unconditionally, causing ResetWidgetTypeSizes() not to be one-shot for only
windows drawn; to fix, move the resetLayout=false into the same conditional that
calls ResetWidgetTypeSizes() so the cache is only cleared when resetLayout was
true and the window was actually drawn (i.e., change the logic around
resetLayout and ResetWidgetTypeSizes() so they remain together), and ensure any
ImGui call using ImGuiCond_Always still relies on that one-shot reset behavior.
- Around line 923-930: The currentCellLightingWidget is being handled as an open
widget elsewhere but is omitted from the File > Save menu and the SaveAll flow;
add currentCellLightingWidget into the same save iteration so it is saved like
the other widget groups. Concretely, update the Save menu construction (where
WidgetFactory::DrawSaveWidgetMenuItems(...) is called for weatherWidgets,
lightingTemplateWidgets, etc.) to also call
WidgetFactory::DrawSaveWidgetMenuItems(currentCellLightingWidget, hasOpen) or
include currentCellLightingWidget in the collection used by that iteration, and
likewise ensure SaveAll() (the function responsible for saving all open widgets)
iterates over or includes currentCellLightingWidget so Ctrl+S and “Save All Open
Widgets” operate on it as well.
In `@src/WeatherEditor/WeatherUtils.cpp`:
- Around line 21-24: UpdateWidgetTypeSize currently writes new sizes only to the
in-memory map s_widgetTypeSizes without marking settings dirty, so resize-only
sessions aren't persisted; modify UpdateWidgetTypeSize(const char* widgetType)
to compare the new ImGui::GetWindowSize() against the existing entry for
widgetType and only update the map when the size actually changes, and when it
does call a single-point "mark dirty" method on the editor singleton (e.g.,
EditorWindow::GetSingleton()->MarkSettingsDirty() or set a bool like
EditorWindow::GetSingleton()->settingsDirty = true) so changes will be picked up
for persistence; additionally ensure EditorWindow's close/reset handlers call
EditorWindow::SaveSettings() (or trigger a debounced save) when the dirty flag
is set so the new sizes are serialized on close or after a short debounce
period.
- Around line 40-46: In SetWidgetTypeSizesFromJson validate that each array
element is numeric before calling get<float>() and clamp values to sensible
minima (e.g., >= 1.0f) to avoid exceptions and unusable windows; specifically,
in the loop over j.items() (function SetWidgetTypeSizesFromJson) check
val[0].is_number() && val[1].is_number() and only then read floats, apply
std::max/min (or clamp) to enforce minimum width/height, and then store the
clamped ImVec2 into s_widgetTypeSizes so EditorWindow::LoadSettings won't
receive invalid or zero/negative sizes.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp`:
- Around line 36-39: The widget currently gates only DrawWidgetHeader() with
BeginWidgetWindow() while calling BeginScrollableContent() and subsequent
property drawing unconditionally, causing structural inconsistency with
PrecipitationWidget.cpp (which wraps all content) and differing behavior from
LensFlareWidget.cpp; update the ImageSpaceWidget.cpp flow so that
BeginScrollableContent() and the property rendering logic are executed only when
BeginWidgetWindow() returns true (i.e., move BeginScrollableContent() and all
dependent UI draws inside the BeginWidgetWindow() block) or, if intentional,
make a comment and mirror LensFlareWidget.cpp behavior for consistency by either
gating only the header in all widgets or gating the entire content in all
widgets (reference BeginWidgetWindow, DrawWidgetHeader, BeginScrollableContent).
🪄 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: a1dda3a1-a028-4867-8bdb-293aff581d5f
📒 Files selected for processing (22)
src/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/LensFlareWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.hsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.h
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/WeatherEditor/EditorWindow.cpp (1)
1344-1345:⚠️ Potential issue | 🟠 MajorDon't clear the shared widget-type size cache here.
ResetWidgetTypeSizes()clears every cached widget type, including ones that were closed this frame. Reopening an untouched widget later will then lose its previous geometry instead of keeping it.🪟 Suggested change
-if (resetLayout) - ResetWidgetTypeSizes(); resetLayout = false;Based on learnings, "The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/EditorWindow.cpp` around lines 1344 - 1345, The call to ResetWidgetTypeSizes() guarded by resetLayout clears the global widget-type size cache (ResetWidgetTypeSizes()) and must not run here; instead change this site (the block using the resetLayout flag) to only clear sizes for widgets rendered this frame: either pass a set of rendered widget types to a new ResetWidgetTypeSizesForRendered(const std::unordered_set<Type>&) overload or add a ResetWidgetTypeSizesForRenderedWidgets() helper that clears only entries in renderedWidgetTypesThisFrame; replace the direct ResetWidgetTypeSizes() invocation with that targeted one so closed/non-rendered windows keep their previous geometry.
🧹 Nitpick comments (1)
src/WeatherEditor/EditorWindow.cpp (1)
945-951: Keep the File menu symmetrical for the cell lighting widget.
currentCellLightingWidgetis now part of Save, Save All, and Open Widgets, but this close-all section still skips it. Adding a dedicated close item here would keep the menu behavior consistent.🔧 Suggested change
WidgetFactory::DrawCloseAllMenuItem("Close All Weather Widgets", weatherWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All Lighting Widgets", lightingTemplateWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All ImageSpace Widgets", imageSpaceWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All Volumetric Lighting Widgets", volumetricLightingWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All Precipitation Widgets", precipitationWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All Lens Flare Widgets", lensFlareWidgets); WidgetFactory::DrawCloseAllMenuItem("Close All Visual Effect Widgets", referenceEffectWidgets); +if (currentCellLightingWidget && currentCellLightingWidget->IsOpen()) { + if (ImGui::MenuItem("Close Current Cell Lighting Widget")) + currentCellLightingWidget->SetOpen(false); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/EditorWindow.cpp` around lines 945 - 951, The File menu's Close All list omits the cell lighting entry; add a corresponding close-all menu item by calling WidgetFactory::DrawCloseAllMenuItem with a label like "Close All Cell Lighting Widgets" and the cell lighting widgets collection (e.g., cellLightingWidgets) so that the cell lighting widgets (including currentCellLightingWidget) are closed consistently with the other categories.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/WeatherEditor/EditorWindow.cpp`:
- Around line 1344-1345: The call to ResetWidgetTypeSizes() guarded by
resetLayout clears the global widget-type size cache (ResetWidgetTypeSizes())
and must not run here; instead change this site (the block using the resetLayout
flag) to only clear sizes for widgets rendered this frame: either pass a set of
rendered widget types to a new ResetWidgetTypeSizesForRendered(const
std::unordered_set<Type>&) overload or add a
ResetWidgetTypeSizesForRenderedWidgets() helper that clears only entries in
renderedWidgetTypesThisFrame; replace the direct ResetWidgetTypeSizes()
invocation with that targeted one so closed/non-rendered windows keep their
previous geometry.
---
Nitpick comments:
In `@src/WeatherEditor/EditorWindow.cpp`:
- Around line 945-951: The File menu's Close All list omits the cell lighting
entry; add a corresponding close-all menu item by calling
WidgetFactory::DrawCloseAllMenuItem with a label like "Close All Cell Lighting
Widgets" and the cell lighting widgets collection (e.g., cellLightingWidgets) so
that the cell lighting widgets (including currentCellLightingWidget) are closed
consistently with the other categories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b64463ea-0adb-4b4d-ada1-9d563f20fe82
📒 Files selected for processing (2)
src/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/WeatherUtils.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WeatherEditor/WeatherUtils.cpp
alandtse
requested changes
Mar 31, 2026
Collaborator
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
Please be careful hardcoding things. They are brittle. Abstracts should be used when possible like loops, etc.
Contributor
Author
|
@alandtse fixed the manual iteration. The widget type names aren't feature names and are already centralized. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/WeatherEditor/EditorWindow.cpp (1)
1324-1326:⚠️ Potential issue | 🟠 MajorDon't clear the shared widget-type size cache here.
The one-shot
resetLayoutflag already reaches the windows that are actually drawn this frame. Clearing the whole cache at the end also resets closed widget types, so reopening one later loses its previous geometry.Based on learnings, "The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows."🪟 Suggested fix
- if (resetLayout) - ResetWidgetTypeSizes(); resetLayout = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/EditorWindow.cpp` around lines 1324 - 1326, Don't clear the global widget-type size cache here; remove the call to ResetWidgetTypeSizes() in the end-of-frame cleanup so closed widget types keep their cached geometry. Keep the one-shot resetLayout flag handling as-is (resetLayout should remain the per-frame/per-window trigger) and ensure ResetWidgetTypeSizes() is only invoked by the windows that actually process resetLayout during their draw path (refer to the ResetWidgetTypeSizes() function and the resetLayout variable) rather than calling it unconditionally in this end-of-frame location.
🤖 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/EditorWindow.cpp`:
- Around line 2053-2062: Undo entries may hold dangling pointers because
currentCellLightingWidget is recreated; instead of trusting state.widget, always
rebind by looking up the widget by ID each time you process the undo state: use
GetWidgetCollections() and compare GetEditorID() against state.widgetId to find
the live widget and assign state.widget to that found pointer (even if
state.widget is already non-null). Update the logic in the code that currently
breaks when state.widget is set so it continues scanning collections to
re-resolve the pointer for currentCellLightingWidget and any undo application
paths that rely on state.widget.
- Line 17: The Settings JSON binding omitted paletteValues and PaletteValueEntry
lacks a JSON adapter, so numeric palette entries are lost on reload; add
paletteValues to the NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT list for
EditorWindow::Settings and implement non-intrusive to_json/from_json functions
(or a macro-based adapter) for PaletteValueEntry in this file so palette entries
serialize/deserialize correctly and round-trip.
---
Duplicate comments:
In `@src/WeatherEditor/EditorWindow.cpp`:
- Around line 1324-1326: Don't clear the global widget-type size cache here;
remove the call to ResetWidgetTypeSizes() in the end-of-frame cleanup so closed
widget types keep their cached geometry. Keep the one-shot resetLayout flag
handling as-is (resetLayout should remain the per-frame/per-window trigger) and
ensure ResetWidgetTypeSizes() is only invoked by the windows that actually
process resetLayout during their draw path (refer to the ResetWidgetTypeSizes()
function and the resetLayout variable) rather than calling it unconditionally in
this end-of-frame location.
🪄 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: 81c4e634-5c40-4f19-a887-69b27c155584
📒 Files selected for processing (3)
src/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/WeatherUtils.h
alandtse
approved these changes
Apr 1, 2026
This was referenced Apr 18, 2026
ParticleTroned
pushed a commit
to ParticleTroned/skyrim-community-shaders
that referenced
this pull request
May 2, 2026
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit edf3a8a)
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.
Allows window size data to be per widget instead of per record. Allows each widget window to be detected by UI.
Summary by CodeRabbit
New Features
Removed
Improvements