feat(weather): std::array & std::vector support#1702
Conversation
📝 WalkthroughWalkthroughThe changes introduce support for weather variables with composite array and vector types through two new specialized template classes (ArrayVariable and VectorVariable), along with comprehensive documentation covering usage, interpolation behavior, and JSON serialization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/weather-system-docs/WeatherVariableRegistration.md (1)
105-106: Fix corrupted text on this line.The text appears to have been incorrectly merged. It should be split into two separate parts.
📝 Suggested fix
-- Handles default values and missing dataanced Usage +- Handles default values and missing data + +## Advanced Usage
🧹 Nitpick comments (2)
docs/weather-system-docs/WeatherVariableRegistration.md (1)
374-376: Remove empty code block.This empty code block at the end of the file serves no purpose.
🧹 Suggested fix
-``` - -```src/WeatherVariableRegistry.h (1)
229-240: Consider usingdefaultValueelements for missing vector entries.Currently, when vectors have different sizes during interpolation, missing elements use
T{}(value-initialization). This differs from how the base class handles missing JSON data (usesdefaultValue).While the current behavior is documented, using elements from
defaultValuemight be more intuitive for users expecting consistent fallback behavior.♻️ Optional: Use defaultValue for missing elements
This would require capturing
defaultValuein the lambda:- [elementLerpFunc](const std::vector<T>& from, const std::vector<T>& to, float factor) { + [elementLerpFunc, defaultValue](const std::vector<T>& from, const std::vector<T>& to, float factor) { size_t maxSize = std::max(from.size(), to.size()); std::vector<T> result; result.reserve(maxSize); for (size_t i = 0; i < maxSize; ++i) { - T fromVal = (i < from.size()) ? from[i] : T{}; - T toVal = (i < to.size()) ? to[i] : T{}; + T defaultElement = (i < defaultValue.size()) ? defaultValue[i] : T{}; + T fromVal = (i < from.size()) ? from[i] : defaultElement; + T toVal = (i < to.size()) ? to[i] : defaultElement; result.push_back(elementLerpFunc(fromVal, toVal, factor)); } return result; })If the current behavior is intentional for your use cases, this is fine as-is since it's properly documented.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherVariableRegistry.h
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ 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:
docs/weather-system-docs/WeatherVariableRegistration.mdsrc/WeatherVariableRegistry.h
**/*.{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/WeatherVariableRegistry.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/WeatherVariableRegistry.h
🧠 Learnings (2)
📓 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.
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.
Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
📚 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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (4)
docs/weather-system-docs/WeatherVariableRegistration.md (1)
129-213: Documentation for array and vector types looks comprehensive.The examples clearly demonstrate:
- Fixed-size arrays with
ArrayVariable<T, N>for primitive and complex types- Dynamic vectors with
VectorVariable<T>- Custom interpolation functions for complex element types
- Important notes on vector interpolation behavior with mismatched sizes
src/WeatherVariableRegistry.h (3)
13-29: LGTM!The header documentation clearly explains the available variable types and provides a concise example for custom element types requiring interpolation functions.
189-218: LGTM!The
ArrayVariableimplementation correctly:
- Uses SFINAE to provide a convenient constructor for floating-point element types
- Captures
elementLerpFuncby value in the lambda (necessary since it outlives the constructor)- Performs per-element interpolation with proper bounds
244-254: LGTM!The helper constructor correctly mirrors the
ArrayVariablepattern with proper SFINAE constraints for floating-point types.
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.