refactor(weather-editor): cleanup and consolidate widget json serialization#1954
Closed
SkrubbySkrubInAShrub wants to merge 14 commits into
Closed
refactor(weather-editor): cleanup and consolidate widget json serialization#1954SkrubbySkrubInAShrub wants to merge 14 commits into
SkrubbySkrubInAShrub wants to merge 14 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughEncapsulates widget internals (moves previously-public members to private and adds accessors), introduces nested Settings types with JSON (de)serialization, consolidates load/save/apply flows, normalizes ImGui Begin/End and change-tracking across many WeatherEditor widgets, and updates codepaths to use accessors (GetCell/GetLightingTemplate). Changes
Sequence Diagram(s)sequenceDiagram
participant EditorWindow
participant Widget
participant GameData
participant JSONStorage
EditorWindow->>Widget: Open / double-click / Enter
Widget->>Widget: GetCell() / GetLightingTemplate()
Widget->>GameData: LoadFromGameSettings() (read runtime data)
GameData-->>Widget: runtime settings/data
Widget->>JSONStorage: LoadSettings() (merge_patch/deserialize)
JSONStorage-->>Widget: merged settings
Widget->>Widget: apply clamping/validate
Widget->>GameData: ApplyChanges() (in-place update or DataID write)
GameData-->>Widget: confirm
Widget->>JSONStorage: SaveSettings() (js = settings)
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 |
|
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: 6
🧹 Nitpick comments (2)
src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
160-181: ApplyChanges uses consistent DataID-based indexing.The refactored approach using the
DataIDalias withstatic_cast<uint32_t>is clearer than hardcoded numeric indices. Note thatLoadFromGameSettings()usesGetSettingValue()whileApplyChanges()uses direct array access—this asymmetry works but could be unified in a future cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 160 - 181, No code change required; the ApplyChanges implementation in PrecipitationWidget::ApplyChanges correctly uses the DataID alias with static_cast<uint32_t> to index runtime.data and set fields (e.g., DataID::kGravityVelocity, DataID::kParticleSizeX, runtime.particleTexture.textureName), but if you want consistency with LoadFromGameSettings() consider refactoring later to use a shared GetSettingValue-based accessor for reading/writing these runtime.data entries to keep load and apply paths uniform.src/WeatherEditor/Weather/LensFlareWidget.cpp (1)
41-43: Consider using explicit JSON extraction for consistency with the explicit deserialization pattern.While
settings = js;works correctly here (implicit conversions are enabled and the macroNLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTexplicitly supports this), the patternjs.get_to(settings)provides more explicit intent. Note: This same pattern is used consistently throughout the codebase inLightingTemplateWidget.cpp,WeatherWidget.cpp,VolumetricLightingWidget.cpp,PrecipitationWidget.cpp, andCellLightingWidget.cpp. If refactoring, consider making the change uniformly across all widget classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/LensFlareWidget.cpp` around lines 41 - 43, Replace the implicit assignment "settings = js" with an explicit JSON extraction using nlohmann::json's get_to to match the project's deserialization pattern; locate the code in LensFlareWidget.cpp where the local json variable "js" is checked and replace the assignment to "settings" with "js.get_to(settings)" (keeping the surrounding if/else logic intact) so it aligns with other widgets like LightingTemplateWidget, WeatherWidget, VolumetricLightingWidget, PrecipitationWidget, and CellLightingWidget.
🤖 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/Weather/LightingTemplateWidget.cpp`:
- Around line 141-158: The code reads both TOD::Sunrise and TOD::Day from
settings.dalc.directional[0] but when writing back it only writes Sunrise, so
edits to the Day row are lost; update the write-back to assign
settings.dalc.directional[0].max = maxColors[TOD::Day] and
settings.dalc.directional[0].min = minColors[TOD::Day] (instead of using
TOD::Sunrise) in the block after TOD::DrawTODColorRow("Positive Direction (+)",
...) and make the analogous change in the second block (the other
DrawTODColorRow call) so the directional[0] entries reflect the Day row edits
rather than always using Sunrise.
- Around line 76-81: The directional slider values (settings.directionalXY,
settings.directionalZ, settings.directionalFade) must be validated and clamped
before any uint32_t casts; locate where those floats are later cast (around the
uint32_t assignments) and replace direct casts with a safe sequence: clamp each
float to the valid range (e.g., for angles clamp to 0.0f..360.0f; for fades
clamp to 0.0f..1.0f or the intended range), round to the nearest integer (e.g.,
std::round) and then static_cast<uint32_t>, and ensure
WeatherUtils::DrawSliderFloat return paths preserve clamped values so
malformed/negative config values cannot wrap or truncate at runtime.
In `@src/WeatherEditor/Weather/LightingTemplateWidget.h`:
- Around line 8-19: The constructor
LightingTemplateWidget(RE::BGSLightingTemplate* a_lightingTemplate) returns
early on a null a_lightingTemplate without initializing snapshot members,
leading to later undefined reads; fix by initializing form to nullptr (or
appropriate default), set settings to a safe default value, and assign
vanillaSettings and originalSettings from that default before returning—ensure
these members (form, settings, vanillaSettings, originalSettings) are all set on
the null-template path so LoadFromGameSettings() is only called for a real
template and snapshots are always initialized.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 25-107: The ImGui::Begin(...) check currently exits immediately,
so the tab bar and all widgets (BeginTabBar, BeginTabItem,
BeginScrollableContent, WeatherUtils::DrawSliderFloat, ImGui::InputInt,
ImGui::InputText, ApplyChanges, etc.) are drawn even when ImGui::Begin returns
false; move the entire block that creates the UI (everything from the first
ImGui::BeginTabBar call down to ImGui::EndTabBar and the changed/apply logic)
inside the scope guarded by if (ImGui::Begin(...)) so those calls only run when
Begin() returned true, and keep a single ImGui::End() after that guarded block
(still call ImGui::End() unconditionally) to properly close the window.
In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp`:
- Around line 41-43: The auto-apply branch calls ApplyChanges() without
recording an undo snapshot; before calling ApplyChanges() when changed &&
editorWindow->settings.autoApplyChanges is true, call
editorWindow->PushUndoState(this) so the change is capturable by undo
history—mirror the pattern used in WeatherWidget.cpp where PushUndoState(...) is
invoked prior to ApplyChanges().
- Around line 9-13: The ImGui::Begin() result is not checked before rendering
the rest of the window, so UI (BeginScrollableContent, other widgets) runs even
when the window is collapsed/closed; change the flow in ReferenceEffectWidget
(and apply the same pattern to LensFlareWidget, ImageSpaceWidget,
VolumetricLightingWidget, PrecipitationWidget, etc.): after calling
ImGui::Begin(GetEditorID().c_str(), &open, ...), immediately test its return
value and if it returns false call ImGui::End() and return/skip the rest of the
function; otherwise continue to call DrawWidgetHeader, BeginScrollableContent,
render UI, and finally call ImGui::End() as usual.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/LensFlareWidget.cpp`:
- Around line 41-43: Replace the implicit assignment "settings = js" with an
explicit JSON extraction using nlohmann::json's get_to to match the project's
deserialization pattern; locate the code in LensFlareWidget.cpp where the local
json variable "js" is checked and replace the assignment to "settings" with
"js.get_to(settings)" (keeping the surrounding if/else logic intact) so it
aligns with other widgets like LightingTemplateWidget, WeatherWidget,
VolumetricLightingWidget, PrecipitationWidget, and CellLightingWidget.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 160-181: No code change required; the ApplyChanges implementation
in PrecipitationWidget::ApplyChanges correctly uses the DataID alias with
static_cast<uint32_t> to index runtime.data and set fields (e.g.,
DataID::kGravityVelocity, DataID::kParticleSizeX,
runtime.particleTexture.textureName), but if you want consistency with
LoadFromGameSettings() consider refactoring later to use a shared
GetSettingValue-based accessor for reading/writing these runtime.data entries to
keep load and apply paths uniform.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/WeatherEditor/EditorWindow.cppsrc/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.h
💤 Files with no reviewable changes (1)
- src/WeatherEditor/Weather/ReferenceEffectWidget.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/WeatherEditor/Weather/ReferenceEffectWidget.cpp (1)
9-13:⚠️ Potential issue | 🟠 MajorGuard window rendering when
ImGui::Begin()returns false.At Line 9,
ImGui::Begin(...)only gatesDrawWidgetHeader, but Line 12+ and Line 45-46 still execute even when Begin is false. This can break ImGui window stack expectations. Early-return afterImGui::End()when Begin is false.Proposed fix
- if (ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings | kStickyHeaderFlags)) { - DrawWidgetHeader("##ReferenceEffectSearch", true, true); - } - BeginScrollableContent("##REScroll"); + if (!ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings | kStickyHeaderFlags)) { + ImGui::End(); + return; + } + DrawWidgetHeader("##ReferenceEffectSearch", true, true); + BeginScrollableContent("##REScroll");#!/bin/bash # Verify whether DrawWidget continues rendering outside the ImGui::Begin guard. rg -n -C4 --type=cpp 'if \(ImGui::Begin\(GetEditorID\(\)\.c_str\(\), &open, ImGuiWindowFlags_NoSavedSettings \| kStickyHeaderFlags\)\)' src/WeatherEditor/Weather/ReferenceEffectWidget.cpp rg -n -C3 --type=cpp 'BeginScrollableContent\("##REScroll"\)|EndScrollableContent\(\)|ImGui::End\(\)' src/WeatherEditor/Weather/ReferenceEffectWidget.cppExpected result:
BeginScrollableContent("##REScroll")should appear only after a successful Begin path (or after an early-return guard pattern).Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp` around lines 9 - 13, The ImGui::Begin(...) call only gates DrawWidgetHeader but subsequent UI calls like BeginScrollableContent("##REScroll") and ImGui::End() run even when Begin returned false; fix by checking the boolean result of ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings | kStickyHeaderFlags) and if it returns false call ImGui::End() and return early from the function so BeginScrollableContent, EndScrollableContent and further rendering (including the DrawWidgetHeader path for the non-begin case) only execute when Begin succeeded; update the control flow around ImGui::Begin / DrawWidgetHeader / BeginScrollableContent / EndScrollableContent / ImGui::End accordingly.src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
192-193:⚠️ Potential issue | 🟠 MajorClamp to
uint32_tupper bound before casting.Current logic only clamps negative values. Malformed large config values can still exceed
uint32_trange at cast time. Please clamp to[0, uint32_max]before the cast.🛠️ Proposed fix
- data.directionalXY = static_cast<std::uint32_t>(std::max(0.0f, std::round(settings.directionalXY))); - data.directionalZ = static_cast<std::uint32_t>(std::max(0.0f, std::round(settings.directionalZ))); + const float u32Max = static_cast<float>(std::numeric_limits<std::uint32_t>::max()); + data.directionalXY = + static_cast<std::uint32_t>(std::clamp(std::round(settings.directionalXY), 0.0f, u32Max)); + data.directionalZ = + static_cast<std::uint32_t>(std::clamp(std::round(settings.directionalZ), 0.0f, u32Max));#!/bin/bash # Verify current casts use lower-bound-only clamping. rg -nP 'directional(XY|Z)\s*=\s*static_cast<std::uint32_t>\(std::max\(0\.0f,\s*std::round\(settings\.directional(XY|Z)\)\)\)' src/WeatherEditor/Weather/LightingTemplateWidget.cppAs per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/LightingTemplateWidget.cpp` around lines 192 - 193, Current assignments to data.directionalXY and data.directionalZ only clamp the lower bound, risking overflow when casting to std::uint32_t; change the logic in LightingTemplateWidget.cpp where data.directionalXY/Z are set (using settings.directionalXY and settings.directionalZ) to first compute the rounded value, then clamp it to the full uint32_t range (0 to std::numeric_limits<std::uint32_t>::max())—for example using std::clamp or std::min/std::max with the rounded float—and only then static_cast to std::uint32_t so malformed large config values cannot overflow the cast.
🤖 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/Weather/LightingTemplateWidget.cpp`:
- Around line 212-216: RevertChanges currently assigns vanillaSettings to
settings, which is wrong for presets loaded from JSON; change
LightingTemplateWidget::RevertChanges to restore originalSettings (the saved
pre-edit snapshot) into settings and then call ApplyChanges() so the widget
truly returns to the original state; update any related comments or tests that
referenced vanillaSettings in RevertChanges if present.
---
Duplicate comments:
In `@src/WeatherEditor/Weather/LightingTemplateWidget.cpp`:
- Around line 192-193: Current assignments to data.directionalXY and
data.directionalZ only clamp the lower bound, risking overflow when casting to
std::uint32_t; change the logic in LightingTemplateWidget.cpp where
data.directionalXY/Z are set (using settings.directionalXY and
settings.directionalZ) to first compute the rounded value, then clamp it to the
full uint32_t range (0 to std::numeric_limits<std::uint32_t>::max())—for example
using std::clamp or std::min/std::max with the rounded float—and only then
static_cast to std::uint32_t so malformed large config values cannot overflow
the cast.
In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp`:
- Around line 9-13: The ImGui::Begin(...) call only gates DrawWidgetHeader but
subsequent UI calls like BeginScrollableContent("##REScroll") and ImGui::End()
run even when Begin returned false; fix by checking the boolean result of
ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings |
kStickyHeaderFlags) and if it returns false call ImGui::End() and return early
from the function so BeginScrollableContent, EndScrollableContent and further
rendering (including the DrawWidgetHeader path for the non-begin case) only
execute when Begin succeeded; update the control flow around ImGui::Begin /
DrawWidgetHeader / BeginScrollableContent / EndScrollableContent / ImGui::End
accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cpp
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/WeatherEditor/Weather/CellLightingWidget.cpp (1)
207-219:⚠️ Potential issue | 🟠 MajorSanitize deserialized values before
ApplyChanges().Line 219 applies deserialized values directly into game lighting structs. A malformed JSON can inject out-of-range values (fog/fade/rotation), so add a clamp/sanitize pass after load and before apply.
Suggested fix (post-load sanitize)
+ auto sanitize = [](Settings& s) { + s.fogNear = std::clamp(s.fogNear, 0.0f, 10000.0f); + s.fogFar = std::clamp(s.fogFar, 0.0f, 50000.0f); + s.fogPower = std::clamp(s.fogPower, 0.0f, 10.0f); + s.fogClamp = std::clamp(s.fogClamp, 0.0f, 1.0f); + s.directionalFade = std::clamp(s.directionalFade, 0.0f, 1.0f); + s.clipDist = std::clamp(s.clipDist, 0.0f, 50000.0f); + s.lightFadeStart = std::clamp(s.lightFadeStart, 0.0f, 10000.0f); + s.lightFadeEnd = std::clamp(s.lightFadeEnd, 0.0f, 20000.0f); + s.directionalXY = std::min<uint32_t>(s.directionalXY, 360); + s.directionalZ = std::min<uint32_t>(s.directionalZ, 360); + s.dalc.fresnelPower = std::clamp(s.dalc.fresnelPower, 0.0f, 10.0f); + }; try { if (!js.empty()) { settings = js; } else { settings = vanillaSettings; } } catch (const std::exception& e) { logger::error("CellLighting {}: Failed to load from JSON: {}", GetEditorID(), e.what()); settings = vanillaSettings; } + sanitize(settings); originalSettings = settings; ApplyChanges();As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/CellLightingWidget.cpp` around lines 207 - 219, After deserializing into settings (or falling back to vanillaSettings) but before assigning originalSettings and calling ApplyChanges(), add a sanitize/clamp pass that validates and clamps any game lighting fields (e.g., fog, fade, rotation, intensity, color components) to safe ranges; update the block that currently sets settings from js/vanillaSettings to run a SanitizedSettings = ClampSettings(settings) (or call a new ValidateAndClampSettings(settings) helper) and then assign originalSettings = SanitizedSettings and call ApplyChanges(); keep the existing exception logger (logger::error with GetEditorID()) but ensure malformed values never get passed directly into ApplyChanges().src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
6-23:⚠️ Potential issue | 🟠 MajorReplace
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVEwith_WITH_DEFAULTto gracefully handle missing fields in older or partial presets.The current macros throw
json.exception.out_of_rangewhen any key is missing. Using_WITH_DEFAULTwill skip missing fields and use member defaults instead. Additionally, wrap the assignment inLoadSettings()(line 259) with try-catch to handle any remaining parse errors:Fix details
-NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE(LightingTemplateWidget::DirectionalColor, max, min) -NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE(LightingTemplateWidget::DALC, specular, fresnelPower, directional) -NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE(LightingTemplateWidget::Settings, +NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT(LightingTemplateWidget::DirectionalColor, max, min) +NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT(LightingTemplateWidget::DALC, specular, fresnelPower, directional) +NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT(LightingTemplateWidget::Settings, ambient, directional, fogColorNear, fogColorFar, fogNear, fogFar, directionalXY, directionalZ, directionalFade, clipDist, fogPower, fogClamp, lightFadeStart, lightFadeEnd, dalc)void LightingTemplateWidget::LoadSettings() { if (!js.empty()) { - settings = js; + try { + settings = js.get<Settings>(); + } catch (const nlohmann::json::exception&) { + settings = vanillaSettings; + } } else { settings = vanillaSettings; } originalSettings = settings; ApplyChanges(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/LightingTemplateWidget.cpp` around lines 6 - 23, Replace the NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE macros for LightingTemplateWidget::DirectionalColor, LightingTemplateWidget::DALC, and LightingTemplateWidget::Settings with the corresponding NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT variants so missing JSON keys fall back to member defaults; then, in LightingTemplateWidget::LoadSettings() wrap the JSON-to-struct assignment (the code that deserializes into the Settings instance) in a try-catch that catches nlohmann::json::exception and handles/logs the error gracefully (e.g., preserve defaults and avoid throwing) to protect against malformed or partial presets.src/WeatherEditor/Weather/LensFlareWidget.cpp (1)
43-54:⚠️ Potential issue | 🟠 MajorValidate loaded values before applying runtime changes
settings = jsaccepts parseable but out-of-range numbers, and Line 54 applies them immediately. This bypasses the UI bounds enforced by sliders and can propagate malformed config values into live weather data.Suggested fix
try { if (!js.empty()) { settings = js; } else { settings = vanillaSettings; } + + if (settings.fadeDistRadiusScale < 0.0f) + settings.fadeDistRadiusScale = 0.0f; + else if (settings.fadeDistRadiusScale > 10.0f) + settings.fadeDistRadiusScale = 10.0f; + + if (settings.colorInfluence < 0.0f) + settings.colorInfluence = 0.0f; + else if (settings.colorInfluence > 1.0f) + settings.colorInfluence = 1.0f; } catch (const std::exception& e) { logger::error("LensFlare {}: Failed to load from JSON: {}", GetEditorID(), e.what()); settings = vanillaSettings; }As per coding guidelines: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/LensFlareWidget.cpp` around lines 43 - 54, The code assigns settings = js and immediately calls ApplyChanges(), which allows parseable but out-of-range values to be applied; modify the load path in LensFlareWidget.cpp so that after parsing js you validate and clamp each configurable field (the members stored in settings) against the UI slider bounds or use vanillaSettings defaults for missing/invalid entries, log a warning via logger::warn with GetEditorID() when values were out-of-range or replaced, only then assign originalSettings = settings and call ApplyChanges(); ensure the validation logic references the same settings members used by ApplyChanges() to prevent malformed config propagation.
🧹 Nitpick comments (4)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
88-105: Consider logging a warning on early exit for debugging purposes.The null check is good defensive programming. However, silently returning when
imageSpaceis null could mask initialization issues during development/debugging.📝 Optional: Add debug logging
void ImageSpaceWidget::LoadSettings() { - if (!imageSpace) + if (!imageSpace) { + logger::warn("LoadSettings called with null imageSpace for {}", GetEditorID()); return; + }🤖 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 88 - 105, The early-return in ImageSpaceWidget::LoadSettings silently hides when imageSpace is null; add a warning log before the return so developers can see this during debugging — e.g., in ImageSpaceWidget::LoadSettings, just before the existing "if (!imageSpace) return;" call logger::warn (including context like GetEditorID() or the widget's identity) to note that imageSpace is null and LoadSettings was skipped, then keep the return; do not change the existing exception handling or ApplyChanges call.src/WeatherEditor/Weather/ReferenceEffectWidget.cpp (1)
7-12: Set current widget context after a successfulImGui::Begin().Line 7 sets the global current widget before the early-return path on Lines 9-11. If the window is collapsed, this can leave an unintended global context for subsequent WeatherUtils fallback undo tracking.
♻️ Proposed adjustment
- WeatherUtils::SetCurrentWidget(this); ImGui::SetNextWindowSizeConstraints(ImVec2(600, 0), ImVec2(FLT_MAX, FLT_MAX)); if (!ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings | kStickyHeaderFlags)) { ImGui::End(); return; } + WeatherUtils::SetCurrentWidget(this);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp` around lines 7 - 12, WeatherUtils::SetCurrentWidget is being called before the ImGui window is confirmed open, which can leave a stale global widget when ImGui::Begin returns false; move the call so it runs only after ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings | kStickyHeaderFlags) returns true (i.e., after the early-return check), or add a corresponding WeatherUtils::ClearCurrentWidget() before the early return; update references around WeatherUtils::SetCurrentWidget, ImGui::Begin, GetEditorID, and the open flag to ensure the global widget context is only set for windows that are actually open.src/WeatherEditor/Weather/WeatherWidget.cpp (1)
173-176: Code is syntactically correct; no extra closing brace issue exists.The brace depth analysis confirms the tab-bar code at line 175 is properly nested inside
DrawWidget()at depth 1, not at file scope. Line 172 closes an inner block, and line 175'sifstatement is correctly indented as part of the function body. No compilation errors from brace mismatch.However, two separate functional concerns warrant attention:
Search dropdown ImGui ID collision (line 79): The hardcoded
"##SearchDropdown"ID can collide if multipleWeatherWidgetwindows are open simultaneously. Use a unique identifier based on window instance or index.Missing undo snapshot in records auto-apply (line 423): The
ApplyChanges()call should be preceded byEditorWindow::GetSingleton()->PushUndoState()to maintain undo/redo consistency when auto-apply triggers, matching the behavior of manual apply.🤖 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 173 - 176, The hardcoded ImGui ID "##SearchDropdown" in DrawWidget() can collide across multiple WeatherWidget instances—replace it with a unique ID per instance (e.g., append the widget pointer/address or an instance index) so each widget's dropdown uses a unique ImGui identifier; additionally, before automatic ApplyChanges() (the auto-apply path that currently calls ApplyChanges() around line 423), call EditorWindow::GetSingleton()->PushUndoState() to create an undo snapshot so auto-applies match the manual Apply behavior and preserve undo/redo consistency.src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
1-1: Consider shortening the PR title to match repository convention length.The title is conventional, but it exceeds the 50-character limit in the repo guideline. A concise option:
refactor(weather): unify precipitation widget json io.
If this is tied to a tracker item, consider addingRelated to #<id>in the PR body.As per coding guidelines, "Format: type(scope): description" and "Length: 50 characters limit for title, 72 for body".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` at line 1, The PR title exceeds the repo's 50-character convention; rename it to follow "type(scope): description" and keep it ≤50 characters (e.g., "refactor(weather): unify precipitation widget json io"), and if this change is linked to a tracker include "Related to #<id>" in the PR body; update the PR title and body accordingly for the PrecipitationWidget changes so it matches the repository guidelines.
🤖 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/Weather/LensFlareWidget.cpp`:
- Around line 12-16: The call to WeatherUtils::SetCurrentWidget(this) is
happening before ImGui::Begin succeeds and can cause the global current widget
to point to an invalid widget on the early-return path; move the
WeatherUtils::SetCurrentWidget(this) call to immediately after the successful
ImGui::Begin(...) check (i.e., after the if-block that checks Begin and before
any UI code/early returns) so the global pointer is only set when Begin returns
true; update any cleanup/End() paths in LensFlareWidget:: whatever-function
contains this code to ensure SetCurrentWidget is not called when Begin fails.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 5-20: Deserialization currently overwrites
PrecipitationWidget::Settings with struct defaults when JSON keys are missing
(the `settings = js;` assignment), so change deserialization to merge incoming
keys into the existing settings instead of blindly replacing them: parse the
JSON into a temporary (e.g., tempSettings initialized from current Settings),
then for each setting key check js.contains("<key>") and copy only those present
into tempSettings (use the unique names like gravityVelocity, rotationVelocity,
particleSizeX, numSubtexturesX, numSubtexturesY, particleType, particleDensity,
etc.), then assign tempSettings back to settings; additionally add validation in
ApplyChanges() to clamp and validate fields (e.g., ensure
numSubtexturesX/numSubtexturesY >= 1, particleDensity >= 0, particleType within
expected enum range, sizes/velocities within sane ranges) before writing to the
shader runtime to avoid applying malformed or legacy-missing values (use
vanillaSettings values as fallbacks where appropriate).
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp`:
- Around line 102-107: The current assignment replaces missing JSON keys with
struct defaults; instead, initialize settings from vanillaSettings and then
apply only the keys present in js so untouched fields keep the vanilla
values—specifically, in VolumetricLightingWidget.cpp set settings =
vanillaSettings first and then use nlohmann::json::get_to (or an equivalent JSON
merge/patch) to write js into settings so only keys present in js overwrite the
initialized vanillaSettings; update the code paths using the symbols settings,
js, and vanillaSettings accordingly.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 423-425: The auto-apply branch currently calls ApplyChanges()
directly when recordChanged and
EditorWindow::GetSingleton()->settings.autoApplyChanges are true, which bypasses
the undo system; modify the block so you create/capture an undo snapshot before
calling ApplyChanges() (use the same undo/transaction API used elsewhere in this
editor), ensuring the snapshot includes the record being edited and any relevant
state, then call ApplyChanges() and finalize/commit the undo snapshot so
auto-applied edits are properly undoable.
- Around line 79-81: The search dropdown uses a constant ImGui window ID
"##SearchDropdown" which will collide across multiple WeatherWidget instances;
change the window ID to be unique per-widget by incorporating a per-instance
identifier (e.g., the widget's this pointer, a member uint64 id, or
ImGui::GetID/PushID) into the string passed to ImGui::Begin (replace
"##SearchDropdown" with a formatted unique ID), or wrap the call with
ImGui::PushID/PopID using the widget instance identifier so each WeatherWidget
has its own popup state; update the call site in WeatherWidget.cpp where
ImGui::Begin("##SearchDropdown", ...) is used.
---
Outside diff comments:
In `@src/WeatherEditor/Weather/CellLightingWidget.cpp`:
- Around line 207-219: After deserializing into settings (or falling back to
vanillaSettings) but before assigning originalSettings and calling
ApplyChanges(), add a sanitize/clamp pass that validates and clamps any game
lighting fields (e.g., fog, fade, rotation, intensity, color components) to safe
ranges; update the block that currently sets settings from js/vanillaSettings to
run a SanitizedSettings = ClampSettings(settings) (or call a new
ValidateAndClampSettings(settings) helper) and then assign originalSettings =
SanitizedSettings and call ApplyChanges(); keep the existing exception logger
(logger::error with GetEditorID()) but ensure malformed values never get passed
directly into ApplyChanges().
In `@src/WeatherEditor/Weather/LensFlareWidget.cpp`:
- Around line 43-54: The code assigns settings = js and immediately calls
ApplyChanges(), which allows parseable but out-of-range values to be applied;
modify the load path in LensFlareWidget.cpp so that after parsing js you
validate and clamp each configurable field (the members stored in settings)
against the UI slider bounds or use vanillaSettings defaults for missing/invalid
entries, log a warning via logger::warn with GetEditorID() when values were
out-of-range or replaced, only then assign originalSettings = settings and call
ApplyChanges(); ensure the validation logic references the same settings members
used by ApplyChanges() to prevent malformed config propagation.
In `@src/WeatherEditor/Weather/LightingTemplateWidget.cpp`:
- Around line 6-23: Replace the NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE macros for
LightingTemplateWidget::DirectionalColor, LightingTemplateWidget::DALC, and
LightingTemplateWidget::Settings with the corresponding
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT variants so missing JSON keys
fall back to member defaults; then, in LightingTemplateWidget::LoadSettings()
wrap the JSON-to-struct assignment (the code that deserializes into the Settings
instance) in a try-catch that catches nlohmann::json::exception and handles/logs
the error gracefully (e.g., preserve defaults and avoid throwing) to protect
against malformed or partial presets.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp`:
- Around line 88-105: The early-return in ImageSpaceWidget::LoadSettings
silently hides when imageSpace is null; add a warning log before the return so
developers can see this during debugging — e.g., in
ImageSpaceWidget::LoadSettings, just before the existing "if (!imageSpace)
return;" call logger::warn (including context like GetEditorID() or the widget's
identity) to note that imageSpace is null and LoadSettings was skipped, then
keep the return; do not change the existing exception handling or ApplyChanges
call.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Line 1: The PR title exceeds the repo's 50-character convention; rename it to
follow "type(scope): description" and keep it ≤50 characters (e.g.,
"refactor(weather): unify precipitation widget json io"), and if this change is
linked to a tracker include "Related to #<id>" in the PR body; update the PR
title and body accordingly for the PrecipitationWidget changes so it matches the
repository guidelines.
In `@src/WeatherEditor/Weather/ReferenceEffectWidget.cpp`:
- Around line 7-12: WeatherUtils::SetCurrentWidget is being called before the
ImGui window is confirmed open, which can leave a stale global widget when
ImGui::Begin returns false; move the call so it runs only after
ImGui::Begin(GetEditorID().c_str(), &open, ImGuiWindowFlags_NoSavedSettings |
kStickyHeaderFlags) returns true (i.e., after the early-return check), or add a
corresponding WeatherUtils::ClearCurrentWidget() before the early return; update
references around WeatherUtils::SetCurrentWidget, ImGui::Begin, GetEditorID, and
the open flag to ensure the global widget context is only set for windows that
are actually open.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 173-176: The hardcoded ImGui ID "##SearchDropdown" in DrawWidget()
can collide across multiple WeatherWidget instances—replace it with a unique ID
per instance (e.g., append the widget pointer/address or an instance index) so
each widget's dropdown uses a unique ImGui identifier; additionally, before
automatic ApplyChanges() (the auto-apply path that currently calls
ApplyChanges() around line 423), call
EditorWindow::GetSingleton()->PushUndoState() to create an undo snapshot so
auto-applies match the manual Apply behavior and preserve undo/redo consistency.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
added 2 commits
March 4, 2026 16:39
This reverts commit d207348.
The 'fix: compile' commit (9e44d10) removed the closing brace for DrawWidget() while trying to undo damage from the auto-formatter, but also dropped the function's own closing brace. This left the BeginTabBar block at global indentation and made every subsequent function definition appear to be inside DrawWidget, causing cascading compile errors. Added the missing closing brace and re-applied clang-format to restore correct indentation throughout the file.
Contributor
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)
175-175:⚠️ Potential issue | 🔴 CriticalMove
ImGui::End()outside theBeginTabBarconditional to ensure the main window always closes.The main window opened at line 59 with
ImGui::Begin()must be closed regardless of whetherBeginTabBarsucceeds. CurrentlyImGui::End()at line 430 is nested inside theif (ImGui::BeginTabBar(...))block at line 175. IfBeginTabBarreturns false, the entire block is skipped and the window never closes, causing ImGui state corruption.Proposed fix
if (ImGui::BeginTabBar("WeatherSettingsTabs", ImGuiTabBarFlags_None)) { ... ImGui::EndTabBar(); - ImGui::End(); } + ImGui::End(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` at line 175, The ImGui window started with ImGui::Begin() must always be closed: move the ImGui::End() call that currently lives inside the if (ImGui::BeginTabBar("WeatherSettingsTabs", ...)) block so that it executes unconditionally after the tab bar conditional ends; locate the ImGui::Begin() call near the top of WeatherWidget.cpp and the matching ImGui::End() currently nested inside the BeginTabBar block and place that End() immediately after the if-block (or ensure a single End() runs in all code paths) to avoid leaving the main window unclosed.
♻️ Duplicate comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
79-81:⚠️ Potential issue | 🟡 MinorUse a per-widget ID for the search dropdown window.
Line 79 uses a constant
"##SearchDropdown"window ID, so multiple openWeatherWidgetinstances can share popup state/focus unexpectedly.Proposed fix
- if (ImGui::Begin("##SearchDropdown", nullptr, + const std::string dropdownId = std::format("##SearchDropdown_{}", GetEditorID()); + if (ImGui::Begin(dropdownId.c_str(), nullptr, ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoMove | ImGuiWindowFlags_NoResize | ImGuiWindowFlags_NoScrollbar | ImGuiWindowFlags_AlwaysAutoResize | ImGuiWindowFlags_NoSavedSettings)) {#!/bin/bash # Verify the dropdown still uses a constant ImGui window ID. rg -n 'ImGui::Begin\("##SearchDropdown"' src/WeatherEditor/Weather/WeatherWidget.cpp # Expected: no matches after fix.🤖 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 79 - 81, The search dropdown uses a constant ImGui window ID ("##SearchDropdown") which lets multiple WeatherWidget instances share popup state; update the ImGui::Begin call in WeatherWidget (where "##SearchDropdown" is used) to generate a per-instance unique ID (for example by embedding a unique token such as the widget pointer/instance id or using ImGui::GetID with a widget-unique string) so each WeatherWidget gets its own window state; ensure the new ID is used consistently for the matching ImGui::End and any related popup/child calls.
🤖 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`:
- Line 175: The ImGui window started with ImGui::Begin() must always be closed:
move the ImGui::End() call that currently lives inside the if
(ImGui::BeginTabBar("WeatherSettingsTabs", ...)) block so that it executes
unconditionally after the tab bar conditional ends; locate the ImGui::Begin()
call near the top of WeatherWidget.cpp and the matching ImGui::End() currently
nested inside the BeginTabBar block and place that End() immediately after the
if-block (or ensure a single End() runs in all code paths) to avoid leaving the
main window unclosed.
---
Duplicate comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 79-81: The search dropdown uses a constant ImGui window ID
("##SearchDropdown") which lets multiple WeatherWidget instances share popup
state; update the ImGui::Begin call in WeatherWidget (where "##SearchDropdown"
is used) to generate a per-instance unique ID (for example by embedding a unique
token such as the widget pointer/instance id or using ImGui::GetID with a
widget-unique string) so each WeatherWidget gets its own window state; ensure
the new ID is used consistently for the matching ImGui::End and any related
popup/child calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70f048ad-8557-4143-9000-c246b2740c83
📒 Files selected for processing (1)
src/WeatherEditor/Weather/WeatherWidget.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
115-146: Consider centralizing the field mapping to avoid drift.
LoadFromGameSettings()andApplyChanges()duplicate the same field map in opposite directions. A small mapper/helper would reduce maintenance risk when fields evolve.Also applies to: 153-178
🤖 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 115 - 146, LoadFromGameSettings duplicates mapping logic with ApplyChanges; extract a small mapper helper that centralizes conversions between imageSpace->data and the widget's settings so both functions call it. Implement two helper functions (e.g., MapImageSpaceDataToSettings(const ImageSpaceData& src, Settings& dst) and MapSettingsToImageSpaceData(const Settings& src, ImageSpaceData& dst)) that copy hdr.*, cinematic.*, tint.*, and depthOfField.* fields, then replace the inline assignments in ImageSpaceWidget::LoadFromGameSettings and ImageSpaceWidget::ApplyChanges to call these helpers, ensuring all referenced symbols (settings, imageSpace->data, hdrEyeAdaptSpeed, cinematicSaturation, tintColor, dofStrength, etc.) are handled.src/WeatherEditor/Weather/WeatherWidget.cpp (3)
944-946: Inconsistent undo handling across auto-apply paths.This method calls
ApplyChanges()withoutPushUndoState(), whileDrawCloudSettings(line 1148) and the Records tab (line 422) do capture undo state. This creates inconsistent undo behavior—changes in DALC, Atmosphere Colors, Fog, and Basic tabs won't be undoable when auto-apply is enabled.Since this PR is about consolidation, consider making undo handling consistent across all tabs.
🔧 Suggested pattern for all auto-apply blocks
if (changed && EditorWindow::GetSingleton()->settings.autoApplyChanges) { + EditorWindow::GetSingleton()->PushUndoState(this); ApplyChanges(); }Apply similar changes to:
DrawWeatherColorSettings(lines 1005-1007)DrawFogSettings(lines 1293-1295)DrawProperties(lines 1379-1381)🤖 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 944 - 946, The auto-apply path calls ApplyChanges() without recording an undo state, causing inconsistent undo behavior; modify the auto-apply branches in the methods DrawCloudSettings, DrawWeatherColorSettings, DrawFogSettings, DrawProperties (and the DALC/Atmosphere/Fog/Basic tabs where the snippet lives) to call PushUndoState() immediately before ApplyChanges() when EditorWindow::GetSingleton()->settings.autoApplyChanges is true, mirroring the pattern used in the Records tab and existing DrawCloudSettings code so every auto-apply path captures the undo snapshot prior to applying changes.
127-127: Minor: Signed/unsigned comparison.Using
int iwithwidgets.size()(returnssize_t) may trigger compiler warnings.🔧 Suggested fix
- for (int i = 0; i < widgets.size(); i++) { + for (size_t i = 0; i < widgets.size(); i++) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` at line 127, The for-loop uses a signed int for the index while comparing against widgets.size() (unsigned), causing a signed/unsigned comparison warning; change the loop index type to an unsigned type matching widgets.size() (e.g., size_t or decltype(widgets.size())) or use a range-based loop to iterate over widgets in the code around the for (int i = 0; i < widgets.size(); i++) line so the comparison and types are consistent.
2020-2023: Optional: Case-insensitive extension check.The current check won't detect uppercase
.DDSextensions, potentially resulting in malformed paths like"texture.DDS.dds".🔧 Suggested fix
- if (resourcePath.size() < 4 || resourcePath.substr(resourcePath.size() - 4) != ".dds") { + auto endsWithDds = [](const std::string& s) { + if (s.size() < 4) return false; + std::string ext = s.substr(s.size() - 4); + std::transform(ext.begin(), ext.end(), ext.begin(), ::tolower); + return ext == ".dds"; + }; + if (!endsWithDds(resourcePath)) { resourcePath += ".dds"; }🤖 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 2020 - 2023, The extension check for resourcePath is case-sensitive and will miss uppercase variants like ".DDS"; update the logic around resourcePath so the last four characters are compared case-insensitively (e.g., convert resourcePath.substr(resourcePath.size()-4) to lowercase before comparing to ".dds" or compare using a case-insensitive routine) and only append ".dds" when no case-insensitive match exists; preserve the existing behavior of appending to resourcePath when missing.
🤖 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/Weather/ImageSpaceWidget.cpp`:
- Around line 153-178: ApplyChanges() currently writes deserialized values
directly into imageSpace->data (fields like hdr.eyeAdaptSpeed,
hdr.bloomBlurRadius, hdr.bloomThreshold, hdr.bloomScale, hdr.white,
hdr.sunlightScale, hdr.skyScale, cinematic.saturation/brightness/contrast,
tint.color.*, tint.amount, depthOfField.strength/distance/range) without
validation; add sanitization by clamping each value to safe ranges (use sensible
min/max per field), reject or default NaN/inf, ensure non-negative for
radii/strengths/distances, constrain tint color components to [0,1] and
cinematic/hdr params to their expected ranges, and on out-of-range or invalid
values substitute a predefined default (or log a warning) before writing into
imageSpace->data so malformed JSON cannot inject extreme values.
In `@src/WeatherEditor/Weather/LightingTemplateWidget.cpp`:
- Around line 258-270: The deserialized Settings (in the block that merges
vanillaSettings with js and assigns settings) can contain non-finite/malformed
float values that are not validated (only directionalXY/Z are hardened later);
add a ValidateSettings(const Settings& vanilla) method (or function) and call it
immediately after settings is set from JSON and before originalSettings =
settings / ApplyChanges(); ValidateSettings should iterate every float and
float3 member in Settings and replace any non-finite value (use std::isfinite)
with the corresponding value from vanillaSettings (at minimum for all scalar
floats and vec3-like members), ensuring robust fallback and preventing
propagation of bad data into ApplyChanges().
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 169-176: ApplyChanges() is mutating settings via clamping which
makes settings differ from the earlier LoadSettings() snapshot
(originalSettings) and triggers HasUnsavedChanges() immediately; move or
duplicate the normalization so the same clamped values are used when
snapshotting: perform the clamp/validation when loading/parsing settings (e.g.,
in LoadSettings() or a new NormalizeSettings() helper) and assign the normalized
values to both settings and originalSettings, or alternatively apply clamping
only at save/apply time but update originalSettings to the normalized result;
refer to ApplyChanges(), LoadSettings(), settings, and originalSettings to
locate where to adjust behavior.
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp`:
- Around line 102-107: After deserializing merged JSON into a Settings instance
(the variable settings assigned from merged.get<Settings>()), validate and clamp
all numeric fields (e.g., floats/doubles) and reject non-finite values before
calling ApplyChanges(); specifically sanitize any
intensity/scale/range/attenuation-like members to prescribed min/max bounds and
replace NaN/inf with safe defaults, then only pass the validated settings into
ApplyChanges() to prevent runtime crashes from malformed JSON.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp`:
- Around line 115-146: LoadFromGameSettings duplicates mapping logic with
ApplyChanges; extract a small mapper helper that centralizes conversions between
imageSpace->data and the widget's settings so both functions call it. Implement
two helper functions (e.g., MapImageSpaceDataToSettings(const ImageSpaceData&
src, Settings& dst) and MapSettingsToImageSpaceData(const Settings& src,
ImageSpaceData& dst)) that copy hdr.*, cinematic.*, tint.*, and depthOfField.*
fields, then replace the inline assignments in
ImageSpaceWidget::LoadFromGameSettings and ImageSpaceWidget::ApplyChanges to
call these helpers, ensuring all referenced symbols (settings, imageSpace->data,
hdrEyeAdaptSpeed, cinematicSaturation, tintColor, dofStrength, etc.) are
handled.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 944-946: The auto-apply path calls ApplyChanges() without
recording an undo state, causing inconsistent undo behavior; modify the
auto-apply branches in the methods DrawCloudSettings, DrawWeatherColorSettings,
DrawFogSettings, DrawProperties (and the DALC/Atmosphere/Fog/Basic tabs where
the snippet lives) to call PushUndoState() immediately before ApplyChanges()
when EditorWindow::GetSingleton()->settings.autoApplyChanges is true, mirroring
the pattern used in the Records tab and existing DrawCloudSettings code so every
auto-apply path captures the undo snapshot prior to applying changes.
- Line 127: The for-loop uses a signed int for the index while comparing against
widgets.size() (unsigned), causing a signed/unsigned comparison warning; change
the loop index type to an unsigned type matching widgets.size() (e.g., size_t or
decltype(widgets.size())) or use a range-based loop to iterate over widgets in
the code around the for (int i = 0; i < widgets.size(); i++) line so the
comparison and types are consistent.
- Around line 2020-2023: The extension check for resourcePath is case-sensitive
and will miss uppercase variants like ".DDS"; update the logic around
resourcePath so the last four characters are compared case-insensitively (e.g.,
convert resourcePath.substr(resourcePath.size()-4) to lowercase before comparing
to ".dds" or compare using a case-insensitive routine) and only append ".dds"
when no case-insensitive match exists; preserve the existing behavior of
appending to resourcePath when missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c08d5b7-beb4-4ebc-9708-7758ec554158
📒 Files selected for processing (8)
src/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/WeatherEditor/Weather/CellLightingWidget.cpp
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Collaborator
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
45-75: Consider centralizing value ranges used by UI and apply paths.The same numeric bounds are duplicated in
DrawWidget()andApplyChanges(). A shared range table/constexprs would prevent drift and keep UI constraints aligned with runtime sanitization.Also applies to: 161-183
🤖 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 45 - 75, DrawWidget() duplicates numeric bounds used later in ApplyChanges(), causing drift; centralize those ranges into shared constants or a constexpr lookup (e.g., HDR_EYE_ADAPT_MIN/MAX, BLOOM_BLUR_MIN/MAX, DOF_DISTANCE_MIN/MAX, TINT_AMOUNT_MIN/MAX, etc.) and replace literal bounds in PropertyDrawer::DrawFloat and any range-sanitizing logic in ApplyChanges() to use those constants; update usages in ImageSpaceWidget::DrawWidget (the PropertyDrawer::DrawFloat and DrawColor calls) and ImageSpaceWidget::ApplyChanges to reference the shared range table/constexprs so UI and runtime validation remain identical.src/WeatherEditor/Weather/PrecipitationWidget.cpp (2)
130-137: Extract normalization into one helper to avoid driftThe same clamp/normalize block exists in both
LoadSettings()andApplyChanges(). Consolidating it into one helper reduces divergence risk.Refactor sketch
+namespace +{ + void NormalizePrecipitationSettings(PrecipitationWidget::Settings& s) + { + s.numSubtexturesX = std::max(1u, s.numSubtexturesX); + s.numSubtexturesY = std::max(1u, s.numSubtexturesY); + s.particleDensity = std::max(0.0f, s.particleDensity); + s.particleType = std::min(s.particleType, 1u); + s.boxSize = std::max(0.0f, s.boxSize); + s.particleSizeX = std::clamp(s.particleSizeX, 0.0f, 10.0f); + s.particleSizeY = std::clamp(s.particleSizeY, 0.0f, 10.0f); + } +} @@ - // Normalize settings so originalSettings matches the clamped values applied in ApplyChanges - settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX); - settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY); - settings.particleDensity = std::max(0.0f, settings.particleDensity); - settings.particleType = std::min(settings.particleType, 1u); - settings.boxSize = std::max(0.0f, settings.boxSize); - settings.particleSizeX = std::clamp(settings.particleSizeX, 0.0f, 10.0f); - settings.particleSizeY = std::clamp(settings.particleSizeY, 0.0f, 10.0f); + NormalizePrecipitationSettings(settings); @@ - settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX); - settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY); - settings.particleDensity = std::max(0.0f, settings.particleDensity); - settings.particleType = std::min(settings.particleType, 1u); - settings.boxSize = std::max(0.0f, settings.boxSize); - settings.particleSizeX = std::clamp(settings.particleSizeX, 0.0f, 10.0f); - settings.particleSizeY = std::clamp(settings.particleSizeY, 0.0f, 10.0f); + NormalizePrecipitationSettings(settings);Also applies to: 178-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 130 - 137, Extract the repeated clamp/normalize logic into a single helper (e.g., NormalizePrecipitationSettings) and call it from both LoadSettings() and ApplyChanges(); move the block that adjusts settings.numSubtexturesX, settings.numSubtexturesY, settings.particleDensity, settings.particleType, settings.boxSize, settings.particleSizeX, and settings.particleSizeY into that helper (also update the identical block at the other occurrence around the 178-185 area to call the helper), ensure the function takes and mutates the same settings type used by LoadSettings/ApplyChanges and keep behavior identical (same std::max/std::min/std::clamp calls).
1-215: PR metadata suggestion: tighten title and add issue linkageSuggested conventional title (≤50 chars):
refactor(weather): unify precipitation json ioIf this maps to a tracked ticket, add one of:
Addresses #<id>/Related to #<id>in the PR description.As per coding guidelines, "When reviewing PRs, please provide suggestions for: 1. Conventional Commit Titles ... 2. Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 1 - 215, Update the PR metadata: set the PR title to a concise conventional commit style (e.g., "refactor(weather): unify precipitation json io") and add an issue reference in the PR description if this change maps to a tracked ticket (use "Addresses #<id>" or "Related to #<id>"). Ensure the PR description includes the short conventional title and the issue linkage line so reviewers can track the change; no code changes are required in PrecipitationWidget.cpp itself.src/WeatherEditor/Weather/WeatherWidget.cpp (1)
157-159: Capture inherit-all undo state before mutation and independent of auto-apply.For consistency and safer undo semantics, snapshot before
InheritAllFromParent()performs bulk mutations, instead of insideInheritAllFromParent()after changes and only under auto-apply.Also applies to: 1477-1479
🤖 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 157 - 159, Before calling InheritAllFromParent() (triggered by Util::ButtonWithFlash("Inherit All")), create the undo snapshot so the pre-mutation state is preserved regardless of auto-apply; move the undo-capture logic out of InheritAllFromParent() and invoke it immediately when the button is pressed (and similarly for the code paths at lines ~1477-1479), then call InheritAllFromParent() to perform the bulk changes and only afterwards mark/apply changes as appropriate.
🤖 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/Weather/ImageSpaceWidget.cpp`:
- Around line 155-183: ApplyChanges() currently clamps values into local data
(using safeClamp) but leaves the original settings object unchanged, causing
persisted JSON and HasUnsavedChanges() to mismatch applied state; after clamping
each value into data (for HDR, Cinematic, Tint, Depth of Field), write the
sanitized values back into the corresponding settings members (e.g.,
settings.hdrEyeAdaptSpeed, settings.hdrBloomBlurRadius,
settings.cinematicSaturation, settings.tintColor.x/y/z, settings.dofStrength,
etc.) so settings mirrors data; keep using safeClamp/std::isfinite for
sanitization and ensure the same bounds are applied when updating settings to
maintain consistency between imageSpace->data and persisted settings.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 179-180: The current validation only enforces a lower bound on
settings.numSubtexturesX and settings.numSubtexturesY (uint32_t) but later they
are written into signed .i members, allowing large JSON values to overflow;
update the checks where settings.numSubtexturesX and settings.numSubtexturesY
are normalized (the two places currently using std::max(1u, ...)) to also clamp
the values to INT32_MAX (use INT32_MAX from <cstdint>/<climits>) before any
assignment to the signed .i fields so the values are safely converted to int32_t
without overflow.
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp`:
- Around line 105-107: The current use of nlohmann::json::merge_patch on
`merged` (initialized from `vanillaSettings`) will delete keys when `js`
contains explicit nulls, causing silent defaults after deserialization into
`Settings`; replace the single `merged.merge_patch(js)` call with explicit
iteration over `js`'s object members and copy only non-null entries into
`merged` (skip any key where `js[key].is_null()`), so `vanillaSettings` values
are preserved when `js` supplies nulls, then deserialize `merged` into
`Settings` and continue applying `safeClamp`/existing validation.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 74-76: The dropdown is forcing focus every frame via
ImGui::SetNextWindowFocus(), which prevents the ImGui::IsWindowFocused() close
path from working; change the logic in WeatherWidget (where
ImGui::SetNextWindowFocus is called) to only call SetNextWindowFocus once when
the dropdown is opened (e.g., guard it with a one-time "justOpened" boolean or
check the previous open state / ImGui popup-open state) instead of every frame,
and remove or skip the unconditional call in the alternate location (lines
98-101) so the existing !ImGui::IsWindowFocused() close path can detect outside
clicks and close the dropdown normally.
- Around line 1380-1382: The block in DrawProperties currently calls
EditorWindow::GetSingleton()->PushUndoState(this) before ApplyChanges(), causing
duplicate undo snapshots because WeatherUtils property editors already push an
undo on interaction start; remove the PushUndoState(this) call from the
auto-apply path so DrawProperties only calls ApplyChanges(), or alternatively
guard PushUndoState with an "undo already pushed" check, leaving ApplyChanges()
unchanged.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp`:
- Around line 45-75: DrawWidget() duplicates numeric bounds used later in
ApplyChanges(), causing drift; centralize those ranges into shared constants or
a constexpr lookup (e.g., HDR_EYE_ADAPT_MIN/MAX, BLOOM_BLUR_MIN/MAX,
DOF_DISTANCE_MIN/MAX, TINT_AMOUNT_MIN/MAX, etc.) and replace literal bounds in
PropertyDrawer::DrawFloat and any range-sanitizing logic in ApplyChanges() to
use those constants; update usages in ImageSpaceWidget::DrawWidget (the
PropertyDrawer::DrawFloat and DrawColor calls) and
ImageSpaceWidget::ApplyChanges to reference the shared range table/constexprs so
UI and runtime validation remain identical.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 130-137: Extract the repeated clamp/normalize logic into a single
helper (e.g., NormalizePrecipitationSettings) and call it from both
LoadSettings() and ApplyChanges(); move the block that adjusts
settings.numSubtexturesX, settings.numSubtexturesY, settings.particleDensity,
settings.particleType, settings.boxSize, settings.particleSizeX, and
settings.particleSizeY into that helper (also update the identical block at the
other occurrence around the 178-185 area to call the helper), ensure the
function takes and mutates the same settings type used by
LoadSettings/ApplyChanges and keep behavior identical (same
std::max/std::min/std::clamp calls).
- Around line 1-215: Update the PR metadata: set the PR title to a concise
conventional commit style (e.g., "refactor(weather): unify precipitation json
io") and add an issue reference in the PR description if this change maps to a
tracked ticket (use "Addresses #<id>" or "Related to #<id>"). Ensure the PR
description includes the short conventional title and the issue linkage line so
reviewers can track the change; no code changes are required in
PrecipitationWidget.cpp itself.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 157-159: Before calling InheritAllFromParent() (triggered by
Util::ButtonWithFlash("Inherit All")), create the undo snapshot so the
pre-mutation state is preserved regardless of auto-apply; move the undo-capture
logic out of InheritAllFromParent() and invoke it immediately when the button is
pressed (and similarly for the code paths at lines ~1477-1479), then call
InheritAllFromParent() to perform the bulk changes and only afterwards
mark/apply changes as appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15db01d3-0946-452b-8916-7e3c87741313
📒 Files selected for processing (5)
src/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
Comment on lines
+155
to
+183
| // Clamp/sanitize all fields; reject non-finite values by substituting the low bound | ||
| auto safeClamp = [](float v, float lo, float hi) { | ||
| return std::isfinite(v) ? std::clamp(v, lo, hi) : lo; | ||
| }; | ||
|
|
||
| // HDR | ||
| data.hdr.eyeAdaptSpeed = safeClamp(settings.hdrEyeAdaptSpeed, 0.0f, 10.0f); | ||
| data.hdr.bloomBlurRadius = safeClamp(settings.hdrBloomBlurRadius, 0.0f, 10.0f); | ||
| data.hdr.bloomThreshold = safeClamp(settings.hdrBloomThreshold, 0.0f, 10.0f); | ||
| data.hdr.bloomScale = safeClamp(settings.hdrBloomScale, 0.0f, 10.0f); | ||
| data.hdr.white = safeClamp(settings.hdrWhite, 0.0f, 10.0f); | ||
| data.hdr.sunlightScale = safeClamp(settings.hdrSunlightScale, 0.0f, 50.0f); | ||
| data.hdr.skyScale = safeClamp(settings.hdrSkyScale, 0.0f, 10.0f); | ||
|
|
||
| // Cinematic | ||
| data.cinematic.saturation = safeClamp(settings.cinematicSaturation, 0.0f, 2.0f); | ||
| data.cinematic.brightness = safeClamp(settings.cinematicBrightness, 0.0f, 2.0f); | ||
| data.cinematic.contrast = safeClamp(settings.cinematicContrast, 0.0f, 2.0f); | ||
|
|
||
| // Tint | ||
| data.tint.color.red = safeClamp(settings.tintColor.x, 0.0f, 1.0f); | ||
| data.tint.color.green = safeClamp(settings.tintColor.y, 0.0f, 1.0f); | ||
| data.tint.color.blue = safeClamp(settings.tintColor.z, 0.0f, 1.0f); | ||
| data.tint.amount = safeClamp(settings.tintAmount, 0.0f, 1.0f); | ||
|
|
||
| // Depth of Field | ||
| data.depthOfField.strength = safeClamp(settings.dofStrength, 0.0f, 10.0f); | ||
| data.depthOfField.distance = safeClamp(settings.dofDistance, 0.0f, 10000.0f); | ||
| data.depthOfField.range = safeClamp(settings.dofRange, 0.0f, 10000.0f); |
Contributor
There was a problem hiding this comment.
Normalize settings after clamp to keep persisted and applied state consistent.
ApplyChanges() clamps before writing to imageSpace->data, but settings keeps original out-of-range values. That can leave JSON persistence and HasUnsavedChanges() out of sync with what is actually applied.
Proposed fix
// HDR
- data.hdr.eyeAdaptSpeed = safeClamp(settings.hdrEyeAdaptSpeed, 0.0f, 10.0f);
- data.hdr.bloomBlurRadius = safeClamp(settings.hdrBloomBlurRadius, 0.0f, 10.0f);
- data.hdr.bloomThreshold = safeClamp(settings.hdrBloomThreshold, 0.0f, 10.0f);
- data.hdr.bloomScale = safeClamp(settings.hdrBloomScale, 0.0f, 10.0f);
- data.hdr.white = safeClamp(settings.hdrWhite, 0.0f, 10.0f);
- data.hdr.sunlightScale = safeClamp(settings.hdrSunlightScale, 0.0f, 50.0f);
- data.hdr.skyScale = safeClamp(settings.hdrSkyScale, 0.0f, 10.0f);
+ settings.hdrEyeAdaptSpeed = safeClamp(settings.hdrEyeAdaptSpeed, 0.0f, 10.0f);
+ settings.hdrBloomBlurRadius = safeClamp(settings.hdrBloomBlurRadius, 0.0f, 10.0f);
+ settings.hdrBloomThreshold = safeClamp(settings.hdrBloomThreshold, 0.0f, 10.0f);
+ settings.hdrBloomScale = safeClamp(settings.hdrBloomScale, 0.0f, 10.0f);
+ settings.hdrWhite = safeClamp(settings.hdrWhite, 0.0f, 10.0f);
+ settings.hdrSunlightScale = safeClamp(settings.hdrSunlightScale, 0.0f, 50.0f);
+ settings.hdrSkyScale = safeClamp(settings.hdrSkyScale, 0.0f, 10.0f);
+ data.hdr.eyeAdaptSpeed = settings.hdrEyeAdaptSpeed;
+ data.hdr.bloomBlurRadius = settings.hdrBloomBlurRadius;
+ data.hdr.bloomThreshold = settings.hdrBloomThreshold;
+ data.hdr.bloomScale = settings.hdrBloomScale;
+ data.hdr.white = settings.hdrWhite;
+ data.hdr.sunlightScale = settings.hdrSunlightScale;
+ data.hdr.skyScale = settings.hdrSkyScale;
// Cinematic
- data.cinematic.saturation = safeClamp(settings.cinematicSaturation, 0.0f, 2.0f);
- data.cinematic.brightness = safeClamp(settings.cinematicBrightness, 0.0f, 2.0f);
- data.cinematic.contrast = safeClamp(settings.cinematicContrast, 0.0f, 2.0f);
+ settings.cinematicSaturation = safeClamp(settings.cinematicSaturation, 0.0f, 2.0f);
+ settings.cinematicBrightness = safeClamp(settings.cinematicBrightness, 0.0f, 2.0f);
+ settings.cinematicContrast = safeClamp(settings.cinematicContrast, 0.0f, 2.0f);
+ data.cinematic.saturation = settings.cinematicSaturation;
+ data.cinematic.brightness = settings.cinematicBrightness;
+ data.cinematic.contrast = settings.cinematicContrast;
// Tint
- data.tint.color.red = safeClamp(settings.tintColor.x, 0.0f, 1.0f);
- data.tint.color.green = safeClamp(settings.tintColor.y, 0.0f, 1.0f);
- data.tint.color.blue = safeClamp(settings.tintColor.z, 0.0f, 1.0f);
- data.tint.amount = safeClamp(settings.tintAmount, 0.0f, 1.0f);
+ settings.tintColor.x = safeClamp(settings.tintColor.x, 0.0f, 1.0f);
+ settings.tintColor.y = safeClamp(settings.tintColor.y, 0.0f, 1.0f);
+ settings.tintColor.z = safeClamp(settings.tintColor.z, 0.0f, 1.0f);
+ settings.tintAmount = safeClamp(settings.tintAmount, 0.0f, 1.0f);
+ data.tint.color.red = settings.tintColor.x;
+ data.tint.color.green = settings.tintColor.y;
+ data.tint.color.blue = settings.tintColor.z;
+ data.tint.amount = settings.tintAmount;
// Depth of Field
- data.depthOfField.strength = safeClamp(settings.dofStrength, 0.0f, 10.0f);
- data.depthOfField.distance = safeClamp(settings.dofDistance, 0.0f, 10000.0f);
- data.depthOfField.range = safeClamp(settings.dofRange, 0.0f, 10000.0f);
+ settings.dofStrength = safeClamp(settings.dofStrength, 0.0f, 10.0f);
+ settings.dofDistance = safeClamp(settings.dofDistance, 0.0f, 10000.0f);
+ settings.dofRange = safeClamp(settings.dofRange, 0.0f, 10000.0f);
+ data.depthOfField.strength = settings.dofStrength;
+ data.depthOfField.distance = settings.dofDistance;
+ data.depthOfField.range = settings.dofRange;As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 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 155 - 183,
ApplyChanges() currently clamps values into local data (using safeClamp) but
leaves the original settings object unchanged, causing persisted JSON and
HasUnsavedChanges() to mismatch applied state; after clamping each value into
data (for HDR, Cinematic, Tint, Depth of Field), write the sanitized values back
into the corresponding settings members (e.g., settings.hdrEyeAdaptSpeed,
settings.hdrBloomBlurRadius, settings.cinematicSaturation,
settings.tintColor.x/y/z, settings.dofStrength, etc.) so settings mirrors data;
keep using safeClamp/std::isfinite for sanitization and ensure the same bounds
are applied when updating settings to maintain consistency between
imageSpace->data and persisted settings.
Comment on lines
+179
to
+180
| settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX); | ||
| settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if file exists and get its size
if [ ! -f "src/WeatherEditor/Weather/PrecipitationWidget.cpp" ]; then
echo "File not found"
exit 1
fi
echo "=== PrecipitationWidget.h - checking Settings struct ==="
cat -n src/WeatherEditor/Weather/PrecipitationWidget.h | head -120
echo ""
echo "=== PrecipitationWidget.cpp - Lines 170-210 (clamping and write logic) ==="
cat -n src/WeatherEditor/Weather/PrecipitationWidget.cpp | sed -n '170,210p'
echo ""
echo "=== Search for runtime.data definition ==="
rg -n "struct.*Data|union.*Data" src/WeatherEditor/ -A5 | head -40
echo ""
echo "=== Search for DataID enum ==="
rg -n "enum.*DataID|kNumSubtexturesX|kNumSubtexturesY" src/WeatherEditor/ -B2 -A2 | head -50Repository: doodlum/skyrim-community-shaders
Length of output: 4536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for runtime.data union/struct definition in Skyrim SDK headers ==="
find . -name "*.h" -o -name "*.hpp" | xargs rg -l "BGSShaderParticleGeometryData|struct.*runtime|union.*Data" 2>/dev/null | head -20
echo ""
echo "=== Look for existing upper-bound clamping patterns with subtextures ==="
rg -n "std::clamp.*numSubtextures|numSubtextures.*std::clamp" --type cpp
echo ""
echo "=== Check other Widget implementations for similar validation patterns ==="
ls -la src/WeatherEditor/Weather/ | grep -i widget
echo ""
echo "=== Check LoadSettings for subtexture validation (lines around 100-140) ==="
if [ -f "src/WeatherEditor/Weather/PrecipitationWidget.cpp" ]; then
wc -l src/WeatherEditor/Weather/PrecipitationWidget.cpp
sed -n '100,145p' src/WeatherEditor/Weather/PrecipitationWidget.cpp | cat -n
fiRepository: doodlum/skyrim-community-shaders
Length of output: 277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== PrecipitationWidget.cpp LoadSettings method ==="
sed -n '100,172p' src/WeatherEditor/Weather/PrecipitationWidget.cpp | cat -n
echo ""
echo "=== Check if JSON parsing could produce > INT_MAX values ==="
rg -n "LoadSettings|json|parse" src/WeatherEditor/Weather/PrecipitationWidget.cpp | head -20
echo ""
echo "=== Check for any use of numeric_limits or INT_MAX in similar validation ==="
rg -n "numeric_limits|INT_MAX|INT32_MAX" src/WeatherEditor/ --type cpp | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 4031
Clamp subtexture counts to prevent signed integer overflow when writing to game data
Lines 179–180 and 132–133 validate only a lower bound. Since numSubtexturesX/Y are uint32_t (lines 39–40), but written to signed .i members (lines 197–198), large JSON values can overflow. Clamp to INT32_MAX before assignment.
Proposed fix
void PrecipitationWidget::LoadSettings()
{
if (!precipitation)
@@ -19,8 +19,10 @@ void PrecipitationWidget::LoadSettings()
}
}
+ constexpr uint32_t kMaxSignedInt = static_cast<uint32_t>(std::numeric_limits<int32_t>::max());
settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX);
+ settings.numSubtexturesX = std::min(settings.numSubtexturesX, kMaxSignedInt);
settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY);
+ settings.numSubtexturesY = std::min(settings.numSubtexturesY, kMaxSignedInt);
settings.particleDensity = std::max(0.0f, settings.particleDensity);
settings.particleType = std::min(settings.particleType, 1u);
settings.boxSize = std::max(0.0f, settings.boxSize);
@@ -25,8 +27,10 @@ void PrecipitationWidget::ApplyChanges()
if (!precipitation)
return;
+ constexpr uint32_t kMaxSignedInt = static_cast<uint32_t>(std::numeric_limits<int32_t>::max());
settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX);
+ settings.numSubtexturesX = std::min(settings.numSubtexturesX, kMaxSignedInt);
settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY);
+ settings.numSubtexturesY = std::min(settings.numSubtexturesY, kMaxSignedInt);
settings.particleDensity = std::max(0.0f, settings.particleDensity);
settings.particleType = std::min(settings.particleType, 1u);
settings.boxSize = std::max(0.0f, settings.boxSize);Also applies to lines 132–133.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 179 - 180,
The current validation only enforces a lower bound on settings.numSubtexturesX
and settings.numSubtexturesY (uint32_t) but later they are written into signed
.i members, allowing large JSON values to overflow; update the checks where
settings.numSubtexturesX and settings.numSubtexturesY are normalized (the two
places currently using std::max(1u, ...)) to also clamp the values to INT32_MAX
(use INT32_MAX from <cstdint>/<climits>) before any assignment to the signed .i
fields so the values are safely converted to int32_t without overflow.
Comment on lines
+105
to
+107
| nlohmann::json merged = vanillaSettings; | ||
| merged.merge_patch(js); | ||
| settings = merged.get<Settings>(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nlohmann json merge_patch RFC 7396 null semantics key deletion
💡 Result:
Per RFC 7396 (JSON Merge Patch), when the patch is an object and a member’s value is null, that member is deleted from the target object (if it exists). If a member is absent from the patch, it is left unchanged. (datatracker.ietf.org)
nlohmann::json::merge_patch() implements this RFC behavior: in the project’s own example, applying a patch containing "f": null removes key f from the nested object. (github.com)
Key implications (often the “gotcha”):
- You cannot use merge-patch to mean “set this field to JSON null” inside an object, because
nullmeans remove the key. (datatracker.ietf.org) - If the entire patch value is not an object (e.g., the patch is
null, a string, an array, etc.), the result is replace the entire target with the patch. (datatracker.ietf.org)
Citations:
- 1: https://datatracker.ietf.org/doc/rfc7396/?utm_source=openai
- 2: https://github.com/nlohmann/json?utm_source=openai
- 3: https://datatracker.ietf.org/doc/rfc7396/?utm_source=openai
- 4: https://datatracker.ietf.org/doc/rfc7396/?utm_source=openai
🏁 Script executed:
# Find and examine the file
fd VolumetricLightingWidget.cppRepository: doodlum/skyrim-community-shaders
Length of output: 130
🏁 Script executed:
# Read the specific file around the mentioned lines
wc -l $(fd VolumetricLightingWidget.cpp)Repository: doodlum/skyrim-community-shaders
Length of output: 134
🏁 Script executed:
# Read the file around the mentioned lines with context
sed -n '95,115p' src/WeatherEditor/Weather/VolumetricLightingWidget.cpp | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 1014
🏁 Script executed:
# Get more context to understand the function and data flow
sed -n '85,125p' src/WeatherEditor/Weather/VolumetricLightingWidget.cpp | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 2394
🏁 Script executed:
# Find Settings struct definition
rg "struct Settings" src/WeatherEditor -A 15Repository: doodlum/skyrim-community-shaders
Length of output: 10524
🏁 Script executed:
# Get exact line numbers - search for the merge_patch call
rg -n "merge_patch" src/WeatherEditor/Weather/VolumetricLightingWidget.cppRepository: doodlum/skyrim-community-shaders
Length of output: 106
🏁 Script executed:
# Check where js is initialized/populated to see if there's upstream validation
rg -n "js =" src/WeatherEditor/Weather/VolumetricLightingWidget.cpp -B 3 -A 1Repository: doodlum/skyrim-community-shaders
Length of output: 190
🏁 Script executed:
# Check the full LoadSettings function to see all context
sed -n '98,145p' src/WeatherEditor/Weather/VolumetricLightingWidget.cpp | cat -nRepository: doodlum/skyrim-community-shaders
Length of output: 2903
Add explicit validation to prevent null-value deletion during JSON merge.
On Line 106, merge_patch treats null values as key deletion per RFC 7396. When merged JSON contains explicit nulls (e.g., legacy or malformed configurations), deleted keys deserialize using struct defaults (e.g., intensity = 1.0f), which may differ from vanillaSettings values, causing silent behavior drift. The existing safeClamp validation only catches non-finite values, not finite struct defaults that silently diverge from vanilla.
Replace the merge_patch call with explicit object validation and conditional iteration to skip null values:
Proposed fix
- nlohmann::json merged = vanillaSettings;
- merged.merge_patch(js);
- settings = merged.get<Settings>();
+ nlohmann::json merged = vanillaSettings;
+ if (!js.is_object()) {
+ throw std::runtime_error("VolumetricLighting JSON payload must be an object");
+ }
+ for (auto it = js.begin(); it != js.end(); ++it) {
+ if (!it.value().is_null()) {
+ merged[it.key()] = it.value();
+ }
+ }
+ settings = merged.get<Settings>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp` around lines 105 -
107, The current use of nlohmann::json::merge_patch on `merged` (initialized
from `vanillaSettings`) will delete keys when `js` contains explicit nulls,
causing silent defaults after deserialization into `Settings`; replace the
single `merged.merge_patch(js)` call with explicit iteration over `js`'s object
members and copy only non-null entries into `merged` (skip any key where
`js[key].is_null()`), so `vanillaSettings` values are preserved when `js`
supplies nulls, then deserialize `merged` into `Settings` and continue applying
`safeClamp`/existing validation.
Comment on lines
+74
to
+76
| ImGui::SetNextWindowPos(ImVec2(ImGui::GetCursorScreenPos().x, ImGui::GetCursorScreenPos().y)); | ||
| ImGui::SetNextWindowSize(ImVec2(200.0f * 1.5f, 0)); | ||
| ImGui::SetNextWindowFocus(); |
Contributor
There was a problem hiding this comment.
Dropdown focus is being force-set every frame, which breaks the outside-click close behavior.
Line 76 calls ImGui::SetNextWindowFocus() unconditionally, so the !ImGui::IsWindowFocused() close path at Line 99 is effectively neutralized.
Suggested fix
- ImGui::SetNextWindowFocus();
+ // Do not force focus every frame; allows outside-click dismissal to work.Also applies to: 98-101
🤖 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 74 - 76, The
dropdown is forcing focus every frame via ImGui::SetNextWindowFocus(), which
prevents the ImGui::IsWindowFocused() close path from working; change the logic
in WeatherWidget (where ImGui::SetNextWindowFocus is called) to only call
SetNextWindowFocus once when the dropdown is opened (e.g., guard it with a
one-time "justOpened" boolean or check the previous open state / ImGui
popup-open state) instead of every frame, and remove or skip the unconditional
call in the alternate location (lines 98-101) so the existing
!ImGui::IsWindowFocused() close path can detect outside clicks and close the
dropdown normally.
Comment on lines
1380
to
1382
| if (changed && EditorWindow::GetSingleton()->settings.autoApplyChanges) { | ||
| EditorWindow::GetSingleton()->PushUndoState(this); | ||
| ApplyChanges(); |
Contributor
There was a problem hiding this comment.
Avoid double undo snapshots in DrawProperties auto-apply path.
This block pushes undo again even though property editors in WeatherUtils already snapshot undo on interaction start (e.g., color/float paths), causing duplicate undo entries and faster stack churn.
Suggested fix
if (changed && EditorWindow::GetSingleton()->settings.autoApplyChanges) {
- EditorWindow::GetSingleton()->PushUndoState(this);
ApplyChanges();
}🤖 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 1380 - 1382, The
block in DrawProperties currently calls
EditorWindow::GetSingleton()->PushUndoState(this) before ApplyChanges(), causing
duplicate undo snapshots because WeatherUtils property editors already push an
undo on interaction start; remove the PushUndoState(this) call from the
auto-apply path so DrawProperties only calls ApplyChanges(), or alternatively
guard PushUndoState with an "undo already pushed" check, leaving ApplyChanges()
unchanged.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
160-183:⚠️ Potential issue | 🟡 MinorSanitized runtime values are not written back to
settings.This keeps persisted JSON /
HasUnsavedChanges()potentially out of sync with what is actually applied.Suggested fix
- data.hdr.eyeAdaptSpeed = safeClamp(settings.hdrEyeAdaptSpeed, 0.0f, 10.0f); + settings.hdrEyeAdaptSpeed = safeClamp(settings.hdrEyeAdaptSpeed, 0.0f, 10.0f); + data.hdr.eyeAdaptSpeed = settings.hdrEyeAdaptSpeed; ... - data.tint.amount = safeClamp(settings.tintAmount, 0.0f, 1.0f); + settings.tintAmount = safeClamp(settings.tintAmount, 0.0f, 1.0f); + data.tint.amount = settings.tintAmount;As per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
🤖 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 160 - 183, The code clamps runtime values into `data` but does not write the sanitized values back into `settings`, leaving persisted JSON and `HasUnsavedChanges()` out of sync; update the same fields on the `settings` object after clamping (for every field set on `data` such as `hdrEyeAdaptSpeed`, `hdrBloomBlurRadius`, `hdrBloomThreshold`, `hdrBloomScale`, `hdrWhite`, `hdrSunlightScale`, `hdrSkyScale`, `cinematicSaturation`, `cinematicBrightness`, `cinematicContrast`, `tintColor.x/y/z`, `tintAmount`, `dofStrength`, `dofDistance`, `dofRange`) so that `settings` contains the sanitized values, and ensure any JSON persistence or `HasUnsavedChanges()` logic will reflect the applied/clamped values.src/WeatherEditor/Weather/WeatherWidget.cpp (1)
74-77:⚠️ Potential issue | 🟡 MinorDropdown focus is still forced every frame, so outside-click close stays unreliable.
Line 76 re-focuses the dropdown continuously, which undermines the close check on Line 99.
Suggested fix
- ImGui::SetNextWindowFocus(); + if (searchDropdownJustOpened) { + ImGui::SetNextWindowFocus(); + searchDropdownJustOpened = false; + }Also applies to: 98-101
🤖 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 74 - 77, The code calls ImGui::SetNextWindowFocus every frame which forces focus and prevents reliable outside-click closing; change logic to call ImGui::SetNextWindowFocus only once when the dropdown is opened (e.g., on the state transition that sets the dropdown open flag) instead of every frame, and remove or gate the continuous calls to ImGui::SetNextWindowFocus at the other occurrence (the block around ImGui::PushStyleVar/SetNextWindowSize); tie the focus call to your dropdown-open boolean (or a one-time "requestFocus" flag) so subsequent frames do not refocus the window.src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
131-132:⚠️ Potential issue | 🟠 MajorClamp subtexture counts to
int32_tmax before writing.iruntime fields.Current normalization only enforces a lower bound. Very large JSON values can overflow/truncate when assigned to
runtime.data[..].i.Suggested fix
+ constexpr uint32_t kMaxI32 = static_cast<uint32_t>(std::numeric_limits<int32_t>::max()); - settings.numSubtexturesX = std::max(1u, settings.numSubtexturesX); - settings.numSubtexturesY = std::max(1u, settings.numSubtexturesY); + settings.numSubtexturesX = std::clamp(settings.numSubtexturesX, 1u, kMaxI32); + settings.numSubtexturesY = std::clamp(settings.numSubtexturesY, 1u, kMaxI32); ... - runtime.data[static_cast<uint32_t>(DataID::kNumSubtexturesX)].i = settings.numSubtexturesX; - runtime.data[static_cast<uint32_t>(DataID::kNumSubtexturesY)].i = settings.numSubtexturesY; + runtime.data[static_cast<uint32_t>(DataID::kNumSubtexturesX)].i = static_cast<int32_t>(settings.numSubtexturesX); + runtime.data[static_cast<uint32_t>(DataID::kNumSubtexturesY)].i = static_cast<int32_t>(settings.numSubtexturesY);Use this script to verify field types and assignment sites:
#!/bin/bash set -euo pipefail echo "=== Locate subtexture runtime writes ===" rg -n -C2 'kNumSubtexturesX|kNumSubtexturesY|runtime\.data\[.*\]\.i' src/WeatherEditor/Weather/PrecipitationWidget.cpp echo echo "=== Locate Data slot type definitions/usages ===" rg -n -C3 'BGSShaderParticleGeometryData|union|struct|\.i\b' src --type cpp --type h --type hppAs per coding guidelines "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".
Also applies to: 179-180, 197-198
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp` around lines 131 - 132, The code currently only enforces a lower bound for settings.numSubtexturesX/Y (in PrecipitationWidget.cpp) which can still be extremely large and overflow/truncate when later written into runtime.data[...].i (the integer slot, referenced by kNumSubtexturesX/kNumSubtexturesY); fix by clamping settings.numSubtexturesX and settings.numSubtexturesY to int32_t maximum before any assignment into runtime.data[].i (use std::min(settings.numSubtexturesX, static_cast<uint32_t>(std::numeric_limits<int32_t>::max())) or equivalent) and then cast to int32_t when writing to runtime.data[..].i so the stored value cannot overflow the .i union field. Ensure the same clamp is applied at all assignment sites that write into runtime.data[..].i (including the instances around the other mentioned lines).src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (1)
105-107:⚠️ Potential issue | 🟠 Major
merge_patch+_WITH_DEFAULTcan still drift from vanilla when payload contains explicitnull.
merge_patchremoves null-valued keys; then deserialization fills missing members from struct defaults (finite), which bypasses your current fallback logic.Suggested fix
nlohmann::json merged = vanillaSettings; - merged.merge_patch(js); + if (!js.is_object()) { + throw std::runtime_error("VolumetricLighting settings JSON must be an object"); + } + for (auto it = js.begin(); it != js.end(); ++it) { + if (!it.value().is_null()) { + merged[it.key()] = it.value(); + } + } settings = merged.get<Settings>();For nlohmann::json merge_patch (RFC 7396), does a member with value null delete that key from the target object? Also, with NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT, how are missing keys populated during from_json?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp` around lines 105 - 107, The current use of merged.merge_patch(js) removes keys when js contains explicit null, so deserialization later fills those missing members from struct defaults (via NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT) and you lose the intended vanilla fallback; after performing merged.merge_patch(js) (vars: merged, vanillaSettings, js, Settings), iterate the keys in vanillaSettings and for any key missing from merged set merged[key] = vanillaSettings[key] (thereby restoring vanilla values for keys deleted by null in js) before calling settings = merged.get<Settings>(); This preserves explicit-null semantics while ensuring fallback to vanillaSettings instead of struct defaults.
🧹 Nitpick comments (1)
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
1-1: Optional: tighten the PR title to match repo commit-title constraints.Current title is clear, but it appears over the 50-char title target. Consider:
refactor(weather-editor): unify widget json io.
If there is a tracking issue, addRelated to #<id>in the PR description.As per coding guidelines "When reviewing PRs, please provide suggestions for: Conventional Commit Titles ... Length: 50 characters ... and Issue References ...".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/WeatherEditor/Weather/WeatherWidget.cpp` at line 1, PR title exceeds the 50-character conventional-commit target; update the PR/commit title to a shorter, conventional form (for example "refactor(weather-editor): unify widget json io") and, if applicable, add a tracking issue reference like "Related to #<id>" in the PR description to satisfy the repo's commit-title and issue-reference guidelines.
🤖 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/Weather/WeatherWidget.cpp`:
- Around line 419-421: The undo snapshot is taken after mutating settings, so
PushUndoState (which snapshots widget->js) can capture a stale state; before any
auto-apply path (where EditorWindow::GetSingleton()->settings.autoApplyChanges
is checked and ApplyChanges is called) capture a pre-edit snapshot: call
EditorWindow::GetSingleton()->PushUndoState(this) before you mutate
settings/other fields or, better, extend PushUndoState to also copy the current
settings state (or add an overload like PushUndoState(this, preEditSettings))
and use that pre-edit snapshot in the auto-apply flows (the branches that call
ApplyChanges and PushUndoState, e.g. the blocks around ApplyChanges,
settings.autoApplyChanges, PushUndoState, and widget->js) so the undo history
reflects the exact state prior to the mutation.
---
Duplicate comments:
In `@src/WeatherEditor/Weather/ImageSpaceWidget.cpp`:
- Around line 160-183: The code clamps runtime values into `data` but does not
write the sanitized values back into `settings`, leaving persisted JSON and
`HasUnsavedChanges()` out of sync; update the same fields on the `settings`
object after clamping (for every field set on `data` such as `hdrEyeAdaptSpeed`,
`hdrBloomBlurRadius`, `hdrBloomThreshold`, `hdrBloomScale`, `hdrWhite`,
`hdrSunlightScale`, `hdrSkyScale`, `cinematicSaturation`, `cinematicBrightness`,
`cinematicContrast`, `tintColor.x/y/z`, `tintAmount`, `dofStrength`,
`dofDistance`, `dofRange`) so that `settings` contains the sanitized values, and
ensure any JSON persistence or `HasUnsavedChanges()` logic will reflect the
applied/clamped values.
In `@src/WeatherEditor/Weather/PrecipitationWidget.cpp`:
- Around line 131-132: The code currently only enforces a lower bound for
settings.numSubtexturesX/Y (in PrecipitationWidget.cpp) which can still be
extremely large and overflow/truncate when later written into
runtime.data[...].i (the integer slot, referenced by
kNumSubtexturesX/kNumSubtexturesY); fix by clamping settings.numSubtexturesX and
settings.numSubtexturesY to int32_t maximum before any assignment into
runtime.data[].i (use std::min(settings.numSubtexturesX,
static_cast<uint32_t>(std::numeric_limits<int32_t>::max())) or equivalent) and
then cast to int32_t when writing to runtime.data[..].i so the stored value
cannot overflow the .i union field. Ensure the same clamp is applied at all
assignment sites that write into runtime.data[..].i (including the instances
around the other mentioned lines).
In `@src/WeatherEditor/Weather/VolumetricLightingWidget.cpp`:
- Around line 105-107: The current use of merged.merge_patch(js) removes keys
when js contains explicit null, so deserialization later fills those missing
members from struct defaults (via
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT) and you lose the intended
vanilla fallback; after performing merged.merge_patch(js) (vars: merged,
vanillaSettings, js, Settings), iterate the keys in vanillaSettings and for any
key missing from merged set merged[key] = vanillaSettings[key] (thereby
restoring vanilla values for keys deleted by null in js) before calling settings
= merged.get<Settings>(); This preserves explicit-null semantics while ensuring
fallback to vanillaSettings instead of struct defaults.
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Around line 74-77: The code calls ImGui::SetNextWindowFocus every frame which
forces focus and prevents reliable outside-click closing; change logic to call
ImGui::SetNextWindowFocus only once when the dropdown is opened (e.g., on the
state transition that sets the dropdown open flag) instead of every frame, and
remove or gate the continuous calls to ImGui::SetNextWindowFocus at the other
occurrence (the block around ImGui::PushStyleVar/SetNextWindowSize); tie the
focus call to your dropdown-open boolean (or a one-time "requestFocus" flag) so
subsequent frames do not refocus the window.
---
Nitpick comments:
In `@src/WeatherEditor/Weather/WeatherWidget.cpp`:
- Line 1: PR title exceeds the 50-character conventional-commit target; update
the PR/commit title to a shorter, conventional form (for example
"refactor(weather-editor): unify widget json io") and, if applicable, add a
tracking issue reference like "Related to #<id>" in the PR description to
satisfy the repo's commit-title and issue-reference guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e5c24fd-8ef7-4873-8995-7afcb7e9a586
📒 Files selected for processing (5)
src/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
Comment on lines
419
to
421
| if (recordChanged && EditorWindow::GetSingleton()->settings.autoApplyChanges) { | ||
| EditorWindow::GetSingleton()->PushUndoState(this); | ||
| ApplyChanges(); |
Contributor
There was a problem hiding this comment.
Undo snapshots in these auto-apply paths can collapse to stale state.
PushUndoState() snapshots widget->js, while these flows mutate settings and immediately apply. Without a guaranteed pre-edit snapshot source, repeated edits can produce duplicate/incorrect undo history.
Suggested direction
+ // capture pre-edit settings before mutating controls in this section
+ const Settings preEdit = settings;
...
if (changed && editorWindow->settings.autoApplyChanges) {
- editorWindow->PushUndoState(this);
+ const auto jsBackup = js;
+ js = preEdit; // push true pre-edit state
+ editorWindow->PushUndoState(this);
+ js = jsBackup;
ApplyChanges();
}Also applies to: 942-944, 1004-1006, 1293-1295, 1380-1382, 1477-1479
🤖 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 419 - 421, The undo
snapshot is taken after mutating settings, so PushUndoState (which snapshots
widget->js) can capture a stale state; before any auto-apply path (where
EditorWindow::GetSingleton()->settings.autoApplyChanges is checked and
ApplyChanges is called) capture a pre-edit snapshot: call
EditorWindow::GetSingleton()->PushUndoState(this) before you mutate
settings/other fields or, better, extend PushUndoState to also copy the current
settings state (or add an overload like PushUndoState(this, preEditSettings))
and use that pre-edit snapshot in the auto-apply flows (the branches that call
ApplyChanges and PushUndoState, e.g. the blocks around ApplyChanges,
settings.autoApplyChanges, PushUndoState, and widget->js) so the undo history
reflects the exact state prior to the mutation.
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
1 participant
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.
The aim of this PR is not to change any functionality, but to clean up the code and reduce the amount of lines used, as well as removing dead code from the weather widgets. Compiles fine and no obvious bugs.
Summary by CodeRabbit
Refactor
Bug Fixes