feat: weather and imagespace editor#1630
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds a Weather Editor subsystem: a weather-variable registry, WeatherManager with per-weather persistence and runtime updates, a WeatherEditor feature and EditorWindow UI with many weather-related widgets, serialization/utilities, and integration into feature/menu/state flows. No existing public APIs were removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Editor
participant Editor as EditorWindow
participant Widget as Widget
participant WeatherMgr as WeatherManager
participant GlobalReg as GlobalWeatherRegistry
participant Disk as Disk (Weathers/*.json)
User->>Editor: interact / Draw()
Editor->>Widget: DrawWidget()
User->>Widget: change values / ApplyChanges()
Widget->>WeatherMgr: SaveSettingsToWeather(weather, feature, json)
WeatherMgr->>WeatherMgr: update in-memory cache
WeatherMgr->>Disk: write Weathers/<key>.json
Note over WeatherMgr,GlobalReg: On runtime weather transition
WeatherMgr->>WeatherMgr: GetCurrentWeathers()
WeatherMgr->>WeatherMgr: UpdateFeatures()
loop per feature with weather support
WeatherMgr->>GlobalReg: UpdateFeatureFromWeathers(feature, curr, next, lerp)
GlobalReg->>GlobalReg: Lerp feature variables between JSON states
GlobalReg->>Widget: update reflected properties
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/UI.cpp (1)
266-266: Update the icon count in the log message.The log message reports "16 icons" but the
texturePointersarray now has 18 elements (lines 180-199).Apply this diff:
- logger::info("InitializeMenuIcons: Loaded {}/16 icons successfully", iconsLoaded); + logger::info("InitializeMenuIcons: Loaded {}/18 icons successfully", iconsLoaded);
🧹 Nitpick comments (26)
features/Weather Editor/Shaders/Features/WeatherEditor.ini (2)
1-2: Add trailing newline at end of file.INI files should end with a newline character per POSIX conventions and many linters. Currently the file ends without one.
[Info] Version = 1-0-0 +
1-2: PR: Consider adding issue reference to description.Per coding guidelines, PRs that implement features should reference related issues. The PR description ("Adds the weather editor with imagespace and lighting template controls") would benefit from an explicit issue reference like "Implements #123" or "Addresses #123" to link this work to tracked requirements.
src/Utils/Form.cpp (1)
3-14: Consider adding input validation for robustness.The parsing logic assumes well-formed input in "PLUGIN|ID" format. Consider adding:
- Validation that the plugin string is not empty
- Error handling for hex parsing failures (the
>>operator can fail silently)- Handling for identifiers without the
|delimiterExample improvement:
auto Util::GetFormFromIdentifier(const std::string& a_identifier) -> RE::TESForm* { + if (a_identifier.empty()) { + return nullptr; + } std::istringstream ss{ a_identifier }; std::string plugin, id; std::getline(ss, plugin, '|'); std::getline(ss, id); + if (plugin.empty() || id.empty()) { + return nullptr; + } RE::FormID relativeID; - std::istringstream{ id } >> std::hex >> relativeID; + std::istringstream idStream{ id }; + if (!(idStream >> std::hex >> relativeID)) { + return nullptr; + } const auto dataHandler = RE::TESDataHandler::GetSingleton(); return dataHandler ? dataHandler->LookupForm(relativeID, plugin) : nullptr; }src/State.cpp (1)
31-32: Consider error handling for WeatherManager singleton.The code assumes
WeatherManager::GetSingleton()always returns a valid pointer. While this is likely safe if the singleton is initialized early, consider adding null checks for defensive coding, especially in Setup() which runs during initialization.Apply this pattern if defensive checks are desired:
+ if (auto* weatherMgr = WeatherManager::GetSingleton()) { - WeatherManager::GetSingleton()->UpdateFeatures(); + weatherMgr->UpdateFeatures(); + }Also applies to: 161-162
src/Deferred.cpp (1)
302-302: Consider null check for WeatherEditor singleton.While unlikely to fail, adding a null check for
WeatherEditor::GetSingleton()would make the code more defensive and consistent with error handling best practices.- WeatherEditor::GetSingleton()->Bind(); + if (auto* editor = WeatherEditor::GetSingleton()) { + editor->Bind(); + }docs/weather-system-docs/examples/ExampleWeatherFeature.cpp (1)
95-106: Consider adding type safety to JSON deserialization.Direct assignment from JSON without type checking can cause runtime exceptions if the JSON contains unexpected types. For documentation/example code, consider demonstrating defensive patterns.
void ExampleWeatherFeature::LoadSettings(json& o_json) { // Load base settings (non-weather-specific) - if (o_json.contains("Intensity")) - settings.intensity = o_json["Intensity"]; - if (o_json.contains("Color")) - settings.color = o_json["Color"]; - if (o_json.contains("TintColor")) - settings.tintColor = o_json["TintColor"]; - if (o_json.contains("Enabled")) - settings.enabled = o_json["Enabled"]; + if (o_json.contains("Intensity") && o_json["Intensity"].is_number()) + settings.intensity = o_json["Intensity"].get<float>(); + if (o_json.contains("Color") && o_json["Color"].is_array()) + settings.color = o_json["Color"].get<float3>(); + if (o_json.contains("TintColor") && o_json["TintColor"].is_array()) + settings.tintColor = o_json["TintColor"].get<float4>(); + if (o_json.contains("Enabled") && o_json["Enabled"].is_boolean()) + settings.enabled = o_json["Enabled"].get<bool>(); }features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl (1)
10-11: Single-threaded dispatch is functional but inefficient.
[numthreads(1, 1, 1)]means all 1024 samples are processed sequentially in a single thread. While acceptable for a one-time or infrequent computation, this could be parallelized if performance becomes a concern.Consider parallelizing the sample accumulation if this shader runs frequently:
// Alternative: Parallelize with atomic adds or reduction pattern [numthreads(32, 32, 1)] void main(...) { // Each thread handles one sample, use GroupMemoryBarrierWithGroupSync for reduction }src/WeatherEditor/Weather/WorldSpaceWidget.cpp (1)
5-7: Consider using defaulted destructor.An empty destructor can be declared as
= defaultfor clarity.In the header file:
~WorldSpaceWidget() = default;src/WeatherEditor/Weather/ImageSpaceWidget.h (1)
8-15: Redundant storage of the same pointer.Both
form(inherited from Widget) andimageSpacestore the same pointer. Consider removing the redundant member and using a cast when needed, or document why both are necessary.If
formin the base class is sufficient:class ImageSpaceWidget : public Widget { public: - RE::TESImageSpace* imageSpace = nullptr; - ImageSpaceWidget(RE::TESImageSpace* a_imageSpace) { form = a_imageSpace; - imageSpace = a_imageSpace; LoadImageSpaceValues(); } + + RE::TESImageSpace* GetImageSpace() const { return static_cast<RE::TESImageSpace*>(form); }Alternatively, if type-safe access is preferred, keep the member but document the intentional duplication.
src/WeatherEditor/WeatherUtils.h (3)
5-17: Consider using a namespace for these utilities.Free functions in the global namespace can cause naming collisions. Wrapping in a
WeatherUtilsnamespace would improve organization.+namespace WeatherUtils { + void Float3ToColor(const float3& newColor, RE::Color& color); void Float3ToColor(const float3& newColor, RE::TESWeather::Data::Color3& color); void ColorToFloat3(const RE::Color& color, float3& newColor); void ColorToFloat3(const RE::TESWeather::Data::Color3& color, float3& newColor); std::string ColorTimeLabel(const int i); std::string ColorTypeLabel(const int i); bool DrawSliderInt8(const std::string& label, int& property); bool DrawColorEdit(const std::string& l, float3& property); bool DrawSliderUint8(const std::string& label, int& property); bool DrawSliderFloat(const std::string& label, float& property); + +} // namespace WeatherUtils
14-17: Pass string parameters by const reference to avoid copies.
std::stringparameters are passed by value, causing unnecessary copies on each call. Useconst std::string&orstd::string_view.-bool DrawSliderInt8(const std::string& label, int& property); -bool DrawColorEdit(const std::string& l, float3& property); -bool DrawSliderUint8(const std::string& label, int& property); -bool DrawSliderFloat(const std::string& label, float& property); +bool DrawSliderInt8(std::string_view label, int& property); +bool DrawColorEdit(std::string_view label, float3& property); +bool DrawSliderUint8(std::string_view label, int& property); +bool DrawSliderFloat(std::string_view label, float& property);Note: If
std::string_viewisn't available, useconst std::string&instead.
19-25: Consider usingenum classfor type safety.Plain
enumvalues are implicitly convertible to integers and can cause accidental misuse.enum classprovides stronger typing.-enum ControlType +enum class ControlType { - INT8_SLIDER = 0, - COLOR3_PICKER, - UINT8_SLIDER, - FLOAT_SLIDER + Int8Slider = 0, + Color3Picker, + Uint8Slider, + FloatSlider };src/Features/WeatherEditor.h (2)
30-31: Document resource ownership and lifecycle.The raw D3D11 pointers (
diffuseIBLTextureanddiffuseIBLCS) lack documentation about their ownership and lifecycle management. Consider documenting:
- Who is responsible for creating these resources
- Who is responsible for releasing them
- Whether they can be null during normal operation
Add a comment block above these members:
// D3D resources for diffuse IBL computation // Owned by WeatherEditor, created in SetupResources(), released in destructor // May be nullptr before SetupResources() is called Texture2D* diffuseIBLTexture = nullptr; ID3D11ComputeShader* diffuseIBLCS = nullptr;
41-41: Add parameter validation for LerpWeather.The
LerpWeathermethod accepts rawRE::TESWeather*pointers without documented validation. Ensure the implementation handles nullptr cases gracefully or document that callers must provide valid pointers.Consider adding validation in the implementation or documenting the preconditions:
// Interpolates between two weather states // @param from - source weather (must not be nullptr) // @param to - target weather (must not be nullptr) // @param factor - interpolation factor [0.0, 1.0] void LerpWeather(RE::TESWeather* from, RE::TESWeather* to, float factor);src/WeatherEditor/WeatherUtils.cpp (2)
51-132: Consider simplifying label functions.The label initialization and switch structure could be simplified by returning directly from each case, avoiding the empty string initialization and making the code more concise.
Example refactor for
ColorTimeLabel:std::string ColorTimeLabel(const int i) { switch (i) { case 0: return "Sunrise"; case 1: return "Day"; case 2: return "Sunset"; case 3: return "Night"; default: return ""; } }Apply similar pattern to
ColorTypeLabel.
134-137: Clarify int8_t vs int parameter type mismatch.
DrawSliderInt8takesint& propertybut operates on the range [-128, 127]. Callers must ensure they're passing an int that will be used with int8_t semantics. Consider documenting this requirement or using a different approach.Add a comment to clarify usage:
// Helper for editing int8_t values stored in int variables // Caller must ensure property stays within int8_t range bool DrawSliderInt8(const std::string& label, int& property)src/WeatherEditor/Widget.h (1)
18-21: Clarify ID generation intent.The
GetNewID()function returns-uniqueID++, which produces the sequence 0, -1, -2, -3, etc. The first call returns 0 (not negative), which may be unexpected if the intent is to generate exclusively negative IDs. If this is intentional, document it; otherwise, consider starting withuniqueID = 1or using pre-increment.If all IDs should be negative:
int GetNewID() { return -(++uniqueID); // Produces -1, -2, -3, ... }Or document the current behavior:
// Returns unique IDs: 0, -1, -2, -3, ... // First ID is 0, subsequent IDs are negative int GetNewID() { return -uniqueID++; }src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
302-312: Add exception handling for JSON operations.Unlike the ImageSpaceWidget version, these methods lack exception handling for JSON operations. Add try-catch blocks to handle malformed JSON gracefully.
void LightingTemplateWidget::LoadSettings() { try { if (!js.empty()) { settings = js; } } catch (const std::exception& e) { logger::error("Failed to load LightingTemplate settings: {}", e.what()); LoadLightingTemplateValues(); // Fallback to defaults } } void LightingTemplateWidget::SaveSettings() { try { js = settings; } catch (const std::exception& e) { logger::error("Failed to save LightingTemplate settings: {}", e.what()); } }src/WeatherManager.h (1)
25-26: Consider marking GetCurrentWeathers as const.The
GetCurrentWeathers()method likely only reads state and should be markedconstto indicate it doesn't modify the WeatherManager. This improves API clarity and enables calling from const contexts.// Get current weather state and transition info CurrentWeathers GetCurrentWeathers() const;src/WeatherEditor/Weather/WeatherWidget.cpp (1)
773-780:InheritFromParentdoes not handle fog properties.The function checks
weatherPropertiesandweatherColorsmaps but ignoresfogProperties. If a fog property name is passed, it silently does nothing.void WeatherWidget::InheritFromParent(const std::string& property) { if (settings.weatherProperties.find(property) != settings.weatherProperties.end()) { settings.weatherProperties[property] = GetParent()->settings.weatherProperties[property]; } else if (settings.weatherColors.find(property) != settings.weatherColors.end()) { settings.weatherColors[property] = GetParent()->settings.weatherColors[property]; + } else if (settings.fogProperties.find(property) != settings.fogProperties.end()) { + settings.fogProperties[property] = GetParent()->settings.fogProperties[property]; } }src/WeatherEditor/EditorWindow.cpp (3)
197-199: DuplicateTableSetColumnIndex(0)call.Line 197 and 199 both call
ImGui::TableSetColumnIndex(0). The second call is redundant.ImGui::TableSetColumnIndex(0); -ImGui::TableSetColumnIndex(0); - // Editor ID column
536-572: Widgets allocated withnewwithout visible cleanup - potential memory leak.
SetupResourcesallocatesWeatherWidget,WorldSpaceWidget,LightingTemplateWidget, andImageSpaceWidgetwithnewbut there's no destructor or cleanup visible. If the EditorWindow is destroyed or resources are reloaded, these allocations will leak.Consider using
std::unique_ptrorstd::vector<std::unique_ptr<Widget>>for automatic cleanup, or ensure the destructor properly deletes all widgets.
702-749: Static variables for rename state may cause issues across frames.
renameInfoandneedsRenameare static, meaning they persist across frames. If the user starts editing a label but the settings window is closed before confirmation,needsRenameremains true and could trigger an unintended rename on the next open. Consider resetting these when the window closes.src/WeatherVariableRegistry.h (3)
94-111: Consider enforcingminValue/maxValueor clarifying their purpose.
FloatVariablestores min/max bounds but doesn't clamp values duringLerporLoadFromJson. If these are meant as hard constraints, override those methods to clamp. If they're UI hints only, a brief comment would clarify intent.
148-192: Thread safety consideration for concurrent access.If
FeatureWeatherRegistrymethods could be called from multiple threads (e.g., registration at load time vs. lerp during gameplay), consider adding synchronization. If registration is guaranteed single-threaded at startup, current design is fine.
204-211: Avoid double map lookup withtry_emplace.
GetOrCreateFeatureRegistryperforms two lookups:find()thenoperator[]. Usetry_emplacefor a single lookup.FeatureWeatherRegistry* GetOrCreateFeatureRegistry(const std::string& featureName) { - auto it = featureRegistries.find(featureName); - if (it == featureRegistries.end()) { - featureRegistries[featureName] = std::make_unique<FeatureWeatherRegistry>(); - } - return featureRegistries[featureName].get(); + auto [it, inserted] = featureRegistries.try_emplace(featureName, nullptr); + if (inserted) { + it->second = std::make_unique<FeatureWeatherRegistry>(); + } + return it->second.get(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
package/Interface/CommunityShaders/Icons/Action Icons/apply-to-game.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Action Icons/delete.pngis excluded by!**/*.pngpackage/Interface/CommunityShaders/Icons/Categories/sky.pngis excluded by!**/*.png
📒 Files selected for processing (44)
docs/weather-system-docs/WeatherVariableRegistration.md(1 hunks)docs/weather-system-docs/examples/ExampleWeatherFeature.cpp(1 hunks)docs/weather-system-docs/examples/ExampleWeatherFeature.h(1 hunks)features/Weather Editor/Shaders/Features/WeatherEditor.ini(1 hunks)features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl(1 hunks)src/Deferred.cpp(6 hunks)src/Feature.cpp(2 hunks)src/Feature.h(1 hunks)src/Features/CloudShadows.h(1 hunks)src/Features/SkySync.h(1 hunks)src/Features/WeatherEditor.cpp(1 hunks)src/Features/WeatherEditor.h(1 hunks)src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)src/Globals.cpp(3 hunks)src/Globals.h(3 hunks)src/Menu.cpp(4 hunks)src/Menu.h(1 hunks)src/Menu/FeatureListRenderer.cpp(1 hunks)src/State.cpp(4 hunks)src/Util.h(1 hunks)src/Utils/Form.cpp(1 hunks)src/Utils/Form.h(1 hunks)src/Utils/Serialize.cpp(1 hunks)src/Utils/Serialize.h(1 hunks)src/Utils/UI.cpp(3 hunks)src/WeatherEditor/EditorWindow.cpp(1 hunks)src/WeatherEditor/EditorWindow.h(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.cpp(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.h(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.cpp(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.h(1 hunks)src/WeatherEditor/Weather/WeatherWidget.cpp(1 hunks)src/WeatherEditor/Weather/WeatherWidget.h(1 hunks)src/WeatherEditor/Weather/WorldSpaceWidget.cpp(1 hunks)src/WeatherEditor/Weather/WorldSpaceWidget.h(1 hunks)src/WeatherEditor/WeatherUtils.cpp(1 hunks)src/WeatherEditor/WeatherUtils.h(1 hunks)src/WeatherEditor/Widget.cpp(1 hunks)src/WeatherEditor/Widget.h(1 hunks)src/WeatherManager.cpp(1 hunks)src/WeatherManager.h(1 hunks)src/WeatherVariableRegistry.h(1 hunks)src/XSEPlugin.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Feature.hsrc/Util.hsrc/XSEPlugin.cppsrc/Globals.hsrc/Features/WeatherPicker.cppsrc/Feature.cppsrc/Utils/Serialize.hsrc/Features/WeatherPicker.hsrc/Features/SkySync.hdocs/weather-system-docs/examples/ExampleWeatherFeature.cppsrc/Globals.cppsrc/Utils/Form.cppfeatures/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlslsrc/Utils/Serialize.cppsrc/Menu/FeatureListRenderer.cppsrc/WeatherEditor/Weather/WorldSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/WorldSpaceWidget.cppsrc/WeatherEditor/Widget.cppsrc/Utils/Form.hsrc/State.cppsrc/WeatherEditor/Widget.hsrc/Features/CloudShadows.hsrc/WeatherManager.hdocs/weather-system-docs/examples/ExampleWeatherFeature.hsrc/Utils/UI.cppsrc/Menu.cppsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherManager.cppsrc/WeatherEditor/EditorWindow.cppsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/Deferred.cppsrc/Menu.hsrc/WeatherVariableRegistry.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Feature.hsrc/Util.hsrc/XSEPlugin.cppsrc/Globals.hsrc/Features/WeatherPicker.cppsrc/Feature.cppsrc/Utils/Serialize.hsrc/Features/WeatherPicker.hsrc/Features/SkySync.hsrc/Globals.cppsrc/Utils/Form.cppsrc/Utils/Serialize.cppsrc/Menu/FeatureListRenderer.cppsrc/WeatherEditor/Weather/WorldSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/WorldSpaceWidget.cppsrc/WeatherEditor/Widget.cppsrc/Utils/Form.hsrc/State.cppsrc/WeatherEditor/Widget.hsrc/Features/CloudShadows.hsrc/WeatherManager.hsrc/Utils/UI.cppsrc/Menu.cppsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherManager.cppsrc/WeatherEditor/EditorWindow.cppsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/Deferred.cppsrc/Menu.hsrc/WeatherVariableRegistry.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Feature.hsrc/Util.hsrc/XSEPlugin.cppsrc/Globals.hsrc/Features/WeatherPicker.cppsrc/Feature.cppsrc/Utils/Serialize.hsrc/Features/WeatherPicker.hsrc/Features/SkySync.hdocs/weather-system-docs/examples/ExampleWeatherFeature.cppsrc/Globals.cppsrc/Utils/Form.cppdocs/weather-system-docs/WeatherVariableRegistration.mdfeatures/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlslsrc/Utils/Serialize.cppsrc/Menu/FeatureListRenderer.cppsrc/WeatherEditor/Weather/WorldSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/WorldSpaceWidget.cppsrc/WeatherEditor/Widget.cppfeatures/Weather Editor/Shaders/Features/WeatherEditor.inisrc/Utils/Form.hsrc/State.cppsrc/WeatherEditor/Widget.hsrc/Features/CloudShadows.hsrc/WeatherManager.hdocs/weather-system-docs/examples/ExampleWeatherFeature.hsrc/Utils/UI.cppsrc/Menu.cppsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.hsrc/WeatherManager.cppsrc/WeatherEditor/EditorWindow.cppsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/Deferred.cppsrc/Menu.hsrc/WeatherVariableRegistry.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
🧠 Learnings (13)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/Feature.hsrc/Globals.hsrc/Feature.cppdocs/weather-system-docs/examples/ExampleWeatherFeature.cppsrc/Globals.cppdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/State.cppdocs/weather-system-docs/examples/ExampleWeatherFeature.hsrc/Features/WeatherEditor.hsrc/Deferred.cppsrc/WeatherVariableRegistry.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/Feature.hsrc/Feature.cppsrc/Globals.cppsrc/State.cppdocs/weather-system-docs/examples/ExampleWeatherFeature.hsrc/Features/WeatherEditor.hsrc/Deferred.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/Feature.hsrc/Globals.hsrc/Features/WeatherPicker.cppsrc/Feature.cppsrc/Features/WeatherPicker.hdocs/weather-system-docs/examples/ExampleWeatherFeature.cppsrc/Globals.cppdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/State.cppsrc/WeatherManager.hdocs/weather-system-docs/examples/ExampleWeatherFeature.hsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherManager.cppsrc/WeatherEditor/EditorWindow.cppsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/Deferred.cpp
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlslsrc/Deferred.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Place all feature shaders under features/YourFeature/Shaders/
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
📚 Learning: 2025-10-02T14:20:33.454Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:515-515
Timestamp: 2025-10-02T14:20:33.454Z
Learning: In the Community Shaders codebase (skyrim-community-shaders repository), hardcoded shader resource slot numbers are used directly in code rather than being defined as named constants. This is the established convention and should not be flagged as an issue.
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
📚 Learning: 2025-07-01T18:01:07.079Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-01T18:01:07.079Z
Learning: In the skyrim-community-shaders project, simple scalar constants in HLSL shaders use #define (e.g., #define NTHREADS 128), while more complex constants use static const within namespaces (e.g., Math namespace in Math.hlsli). For epsilon standardization, #define is the appropriate choice since epsilon values are simple scalar constants.
Applied to files:
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
src/Utils/UI.cpp
📚 Learning: 2025-06-08T11:25:14.536Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Applied to files:
src/WeatherEditor/WeatherUtils.cpp
📚 Learning: 2025-07-18T15:21:03.641Z
Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Applied to files:
src/Deferred.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (81)
features/Weather Editor/Shaders/Features/WeatherEditor.ini (1)
1-2: Verify version format compatibility.The version is specified as
1-0-0(hyphenated). Confirm this matches the format expected by your feature system's version parser. Standard semantic versioning typically uses dots (e.g.,1.0.0).src/Features/CloudShadows.h (1)
20-20: LGTM! Category relabeling is consistent.The update from "Sky" to "Sky & Weather" aligns with similar changes across other features in this PR, supporting consistent UI organization for weather-related features.
src/Features/SkySync.h (1)
12-12: LGTM! Consistent category update.The category update matches the broader pattern across weather-related features.
src/Utils/Form.h (1)
1-7: LGTM! Clean utility header.The function declarations provide a clear API for form/identifier conversion. Trailing return type syntax is appropriate here.
src/Utils/UI.cpp (3)
180-199: LGTM! Icon additions are properly structured.The array expansion and new icon entries for
deleteSettingsandapplyToGameare correctly integrated with the texture loading flow.
233-237: LGTM! Icon loading follows established pattern.The loading of new icons with logging follows the existing pattern correctly.
404-404: LGTM! Category comparison updated consistently.The update to "Sky & Weather" aligns with the category changes in CloudShadows.h and SkySync.h.
src/Util.h (1)
5-5: LGTM! Header inclusion exposes new form utilities.The include makes the form identifier utilities available to code that includes Util.h.
src/Globals.h (3)
31-31: LGTM! Forward declaration follows established pattern.The WeatherEditor forward declaration is consistent with other feature forward declarations in the file.
84-84: LGTM! Feature registration follows coding guidelines.The weatherEditor extern declaration in
globals::featuresnamespace aligns with the established pattern for feature registration.Based on learnings, new features should be registered in the globals::features namespace.
204-206: LGTM! Game system extern declarations.The addition of
tesandmemoryManagerextern declarations provides access to required game systems for the weather editor functionality.src/XSEPlugin.cpp (1)
114-115: Verify the SetupResources call placement.The
EditorWindow::GetSingleton()->SetupResources()call is placed outside theif (errors.empty())check, meaning it executes even when initialization errors have occurred. Please verify this is intentional.If the editor window requires resources regardless of error state (e.g., to display UI), the current placement is correct. Otherwise, consider moving it inside the error check:
for (auto it = errors.begin(); it != errors.end(); ++it) { auto& errorMessage = *it; RE::DebugMessageBox(std::format("Community Shaders\n{}, will disable all hooks and features", errorMessage).c_str()); } - EditorWindow::GetSingleton()->SetupResources(); - if (errors.empty()) { + EditorWindow::GetSingleton()->SetupResources(); globals::OnDataLoaded();src/Globals.cpp (2)
31-32: WeatherEditor registration in globals is consistent with feature patternIncluding
Features/WeatherEditor.hand addingWeatherEditor weatherEditor{}underglobals::featuresfollows the established pattern for registering new features in the global feature namespace, so other systems can access it cleanly.Based on learnings, this matches the expected registration location for new features.
Also applies to: 80-80
89-96: EnsuretesandmemoryManagerare initialized before first use
globals::game::tesandglobals::game::memoryManagerare added here but not initialized inOnInit,ReInit, orOnDataLoadedin this file. If any new code assumes they are non-null, they must be assigned (e.g., via the relevantGetSingleton/relocation) before first use to avoid null dereferences across SE/AE/VR.Can you confirm where these are set up, and that all call sites defensively handle the null case if initialization can fail?
src/Feature.h (1)
131-137: NewRegisterWeatherVariableshook is well-scoped and non-breakingAdding a default no-op
virtual void RegisterWeatherVariables()with clear documentation gives features a single, well-defined place to wire weather-controllable variables into the global registry, without impacting existing subclasses.src/Features/WeatherPicker.h (1)
8-16: Category relabeling to “Sky & Weather” is consistent with menu groupsUpdating
GetCategory()to return"Sky & Weather"aligns WeatherPicker with the new shared category used for sky/weather tooling in the feature list renderer and related features.src/Menu/FeatureListRenderer.cpp (1)
120-123: Category order updated cleanly for “Sky & Weather”Including
"Sky & Weather"incategoryOrderkeeps the renamed category in a stable position while preserving behavior for other categories. Features still returning the legacy"Sky"label will simply appear in the fallback “categories not in predefined order” section, which is acceptable during migration.src/Features/WeatherPicker.cpp (1)
510-521: Instant weather change control is safe and clearly scopedThe “Instantly Change Weather” button correctly guards the selected index, uses
ForceWeatherfor an immediate switch, and documents the difference from the regular/accelerated transitions in the tooltip. GivenRenderCoreWeatherDetailsonly callsRenderWeatherControlswhenskyis valid, this looks robust.If
ForceWeatherhas any engine-specific side effects (e.g., persistence or script firing differences vsSetWeather), please confirm this behavior is acceptable for editor/test workflows.src/Utils/Serialize.h (1)
16-21: FogData JSON hooks follow existing serialization patternAdding
to_json/from_jsonoverloads forRE::TESWeather::FogDatamirrors the existingRE::NiColorcustomization and cleanly extends the serialization surface for weather data. Header-side declarations look correct.Please ensure the corresponding definitions exist in
Serialize.cpp(or another TU) and that all modules needing FogData persistence include this header to pick up the ADL hooks.src/Menu.h (1)
185-210: New UI icons are fine; pair with destructor updates in Menu.cppThe additions of
deleteSettingsandapplyToGametoUIIconsare straightforward and keep weather-editor-specific UI resources centralized. Just make sure theirtexturefields are released inMenu::~Menu()alongside the existing icons (see comment inMenu.cpp).src/Menu.cpp (3)
418-421: Weather Editor takeover inDrawSettingsis clear and guardedThe early check on
EditorWindow::GetSingleton()->opento render the Weather Editor and return before building the main menu cleanly gives the editor exclusive control when active, while still applying global styling and docking. This avoids overlapping menus.Please confirm that
EditorWindow::GetSingleton()is safe to call wheneverDrawSettings()runs (e.g., it doesn’t depend on deferred initialization beyondMenu::Init) and that closing the editor reliably resetsopenso normal menu rendering resumes.
734-736: ESC behavior now respects Weather Editor visibilityConditioning the ESC-to-close-menu path on
!EditorWindow::GetSingleton()->openavoids closing the main menu in the middle of an active Weather Editor session, which would be confusing. This keeps input handling centralized while correctly deferring to the editor’s own close logic.If the Weather Editor also uses ESC (or other keys) for closing, please verify there is no conflict in practice and that focus/input routing remains intuitive in VR and non-VR modes.
175-201: Verify UIIcons structure and complete icon texture cleanupReview requires codebase verification to confirm: (1)
deleteSettingsandsearchare actual UIIcons members, (2) these fields are indeed missing from the current destructor, and (3) no other icon textures are left unreleased. Manual inspection of UIIcons struct definition and Menu::~Menu() implementation is necessary to validate the resource management concern.src/Feature.cpp (1)
32-32: LGTM! WeatherEditor feature properly registered.The WeatherEditor feature is correctly added to the global feature list with appropriate include directive. The registration follows the established pattern for feature integration.
Also applies to: 231-232
src/State.cpp (2)
353-354: LGTM! Weather variable registration properly sequenced.The call to
RegisterWeatherVariables()is correctly placed afterfeature->Load(settings), ensuring features are initialized before registering weather-controllable variables.
31-32: Verify performance impact of per-frame weather updates.
UpdateFeatures()is called every frame in the draw loop. Ensure this operation is lightweight and doesn't introduce performance overhead, especially during weather transitions.src/WeatherEditor/Widget.cpp (3)
6-63: LGTM! Robust file I/O and error handling.The
Save()method demonstrates excellent error handling with:
- Directory creation with exception handling
- Multiple stream state checks
- JSON validation before writing
- Comprehensive exception handling for JSON and I/O errors
65-119: LGTM! Proper handling of missing files and errors.The
Load()method correctly:
- Handles missing files by resetting to vanilla defaults
- Validates loaded JSON is not null
- Provides user notifications for errors
- Includes comprehensive exception handling
149-202: LGTM! Good UX with deletion confirmation.The confirmation modal with suppressible warning provides good user experience while preventing accidental deletions. The immediate save of the suppression preference is appropriate.
src/Deferred.cpp (2)
430-448: LGTM! Shader resource array properly expanded.The SRV array is correctly expanded from 16 to 17 elements to accommodate the WeatherEditor diffuse IBL texture, with proper null checks for loaded state and consistent clearing.
Also applies to: 468-468
450-453: Verify sampler array size consistency.One sampler is set at line 453 (
ARRAYSIZE(samplers)where samplers has 1 element), but two samplers are cleared at line 478. Confirm whether this mismatch is intentional and that the second sampler slot doesn't retain stale state.docs/weather-system-docs/WeatherVariableRegistration.md (1)
1-271: LGTM! Comprehensive and well-structured documentation.The documentation effectively covers:
- System architecture and data flow
- Step-by-step implementation guide
- Code examples for basic and advanced usage
- Integration details with the weather manager
- Memory management and threading considerations
This will be valuable for developers implementing weather-aware features.
src/WeatherEditor/Weather/WorldSpaceWidget.h (1)
1-30: LGTM! Clean widget interface following established pattern.The
WorldSpaceWidgetclass properly:
- Derives from the
Widgetbase class- Initializes both base and derived class members in constructor
- Declares required virtual overrides (
DrawWidget,LoadSettings,SaveSettings)- Provides widget-specific methods (
ApplyChanges,RevertChanges)The empty
Settingsstruct suggests this widget may have minimal configuration needs or is prepared for future expansion.docs/weather-system-docs/examples/ExampleWeatherFeature.h (1)
1-30: LGTM! Clear example demonstrating weather feature pattern.This example effectively demonstrates:
- Feature class structure with proper inheritance
- Settings organization with sensible defaults
- Required method overrides (including
RegisterWeatherVariables)- Clear comments emphasizing the simplicity of the weather variable system
The example serves its educational purpose well and follows the coding guidelines for feature implementation.
docs/weather-system-docs/examples/ExampleWeatherFeature.cpp (1)
9-60: Well-structured example demonstrating the weather variable registration pattern.The registration pattern is clean and demonstrates all supported variable types (float, float3, float4, and custom bool with lerp). The custom lerp function for bool at line 51-53 is a practical example of threshold-based transitioning.
features/Weather Editor/Shaders/WeatherEditor/DiffuseIBLCS.hlsl (2)
17-33: Correct SH accumulation with linear color space handling.The implementation correctly:
- Converts sampled colors to linear space before accumulation (line 26)
- Uses uniform sphere sampling with proper offset (0.5 start for center of cells)
- Accumulates SH coefficients per color channel
The negated
-rayDirat line 24 is the standard convention for reflection lookups where the sample direction should be inverted for cubemap sampling.
35-44: Solid angle normalization and output are correct.The factor
4π / N²properly normalizes the Monte Carlo integration over the sphere. Output layout storing R/G/B coefficients at adjacent texels (0,0), (1,0), (2,0) is clear.src/WeatherEditor/Weather/WorldSpaceWidget.cpp (2)
23-48: Icon-based UI path is well-implemented.Proper style push/pop pairing, tooltip handling, and button callbacks are correctly implemented. The style modifications create a clean icon-only appearance.
84-92: Stub implementations are appropriate for placeholder.These empty methods serve as extension points. The comments clarify intent.
src/WeatherEditor/Weather/ImageSpaceWidget.h (2)
10-15: Constructor performs I/O via LoadImageSpaceValues().Calling
LoadImageSpaceValues()in the constructor means construction can have side effects or fail. This is acceptable for UI widgets but worth noting—if loading fails, consider how the widget handles a partially initialized state.
17-40: Settings struct is well-organized with sensible defaults.The grouping by category (HDR, Cinematic, Tint, DOF) and zero-initialization of most values is appropriate for image space parameters.
src/WeatherEditor/Weather/LightingTemplateWidget.h (1)
17-61: Well-structured widget design.The nested struct hierarchy (DirectionalColor, DALC, Settings) provides clear organization of lighting template data. The separation of concerns with
ApplyChanges/RevertChangesand the bidirectional synchronization methods (SetLightingTemplateValues/LoadLightingTemplateValues) follows good UI design patterns.src/WeatherEditor/WeatherUtils.cpp (1)
23-49: Color conversion functions depend on fixed numeric conversions.These color conversion functions correctly delegate to the appropriate numeric conversion helpers. Once the input clamping issue in
FloatToInt8andFloatToUint8is addressed, these functions will handle out-of-range color values safely.src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
238-268: Good defensive programming with null check.The null check for
imageSpacebefore accessing its members is appropriate defensive programming. The direct data assignments assume thesettingsvalues are valid, which is reasonable if controlled through the UI sliders.src/WeatherManager.h (1)
34-47: Well-designed public API.The API design demonstrates good practices:
- Const-correctness on
HasWeatherSettings()- Pass-by-const-reference for
settingsparameter- Static helper
GetWeatherKey()for utility function- Clear separation between save and load operations
Ensure the implementation validates weather pointers before dereferencing in
SaveSettingsToWeather()andLoadSettingsFromWeather().src/WeatherEditor/EditorWindow.h (2)
80-93: Well-designed settings structure with sensible defaults.The
Settingsstruct provides good default values for the editor configuration, including predefined record markers with semantically meaningful colors (red for "To Do", yellow/orange for "In Progress", green for "Complete"). The inline initialization is clean and readable.
68-78: Verify notification cleanup to prevent unbounded growth.The
notificationsvector could grow indefinitely if notifications are added faster than they're removed. EnsureRenderNotifications()removes expired notifications to prevent memory growth.Run the following script to check if RenderNotifications implements cleanup:
#!/bin/bash # Description: Verify that RenderNotifications removes expired notifications # Search for RenderNotifications implementation ast-grep --pattern 'void EditorWindow::RenderNotifications() { $$$ }' # Also search for erase/remove operations on notifications vector rg -n -A 10 'void.*RenderNotifications' --type cpp | rg 'erase|remove_if|clear'src/Features/WeatherEditor.h (1)
18-18: Consider more specific shader define naming to avoid conflicts.The shader define name "WEATHER" is generic and could potentially conflict with existing shader code or weather-related defines. Consider using a more specific name like "WEATHER_EDITOR" or "WEATHER_EDITOR_FEATURE" to align with explicit naming practices.
src/WeatherManager.cpp (7)
5-19: LGTM!
GetCurrentWeatherscorrectly handles the null sky case and safely extracts weather state from the game engine.
21-64: LGTM!Robust implementation with proper directory existence check, file validation, JSON parsing with exception handling, and informative logging throughout the loading process.
110-167: LGTM!The save function properly:
- Updates cache before disk write
- Creates directories as needed with exception handling
- Merges with existing file data
- Has proper error handling for all I/O operations
169-188: LGTM!Clean cache lookup implementation with proper null checks and two-level map navigation.
190-213: LGTM!
GetWeatherKeyprovides good fallback logic (EditorID → FormID hex) and handles null input safely.HasWeatherSettingscorrectly delegates to the cache lookup.
215-220: LGTM!Simple and effective cache clearing with logging for debugging purposes.
66-108: Verify theUpdateFeatureFromWeatherscall signature and behavior.The function iterates features and delegates interpolation to the global registry. However, when
lerpFactoris exactly1.0f,nextWeatherSettingswill be empty (Line 97 condition), but the interpolation call still receives it. Confirm thatUpdateFeatureFromWeathershandles an emptynextWeatherSettingsJSON correctly when lerp is complete.src/WeatherEditor/Weather/WeatherWidget.h (2)
49-72: LGTM!The
ImageSpaceSettingsstruct has sensible default values for all fields, preventing uninitialized data issues.
74-91: LGTM!The
Settingsstruct is well-organized with appropriate container types for the various weather data categories.src/Features/WeatherEditor.cpp (7)
10-20: LGTM!
LerpInt8_tandLerpUint8_tcorrectly handle clamping to prevent overflow/underflow when converting back to narrow integer types.
29-44: LGTM!The
RE::Coloroverload ofLerpColorandLerpDirectionalcorrectly use the helper functions for safe interpolation.
155-167: LGTM!
ClearShaderCacheandGetDiffuseIBLCSimplement lazy loading with proper cleanup on cache clear.
169-235: LGTM!
LerpWeathercomprehensively interpolates all weather properties using the appropriate helper functions. The iteration over color types, DALC, and cloud layers is correct.
237-273: LGTM!
DrawWeatherStatusPanelprovides useful status information with proper null checks and visual feedback for custom settings presence.
72-117: Verify slot binding consistency for diffuse IBL texture.Line 86 unbinds PS slot 80, but line 115 rebinds to slot 100 after the compute shader operation completes. Confirm whether this slot change is intentional or if line 115 should use slot 80 to match the original binding location.
119-153: Verify memory management fordiffuseIBLTexture.
diffuseIBLTextureis allocated withnew Texture2D(texDesc)inSetupResources(). Confirm that either:
- The member variable uses a smart pointer (e.g.,
unique_ptr<Texture2D>) that handles automatic cleanup- A destructor properly deletes this resource
- Texture2D is a wrapper class that manages its own lifetime
If none of these apply, replace
newwith a smart pointer allocation.src/WeatherEditor/Weather/WeatherWidget.cpp (6)
1-39: LGTM!JSON serialization macros are correctly defined for all nested structures, enabling proper serialization/deserialization of weather settings.
45-232: LGTM!
DrawWidgetprovides a comprehensive UI with weather lock controls, time pause, icon/text fallback for buttons, and proper delete confirmation flow. The parent widget selection logic is well-implemented.
329-384: LGTM!
LoadSettingshas robust error handling with validation of critical fields, fallback to vanilla values on parse errors, and user notifications for failures.
386-414: LGTM!
SaveSettingsincludes validation logging to catch serialization issues early.
890-984: LGTM!
DrawImageSpaceSettingsprovides a well-organized UI with time-of-day tabs and appropriate slider ranges for HDR, cinematic, tint, and DOF settings.
986-1062: LGTM!
LoadImageSpaceValuesandSetImageSpaceValuescorrectly synchronize all ImageSpace fields between the settings structure and the game's ImageSpace data.src/WeatherEditor/EditorWindow.cpp (9)
10-45: LGTM!Helper functions for case-insensitive search, disabled text rendering, and tooltips are clean implementations.
46-172: LGTM!
ShowObjectsWindowprovides good UX with category filtering, current lighting template highlighting, context menus for marking records, and proper table layout.
254-284: LGTM!
ShowViewportWindowcorrectly calculates aspect-aware image sizing to fit the available space while preserving proportions.
286-311: LGTM!
ShowWidgetWindowiterates all widget types and renders those that are open.
313-534: LGTM!
RenderUIprovides a comprehensive menu bar with file operations, settings, window management, weather lock/time pause indicators, and keyboard shortcuts. The layout calculations for window sizing are appropriate.
574-619: LGTM!
Drawproperly manages vanity camera state on editor open/close, re-enforces weather lock, and handles texture capture for the viewport.
784-839: LGTM!
SaveandLoadhave proper error handling with logging. The file existence check before loading prevents unnecessary warnings.
841-928: LGTM!Weather lock, time pause, and vanity camera management functions are well-implemented with proper state tracking and logging.
930-984: LGTM!The notification system with fade-out animation and stacking is a nice UX touch for providing feedback to users.
src/WeatherVariableRegistry.h (2)
15-27: Clean interface design.The
IWeatherVariableabstract interface is well-structured with proper virtual destructor and clear method contracts.
113-146: LGTM!The
Float3VariableandFloat4Variablespecializations cleanly provide component-wise lerp functions.
…skyrim-community-shaders into weather-editor-updated
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/UI.cpp (1)
180-199: AddpauseTimetexture to the release array.
texturePointersnow coversdeleteSettingsandapplyToGamebut notpauseTime, so a re‑call toInitializeMenuIconswould leak the old pause texture.Consider:
std::array<ID3D11ShaderResourceView**, 19> texturePointers = { &menu->uiIcons.saveSettings.texture, &menu->uiIcons.loadSettings.texture, &menu->uiIcons.deleteSettings.texture, &menu->uiIcons.clearCache.texture, &menu->uiIcons.logo.texture, &menu->uiIcons.featureSettingRevert.texture, &menu->uiIcons.applyToGame.texture, &menu->uiIcons.pauseTime.texture, &menu->uiIcons.discord.texture, // ...rest unchanged... };Also update the trailing info log to match the new total icon count (currently still
{}/16 icons).
♻️ Duplicate comments (10)
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (2)
250-291: StaticshowDeleteConfirmshares state across all widgets.Using a
static bool showDeleteConfirmmeans allImageSpaceWidgetinstances share one confirmation flag; deleting from one widget could unexpectedly drive the popup state of another. Prefer an instance member flag instead (initialized tofalsein the class).
296-335: Zero-value rejection can discard valid settings.The “hasValidData” loop treats all‑zero numbers or empty arrays as invalid and falls back to defaults, which can wrongly reject a legitimately zeroed ImageSpace. Relying on the presence and type of the
"Settings"object (and its keys) is safer than inspecting numeric values.For example, simply:
if (!js.empty() && js.contains("Settings") && js["Settings"].is_object()) { settings = js["Settings"]; logger::info("ImageSpace settings loaded successfully for {}", GetEditorID()); } else { LoadImageSpaceValues(); }keeps the existing try/catch behavior without misclassifying valid data.
src/WeatherEditor/Weather/LightingTemplateWidget.cpp (2)
232-264: Add null check and clamp float‑to‑std::uint32_tconversions.
SetLightingTemplateValues()dereferenceslightingTemplatewithout a guard and castsdirectionalXY/directionalZfromfloattostd::uint32_tdirectly. This risks crashes on null and undefined behavior for negative/very large values.A safer pattern:
void LightingTemplateWidget::SetLightingTemplateValues() { if (!lightingTemplate) { logger::error("SetLightingTemplateValues called with null lightingTemplate"); return; } auto& data = lightingTemplate->data; auto& dalc = lightingTemplate->directionalAmbientLightingColors; // ...existing color copies... auto clampToU32 = [](float v) { float clamped = std::clamp(v, 0.0f, static_cast<float>(std::numeric_limits<std::uint32_t>::max())); return static_cast<std::uint32_t>(clamped); }; data.directionalXY = clampToU32(settings.directionalXY); data.directionalZ = clampToU32(settings.directionalZ); // rest unchanged... }
266-298: Mirror the null check inLoadLightingTemplateValues().For consistency and safety,
LoadLightingTemplateValues()should also guard against a nulllightingTemplatebefore dereferencing it, logging and returning early if needed.void LightingTemplateWidget::LoadLightingTemplateValues() { if (!lightingTemplate) { logger::error("LoadLightingTemplateValues called with null lightingTemplate"); return; } // existing body... }src/WeatherEditor/Widget.h (1)
31-46: Add defensive null checks aroundformusage.
GetEditorID(),GetFormID(), andGetFilename()all dereferenceformdirectly, butformis initialized tonullptrand public. To avoid crashes when a widget is mis‑constructed or reused, guard these calls:virtual std::string GetEditorID() const { if (!form) { return ""; } return form->GetFormEditorID(); } virtual std::string GetFormID() const { if (!form) { return "00000000"; } return std::format("{:08X}", form->GetFormID()); } virtual std::string GetFilename() const { if (!form) { return "Generated"; } if (auto file = form->GetFile()) { return std::format("{}", file->GetFilename()); } return "Generated"; }src/WeatherEditor/Weather/WeatherWidget.h (1)
16-23: Guard constructor against nullTESWeather*.
LoadWeatherValues()will dereferenceweather, so constructing with a nulla_weatherwill crash. Either enforce non‑null or add a simple check:WeatherWidget(TESWeather* a_weather) { form = a_weather; weather = a_weather; if (weather) { LoadWeatherValues(); vanillaSettings = settings; } else { logger::error("WeatherWidget constructed with null TESWeather"); } }src/WeatherEditor/WeatherUtils.cpp (1)
24-32: Add input clamping for float-to-integer conversions.The
FloatToInt8andFloatToUint8functions usestd::lerpwithout clamping the inputvalueto [0, 1]. Values outside this range produce results outside the valid int8_t/uint8_t range, causing undefined behavior when casting.Apply this diff to add clamping:
int8_t FloatToInt8(const float& value) { - return (int8_t)std::lerp(-128, 127, value); + float clamped = std::clamp(value, 0.0f, 1.0f); + return static_cast<int8_t>(std::lerp(-128.0f, 127.0f, clamped)); } uint8_t FloatToUint8(const float& value) { - return (uint8_t)std::lerp(0, 255, value); + float clamped = std::clamp(value, 0.0f, 1.0f); + return static_cast<uint8_t>(std::lerp(0.0f, 255.0f, clamped)); }src/WeatherEditor/EditorWindow.h (1)
24-29: Document ownership of pointer members and consider smart pointers.The raw pointers (
tempTextureand widget vectors) lack documentation about ownership and lifecycle management. While the implementation may handle cleanup correctly, explicit documentation orstd::unique_ptrusage would make ownership clear and prevent future bugs.src/WeatherEditor/Weather/WeatherWidget.cpp (2)
588-592: Bug: Address-of operator on bool reference always evaluates to true.Line 588 uses
&doesInheritwheredoesInheritis abool&reference. Taking the address of a reference yields a non-null pointer, which always evaluates to true. The condition should check the boolean value, not its address.-if (&doesInherit && HasParent()) { +if (doesInherit && HasParent()) {
621-629: Bug: Cloud inheritance copies DALC settings instead of cloud settings.The cloud inheritance block incorrectly copies
settings.dalc[i]from the parent instead ofsettings.clouds[i]. Additionally, the loop iterates overColorTimes::kTotal(4) instead ofTESWeather::kTotalLayersfor clouds.if (doesInherit && HasParent()) { - for (size_t i = 0; i < RE::TESWeather::ColorTimes::kTotal; i++) { - settings.dalc[i] = GetParent()->settings.dalc[i]; + for (size_t i = 0; i < RE::TESWeather::kTotalLayers; i++) { + settings.clouds[i] = GetParent()->settings.clouds[i]; } }
🧹 Nitpick comments (9)
src/Menu.cpp (1)
420-423: Early handoff toEditorWindowis fine, but avoid doubleGetSingleton()calls.The early return when the weather editor is open is a clean way to keep only one main editor visible. You can micro‑simplify by caching the singleton locally:
auto* editor = EditorWindow::GetSingleton(); if (editor->open) { editor->Draw(); return; }src/WeatherEditor/Widget.cpp (2)
183-187: Prefer makingGetFolderName()const to avoidconst_cast.
HasSavedFile()currently needsconst_cast<Widget*>(this)->GetFolderName()becauseGetFolderName()is non‑const. MakingGetFolderName()const(and keeping it read‑only) would remove this cast and simplify usage.std::string GetFolderName() const;and adjust the definition accordingly.
500-527: Shared delete confirmation popup behaves correctly but could use per‑widget IDs.The global
"ConfirmDelete"popup works as long as only one delete confirmation is needed at a time. For more explicit ownership (and to simplify future debugging), you might suffix the popup ID with a widget‑specific token (e.g., editor ID).std::string popupId = std::format("ConfirmDelete##{}", GetEditorID()); ImGui::OpenPopup(popupId.c_str()); // ... if (ImGui::BeginPopupModal(popupId.c_str(), nullptr, ImGuiWindowFlags_AlwaysAutoResize)) { // ... }[Suggesting as a nicety; current code is functionally acceptable.]
src/WeatherEditor/Widget.h (1)
12-21: Confirm negative ID usage is intentional.
WidgetSharedData::GetNewID()returns-uniqueID++, yielding 0, -1, -2, ... If these IDs are only for ImGui/internal usage that’s fine; otherwise, consider documenting or switching to strictly positive IDs for clarity.Would you confirm these IDs are only used for internal/editor purposes (e.g., ImGui IDs) and never serialized or exposed?
src/WeatherEditor/WeatherUtils.cpp (1)
160-163: Hardcoded slider max value of 50000 may be inappropriate for all use cases.The
DrawSliderFloatfunction uses a hardcoded maximum of 50000, which seems tailored for fog distance values. Consider making the min/max configurable or documenting the intended use case.-bool DrawSliderFloat(const std::string& label, float& property) +bool DrawSliderFloat(const std::string& label, float& property, float minValue = 0.0f, float maxValue = 50000.0f) { - return ImGui::SliderFloat(label.c_str(), &property, 0, 50000); + return ImGui::SliderFloat(label.c_str(), &property, minValue, maxValue); }src/WeatherEditor/Weather/WeatherWidget.cpp (2)
116-118: Preferstatic_castover C-style cast for type safety.C-style casts bypass compile-time type checking. Using
static_castmakes the intent explicit and catches errors at compile time.-parent = (WeatherWidget*)widget; +parent = static_cast<WeatherWidget*>(widget);
108-126: Usesize_tfor loop variable to match container size type.Using
intfor the loop variable when comparing againstsize()(which returnssize_t) can cause signed/unsigned comparison warnings and potential issues with very large collections.-for (int i = 0; i < widgets.size(); i++) { +for (size_t i = 0; i < widgets.size(); i++) {src/WeatherEditor/WeatherUtils.h (1)
3-4: Unused<functional>header include.The
<functional>header is included but nostd::functionor related types are used in this header's declarations.#include "Util.h" -#include <functional>src/WeatherEditor/EditorWindow.cpp (1)
531-564: Ctrl+W close logic iterates all collections after finding a match.The
breakstatements only exit the inner loops, so if a widget is closed inweatherWidgets, the code still iterates through all other collections unnecessarily. Consider using an early return or a flag.if (ImGui::GetIO().KeyCtrl && ImGui::IsKeyPressed(ImGuiKey_W, false)) { - for (int i = 0; i < (int)weatherWidgets.size(); i++) { - auto widget = weatherWidgets[i]; - if (widget->IsOpen() && ImGui::IsWindowFocused(ImGuiFocusedFlags_AnyWindow)) { - widget->SetOpen(false); - break; + auto closeFirstFocused = [](std::vector<Widget*>& widgets) -> bool { + for (auto* widget : widgets) { + if (widget->IsOpen() && ImGui::IsWindowFocused(ImGuiFocusedFlags_AnyWindow)) { + widget->SetOpen(false); + return true; + } } - } - // ... repeated for other collections + return false; + }; + + closeFirstFocused(weatherWidgets) || + closeFirstFocused(worldSpaceWidgets) || + closeFirstFocused(lightingTemplateWidgets) || + closeFirstFocused(imageSpaceWidgets); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Interface/CommunityShaders/Icons/Action Icons/pause.pngis excluded by!**/*.png
📒 Files selected for processing (15)
src/Menu.cpp(4 hunks)src/Menu.h(1 hunks)src/Utils/UI.cpp(3 hunks)src/WeatherEditor/EditorWindow.cpp(1 hunks)src/WeatherEditor/EditorWindow.h(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.cpp(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.h(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.cpp(1 hunks)src/WeatherEditor/Weather/WeatherWidget.cpp(1 hunks)src/WeatherEditor/Weather/WeatherWidget.h(1 hunks)src/WeatherEditor/Weather/WorldSpaceWidget.cpp(1 hunks)src/WeatherEditor/WeatherUtils.cpp(1 hunks)src/WeatherEditor/WeatherUtils.h(1 hunks)src/WeatherEditor/Widget.cpp(1 hunks)src/WeatherEditor/Widget.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Menu.h
- src/WeatherEditor/Weather/WorldSpaceWidget.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Menu.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Menu.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Menu.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
src/Utils/UI.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.h
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
🔇 Additional comments (33)
src/Menu.cpp (3)
45-45: Weather editor integration include looks correct.Including
WeatherEditor/EditorWindow.hhere cleanly wires the menu to the editor without obvious dependency issues.
175-185: Icon lifetime handling is consistent with new icons.Releasing
deleteSettings,applyToGame, andpauseTimealongside the existing icons matches the initialization logic and avoids dangling D3D resources on shutdown.
736-738: ESC behavior correctly respects weather editor focus.Conditioning ESC close on
!EditorWindow::GetSingleton()->openprevents the main menu from closing while the weather editor is active, which matches the intended UX.src/Utils/UI.cpp (2)
231-239: Per‑icon load additions are consistent with existing scheme.Loading
delete.png,apply-to-game.png, andpause.pngvialoadIconWithLoggingfits the existing pattern and keeps failures isolated to individual icons.
391-417: Category header rename to “Sky & Weather” is aligned.Mapping
"Sky & Weather"to the existingmenu.skytexture keeps icon reuse simple while reflecting the new category naming used elsewhere.src/WeatherEditor/Weather/ImageSpaceWidget.cpp (2)
350-380: Value mapping toimageSpace->datalooks consistent.HDR, cinematic, tint, and DOF fields are written 1:1 from
settingsintoimageSpace->datawith appropriate component mapping; nullimageSpaceis guarded early.
382-412: Loading fromimageSpace->datamirrors the write path correctly.
LoadImageSpaceValues()reverses the mapping fromimageSpace->databack intosettingswith matching fields and a null guard, which keeps Apply/Revert symmetric.src/WeatherEditor/Widget.cpp (5)
7-14: Search helper matches base‑class contract.
MatchesSearchcorrectly short‑circuits when the buffer is empty and delegates toContainsStringIgnoreCaseotherwise; this is a good, reusable base implementation.
75-150: Load path and error handling are robust.
Load()covers all main cases: missing file (resets to vanilla), invalid/null JSON, parse errors, and generic exceptions, each with logging plus a user notification and fallback toLoadSettings().
152-181: Delete flow correctly reverts to vanilla values.On delete, the file is removed, in‑memory JSON cleared, defaults reloaded and applied, and a success notification is shown; filesystem errors are logged without crashing.
189-242: Menu bar helpers are fine for manual use/debug.
DrawMenu()offers Save/Load/Revert plus a confirmation popup wired to the sharedsuppressDeleteWarningflag and persists that preference immediately; behavior looks consistent with the main header controls.
258-371: Shared header UX is well‑structured.The header combines search, optional “Force Weather”, and Apply/Revert/Save/Load/Delete in both icon and text modes, correctly respecting
useTextButtons,ShowActionIcons,autoApplyChanges, and icon availability. IDs derived fromsearchIdavoid ImGui clashes.src/WeatherEditor/Weather/LightingTemplateWidget.cpp (5)
29-56: Lighting template main window wiring looks good.The tabbed layout plus shared
DrawWidgetHeaderprovide a clear UX for basic/fog/DALC sections, andopenis respected correctly.
58-113: Basic section auto‑apply behavior is correct.Use of
MatchesSearchwith guarded spacing, plus auto‑apply viaEditorWindow::GetSingleton()->settings.autoApplyChanges, matches the intended interactive editing pattern.
154-219: DALC TOD mapping is coherent.Mapping X→Sunrise/Day, Y→Sunset, Z→Night into the TOD helpers and writing back into
settings.dalc.directionalkeeps the engine’s coordinate system while exposing a user‑friendly time‑of‑day view.
221-225: ApplyChanges also persists settings JSON.Calling both
SetLightingTemplateValues()andSaveSettings()ensures that the in‑game data and on‑disk JSON stay in sync.
300-312: Settings JSON round‑trip is straightforward.
LoadSettings()falls back to live template values whenjsis empty, andSaveSettings()simply assignssettingsback intojs, matching the generic Widget persistence pattern.src/WeatherEditor/Widget.h (2)
48-71: Widget lifecycle and defaults are reasonable.Pure virtual
DrawWidget, required Load/SaveSettings/ApplyChanges, and defaultRevertChanges()delegating toLoadSettings()give derived widgets a clear minimal contract.
73-81: Search buffer andMatchesSearchhook are well‑placed.Exposing
searchBufferandsearchActiveplus a baseMatchesSearchmethod makes it easy for widgets to integrate filtered UI without duplicating state.src/WeatherEditor/Weather/WeatherWidget.h (3)
25-93: Weather state structs are well‑organized for serialization.
DirectionalColor,DALC,Atmosphere,Cloud,ImageSpaceSettings, and the aggregateSettings(including per‑feature JSON) provide a clear, extensible container for per‑weather data across TOD and features.
95-119: Public API surface covers key editor operations.The overrides (DrawWidget/LoadSettings/SaveSettings), plus Set/LoadWeatherValues, Apply/Revert, feature settings and ImageSpace helpers, give WeatherEditor enough hooks to drive both UI and persistence.
120-144: Search result scaffolding looks appropriate.
SearchResultplusUpdateSearchResults,NavigateToSetting,ShouldHighlight, and tab/highlight state will support a good search UX across weather tabs.src/WeatherEditor/WeatherUtils.cpp (2)
202-218: Time period transition at sunrise start is not handled.When
currentTimeis exactly 5.0 (sunrise start), the conditioncurrentTime >= sunriseStart && currentTime < sunriseEndwill setoutFactors[Sunrise] = 1.0andoutFactors[Day] = 0.0, which is correct. However, there's no blending between Night and Sunrise - the transition is abrupt. If smooth blending is intended, consider adding a transition period.Is an abrupt transition from Night to Sunrise at 5:00 AM intentional, or should there be a gradual blend similar to other transitions?
465-468:EndWidgetSearchBaris an empty function.The function body only contains a comment about future use. If this is intentional for API symmetry with
BeginWidgetSearchBar, consider adding a[[maybe_unused]]attribute or a brief inline comment in the header to clarify.src/WeatherEditor/EditorWindow.h (2)
68-74: Consider using a timestamp type instead of raw float for notification timing.The
Notificationstruct storesstartTimeas a rawfloat. This works but could lose precision over long play sessions. For editor use this is likely acceptable.The notification system design is clean and self-contained.
80-96: Settings struct provides reasonable defaults.The default record markers (To Do, In Progress, Complete) with appropriate colors and sensible defaults for other settings are well thought out for the editor workflow.
src/WeatherEditor/Weather/WeatherWidget.cpp (3)
444-448: Bug: DALC loading writes to settingsDalc instead of reading from dalc.Line 448 assigns
dalc.fresnelPowertosettingsDalc.fresnelPower, but within theLoadWeatherValuesfunction context, it should read FROM the game data (dalc) INTO settings. The current code is correct. However, this differs from lines 357 inSetWeatherValueswhich does the opposite correctly.Actually, reviewing more carefully - line 448 reads
dalc.fresnelPowerand assigns tosettingsDalc.fresnelPower, which IS the correct direction for LoadWeatherValues.
196-251: Robust JSON deserialization with fallback behavior.The
LoadSettingsfunction properly validates loaded data, logs warnings for inconsistencies, and falls back to vanilla values on errors. This defensive approach prevents corrupted saves from breaking the editor.
962-1132: ImageSpace settings UI is well-structured with proper TOD table rendering.The
DrawImageSpaceSettingsfunction correctly handles null weather checks, uses the TOD table helpers consistently, and applies changes when auto-apply is enabled. The parameter organization (HDR, Cinematic, Tint, DOF) with visual separators improves usability.src/WeatherEditor/WeatherUtils.h (1)
31-78: Well-designed TOD namespace with clear documentation.The TOD namespace provides a clean abstraction for time-of-day operations with appropriate documentation comments for each function. The Period enum values (Sunrise, Day, Sunset, Night, Count) align well with Skyrim's 4-period weather system.
src/WeatherEditor/EditorWindow.cpp (3)
39-66: Well-implemented custom star icon drawing.The
DrawIconStarfunction correctly handles both filled and outlined rendering using triangle decomposition for the filled case (avoiding artifacts) and line segments for the outline.
1207-1240: Weather lock implementation is correct and handles edge cases.The
LockWeatherandUnlockWeathermethods properly manage weather state with null checks, logging, and correct use ofForceWeather/ReleaseWeatherOverride.
1269-1294: Vanity camera control preserves user settings correctly.The implementation saves the original delay value before modifying it and restores it properly, with appropriate guard checks to prevent double-disable/restore.
|
✅ A pre-release build is available for this PR: |
…skyrim-community-shaders into weather-editor-updated
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (4)
src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
232-246: Add null check and validate float-to-uint32 conversions.
SetLightingTemplateValues()dereferenceslightingTemplatewithout a null guard, unlikeLoadLightingTemplateValues()which has one. Additionally, the casts from float tostd::uint32_tat lines 244-245 could produce unexpected results for negative values.void LightingTemplateWidget::SetLightingTemplateValues() { + if (!lightingTemplate) { + logger::error("SetLightingTemplateValues called with null lightingTemplate"); + return; + } + auto& data = lightingTemplate->data; auto& dalc = lightingTemplate->directionalAmbientLightingColors; // ... - data.directionalXY = static_cast<std::uint32_t>(settings.directionalXY); - data.directionalZ = static_cast<std::uint32_t>(settings.directionalZ); + data.directionalXY = static_cast<std::uint32_t>(std::max(0.0f, settings.directionalXY)); + data.directionalZ = static_cast<std::uint32_t>(std::max(0.0f, settings.directionalZ));src/WeatherEditor/Widget.cpp (1)
234-256: Add null check forformpointer.This follows up on a previous review comment. The switch on
form->GetFormType()will crash ifformis nullptr. Add a guard at the start of the function.std::string Widget::GetFolderName() { + if (!form) { + return "Unknown"; + } switch (form->GetFormType()) {src/WeatherEditor/EditorWindow.cpp (1)
1607-1614: Incorrect exception type:dump()doesn't throwparse_error.Line 1611 catches
nlohmann::json::parse_error, butjson::dump()(called at line 1608) throwsnlohmann::json::type_errorwhen serialization fails, notparse_error. Theparse_errorexception is only thrown when parsing JSON from text (e.g., injson::parse()), not during serialization. This means serialization errors won't be caught.Fix the exception handler:
try { settingsFile << j.dump(1); - settingsFile.close(); -} catch (const nlohmann::json::parse_error& e) { +} catch (const nlohmann::json::exception& e) { logger::warn("Error parsing settings for settings file ({}) : {}\n", filePath, e.what()); settingsFile.close(); }Note: Consider using RAII (letting
ofstreamdestructor close the file) instead of explicitclose()calls to ensure cleanup on all paths.src/WeatherVariableRegistry.h (1)
3-11: Make the header self‑contained for json/std::lerpand drop unused<variant>.This header uses
nlohmann::json,std::lerp, andstd::is_floating_point_vbut doesn’t include their headers, and it still pulls in unused<variant>. That makes the header fragile and reliant on transitive/PCH includes.Consider:
#include <functional> #include <map> #include <memory> #include <string> -#include <variant> #include <vector> + +#include <type_traits> +#include <cmath> +#include <nlohmann/json.hpp> using json = nlohmann::json;Also confirm the project is built with a standard/library version that provides
std::lerp(C++20<cmath>).
🧹 Nitpick comments (23)
src/WeatherEditor/Weather/SimpleFormWidget.h (1)
1-4: Consider adding direct includes for dependencies.The file uses
std::string,std::format, and ImGui types but doesn't directly include<string>,<format>, or ImGui headers. While these are likely provided transitively throughWidget.h, following the "include what you use" principle improves clarity and reduces fragility if the base class changes its includes.#pragma once +#include <format> +#include <string> + #include "../Widget.h"src/WeatherEditor/Weather/ReferenceEffectWidget.cpp (1)
53-70: Add type validation for loaded forms.The code uses
LookupByID<T>which performs an implicit type check, but if the JSON contains an ID for a form of the wrong type, the result will benullptrwithout logging the type mismatch. Consider logging when forms fail to load to aid debugging.if (js.contains("artObject")) { std::string formIDStr = js["artObject"].get<std::string>(); if (formIDStr != "00000000") { uint32_t formID = std::stoul(formIDStr, nullptr, 16); settings.artObject = RE::TESForm::LookupByID<RE::BGSArtObject>(formID); if (!settings.artObject) { logger::warn("ReferenceEffect {}: Failed to load artObject with ID {:08X} - form not found or wrong type", GetEditorID(), formID); } } else { settings.artObject = nullptr; } }src/Features/WeatherEditor.h (2)
16-28: Add override keywords for consistency.The virtual methods override base Feature methods but lack the
overridekeyword. While not strictly required since they're markedvirtual, addingoverrideimproves clarity and enables compiler checks.- virtual inline std::string GetName() override { return "Weather Editor"; } - virtual inline std::string GetShortName() override { return "WeatherEditor"; } - virtual inline std::string_view GetShaderDefineName() override { return "WEATHER"; } - virtual inline std::string_view GetCategory() const override { return "Sky & Weather"; } - virtual inline std::pair<std::string, std::vector<std::string>> GetFeatureSummary() override + inline std::string GetName() override { return "Weather Editor"; } + inline std::string GetShortName() override { return "WeatherEditor"; } + inline std::string_view GetShaderDefineName() override { return "WEATHER"; } + inline std::string_view GetCategory() const override { return "Sky & Weather"; } + inline std::pair<std::string, std::vector<std::string>> GetFeatureSummary() override
32-32: Document LerpWeather method behavior.The
LerpWeathermethod signature suggests weather interpolation but lacks documentation on:
- Parameter meanings (fromWeather, toWeather, factor?)
- Expected factor range (0.0-1.0?)
- Side effects on game state
- Thread safety
Add a comment block:
/** * Interpolates between two weather states * @param fromWeather Source weather (factor=0.0) * @param toWeather Target weather (factor=1.0) * @param factor Interpolation factor [0.0, 1.0] */ void LerpWeather(RE::TESWeather*, RE::TESWeather*, float);src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
30-218: Consider refactoring repetitive slider code into helper function.The DrawWidget method contains ~14 very similar slider patterns. While functional, extracting a helper function would improve maintainability and reduce code duplication.
Consider a helper like:
bool DrawImageSpaceSlider(const char* label, const char* id, float& value, float min, float max, const char* format) { if (!MatchesSearch(label)) return false; ImGui::TableNextRow(); ImGui::TableSetColumnIndex(0); ImGui::Text("%s", label); ImGui::TableSetColumnIndex(1); ImGui::SetNextItemWidth(-1); return ImGui::SliderFloat(id, &value, min, max, format); }src/WeatherEditor/Weather/ReferenceEffectWidget.h (1)
8-12: Add null pointer guard in constructor for consistency.Other widgets in this PR (e.g.,
ImageSpaceWidget) validate the input pointer before storing it. For defensive programming and consistency, consider adding a null check here:ReferenceEffectWidget(RE::BGSReferenceEffect* a_referenceEffect) : referenceEffect(a_referenceEffect) { + if (!a_referenceEffect) { + logger::error("ReferenceEffectWidget created with null pointer"); + return; + } form = a_referenceEffect; }src/WeatherEditor/Weather/ImageSpaceWidget.h (1)
50-52: Remove redundantvirtualkeyword.When using
override, thevirtualkeyword is redundant sinceoverridealready implies the method is virtual. This is a minor style nit for consistency with the other widget classes in this PR.- virtual void DrawWidget() override; - virtual void LoadSettings() override; - virtual void SaveSettings() override; + void DrawWidget() override; + void LoadSettings() override; + void SaveSettings() override;src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
64-71: Consider consolidating duplicate MatchesSearch checks.The pattern of calling
MatchesSearch()twice per control (once for the control, once for spacing) is repeated throughout. A small helper could reduce this duplication:// Example consolidation auto DrawSearchableColor = [&](const char* label, float3& color) { if (MatchesSearch(label)) { if (WeatherUtils::DrawColorEdit(label, color)) changed = true; ImGui::Spacing(); } };This is a minor optional refactor that could be addressed in a follow-up.
src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (2)
140-159: Consider calling SaveSettings() in ApplyChanges().Unlike
LightingTemplateWidget::ApplyChanges()which callsSaveSettings()after applying, this widget doesn't persist the settings to JSON when applying. This could lead to inconsistent behavior where applied changes aren't saved to the JSON store.void VolumetricLightingWidget::ApplyChanges() { if (!volumetricLighting) return; // ... apply settings ... originalSettings = settings; + SaveSettings(); }
72-122: Consider using NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE for consistency.
LightingTemplateWidgetuses the nlohmann JSON macro for automatic serialization. This widget uses manual field-by-field JSON loading. While the manual approach handles partial JSON gracefully, using the macro would reduce code duplication and improve maintainability across widgets.src/WeatherEditor/Weather/CellLightingWidget.cpp (2)
349-415: Consider calling SaveSettings() after applying changes.Similar to the feedback on
VolumetricLightingWidget, this doesn't persist settings to JSON when applying. For consistency withLightingTemplateWidget::ApplyChanges():// Apply inheritance flags // ... originalSettings = settings; + SaveSettings(); }
422-457: Consider implementingoperator==for Settings struct.The long chain of comparisons could be simplified by implementing an equality operator on the
Settingsstruct. This would also make the comparison more maintainable as fields are added:bool operator==(const Settings& other) const = default; // C++20 // or manual implementation for older standardsThen:
return !(settings == originalSettings);src/WeatherEditor/Weather/VolumetricLightingWidget.h (1)
8-12: Add nullptr validation in constructor.Similar to the issue flagged for
LightingTemplateWidget, this constructor accepts a raw pointer without validation. If nullptr is passed, subsequent operations onvolumetricLightingwill crash. Apply the same defensive pattern used inLightingTemplateWidget:VolumetricLightingWidget(RE::BGSVolumetricLighting* a_volumetricLighting) : volumetricLighting(a_volumetricLighting) { + if (!a_volumetricLighting) { + logger::error("VolumetricLightingWidget created with null pointer"); + return; + } form = a_volumetricLighting; }src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
193-207: Consider using a tolerance-based comparison for floats.Comparing floats with
!=can produce unexpected results due to floating-point precision. While this may work in practice since values are round-tripped through the same path, a more robust approach would use a helper ormemcmpon the entire struct.One option is to define
operator==for theSettingsstruct:bool operator==(const Settings& other) const = default;Then simplify:
bool PrecipitationWidget::HasUnsavedChanges() const { return !(settings == originalSettings); }src/WeatherEditor/Widget.cpp (2)
173-177: MakeGetFolderName()const to avoid const_cast.The
const_casthere is a code smell indicating thatGetFolderName()should be aconstmethod. Since it only readsform->GetFormType(), there's no reason it needs to be non-const.In
Widget.h, change the declaration to:std::string GetFolderName() const;Then update
Widget.cpp:-std::string Widget::GetFolderName() +std::string Widget::GetFolderName() constAnd simplify
HasSavedFile:- std::string filePath = std::format("{}\\{}\\{}.json", Util::PathHelpers::GetCommunityShaderPath().string(), const_cast<Widget*>(this)->GetFolderName(), GetEditorID()); + std::string filePath = std::format("{}\\{}\\{}.json", Util::PathHelpers::GetCommunityShaderPath().string(), GetFolderName(), GetEditorID());
37-41: Redundant failure check afteris_open()andgood().The
settingsFile.fail()check on line 37 is redundant immediately after checking!settingsFile.good() || !settingsFile.is_open()on line 32. If the file failed to open, the earlier check would have already returned.- if (settingsFile.fail()) { - logger::warn("Unable to create settings file: {}", file); - settingsFile.close(); - return; - }src/WeatherEditor/Weather/LightingTemplateWidget.h (1)
57-64: Missingoverridekeyword onApplyChangesandRevertChanges.If
Widgetbase class declaresApplyChanges()andRevertChanges()as virtual (which it appears to based on other widgets), these should have theoverridespecifier for consistency and compile-time safety.- void ApplyChanges(); - void RevertChanges(); + void ApplyChanges() override; + void RevertChanges() override;src/WeatherEditor/WeatherUtils.cpp (1)
149-177: Static maps for debouncing may grow unboundedly over time.The
pendingValuesandlastChangeTimestatic maps accumulate entries but only remove them after the debounce delay. If the UI changes labels dynamically or if tracking never completes (e.g., user closes UI mid-edit), entries may persist indefinitely.Consider periodically pruning stale entries or using a bounded cache structure.
bool DrawSliderInt8(const std::string& label, int& property) { static std::map<std::string, int> pendingValues; static std::map<std::string, double> lastChangeTime; const double debounceDelay = 2.0; + const double staleThreshold = 60.0; // Prune entries older than 60 seconds bool changed = ImGui::SliderInt(label.c_str(), &property, -128, 127); if (changed) { pendingValues[label] = property; lastChangeTime[label] = ImGui::GetTime(); } // Check for any pending values that should be tracked std::vector<std::string> toTrack; + std::vector<std::string> toRemove; for (const auto& [key, changeTime] : lastChangeTime) { - if (ImGui::GetTime() - changeTime >= debounceDelay) { + double age = ImGui::GetTime() - changeTime; + if (age >= debounceDelay) { toTrack.push_back(key); + } else if (age >= staleThreshold) { + toRemove.push_back(key); } } + + // Remove stale entries + for (const auto& key : toRemove) { + pendingValues.erase(key); + lastChangeTime.erase(key); + }src/WeatherEditor/PaletteWindow.cpp (3)
4-5: Consider movingDrawIconStardeclaration to a shared header.Forward-declaring a function from another
.cppfile creates a fragile dependency. IfDrawIconStar's signature changes inEditorWindow.cpp, this forward declaration won't trigger a compile error.
363-412: FrequentSave()calls on every tracked usage may impact performance.Both
TrackColorUsageandTrackValueUsagecallSave()immediately after updating entries. With the 2-second debounce in the slider/color helpers, this means a save occurs every time a user finishes adjusting a control. Consider batching saves or using a dirty flag with periodic flush.+private: + bool dirty = false; + double lastSaveTime = 0.0; + static constexpr double saveInterval = 5.0; // Save at most every 5 seconds + +public: + void FlushIfNeeded() { + if (dirty && ImGui::GetTime() - lastSaveTime >= saveInterval) { + Save(); + dirty = false; + lastSaveTime = ImGui::GetTime(); + } + } void PaletteWindow::TrackColorUsage(const float3& color) { // ... existing tracking logic ... - Save(); + dirty = true; }Then call
FlushIfNeeded()fromDraw().
273-316: Considerstd::partial_sortfor efficiency with large palettes.When only the top N entries are needed,
std::partial_sortis more efficient than full sorting. This is a minor optimization for current expected palette sizes.std::vector<PaletteWindow::ColorEntry*> PaletteWindow::GetRecentColors(int count) { - std::vector<ColorEntry*> result; std::vector<ColorEntry*> allColors; - for (auto& entry : colorEntries) { allColors.push_back(&entry); } - std::sort(allColors.begin(), allColors.end(), [](ColorEntry* a, ColorEntry* b) { + size_t n = std::min(static_cast<size_t>(count), allColors.size()); + std::partial_sort(allColors.begin(), allColors.begin() + n, allColors.end(), [](ColorEntry* a, ColorEntry* b) { return a->lastUsedTime > b->lastUsedTime; }); - for (int i = 0; i < std::min(count, (int)allColors.size()); i++) { - result.push_back(allColors[i]); - } - - return result; + allColors.resize(n); + return allColors; }src/WeatherVariableRegistry.h (1)
218-279: Global registry design is reasonable for single‑threaded use; consider future thread‑safety if that changes.
GlobalWeatherRegistry’s singleton and map ofunique_ptr<FeatureWeatherRegistry>is fine given the documented single‑threaded (main game thread) access pattern. If you ever move weather updates or editor operations off the main thread, you’ll need synchronization aroundfeatureRegistriesandGetOrCreateFeatureRegistry.docs/weather-system-docs/WeatherVariableRegistration.md (1)
100-107: Clean up minor text artifact in behavior bullet.The bullet:
Handles default values and missing dataanced Usagelooks like a merge artifact. Suggest:
- - Handles default values and missing dataanced Usage + - Handles default values and missing dataThe following “Custom Variable Types” section already clearly marks the start of more advanced usage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
docs/weather-system-docs/WeatherVariableRegistration.md(1 hunks)src/Deferred.cpp(2 hunks)src/Features/WeatherEditor.cpp(1 hunks)src/Features/WeatherEditor.h(1 hunks)src/Utils/Form.cpp(1 hunks)src/Utils/Serialize.cpp(1 hunks)src/Utils/UI.cpp(6 hunks)src/Utils/UI.h(1 hunks)src/WeatherEditor/EditorWindow.cpp(1 hunks)src/WeatherEditor/EditorWindow.h(1 hunks)src/WeatherEditor/PaletteWindow.cpp(1 hunks)src/WeatherEditor/PaletteWindow.h(1 hunks)src/WeatherEditor/Weather/CellLightingWidget.cpp(1 hunks)src/WeatherEditor/Weather/CellLightingWidget.h(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.cpp(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.h(1 hunks)src/WeatherEditor/Weather/LensFlareWidget.cpp(1 hunks)src/WeatherEditor/Weather/LensFlareWidget.h(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.cpp(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.h(1 hunks)src/WeatherEditor/Weather/PrecipitationWidget.cpp(1 hunks)src/WeatherEditor/Weather/PrecipitationWidget.h(1 hunks)src/WeatherEditor/Weather/ReferenceEffectWidget.cpp(1 hunks)src/WeatherEditor/Weather/ReferenceEffectWidget.h(1 hunks)src/WeatherEditor/Weather/SimpleFormWidget.h(1 hunks)src/WeatherEditor/Weather/VolumetricLightingWidget.cpp(1 hunks)src/WeatherEditor/Weather/VolumetricLightingWidget.h(1 hunks)src/WeatherEditor/Weather/WeatherWidget.cpp(1 hunks)src/WeatherEditor/Weather/WeatherWidget.h(1 hunks)src/WeatherEditor/WeatherUtils.cpp(1 hunks)src/WeatherEditor/WeatherUtils.h(1 hunks)src/WeatherEditor/Widget.cpp(1 hunks)src/WeatherEditor/Widget.h(1 hunks)src/WeatherVariableRegistry.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Deferred.cpp
- src/Utils/Form.cpp
- src/Utils/Serialize.cpp
- src/WeatherEditor/Widget.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Utils/UI.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/LensFlareWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.hsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.hsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hsrc/WeatherEditor/Weather/SimpleFormWidget.hsrc/WeatherEditor/Weather/CellLightingWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherVariableRegistry.hsrc/WeatherEditor/PaletteWindow.hsrc/WeatherEditor/PaletteWindow.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Utils/UI.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/LensFlareWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.hsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.hsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hsrc/WeatherEditor/Weather/SimpleFormWidget.hsrc/WeatherEditor/Weather/CellLightingWidget.hsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherVariableRegistry.hsrc/WeatherEditor/PaletteWindow.hsrc/WeatherEditor/PaletteWindow.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Utils/UI.hsrc/WeatherEditor/Weather/ReferenceEffectWidget.cppsrc/Utils/UI.cppsrc/WeatherEditor/Weather/LensFlareWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.hsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/ReferenceEffectWidget.hsrc/Features/WeatherEditor.hsrc/WeatherEditor/Weather/ImageSpaceWidget.hsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Widget.cppsrc/WeatherEditor/Weather/LensFlareWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hsrc/WeatherEditor/Weather/SimpleFormWidget.hsrc/WeatherEditor/Weather/CellLightingWidget.hdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/Weather/LightingTemplateWidget.hsrc/WeatherEditor/WeatherUtils.hsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherVariableRegistry.hsrc/WeatherEditor/PaletteWindow.hsrc/WeatherEditor/PaletteWindow.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
src/Utils/UI.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/Features/WeatherEditor.hsrc/Features/WeatherEditor.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.hdocs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.hsrc/WeatherEditor/Weather/WeatherWidget.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/Features/WeatherEditor.h
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/WeatherVariableRegistry.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (40)
src/WeatherEditor/Weather/SimpleFormWidget.h (5)
5-10: LGTM!The class declaration and lifecycle management are clean and follow best practices with defaulted special members and proper use of the
overridekeyword.
12-17: LGTM!The method properly handles null pointer inputs with appropriate fallbacks, providing defensive programming for a robust display widget.
23-30: LGTM!The ImGui rendering logic is clean and provides helpful user guidance. The consistent hex formatting and informational text enhance the user experience.
32-39: LGTM!The no-op overrides are appropriate for a read-only widget, and the private member initialization is clean with proper encapsulation.
19-21: Verify C++20 compatibility across the project's build configuration.Line 20 uses
std::format, which requires C++20. Confirm that the project's CMake configuration or build system consistently targets C++20 or later for all supported platforms (SE/AE/VR). If the project supports older C++ standards, provide an alternative implementation or conditional compilation wrapper.src/Utils/UI.h (1)
254-257: LGTM! DDS texture loading function properly declared.The new
LoadDDSTextureFromFilefunction mirrors the existingLoadTextureFromFilesignature, maintaining API consistency for texture loading operations.src/WeatherEditor/Weather/PrecipitationWidget.h (2)
8-12: LGTM! Constructor properly initializes members.The constructor correctly initializes both the
precipitationmember and the inheritedformmember, maintaining consistency with other widget implementations.
26-41: Clarify expected texture path format in particleTexture field documentation.The
particleTexturefield lacks documentation on its expected format. Add an inline comment specifying whether it should be a relative file path, BSA resource reference, or other format, and describe how it's resolved at runtime.src/Utils/UI.cpp (3)
165-242: LGTM! DDS texture loading properly implemented with robust error handling.The implementation follows a safe pattern:
- Parameter validation at entry
- BSA resource loading via Skyrim's native system
- Exception-safe DirectX operations with proper cleanup
- Texture resource properly released after SRV creation
- Comprehensive logging for diagnostics
261-320: Icon loading expanded correctly with proper error handling.The code properly:
- Increases capacity from 16 to 18 icons
- Adds deleteSettings, applyToGame, and pauseTime icons
- Maintains individual icon loading with logging
- Reports total loaded count dynamically
486-487: Category rename aligns with PR objectives.The category name change from "Sky" to "Sky & Weather" properly reflects the expanded scope of weather-related features introduced in this PR.
src/WeatherEditor/Weather/LensFlareWidget.h (2)
8-12: LGTM! Constructor follows consistent widget pattern.Properly initializes both the specific
lensFlaremember and the inheritedformmember, maintaining consistency across all widget implementations.
26-30: LGTM! Settings structure is appropriately minimal.The lens flare settings are concise and use sensible defaults (1.0 for scale, 0.2 for influence), which are typical for this type of effect parameter.
src/WeatherEditor/Weather/CellLightingWidget.h (2)
8-12: LGTM! Constructor properly initializes cell reference.Follows the consistent pattern of initializing both the specific widget member (
cell) and the inheritedformmember.
26-66: LGTM! Comprehensive cell lighting settings structure.The Settings struct properly covers:
- Interior lighting data (ambient, directional, fog)
- Directional ambient lighting colors (DALC)
- Inheritance flags for each property
All defaults are sensible (colors at 1.0, distances at 0/10000, flags at false).
src/WeatherEditor/Weather/ReferenceEffectWidget.cpp (2)
5-44: LGTM! Widget rendering properly structured.The DrawWidget implementation:
- Safely checks for empty form collections before rendering pickers
- Uses cached form pickers from EditorWindow
- Respects auto-apply setting
- Provides clear UI feedback
100-117: LGTM! Changes properly applied to game data.The ApplyChanges method correctly:
- Writes settings to the underlying RE::BGSReferenceEffect
- Resets flags before setting to avoid stale state
- Updates originalSettings to reflect current state
src/WeatherEditor/Weather/ImageSpaceWidget.cpp (3)
221-233: LGTM! LoadSettings properly simplified based on previous feedback.The validation now correctly accepts Settings when it exists and is a JSON object, avoiding the previous issue of rejecting legitimate zero-valued settings. Good improvement!
240-270: LGTM! SetImageSpaceValues correctly writes to game data.The method properly:
- Guards against null imageSpace
- Writes all settings to the appropriate data fields
- Maintains data structure consistency
272-302: LGTM! LoadImageSpaceValues correctly reads from game data.Mirrors SetImageSpaceValues correctly, reading all data fields back into settings. The symmetry ensures data integrity.
src/Features/WeatherEditor.h (1)
18-18: Consider using a prefixed shader define name to avoid potential naming conflicts.Industry best practices recommend using a project or feature-specific prefix for shader defines (e.g.,
CR_WEATHERor similar) rather than generic names likeWEATHER. While "WEATHER" is domain-specific to this feature, unprefixed names increase the risk of collision with other features, third-party integrations, or driver-injected defines. Check if your project uses a standard prefix convention (the codebase may already useCR_prefix for other defines).src/WeatherEditor/Weather/ReferenceEffectWidget.h (1)
1-40: LGTM - well-structured widget header.The class follows the established Widget pattern with proper overrides and a clean separation of public interface and private implementation details.
src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
303-315: LGTM - Clean settings persistence pattern.The LoadSettings/SaveSettings implementation correctly uses the nlohmann JSON macros for automatic serialization, with a sensible fallback to loading from the game form when no JSON data exists.
src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (1)
166-180: Float comparison is acceptable here, but consider epsilon-based comparison for robustness.Direct float equality works because these values are copied directly without computation. However, if future changes introduce any float arithmetic, consider using an epsilon-based comparison or a structured comparison operator.
src/WeatherEditor/Weather/CellLightingWidget.cpp (1)
5-25: LGTM - Proper ImGui Begin/End handling with early exits.The early return paths correctly call
ImGui::End()before returning, preventing ImGui stack corruption. The validation for interior cells and lighting availability provides clear user feedback.src/WeatherEditor/Weather/ImageSpaceWidget.h (1)
54-57: Add missingoverridespecifiers.Based on
ReferenceEffectWidget.h,ApplyChanges()andRevertChanges()appear to be virtual methods in the baseWidgetclass. Addoverridefor consistency and compile-time verification:void SetImageSpaceValues(); void LoadImageSpaceValues(); - void ApplyChanges(); - void RevertChanges(); + void ApplyChanges() override; + void RevertChanges() override;src/WeatherEditor/Weather/LensFlareWidget.cpp (1)
5-75: LGTM - Clean and consistent implementation.The widget follows the established pattern well with proper null checks, JSON loading with fallback, and auto-apply support. The implementation is straightforward and maintainable.
src/WeatherEditor/Weather/LightingTemplateWidget.h (1)
10-19: Null check added, but consider a more robust error-handling pattern.The early return prevents crashes but leaves the widget in an inconsistent state. Since this is runtime plugin code (
src/**/*.h), the coding guidelines require robust error handling with graceful degradation. The current pattern—where the constructor "succeeds" but the object is partially initialized—forces every method to check for null state.Instead, leverage one of these patterns:
- Throw an exception in the constructor if initialization fails (cleanest for invalid states that should not occur)
- Use a factory function returning
std::optional<LightingTemplateWidget>or a raw pointer to signal creation failure- Validate before construction so the constructor receives only valid pointers
Verify that all public methods (
DrawWidget,LoadSettings, etc.) handle the null state consistently, or adopt one of the above patterns to eliminate this responsibility at the call site.src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
183-183: No action required; assignment is safe.The
textureNamemember is aRE::BSFixedString, which manages string lifetime through CommonLibSSE's string-pooling mechanism. When assigned fromc_str(), theBSFixedString::operator=calls the constructor that registers the string in the pool with reference counting. The string pool, not the caller, manages the lifetime, so there is no risk of a dangling pointer.Likely an incorrect or invalid review comment.
src/WeatherEditor/PaletteWindow.h (1)
1-60: LGTM! Clean header design for the palette window.The singleton pattern, data structures, and API surface are well-defined. The use of
std::optionalfor favorite color slots is a nice touch for representing empty states.src/Features/WeatherEditor.cpp (2)
59-125: Comprehensive weather interpolation implementation.The function properly handles all weather property types with appropriate interpolation methods. The use of enum constants for loop bounds (
RE::TESWeather::ColorTypes::kTotal, etc.) ensures maintainability.
127-163: LGTM! Proper null safety in weather status panel.The implementation correctly handles null weather pointers and missing editor IDs with appropriate fallbacks.
src/WeatherEditor/WeatherUtils.cpp (3)
293-327: Time-of-day factor calculation looks correct.The implementation properly handles the 4-period system with smooth transitions between adjacent periods. The boundary conditions at period transitions are handled correctly.
444-560: LGTM! Well-implemented TOD color row with proper UX features.The implementation includes drag-and-drop, Ctrl+Z undo in popups, palette tracking, and proper alignment with the TOD header. The static state management for popup tracking is appropriate for ImGui patterns.
942-977: LGTM! Clean search bar implementation with keyboard shortcut support.The Ctrl+F activation pattern with deferred focus setting is a proper ImGui idiom.
src/WeatherEditor/WeatherUtils.h (3)
94-157: LGTM! Well-designed template for cached form picker UI.The implementation properly handles null forms, provides fallback to FormID display, and follows standard ImGui combo patterns. The caller is responsible for ensuring type compatibility with the widget container.
159-222: LGTM! Solid fallback form picker implementation.The lambda for safe editor ID retrieval handles null pointers and empty strings correctly. Good documentation noting this is the slower legacy path.
19-25: Verify thatControlTypeenum is used elsewhere in the codebase.This enum is declared in WeatherUtils.h but not referenced within the header itself. Determine if it's used in other source files; if so, add a brief comment indicating its purpose or the context where it's applied. If unused, remove it.
src/WeatherVariableRegistry.h (2)
34-48: Good default lerp behavior and type safety inWeatherVariable<T>.The constructor’s defaulting of
lerpFuncfor floating‑pointTusingstd::lerpis clean and makes non‑float types opt‑in. Combined with the raw pointer to feature state anddefaultValue, it keeps registration simple while avoiding implicit behavior for complex types.
50-76: Robust JSON error handling inLerpandLoadFromJson.The try/catch around
get<T>()withlogger::debugand fallback todefaultValue(or preserving current value) is a solid improvement for dealing with corrupted or mismatched JSON without crashing or leaving uninitialized state.Also applies to: 86-96
…skyrim-community-shaders into weather-editor-updated
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/UI.cpp (1)
261-288: Include new pause/undo textures in cleanup list to avoid GPU leaks
texturePointersnow includes 18 entries but still omitspauseTimeandundo, which you load later. IfInitializeMenuIconsis called multiple times, previously created SRVs for these two icons will never be released.Recommend:
- std::array<ID3D11ShaderResourceView**, 18> texturePointers = { + std::array<ID3D11ShaderResourceView**, 20> texturePointers = { &menu->uiIcons.saveSettings.texture, &menu->uiIcons.loadSettings.texture, &menu->uiIcons.deleteSettings.texture, &menu->uiIcons.clearCache.texture, &menu->uiIcons.logo.texture, &menu->uiIcons.featureSettingRevert.texture, &menu->uiIcons.applyToGame.texture, &menu->uiIcons.discord.texture, &menu->uiIcons.characters.texture, &menu->uiIcons.display.texture, &menu->uiIcons.grass.texture, &menu->uiIcons.lighting.texture, &menu->uiIcons.sky.texture, &menu->uiIcons.landscape.texture, &menu->uiIcons.water.texture, &menu->uiIcons.debug.texture, &menu->uiIcons.materials.texture, - &menu->uiIcons.postProcessing.texture + &menu->uiIcons.postProcessing.texture, + &menu->uiIcons.pauseTime.texture, + &menu->uiIcons.undo.texture };This keeps the release logic in sync with all icons you load below.
♻️ Duplicate comments (3)
src/Utils/UI.cpp (1)
165-242: Handle potential partial reads and add a small safety net around the DDS load pathTwo points worth tightening in
LoadDDSTextureFromFile:
Verify that the stream read fills the entire buffer (same concern as earlier review).
BSResourceNiBinaryStream::read(viaNiBinaryStream) is allowed to return fewer bytes than requested. Right now you ignore the return value, so a short read would silently feed truncated data intoDirectX::LoadFromDDSMemory, which can lead to sporadic failures or corrupted textures instead of a clean error.Suggest:
- ddsData.resize(size); - bsaStream.read(reinterpret_cast<char*>(ddsData.data()), size);
ddsData.resize(size);auto bytesRead = bsaStream.read(reinterpret_cast<char*>(ddsData.data()), size);if (bytesRead != size) {logger::warn("LoadDDSTextureFromFile: Incomplete read for {} – expected {} bytes, got {}",filename,size,bytesRead);return false;}This keeps the function’s behaviour consistent and avoids using incomplete DDS data. 2. **(Optional) Guard against non‑2D DDS content.** `DirectX::CreateTexture` can, in principle, produce non‑2D resources depending on DDS metadata. Since you `reinterpret_cast` to `ID3D11Texture2D` and build a `TEXTURE2D` SRV, you may want to assert or early‑out if `image.GetMetadata().dimension != TEX_DIMENSION_TEXTURE2D` to fail fast on unexpected assets. ```diff + const auto& meta = image.GetMetadata(); + if (meta.dimension != DirectX::TEX_DIMENSION_TEXTURE2D) { + logger::warn("LoadDDSTextureFromFile: {} is not a 2D texture (dimension={})", + filename, static_cast<int>(meta.dimension)); + return false; + }This is a defensive check and can be skipped if you know these DDS files are always 2D.
src/WeatherEditor/EditorWindow.cpp (2)
490-698: Duplicate widget list rendering loops – items rendered twiceThere are two “Filtered display of widgets – regular list” loops back‑to‑back:
- Lines 490–608: full list, including weather‑specific tooltip logic.
- Lines 611–698: almost identical filtering and rendering logic, but without the extra tooltip.
This causes each eligible widget row to be processed and drawn twice, leading to duplicated entries and inconsistent behavior. A similar issue was flagged in a previous review and appears to have resurfaced.
A straightforward fix is to keep a single loop (e.g., the first one that also handles weather tooltips) and delete the second block:
- // Filtered display of widgets - regular list - for (int i = 0; i < sortedWidgets.size(); ++i) { - // Skip current cell's lighting template if already shown - if (currentCellLightingTemplate && selectedCategory == "Lighting Template") { - auto* ltWidget = dynamic_cast<LightingTemplateWidget*>(sortedWidgets[i]); - if (ltWidget && ltWidget->lightingTemplate == currentCellLightingTemplate) - continue; - } - - if (!ContainsStringIgnoreCase(sortedWidgets[i]->GetEditorID(), filterBuffer)) - continue; - - // Apply quick filters - if (showOnlyFavorites && !IsFavorite(sortedWidgets[i]->GetEditorID())) - continue; - if (showOnlyFlagged && settings.markedRecords.find(sortedWidgets[i]->GetEditorID()) == settings.markedRecords.end()) - continue; - // ... rows 629–697 ... - }
1615-1670: JSON load uses wrong stream operator; catch type on save is too narrowTwo related issues around settings persistence:
- Load direction is reversed
j << settingsFile;For nlohmann::json, the standard pattern is
settingsFile >> j;(istream >> json) when reading, andostream << j;when writing.j << settingsFile;is the wrong direction and will not deserialize as intended.
- Save catch uses
parse_errorfordump()In
Save():try { settingsFile << j.dump(1); settingsFile.close(); } catch (const nlohmann::json::parse_error& e) { ... }
dump()does not throwparse_error; failures are reported vianlohmann::json::exceptionsubclasses liketype_error. Catching onlyparse_errormeans some serialization failures won’t be logged. This was previously noted in an earlier review.Suggested fix:
- try { - settingsFile << j.dump(1); - settingsFile.close(); - } catch (const nlohmann::json::parse_error& e) { - logger::warn("Error parsing settings for settings file ({}) : {}\n", filePath, e.what()); - settingsFile.close(); - } + try { + settingsFile << j.dump(1); + settingsFile.close(); + } catch (const nlohmann::json::exception& e) { + logger::warn("Error writing settings file ({}) : {}\n", filePath, e.what()); + }And in
Load():- try { - j << settingsFile; - settingsFile.close(); - } catch (const nlohmann::json::parse_error& e) { + try { + settingsFile >> j; + settingsFile.close(); + } catch (const nlohmann::json::parse_error& e) { logger::warn("Error parsing settings for file ({}) : {}\n", filePath, e.what()); - settingsFile.close(); }
🧹 Nitpick comments (5)
src/Utils/UI.cpp (1)
1-3: Optional: align PR metadata with Conventional Commits and issuesThe current title
feat: weather and imagespace editoris already close to Conventional Commits. If you want to be stricter and add scope, something like:
feat(ui): add weather and imagespace editorwould fit the pattern.
If this work corresponds to a tracked feature or issue, consider referencing it in the PR description with one of:
Implements #<id>/Addresses #<id>for featuresRelated to #<id>if it’s only part of a larger effortto make traceability clearer.
src/WeatherEditor/Weather/WeatherWidget.cpp (1)
1631-1660: Cache failed texture loads to avoid repeated attempts.When
LoadDDSTextureFromFilefails (line 1654), the function returnsnullptrwithout caching the failure. This causes the same failed texture to be loaded from disk/BSA every frame, potentially impacting performance for weathers with invalid cloud texture paths.Apply this diff to cache failed loads:
if (Util::LoadDDSTextureFromFile(globals::d3d::device, resourcePath.c_str(), &srv, textureSize)) { cloudTextureCache[layerIndex] = srv; return srv; } + // Cache the failure to avoid repeated load attempts + cloudTextureCache[layerIndex] = nullptr; return nullptr; }Then update the cache check at line 1633:
if (cloudTextureCache.contains(layerIndex)) { return cloudTextureCache[layerIndex]; }This already returns
nullptrcorrectly if cached.src/WeatherEditor/Widget.h (1)
18-21: ClarifyGetNewIDsemantics; avoid-uniqueID++
return -uniqueID++;is hard to read and yields0on the first call (then-1, -2, ...). If the intent is to always return negative IDs, it’d be clearer and less error‑prone to pre‑decrement:- int GetNewID() - { - return -uniqueID++; - } + int GetNewID() + { + // Generate strictly negative IDs: -1, -2, -3, ... + return --uniqueID; + }src/WeatherEditor/EditorWindow.cpp (2)
10-10: Settings serialization omits palette fields
EditorWindow::SettingsincludespaletteColors,paletteValues, andpaletteFavorites, butNLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULTonly lists the non‑palette members. That means any palette data stored insettingswill never be serialized/deserialized viaj = settings/settings = j.If these palette fields are meant to persist, they should be added to the macro; if they’re intentionally transient, consider moving them out of
Settingsto avoid confusion.
1878-1908: Session/favorites changes only update in‑memory JSON, not disk
AddToRecentandToggleFavoritecallSaveSettings()(i.e.,j = settings;) but don’t callSave(). Similarly,SaveSessionWidgets()ends withSaveSettings()only. This means:
- Recent items, favorites, and last‑open widgets are kept in memory for the current run,
- But they won’t be flushed to disk unless something else later calls
Save().If the intent is to persist these across game sessions, consider invoking
Save()at the end of these methods or at a well‑defined “session end” point (e.g., when the editor closes).void EditorWindow::ToggleFavorite(const std::string& widgetId) { ... - SaveSettings(); + Save(); }Same idea for
AddToRecentandSaveSessionWidgets.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package/Interface/CommunityShaders/Icons/Action Icons/undo.pngis excluded by!**/*.png
📒 Files selected for processing (7)
src/Menu.cpp(4 hunks)src/Menu.h(1 hunks)src/Utils/UI.cpp(6 hunks)src/WeatherEditor/EditorWindow.cpp(1 hunks)src/WeatherEditor/EditorWindow.h(1 hunks)src/WeatherEditor/Weather/WeatherWidget.cpp(1 hunks)src/WeatherEditor/Widget.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Menu.cpp
- src/Menu.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Utils/UI.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Utils/UI.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/Utils/UI.cppsrc/WeatherEditor/Widget.hsrc/WeatherEditor/EditorWindow.hsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
🧠 Learnings (4)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-05-31T02:51:24.195Z
Learnt from: davo0411
Repo: doodlum/skyrim-community-shaders PR: 1107
File: src/Menu.cpp:310-315
Timestamp: 2025-05-31T02:51:24.195Z
Learning: In the Community Shaders codebase, when validating UI icons, checking for texture presence (non-null pointer) is sufficient. Avoid adding dimension validation checks as icon sizes should vary based on user needs (4K+ resolution users may want higher resolution icons). Don't artificially limit icon dimensions.
Applied to files:
src/Utils/UI.cpp
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
src/WeatherEditor/Weather/WeatherWidget.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (4)
src/Utils/UI.cpp (3)
5-14: DirectXTex and WRL includes look appropriate for new DDS pathThe added DirectXTex and WRL headers match the new DDS loader usage in this file; nothing else to change here from a UI/D3D11 standpoint.
311-321: New action icons wiring and summary logging look consistentThe additional action icons (
delete,apply-to-game,pause,undo) are wired through the existingloadIconWithLogginghelper and are covered by the finaliconsLoadedsummary. Paths and fields line up with the struct members used elsewhere, and failure handling matches the existing pattern.Also applies to: 349-351
473-499: Category name remap to "Sky & Weather" is correct and backwards-compatibleUpdating the category check to
"Sky & Weather"while still mapping to the existingmenu.sky.texturekeeps icon usage consistent with the renamed category. This looks aligned with the new Sky & Weather grouping introduced by the PR.src/WeatherEditor/EditorWindow.h (1)
26-45: Ownership and widget container design look solidUsing
std::unique_ptr<Widget>for all widget collections plus documenting ownership oftempTextureandcurrentCellLightingWidgetmakes lifetime management clear; this pairs well with the destructor in the.cpp. No changes needed here from an ownership/API perspective.
…skyrim-community-shaders into weather-editor-updated
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
src/WeatherEditor/Weather/CellLightingWidget.cpp (1)
261-265: JSON parse failure still leaves settings in inconsistent state.The catch block logs the error but doesn't actually fall through to load from the form as the comment suggests. If JSON parsing partially succeeds before throwing,
settingswill contain a mix of JSON and default values.src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
233-265: Add null check forlightingTemplatebefore dereferencing.
SetLightingTemplateValues()dereferenceslightingTemplateat line 235 without a null check, whileLoadLightingTemplateValues()has one. Additionally, the float-to-uint32_t casts at lines 245-246 could produce unexpected results with negative values.void LightingTemplateWidget::SetLightingTemplateValues() { + if (!lightingTemplate) + return; + auto& data = lightingTemplate->data; auto& dalc = lightingTemplate->directionalAmbientLightingColors; // ... existing code ... - data.directionalXY = static_cast<std::uint32_t>(settings.directionalXY); - data.directionalZ = static_cast<std::uint32_t>(settings.directionalZ); + data.directionalXY = static_cast<std::uint32_t>(std::max(0.0f, settings.directionalXY)); + data.directionalZ = static_cast<std::uint32_t>(std::max(0.0f, settings.directionalZ));src/WeatherEditor/Weather/WeatherWidget.cpp (2)
405-406: Remove unused logic or implement intended behavior.The
recordChangedvariable is tracked throughout the Records tab (lines 227, 244, 256, 291, 333) but this empty conditional does nothing. Either remove the unused variable and checks, or implement the intended action (e.g., save settings, apply changes).Apply this diff to remove the unused code:
- if (recordChanged) { - } - ImGui::EndTabItem();
658-658: Critical: Fresnel power assignment is still backwards.Line 658 assigns
dalc.fresnelPower = settingsDalc.fresnelPower(from settings to game), butLoadWeatherValues()should load FROM game TO settings. This will fail to load the fresnel power value from the weather form.Apply this diff to correct the assignment direction:
auto& dalc = weather->directionalAmbientLightingColors[i]; auto& settingsDalc = settings.dalc[i]; - dalc.fresnelPower = settingsDalc.fresnelPower; + settingsDalc.fresnelPower = dalc.fresnelPower;src/WeatherEditor/EditorWindow.cpp (3)
660-660: Guard against nulltempTexturebefore dereferencing.
tempTexture->srv.get()is called without checking iftempTextureis initialized. IfShowViewportWindow()is called beforeDraw()creates the texture (line 1334), this will crash.Apply this diff to add a null check:
- ImGui::Image((void*)tempTexture->srv.get(), imageSize); + if (tempTexture && tempTexture->srv) { + ImGui::Image((void*)tempTexture->srv.get(), imageSize); + } else { + ImGui::TextUnformatted("Viewport not ready"); + }
668-726: Ctrl+W closes first open widget per category, not the focused widget.The loops check
ImGui::IsWindowFocused(ImGuiFocusedFlags_AnyWindow)which is true if any window is focused, not specifically the widget's window. This can close the first open widget in each category rather than just the focused one. Consider tracking the last focused widget centrally and closing only that one.
1580-1587: Exception type mismatch:dump()doesn't throwparse_error.
json::dump()can throwtype_error(notparse_error) if serialization fails. The catch block won't handle dump failures properly.Apply this diff to fix the exception handling:
try { settingsFile << j.dump(1); settingsFile.close(); -} catch (const nlohmann::json::parse_error& e) { +} catch (const nlohmann::json::exception& e) { logger::warn("Error parsing settings for settings file ({}) : {}\n", filePath, e.what()); settingsFile.close(); }
🧹 Nitpick comments (5)
src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (1)
125-139: Consider adding null check for consistency with ApplyChanges.
SaveSettings()doesn't check for nullvolumetricLightingbefore saving, whileApplyChanges()does. AlthoughSaveSettingsonly writes to JSON (not dereferencingvolumetricLighting), adding a guard would maintain consistency with the pattern used elsewhere.void VolumetricLightingWidget::SaveSettings() { + if (!volumetricLighting) + return; + js["intensity"] = settings.intensity;src/WeatherEditor/Weather/ImageSpaceWidget.cpp (1)
26-28: Empty destructor can be defaulted.The destructor has an empty body. Consider using
= defaultin the header instead.src/WeatherEditor/Weather/PrecipitationWidget.cpp (1)
127-143: Remove unusedruntimevariable in else branch.The
runtimevariable declared at line 128 is unused since all the property reads useprecipitation->GetSettingValue()instead.} else { - auto& runtime = precipitation->GetRuntimeData(); - settings.gravityVelocity = precipitation->GetSettingValue(RE::BGSShaderParticleGeometryData::DataID::kGravityVelocity).f; // ... other GetSettingValue calls ... + auto& runtime = precipitation->GetRuntimeData(); settings.particleTexture = runtime.particleTexture.textureName.c_str(); }src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
25-27: Empty destructor can be defaulted.Consider using
= defaultin the header.src/WeatherEditor/WeatherUtils.cpp (1)
157-213: Static maps may accumulate entries over application lifetime.The static maps (
pendingValues,lastChangeTime,wasActive,undoPushedForSession) keyed by label strings will retain entries for every unique label ever used. This is unlikely to be problematic in practice but could be a minor memory concern if labels are generated dynamically.Consider clearing entries after a longer idle period or using a bounded cache if this becomes an issue.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/WeatherEditor/EditorWindow.cpp(1 hunks)src/WeatherEditor/Weather/CellLightingWidget.cpp(1 hunks)src/WeatherEditor/Weather/ImageSpaceWidget.cpp(1 hunks)src/WeatherEditor/Weather/LightingTemplateWidget.cpp(1 hunks)src/WeatherEditor/Weather/PrecipitationWidget.cpp(1 hunks)src/WeatherEditor/Weather/VolumetricLightingWidget.cpp(1 hunks)src/WeatherEditor/Weather/WeatherWidget.cpp(1 hunks)src/WeatherEditor/WeatherUtils.cpp(1 hunks)src/WeatherEditor/WeatherUtils.h(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/WeatherEditor/Weather/ImageSpaceWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/VolumetricLightingWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cppsrc/WeatherEditor/WeatherUtils.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/EditorWindow.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/WeatherEditor/WeatherUtils.cppsrc/WeatherEditor/WeatherUtils.h
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to **/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py} : Do not include TODO/FIXME placeholders; provide complete, working solutions
Applied to files:
src/WeatherEditor/Weather/WeatherWidget.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (22)
src/WeatherEditor/Weather/VolumetricLightingWidget.cpp (1)
5-71: LGTM!The widget implementation follows a clean pattern with proper null checks, tab-based UI organization, and auto-apply integration. The separation of settings into Basic, Density, and Advanced tabs provides good UX.
src/WeatherEditor/Weather/CellLightingWidget.cpp (1)
5-149: LGTM!The widget handles interior cell validation correctly with early returns, and the tab-based organization for Colors, Fog, Directional Ambient, Advanced, and Inheritance settings is well-structured. The inheritance flag handling with
RE::INTERIOR_DATA::Inheritenums is properly implemented.src/WeatherEditor/Weather/ImageSpaceWidget.cpp (2)
222-234: LGTM on the improved LoadSettings implementation.The simplified validation that checks for Settings key existence instead of zero-value validation addresses the previous review feedback correctly.
305-313: MissingHasUnsavedChanges()implementation.Other widgets in this PR (VolumetricLightingWidget, CellLightingWidget, PrecipitationWidget) implement
HasUnsavedChanges()to track dirty state. This widget lacks that method, which could affect unsaved changes detection in the editor framework.src/WeatherEditor/Weather/PrecipitationWidget.cpp (2)
5-89: LGTM!The widget UI is well-organized with Particle, Position, and Texture tabs. The combo box for particle type and the texture path input handling are appropriately implemented.
184-184: Verify lifetime safety of texture name assignment.Assigning
settings.particleTexture.c_str()toruntime.particleTexture.textureNamemay have lifetime issues iftextureNamestores the raw pointer rather than copying the string. Inspect the RE type definition to confirm howtextureNamehandles string assignment—whether it stores a pointer to the temporary or creates a copy. If it stores a pointer, this will cause undefined behavior when the temporary string is destroyed.src/WeatherEditor/Weather/LightingTemplateWidget.cpp (1)
155-220: LGTM!The TOD-based DALC rendering with proper X/Y/Z to Sunrise/Day/Sunset/Night mapping is well-implemented. The directional color mapping logic correctly handles the engine's axis-based representation.
src/WeatherEditor/WeatherUtils.cpp (3)
29-37: LGTM on the clamped float-to-integer conversions.The
std::clampaddition properly bounds input values to [0, 1] before thestd::lerpcall, addressing the previous review feedback.
8-17: LGTM on the safe tolower usage.The
static_cast<unsigned char>prevents undefined behavior whencharis signed and the value is negative.
365-399: LGTM on TOD factor calculation.The time period boundaries and blend factor calculations correctly implement Skyrim's 4-period day cycle with smooth transitions between periods.
src/WeatherEditor/WeatherUtils.h (3)
100-163: Template performs unchecked static_cast on widget->form.At line 140,
static_cast<T*>(widget->form)assumes the widget's form type matchesT. If the widget container contains forms of a different type, this could lead to undefined behavior.Consider using
dynamic_castwith a null check if runtime type safety is needed, or document the precondition that all widgets in the container must have forms of typeT.
30-81: LGTM!The TOD namespace provides a clean API for time-of-day calculations and UI rendering, with well-documented period indices and helper functions.
96-96: Unable to verify default parameter mismatch due to inaccessible repository.The claim that
DrawSliderFloathas inconsistent default values between header (max = 1.0f) and source (max = 50000.0f) cannot be confirmed without access to the actual source code. Verify the default parameter values in bothWeatherUtils.handWeatherUtils.cppdirectly.src/WeatherEditor/Weather/WeatherWidget.cpp (4)
45-53: LGTM!The destructor properly releases COM resources by calling
Release()on each cached texture SRV and clearing the cache.
415-466: LGTM!Robust error handling with JSON deserialization validation, fallback to vanilla values on failure, and user notifications. The validation checks for empty critical fields and the try-catch properly handles JSON exceptions.
1639-1668: LGTM!Proper texture loading and caching with resource management. The SRV is cached and will be properly released in the destructor (lines 47-52).
1397-1480: LGTM!Excellent validation logic that checks for missing features in JSON settings and provides clear user feedback via notifications and the FeatureIssues system. This prevents confusion when settings reference features that aren't installed.
src/WeatherEditor/EditorWindow.cpp (5)
1184-1197: LGTM!Proper resource cleanup with
deletefor raw pointertempTextureand clearing ofunique_ptrwidget collections. No memory leaks detected.
1324-1344: LGTM!Proper COM resource management. The resource obtained from
GetResource()(line 1325) is correctly released at line 1343 after use.
782-1182: LGTM!Well-structured UI rendering with proper keyboard shortcut handling, menu organization, and ImGui style management. Style variables are correctly pushed and popped, and the notification system is integrated cleanly.
1199-1294: LGTM!Comprehensive resource initialization using
unique_ptrfor all widget types. Consistent pattern ofCacheFormData()andLoad()calls ensures proper setup. No memory management issues.
1705-1764: LGTM!Robust undo system that properly saves/restores widget state via JSON, handles widget pointer invalidation by searching by ID, and provides user feedback through notifications. The max undo limit prevents unbounded memory growth.
…skyrim-community-shaders into weather-editor-updated
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.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Utils/UI.cpp (1)
1206-1207: Verify thatread()reads all requested bytes.The
read()method can return fewer bytes than requested (partial read). The code does not verify that all bytes were actually read, which could result in uninitialized data being passed to DirectX, potentially causing crashes or texture corruption.🔎 Proposed fix
ddsData.resize(size); -bsaStream.read(reinterpret_cast<char*>(ddsData.data()), size); +size_t bytesRead = bsaStream.read(reinterpret_cast<char*>(ddsData.data()), size); +if (bytesRead != size) { + logger::warn("LoadDDSTextureFromFile: Incomplete read, expected {} bytes, got {}", size, bytesRead); + return false; +}Note: This issue was flagged in a previous review and remains unaddressed.
🧹 Nitpick comments (4)
src/Utils/UI.cpp (2)
10-10: Consider using ComPtr for safer resource management.The
wrl/client.hinclude is added butMicrosoft::WRL::ComPtris not used in theLoadDDSTextureFromFilefunction. If this include is for future use, consider refactoring the function to useComPtr<ID3D11Texture2D>andComPtr<ID3D11ShaderResourceView>instead of raw COM pointers with manualRelease()calls to prevent potential resource leaks.Also applies to: 15-15
1233-1236: Consider type-safe casting for the texture resource.The
reinterpret_castassumes the resource is a 2D texture without runtime verification. WhileDirectX::CreateTextureshould return the correct type for DDS data, consider usingQueryInterfacefor type safety or validating the resource dimension before casting.🔎 Type-safe alternative using QueryInterface
Based on DirectXTex source code,
CreateTextureinternally usesreinterpret_castwhen creating typed textures, so the cast in this code mirrors that pattern. However, for additional safety you could verify the resource type:ID3D11Texture2D* pTexture = nullptr; HRESULT queryHr = pResource->QueryInterface(__uuidof(ID3D11Texture2D), reinterpret_cast<void**>(&pTexture)); pResource->Release(); // Release the original resource reference if (FAILED(queryHr) || !pTexture) { logger::warn("LoadDDSTextureFromFile: Resource is not a 2D texture"); return false; }src/Features/WeatherEditor.h (1)
7-14: Feature inheritance is correct.The WeatherEditor properly inherits from Feature as required by coding guidelines. The singleton pattern using a static local variable is thread-safe in C++11+.
Optional: Mark GetSingleton() noexcept
Consider adding
noexceptto strengthen guarantees:- static WeatherEditor* GetSingleton() + static WeatherEditor* GetSingleton() noexcept { static WeatherEditor singleton; return &singleton; }src/WeatherEditor/WeatherUtils.cpp (1)
6-8: Consider encapsulating the global widget context for better maintainability.The global
g_currentWidgetpointer is set viaSetCurrentWidgetand used across multiple drawing functions. While functional, this pattern is fragile—ifSetCurrentWidgetis not called before drawing operations, undo functionality silently fails.Alternative approach
Consider encapsulating the widget context in a class or struct:
class WidgetContext { Widget* current = nullptr; public: void SetWidget(Widget* w) { current = w; } Widget* GetWidget() const { return current; } // RAII helper struct Scope { WidgetContext& ctx; Widget* prev; Scope(WidgetContext& c, Widget* w) : ctx(c), prev(c.GetWidget()) { ctx.SetWidget(w); } ~Scope() { ctx.SetWidget(prev); } }; }; // Usage: // WidgetContext::Scope scope(g_widgetContext, widget);This provides better encapsulation and prevents accidental context leaks when exceptions occur or early returns happen.
Also applies to: 153-156
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Features/WeatherEditor.hsrc/Features/WeatherPicker.cppsrc/Features/WeatherPicker.hsrc/State.cppsrc/Utils/UI.cppsrc/WeatherEditor/WeatherUtils.cpp
💤 Files with no reviewable changes (1)
- src/Features/WeatherPicker.cpp
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/State.cppsrc/Features/WeatherPicker.hsrc/Features/WeatherEditor.hsrc/Utils/UI.cppsrc/WeatherEditor/WeatherUtils.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/State.cppsrc/Features/WeatherPicker.hsrc/Features/WeatherEditor.hsrc/Utils/UI.cppsrc/WeatherEditor/WeatherUtils.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/State.cppsrc/Features/WeatherPicker.hsrc/Features/WeatherEditor.hsrc/Utils/UI.cppsrc/WeatherEditor/WeatherUtils.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
📚 Learning: 2025-06-24T07:17:36.604Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
Applied to files:
src/State.cppsrc/Features/WeatherPicker.hsrc/Features/WeatherEditor.hsrc/WeatherEditor/WeatherUtils.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Feature classes must inherit from Feature (src/Feature.h) and implement DrawSettings(), LoadSettings(), and SaveSettings()
Applied to files:
src/State.cppsrc/Features/WeatherEditor.h
📚 Learning: 2025-06-17T05:40:22.785Z
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
Applied to files:
src/State.cpp
📚 Learning: 2025-08-17T18:37:35.839Z
Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/**/*.{cpp,cxx,cc,h,hpp,hxx} : Register new features in the globals::features namespace
Applied to files:
src/State.cpp
📚 Learning: 2025-08-05T18:22:40.578Z
Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:277-293
Timestamp: 2025-08-05T18:22:40.578Z
Learning: In the skyrim-community-shaders SnowCover feature, the wstrtostr and strtowstr utility functions defined in src/Features/SnowCover.cpp are unused and should be removed rather than fixed, as confirmed by the original author ThePagi.
Applied to files:
src/WeatherEditor/WeatherUtils.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (11)
src/Features/WeatherPicker.h (1)
14-14: Category change aligns with PR objectives.The change from "Debug" to "Sky" appropriately moves WeatherPicker into the new category structure as discussed in the PR comments.
Please verify the category naming is consistent across the codebase. The PR description mentions "Sky & Weather" but this uses "Sky". Ensure this matches the category name used by WeatherEditor and other related features:
#!/bin/bash # Description: Verify category naming consistency across weather-related features # Search for GetCategory implementations in the codebase rg -n -C2 'GetCategory.*override' --type=cpp # Search for "Sky" and "Sky & Weather" category references rg -n '"Sky"' --type=cpp rg -n '"Sky & Weather"' --type=cppsrc/Utils/UI.cpp (1)
1184-1196: LGTM: Robust parameter validation and resource initialization.The function properly validates parameters, initializes output to a safe state, and uses Skyrim's resource system correctly with appropriate error logging.
src/State.cpp (4)
14-14: LGTM: Clean integration of weather system headers.The include statements properly bring in the WeatherEditor feature and WeatherManager components.
Also applies to: 20-20
29-29: LGTM: Consistent feature integration pattern.The WeatherEditor integration follows the established pattern used by other features (terrainBlending, cloudShadows, terrainHelper), with proper conditional execution based on the loaded flag.
Also applies to: 34-36
162-164: Verify the unconditional weather settings load is intentional.In
Setup(),LoadPerWeatherSettingsFromDisk()is called unconditionally, whereasUpdateFeatures()inDraw()is gated onweatherEditor.loaded. Given the past review comment mentioning "WeatherManager is unique in its behaviour," this may be intentional (settings loading is a one-time operation needed regardless of editor state, while per-frame updates should be conditional).Please confirm this design is correct and that the WeatherManager can handle being called even if
weatherEditor.loadedis false.
355-356: LGTM: Clean weather variable registration hook.The placement of
RegisterWeatherVariables()afterfeature->Load(settings)is correct, allowing features to register their weather-controllable variables after initialization. The opt-in design via virtual method override is appropriate, and error handling is already covered by the outer try-catch block.src/Features/WeatherEditor.h (5)
1-6: LGTM: Clean header structure.The header guards and includes are properly structured for the WeatherEditor feature class.
16-28: Clarify the category name.The PR objectives mention creating a new "Sky & Weather" category, but
GetCategory()returns just"Sky". Please confirm whether:
- The category is internally named "Sky" but displays as "Sky & Weather" in the UI, or
- This should be updated to return "Sky & Weather"
30-31: LGTM: Standard Feature lifecycle methods.The virtual method declarations for
DataLoaded()andDrawSettings()follow the Feature interface requirements.
33-33: Verify null pointer handling in LerpWeather.The
LerpWeathermethod accepts raw pointers to weather objects. Please ensure the implementation includes null checks and graceful error handling, as emphasized in the coding guidelines for plugin code.Additionally, consider documenting the expected range for the interpolation factor (presumably 0.0-1.0).
36-36: LGTM: Private UI helper.The
DrawWeatherStatusPanel()private method is appropriately scoped for internal UI rendering.
| bool DrawColorEdit(const std::string& l, float3& property, Widget* widget) | ||
| { | ||
| static std::map<std::string, float3> colorCache; | ||
| static std::string activeColorId; | ||
| static std::map<std::string, bool> wasPickerOpen; | ||
|
|
||
| std::string cacheId = l; | ||
| bool isActive = ImGui::IsPopupOpen(l.c_str(), ImGuiPopupFlags_AnyPopupId); | ||
| bool wasActive = wasPickerOpen[cacheId]; | ||
|
|
||
| // Cache the original color and push undo state when picker is first activated | ||
| if (isActive && activeColorId != cacheId) { | ||
| colorCache[cacheId] = property; | ||
| activeColorId = cacheId; | ||
| // Push undo state before change (prefer parameter, fallback to global) | ||
| Widget* w = widget ? widget : g_currentWidget; | ||
| if (w) { | ||
| EditorWindow::GetSingleton()->PushUndoState(w); | ||
| } | ||
| } else if (!isActive && activeColorId == cacheId) { | ||
| activeColorId.clear(); | ||
| } | ||
|
|
||
| // Check for Ctrl+Z while picker is active | ||
| if (isActive && ImGui::IsKeyDown(ImGuiKey_LeftCtrl) && ImGui::IsKeyPressed(ImGuiKey_Z)) { | ||
| if (colorCache.contains(cacheId)) { | ||
| property = colorCache[cacheId]; | ||
| wasPickerOpen[cacheId] = isActive; | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| bool changed = ImGui::ColorEdit3(l.c_str(), (float*)&property); | ||
|
|
||
| // Track color usage only when picker closes | ||
| if (wasActive && !isActive) { | ||
| PaletteWindow::GetSingleton()->TrackColorUsage(property); | ||
| } | ||
|
|
||
| wasPickerOpen[cacheId] = isActive; | ||
|
|
||
| // Drag-and-drop source | ||
| if (ImGui::BeginDragDropSource(ImGuiDragDropFlags_None)) { | ||
| ImGui::SetDragDropPayload("COLOR_DND", &property, sizeof(float3)); | ||
| ImGui::ColorButton("##preview", ImVec4(property.x, property.y, property.z, 1.0f), ImGuiColorEditFlags_NoAlpha); | ||
| ImGui::EndDragDropSource(); | ||
| } | ||
|
|
||
| // Drag-and-drop target | ||
| if (ImGui::BeginDragDropTarget()) { | ||
| if (const ImGuiPayload* payload = ImGui::AcceptDragDropPayload("COLOR_DND")) { | ||
| if (payload->DataSize == sizeof(float3)) { | ||
| float3 droppedColor = *(const float3*)payload->Data; | ||
| property = droppedColor; | ||
| changed = true; | ||
| } | ||
| } | ||
| ImGui::EndDragDropTarget(); | ||
| } | ||
|
|
||
| return changed; | ||
| } |
There was a problem hiding this comment.
Consider cleanup for static state maps to prevent memory leaks.
The static maps colorCache, activeColorId, and wasPickerOpen accumulate entries indefinitely as new color properties are edited. Over extended editor sessions with many weather configurations, this could lead to unbounded memory growth. Similar patterns exist in DrawTODColorRow and other functions with static state.
Consider implementing periodic cleanup of inactive entries or using a bounded cache structure.
Suggested cleanup approach
One approach: periodically remove entries from wasPickerOpen and colorCache that haven't been accessed recently. For example:
// Track last access time
static std::map<std::string, double> lastAccessTime;
double currentTime = ImGui::GetTime();
// Update access time
lastAccessTime[cacheId] = currentTime;
// Periodic cleanup (e.g., every 60 seconds)
static double lastCleanup = 0.0;
if (currentTime - lastCleanup > 60.0) {
for (auto it = lastAccessTime.begin(); it != lastAccessTime.end();) {
if (currentTime - it->second > 300.0) { // 5 minutes idle
colorCache.erase(it->first);
wasPickerOpen.erase(it->first);
it = lastAccessTime.erase(it);
} else {
++it;
}
}
lastCleanup = currentTime;
}Apply similar cleanup to other static maps in TOD functions.
doodlum
moved this from Priority
to In progress
in Community Shaders Development Tracker
doodlum
moved this from In progress
to Deprioritised
in Community Shaders Development Tracker
davo0411
moved this from Deprioritised
to In review
in Community Shaders Development Tracker
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/WeatherEditor/WeatherUtils.h (1)
343-406: Consider making the lambda static.The
GetFormEditorIDSafelambda (Line 349) doesn't capture anything and could be declaredstaticfor clarity and potential optimization. However, this is a minor style point and the code works correctly as-is.🔎 Optional enhancement
- auto GetFormEditorIDSafe = [](T* form) -> std::string { + static auto GetFormEditorIDSafe = [](T* form) -> std::string { if (!form) return "";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/WeatherEditor/WeatherUtils.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/WeatherEditor/WeatherUtils.h
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/WeatherEditor/WeatherUtils.h
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/WeatherEditor/WeatherUtils.h
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-06-24T07:17:36.604Z
Learning: When reviewing PRs, always clarify the scope if there are multiple related features or dependencies. WeatherPicker was a separate PR that was already merged, while this PR focuses specifically on WetnessEffects climate preset system enhancements.
🔇 Additional comments (8)
src/WeatherEditor/WeatherUtils.h (8)
1-15: LGTM!The includes are appropriate, and forward-declaring
EditorWindowis good practice for reducing compilation dependencies.
96-123: LGTM!The PropertyDrawer API provides a clean, unified interface for table-based property editing with search support.
125-186: LGTM!The WidgetFactory templates provide a clean abstraction for widget population and rendering. The null checks are properly placed, and the focus tracking implementation is correct.
188-203: LGTM!The color conversion utilities and
ControlTypeenum are well-defined with clear naming conventions.
205-259: LGTM!The TOD namespace provides a comprehensive API for time-of-day-based weather editing. The
Periodenum with aCountmember is a good pattern for iteration.
261-263: LGTM!Simple, clear API for widget search functionality.
265-277: LGTM!The WeatherUtils UI helper functions have a consistent API design with appropriate optional parameters.
27-32: Unable to verify the review comment. The filesrc/WeatherEditor/WeatherUtils.hand theDebouncedTrackerclass with theOnValueChangedmethod could not be located in the current repository. The code may exist in a pull request that hasn't been merged yet, or the file path may be incorrect. Manual verification is required to confirm whether the return value inconsistency is an actual bug or misleading documentation.
| template <typename T, typename WidgetContainer> | ||
| bool DrawFormPickerCached(const char* label, T*& currentForm, const WidgetContainer& widgets, bool showFormID = true, bool allowNone = true, float width = 450.0f) | ||
| { | ||
| bool changed = false; | ||
|
|
||
| std::string previewText; | ||
| if (currentForm) { | ||
| // Find the widget for current form | ||
| std::string editorID; | ||
| for (const auto& widget : widgets) { | ||
| if (widget->form == currentForm) { | ||
| editorID = widget->GetEditorID(); | ||
| break; | ||
| } | ||
| } | ||
| if (editorID.empty()) { | ||
| editorID = std::format("{:08X}", currentForm->GetFormID()); | ||
| } | ||
|
|
||
| if (showFormID) { | ||
| previewText = std::format("{} (0x{:08X})", editorID, currentForm->GetFormID()); | ||
| } else { | ||
| previewText = editorID; | ||
| } | ||
| } else { | ||
| previewText = "None"; | ||
| } | ||
|
|
||
| if (width > 0.0f) { | ||
| ImGui::SetNextItemWidth(width); | ||
| } | ||
|
|
||
| if (ImGui::BeginCombo(label, previewText.c_str())) { | ||
| if (allowNone && ImGui::Selectable("None", currentForm == nullptr)) { | ||
| currentForm = nullptr; | ||
| changed = true; | ||
| } | ||
|
|
||
| for (const auto& widget : widgets) { | ||
| if (widget && widget->form) { | ||
| T* form = static_cast<T*>(widget->form); | ||
| std::string editorID = widget->GetEditorID(); | ||
| std::string comboLabel; | ||
| if (showFormID) { | ||
| comboLabel = std::format("{} (0x{:08X})", editorID, form->GetFormID()); | ||
| } else { | ||
| comboLabel = editorID; | ||
| } | ||
|
|
||
| bool isSelected = (currentForm == form); | ||
| if (ImGui::Selectable(comboLabel.c_str(), isSelected)) { | ||
| currentForm = form; | ||
| changed = true; | ||
| } | ||
| if (isSelected) { | ||
| ImGui::SetItemDefaultFocus(); | ||
| } | ||
| } | ||
| } | ||
| ImGui::EndCombo(); | ||
| } | ||
|
|
||
| return changed; | ||
| } |
There was a problem hiding this comment.
Add type safety to the form cast.
Line 318 uses static_cast<T*>(widget->form) without verifying that widget->form is actually of type T*. If the WidgetContainer contains widgets with heterogeneous form types, this can cause undefined behavior.
Consider one of these solutions:
🔎 Proposed fixes
Solution 1 (Recommended): Use dynamic_cast with runtime checking
for (const auto& widget : widgets) {
if (widget && widget->form) {
- T* form = static_cast<T*>(widget->form);
+ T* form = dynamic_cast<T*>(widget->form);
+ if (!form) continue; // Skip incompatible form types
std::string editorID = widget->GetEditorID();Solution 2: Add compile-time constraints with a concept (C++20)
template <typename T, typename WidgetContainer>
requires std::same_as<typename WidgetContainer::value_type::element_type::FormType, T>
bool DrawFormPickerCached(...)Solution 3: Document the requirement
Add a comment clarifying that WidgetContainer must contain only widgets of the target form type:
// Generic form picker combo box using cached widget EditorIDs for performance
+ // REQUIRES: WidgetContainer must contain only widgets with form type T
// Returns true if selection changed
template <typename T, typename WidgetContainer>🤖 Prompt for AI Agents
In src/WeatherEditor/WeatherUtils.h around lines 278 to 341, the code uses
static_cast<T*>(widget->form) which can yield undefined behavior if widget->form
is not actually a T; replace the static_cast with a runtime-checked
dynamic_cast<T*>(widget->form) and skip/continue when the dynamic_cast returns
nullptr (so you only build combo entries for widgets whose form is actually a
T), update the isSelected comparison and any uses of form accordingly, and
optionally add a short comment noting the runtime type check.
* build: bump eastl to 3.27.01 (community-shaders#1673) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> * fix(grass collision): ignore hkpListShape (community-shaders#1661) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(upscaling): update SDKs (community-shaders#1684) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * chore(terrain shadows): modernize (community-shaders#1678) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: add user wiki link to readme (community-shaders#1689) * feat(LLF): add debug overlay in llf visualiser (community-shaders#1666) * feat: linear lighting (community-shaders#1359) Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> * feat: weather and imagespace editor (community-shaders#1630) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * fix(ui): load previously selected theme on startup (community-shaders#1664) --------- Co-authored-by: Alan Tse <alandtse@users.noreply.github.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jiayev <l936249247@hotmail.com> Co-authored-by: jiayev <jiayev@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
4 participants
adds the weather editor with imagespace and lighting template controls.
adds a universal system with lots of documentation (thx claude) on implementing custom weather-based variables for features.
None implemented with this PR. Saving/loading/changing of these variables is controlled by the weather system, not the features.
creates a new "Sky & Weather" category.
Summary by CodeRabbit
New Features
Enhancements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.