feat: add Weather Picker#1167
Conversation
|
""" WalkthroughThis change introduces a new WeatherPicker feature, enabling an interactive weather control and analysis system with a detailed UI for selecting, filtering, and inspecting in-game weather. It adds supporting utilities for weather formatting, color-coded value display, and unit conversions. The menu system is extended to support a persistent weather details window, and the Feature class is updated for weather analysis integration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant WeatherPicker
participant GameData
participant Feature
User->>Menu: Opens overlay or weather details window
Menu->>WeatherPicker: DrawWeatherDetailsWindow()
WeatherPicker->>GameData: LoadAllWeathers() (on first use)
WeatherPicker->>WeatherPicker: UpdateFilteredWeathers()
WeatherPicker->>WeatherPicker: Render weather selection/filter UI
User->>WeatherPicker: Selects weather / toggles filters
WeatherPicker->>GameData: Apply selected weather (immediate or transition)
WeatherPicker->>Feature: For each feature, call GetWeatherAnalysisConfig()
Feature-->>WeatherPicker: WeatherAnalysisConfig (sectionName, drawFunction)
WeatherPicker->>WeatherPicker: Render feature-specific weather analysis sections
WeatherPicker->>Menu: Update window state as needed
Possibly related PRs
Suggested reviewers
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
alandtse
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
src/Menu.cpp (1)
325-333: Consider removing commented debug codeThe commented debug logging code should either be removed to keep the codebase clean or converted to conditional debug logging if it provides ongoing value for troubleshooting.
- // // Debug logging for icon availability - // if (settings.Theme.ShowActionIcons) { - // logger::debug("Icon status - Save: {}, Load: {}, Cache: {}, Disk: {}, Logo: {}", - // uiIcons.saveSettings.texture ? "OK" : "NULL", - // uiIcons.loadSettings.texture ? "OK" : "NULL", - // uiIcons.clearCache.texture ? "OK" : "NULL", - // uiIcons.clearDiskCache.texture ? "OK" : "NULL", - // uiIcons.logo.texture ? "OK" : "NULL"); - // }src/Utils/UI.h (1)
151-175: Well-structured configuration for color-coded displays.The design provides excellent flexibility with threshold-based coloring. Minor formatting suggestion below.
- bool sameLine = true; // Whether to put value on same line as label - // Helper methods for common patterns (implemented in UI.cpp to avoid header dependencies) + bool sameLine = true; // Whether to put value on same line as label + + // Helper methods for common patterns (implemented in UI.cpp to avoid header dependencies)src/Utils/Game.h (1)
95-114: Consider safer string formatting approach.The current buffer-based approach requires callers to manage buffer lifetime and size. Consider returning std::string for safety.
Example refactor for one function:
- inline const char* FormatDistance(float gameUnits, char* buffer, size_t bufferSize) - { - snprintf(buffer, bufferSize, "%.1f units (%.2f m, %.1f ft)", - gameUnits, GameUnitsToMeters(gameUnits), GameUnitsToFeet(gameUnits)); - return buffer; - } + inline std::string FormatDistance(float gameUnits) + { + return std::format("%.1f units (%.2f m, %.1f ft)", + gameUnits, GameUnitsToMeters(gameUnits), GameUnitsToFeet(gameUnits)); + }src/Features/WeatherPicker.cpp (3)
314-315: Remove unnecessary negative check for unsigned value.Since
weather->data.windSpeedis an unsigned 8-bit value (0-255), dividing it by 255.0f cannot produce a negative result. The clamping to 0 is unnecessary.float windSpeedDisplay = weather->data.windSpeed / 255.0f; -if (windSpeedDisplay < 0) - windSpeedDisplay = 0; // Clamp to prevent negative values
232-234: Simplify color component casting.The nested casts can be simplified. Since you're converting from normalized float (0-1) to byte range (0-255) and storing as signed char, the intermediate unsigned cast is unnecessary.
-weather->data.lightningColor.red = static_cast<std::int8_t>(static_cast<std::uint8_t>(lightningColor[0] * 255.0f)); -weather->data.lightningColor.green = static_cast<std::int8_t>(static_cast<std::uint8_t>(lightningColor[1] * 255.0f)); -weather->data.lightningColor.blue = static_cast<std::int8_t>(static_cast<std::uint8_t>(lightningColor[2] * 255.0f)); +weather->data.lightningColor.red = static_cast<std::int8_t>(lightningColor[0] * 255.0f); +weather->data.lightningColor.green = static_cast<std::int8_t>(lightningColor[1] * 255.0f); +weather->data.lightningColor.blue = static_cast<std::int8_t>(lightningColor[2] * 255.0f);
432-432: Use size_t for loop counter to match container size type.The loop counter should use
size_tto match the return type ofsize()and avoid signed/unsigned comparison warnings.-for (int i = 0; i < filters.size(); ++i) { +for (size_t i = 0; i < filters.size(); ++i) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
extern/CommonLibSSE-NG(1 hunks)features/Weather Picker/Shaders/Features/WeatherPicker.ini(1 hunks)src/Feature.cpp(2 hunks)src/Feature.h(1 hunks)src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)src/Globals.cpp(3 hunks)src/Globals.h(2 hunks)src/Menu.cpp(5 hunks)src/Menu.h(2 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(2 hunks)src/Utils/UI.cpp(1 hunks)src/Utils/UI.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (32)
extern/CommonLibSSE-NG (1)
1-1: Submodule bump: CommonLibSSE-NG updated
The subproject reference has been advanced to commita9bc9fe07ed912aeb0319a6c1992392d720e5592to support the new WeatherPicker feature. Ensure full CI passes and verify there are no breaking API changes affecting Skyrim SE and VR builds.features/Weather Picker/Shaders/Features/WeatherPicker.ini (1)
1-2: Add version descriptor for WeatherPicker shader config
The new[Info]section correctly defines the feature version and aligns with the loader logic inFeature::Load.src/Globals.cpp (3)
37-37: Include WeatherPicker header
Adding theFeatures/WeatherPicker.hinclude integrates the new feature into the global compilation unit.
78-78: Declare global WeatherPicker pointer
TheweatherPickerpointer inglobals::featuresfollows the existing pattern for feature singletons.
162-162: Initialize WeatherPicker singleton in OnInit
AssigningWeatherPicker::GetSingleton()ensures the feature is ready for global access.src/Feature.cpp (2)
28-28: Include WeatherPicker in feature registry
This header inclusion is required before addingweatherPickerto the feature list.
212-212: ```shell
#!/bin/bashSearch for SupportsVR override in WeatherPicker source files
rg --no-heading --line-number -e "SupportsVR" src/Features/WeatherPicker.h src/Features/WeatherPicker.cpp || echo "No SupportsVR occurrences found"
</details> <details> <summary>src/Globals.h (2)</summary> `26-26`: **Forward-declare WeatherPicker** The new forward declaration matches other feature structs and enables the external pointer. --- `79-79`: **Declare external WeatherPicker pointer** Adding `extern WeatherPicker* weatherPicker` aligns with the global feature pointer pattern. </details> <details> <summary>src/Menu.cpp (4)</summary> `73-78`: **LGTM: Consistent JSON serialization pattern** The `WeatherDetailsWindowSettings` serialization follows the established pattern used by other settings structures in the file, properly defining the three expected fields for window management. --- `1477-1478`: **Good integration of weather details window** The placement in `DrawOverlay()` is appropriate, allowing the weather details window to render independently of the main menu state. The descriptive comment clearly explains this behavior. --- `2565-2569`: **LGTM: Clean feature navigation method** The `SelectFeatureMenu` method has a clear, focused implementation that properly queues navigation and provides helpful logging for debugging. --- `2571-2583`: **Solid implementation with good defensive programming** The method properly checks prerequisites, handles null cases, and delegates rendering responsibility to the appropriate feature. The use of a pointer for the enabled state allows proper window close handling. </details> <details> <summary>src/Feature.h (1)</summary> `111-132`: **Excellent extensible design for weather analysis integration** The `WeatherAnalysisConfig` struct and `GetWeatherAnalysisConfig()` method provide a clean, opt-in mechanism for features to participate in weather analysis UI. Key strengths: - Proper use of `std::move` for the function parameter - Clear documentation explaining the opt-in behavior - Default empty configuration that doesn't require all features to implement weather analysis - Clean separation of concerns with the drawFunction callback </details> <details> <summary>src/Utils/Game.cpp (2)</summary> `181-214`: **Robust form formatting with excellent null safety** The `FormatTESForm` function demonstrates excellent defensive programming practices: - Proper null pointer handling - Thorough validation of names including whitespace-only detection - Consistent hex formatting for FormIDs - Clear conditional logic for different formatting scenarios The implementation handles edge cases well and provides reliable formatting for UI display purposes. --- `215-277`: **Comprehensive weather formatting with excellent flag handling** The `FormatWeather` function is well-designed with several notable strengths: - Efficient reuse of `FormatTESForm` for base formatting - Smart use of `magic_enum` for robust flag enumeration - Clever detection of unknown flags through bitwise operations - Human-readable flag name conversions (removing 'k' prefix, special cases) - Proper handling of edge cases (no flags, unknown flags) The implementation provides comprehensive weather information formatting that will be valuable for the WeatherPicker UI. </details> <details> <summary>src/Utils/UI.h (3)</summary> `142-149`: **LGTM! Well-designed API for collapsible headers.** The function signature enhancement provides flexibility while maintaining backward compatibility through default parameters. The bool return value appropriately indicates state changes for reactive UI updates. --- `177-184`: **Clean API for color-coded value rendering.** The function signature effectively uses the configuration struct pattern for flexibility and maintainability. --- `201-206`: **Useful helper for complex tooltips.** Good design with optional per-line coloring support. The default empty vector parameter provides sensible defaults. </details> <details> <summary>src/Menu.h (4)</summary> `45-46`: **Good separation of concerns for weather UI functionality.** The distinction between window rendering and core functionality with the extensibility comment provides a clean architecture for feature integration. --- `217-223`: **Consistent settings structure for weather window.** The struct follows the established pattern from PerfOverlaySettings with appropriate defaults for window positioning. --- `225-225`: **Appropriate addition for mutable settings access.** The method complements the existing read-only GetTheme() accessor and enables proper settings modification by other components. --- `229-230`: **Good refactoring to expose utility function.** Converting KeyIdToString to public static appropriately exposes this utility for broader use while maintaining encapsulation. </details> <details> <summary>src/Utils/UI.cpp (4)</summary> `401-462`: **Well-implemented flexible section header rendering.** The implementation properly handles both collapsible and non-collapsible modes while maintaining visual consistency with the existing UI style. State change tracking is correctly implemented. --- `465-489`: **Clean implementation of color threshold helpers.** The static helper methods effectively use the theme system and provide intuitive color progressions for different value semantics. --- `491-518`: **Correct implementation of threshold-based color coding.** The function properly iterates through thresholds and applies colors based on value ranges. The optional tooltip integration is well done. --- `520-532`: **Simple and effective multi-line tooltip rendering.** The implementation correctly handles optional per-line coloring with appropriate bounds checking and fallback behavior. </details> <details> <summary>src/Utils/Game.h (1)</summary> `167-169`: **Good addition of formatting utilities.** The function signatures appropriately use std::string returns and maintain const correctness. </details> <details> <summary>src/Features/WeatherPicker.h (3)</summary> `7-26`: **Well-structured feature class with appropriate metadata.** The singleton pattern and virtual overrides correctly establish this as a core debug feature with VR support. --- `30-65`: **Comprehensive public API for weather visualization.** The static methods provide a flexible interface for weather display with good documentation. The color system based on weather flags is well thought out. --- `78-95`: **Well-designed comparator with robust fallback logic.** The display name resolution chain (name → editorID → formID) ensures consistent sorting even for weather objects with missing data. </details> <details> <summary>src/Features/WeatherPicker.cpp (1)</summary> `1-868`: **Well-structured weather control feature implementation!** The WeatherPicker feature is comprehensively implemented with: - Clean separation of concerns between UI rendering and data management - Proper integration points for other features to provide weather analysis - Good use of ImGui patterns for interactive UI elements - Thorough weather information display with helpful tooltips - Robust filtering and sorting mechanisms The code follows good practices with appropriate null checks, proper use of singletons, and clear function responsibilities. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/WeatherPicker.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (4)
src/Features/WeatherPicker.h (4)
72-74: Good implementation of previous feedback.The named constants
ALL_WEATHER_FLAGSandUNCLASSIFIED_FLAGsuccessfully address the previous review comment about replacing magic numbers with descriptive constants.
7-11: LGTM: Proper singleton implementation.The singleton pattern is correctly implemented with a static local variable, ensuring thread-safe initialization and proper lifetime management.
86-103: LGTM: Robust weather name comparison logic.The
WeatherNameComparatorprovides excellent fallback logic by trying the display name first, then editor ID, and finally form ID. The lambda-based approach for getting display names is clean and maintainable.
36-50: LGTM: Well-documented public API.The documentation clearly explains the priority system for weather type colors and the multi-color rendering functionality. The return value documentation for
RenderMultiColorWeatherNameis particularly helpful.
…ysis - Add interactive weather control system with instant weather switching - Implement weather filtering by type (Pleasant, Cloudy, Rainy, Snow, Aurora) - Add detailed weather information display with wind, precipitation, and lightning data - Provide color-coded weather names showing all properties at a glance - Support persistent overlay window for continuous weather monitoring - Add weather analysis infrastructure for other features to extend - Enhance weather and form formatting utilities with magic_enum integration - Include robust tooltip system for detailed weather information display Core feature marked for inclusion in main Community Shaders distribution. Supports both SE and VR platforms.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Features/WeatherPicker.h (1)
72-73: Replace magic numbers with named constants.The hex values 0x7F and 0x40 lack clarity about their meaning and purpose.
Add constants to clarify the bit flags:
+ // Weather flag filter bits (for 7 weather types) + static constexpr uint32_t ALL_WEATHER_FLAGS = 0x7F; // Bits 0-6 all enabled + static constexpr uint32_t UNCLASSIFIED_FLAG = 0x40; // Bit 6 only + static inline bool s_weathersLoaded = false; static inline std::vector<RE::TESWeather*> s_allWeathers; static inline std::vector<RE::TESWeather*> s_filteredWeathers; static inline int s_selectedWeatherIdx = -1; - static inline uint32_t s_weatherFlagFilter = 0x7F; // Start with all filters enabled by default (bits 0-6) - static inline uint32_t s_lastWeatherFlagFilter = 0x40; + static inline uint32_t s_weatherFlagFilter = ALL_WEATHER_FLAGS; // Start with all filters enabled by default + static inline uint32_t s_lastWeatherFlagFilter = UNCLASSIFIED_FLAG;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
extern/CommonLibSSE-NG(1 hunks)features/Weather Picker/Shaders/Features/WeatherPicker.ini(1 hunks)src/Feature.cpp(2 hunks)src/Feature.h(1 hunks)src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)src/Globals.cpp(3 hunks)src/Globals.h(2 hunks)src/Menu.cpp(4 hunks)src/Menu.h(2 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(2 hunks)src/Utils/UI.cpp(1 hunks)src/Utils/UI.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- features/Weather Picker/Shaders/Features/WeatherPicker.ini
- extern/CommonLibSSE-NG
- src/Feature.cpp
- src/Globals.h
- src/Feature.h
- src/Globals.cpp
- src/Utils/Game.h
- src/Menu.h
- src/Menu.cpp
- src/Utils/Game.cpp
- src/Utils/UI.h
⏰ Context from checks skipped due to timeout of 90000ms (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 (15)
src/Utils/UI.cpp (4)
401-462: LGTM! Well-designed collapsible header implementation.The refactored
DrawSectionHeaderfunction provides excellent flexibility with both collapsible and non-collapsible modes. The state change return value enables parent components to react to expansions/collapses, and the consistent theming integration maintains visual coherence.The implementation correctly handles:
- State tracking for collapsible headers
- Consistent theming across both modes
- Proper ImGui ID management to prevent conflicts
464-489: LGTM! Clean factory methods for color-coded configurations.The static helper methods
HighIsBadandHighIsGoodprovide intuitive APIs for creating threshold-based color configurations. The use of theme colors ensures consistency with the application's visual design.
491-526: LGTM! Robust color-coded value display function.The
DrawColorCodedValuefunction provides comprehensive support for threshold-based color coding with proper fallback handling. The optional bullet styling, same-line layout, and tooltip integration make it versatile for various UI contexts.The threshold checking logic is efficient and the color determination is clear and predictable.
528-540: LGTM! Simple and effective multi-line tooltip utility.The
DrawMultiLineTooltipfunction provides a clean way to render multi-line tooltips with optional per-line coloring. The bounds checking for the colors vector prevents potential crashes.src/Features/WeatherPicker.h (2)
77-95: LGTM! Well-designed weather comparator.The
WeatherNameComparatorprovides robust sorting with a clear fallback hierarchy: display name → editor ID → form ID. The lambda-based implementation for display name extraction is clean and handles null cases appropriately.
36-64: LGTM! Well-documented public API.The public methods for weather color handling are well-documented with clear parameter descriptions and return value specifications. The multi-color weather name rendering function provides good detail about its behavior and return value meaning.
src/Features/WeatherPicker.cpp (9)
6-9: LGTM! Simple and correct data loading.The
DataLoadedfunction properly triggers weather loading when game data becomes available.
11-24: LGTM! Clear feature summary.The feature summary provides a comprehensive overview of capabilities with well-structured key features list.
102-137: LGTM! Comprehensive weather type color mapping.The
GetWeatherTypeColorfunction implements a clear priority system for weather classification with appropriate fallback handling. The use of theme colors ensures visual consistency.
139-386: LGTM! Comprehensive weather information display.The
DisplayWeatherInfofunction provides detailed weather data presentation with:
- Multi-color weather name rendering with tooltips
- Comprehensive precipitation data with proper normalization
- Interactive lightning color picker with proper state management
- Detailed thunder frequency analysis with contextual information
- Wind data with player-relative calculations
The implementation handles edge cases well and provides informative tooltips throughout.
603-625: LGTM! Efficient weather loading and caching.The
LoadAllWeathersfunction properly implements one-time loading with appropriate sorting and filtering initialization.
627-667: LGTM! Robust weather filtering logic.The
UpdateFilteredWeathersfunction correctly implements the filtering logic with proper handling of the "None" category for unclassified weathers. The tracked flags mask is comprehensive and the filtering logic is sound.
718-768: LGTM! Comprehensive flag name extraction.The
GetWeatherFlagNamesfunction uses magic_enum effectively to extract flag names with proper string transformation and unknown flag detection. The handling of edge cases (no flags, unknown flags) is robust.
770-829: LGTM! Sophisticated multi-color rendering.The
RenderMultiColorWeatherNamefunction provides excellent visual feedback by rendering weather names with color-coded flag segments. The base name hover detection is well-implemented and the color chip display is clean.
831-876: LGTM! Consistent color mapping utilities.Both
GetWeatherFlagColorandGetWeatherFlagColorByNameprovide consistent color mapping with appropriate theme integration and fallback handling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/Features/WeatherPicker.cpp (5)
416-416: Replace magic number with named constant.The value
0x00should be replaced with a named constant for consistency.- s_weatherFlagFilter = 0x00; // No flags + static constexpr uint32_t NO_WEATHER_FLAGS = 0x00; + s_weatherFlagFilter = NO_WEATHER_FLAGS; // No flagsAlternatively, define the constant in the header file alongside
ALL_WEATHER_FLAGSandUNCLASSIFIED_FLAG.
139-386: Consider refactoring this long function for better maintainability.The
DisplayWeatherInfofunction is 247 lines long and handles multiple responsibilities. Consider extracting the following sections into separate functions:
- Precipitation data display (lines 180-208) →
DisplayPrecipitationData()- Lightning data display (lines 211-320) →
DisplayLightningData()- Wind data display (lines 322-385) →
DisplayWindData()This would improve readability, testability, and follow the Single Responsibility Principle.
388-601: Consider breaking down this function for improved readability.The
RenderCoreWeatherDetailsfunction spans 213 lines and manages both weather controls and information display. Consider extracting:
- Weather filter controls (lines 410-473) →
RenderWeatherFilterControls()- Weather selection combo (lines 509-550) →
RenderWeatherSelectionCombo()- Weather information table (lines 570-587) →
RenderWeatherComparisonTable()
163-166: Optimize string concatenation for better performance.Using
+=for string concatenation in a loop is inefficient. Consider usingstd::ostringstreamor pre-reserving string capacity.- std::string joinedFlags = flagNames[0]; - for (size_t j = 1; j < flagNames.size(); ++j) { - joinedFlags += ", " + flagNames[j]; - } + std::ostringstream oss; + oss << flagNames[0]; + for (size_t j = 1; j < flagNames.size(); ++j) { + oss << ", " << flagNames[j]; + } + std::string joinedFlags = oss.str();
669-679: Consider using STL algorithm for more idiomatic code.The manual loop could be replaced with
std::findfor cleaner code.int WeatherPicker::FindWeatherIndex(RE::TESWeather* targetWeather) { if (!targetWeather) return -1; - for (size_t i = 0; i < s_filteredWeathers.size(); ++i) { - if (s_filteredWeathers[i] == targetWeather) { - return static_cast<int>(i); - } - } - return -1; + auto it = std::find(s_filteredWeathers.begin(), s_filteredWeathers.end(), targetWeather); + return (it != s_filteredWeathers.end()) ? static_cast<int>(std::distance(s_filteredWeathers.begin(), it)) : -1; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (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 (1)
src/Features/WeatherPicker.h (1)
82-99: Well-implemented weather comparator with proper fallback logic.The comparator handles null cases gracefully and provides a consistent ordering strategy (name → editor ID → form ID).
|
✅ A pre-release build is available for this PR: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Features/WeatherPicker.cpp (3)
84-84: Extract complex condition into a helper function.The condition for determining if interactive elements should be enabled is complex and could benefit from extraction into a named helper function for clarity and reusability.
+ // Helper function to determine if interactive elements should be enabled + auto shouldEnableInteractiveElements = []() -> bool { + return !(Menu::GetSingleton()->ShouldSwallowInput() || + (globals::game::ui && globals::game::ui->IsMenuOpen(RE::CursorMenu::MENU_NAME))); + }; + - bool menuOpen = Menu::GetSingleton()->ShouldSwallowInput() || (globals::game::ui && globals::game::ui->IsMenuOpen(RE::CursorMenu::MENU_NAME)); - RenderCoreWeatherDetails(!menuOpen); + RenderCoreWeatherDetails(shouldEnableInteractiveElements());
175-176: Document the GET_INSTANCE_MEMBER macro usage.The
GET_INSTANCE_MEMBERmacro usage might not be immediately clear to other developers. Consider adding a comment explaining its purpose and behavior.+ // Extract particle texture from precipitation data using macro for safe member access GET_INSTANCE_MEMBER(particleTexture, weather->precipitationData)
204-207: Simplify redundant type casting.The casting chain from int8_t → unsigned int → uint8_t is unnecessarily complex and could be simplified.
- unsigned int lightningR = static_cast<unsigned int>(static_cast<std::uint8_t>(weather->data.lightningColor.red)); - unsigned int lightningG = static_cast<unsigned int>(static_cast<std::uint8_t>(weather->data.lightningColor.green)); - unsigned int lightningB = static_cast<unsigned int>(static_cast<std::uint8_t>(weather->data.lightningColor.blue)); + uint8_t lightningR = static_cast<uint8_t>(weather->data.lightningColor.red); + uint8_t lightningG = static_cast<uint8_t>(weather->data.lightningColor.green); + uint8_t lightningB = static_cast<uint8_t>(weather->data.lightningColor.blue);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/WeatherPicker.cpp(1 hunks)
🔇 Additional comments (4)
src/Features/WeatherPicker.cpp (4)
1-5: LGTM: Clean includes and structure.The include structure is well-organized and follows proper dependency management.
594-616: LGTM: Well-structured data loading with proper error handling.The weather loading logic is well-implemented with proper null checks, memory reservation, and sorting. The singleton pattern usage and early return for already-loaded data are good practices.
709-759: LGTM: Excellent use of magic_enum and comprehensive flag handling.The implementation properly leverages magic_enum for flag name conversion and includes robust handling of unknown flags. The string transformation logic (removing 'k' prefix, readable name conversion) is well thought out.
845-867: LGTM: Efficient flag name to color mapping.The use of static unordered_map for flag name lookup is efficient and the fallback color for unknown flags is appropriate. Good separation of concerns between name mapping and color determination.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)src/Utils/Game.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Utils/Game.h
- src/Features/WeatherPicker.cpp
⏰ Context from checks skipped due to timeout of 90000ms (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 (3)
src/Features/WeatherPicker.h (3)
71-87: Well-structured constants and static state management.The named constants
ALL_WEATHER_FLAGSandUNCLASSIFIED_FLAGeffectively address the previous magic number concerns, and the static state variables are appropriately organized and documented.
40-68: Comprehensive weather color and rendering utilities.The public utility methods are well-documented with clear parameter descriptions and return value specifications. The color-coding system with priority handling provides good visual feedback for weather types.
5-26: Proper Feature integration with appropriate metadata.The singleton pattern implementation is correct, and the virtual overrides provide appropriate feature metadata. The core feature designation and VR support alignment matches the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Features/WeatherPicker.h (2)
17-20: Remove redundantvirtualkeywords when usingoverride.The
virtualkeyword is redundant whenoverrideis used, asoverrideimplies the function is virtual.- virtual bool SupportsVR() override { return true; } - virtual bool IsCore() const override { return true; } - virtual std::string_view GetCategory() const override { return "Debug"; } - virtual bool IsInMenu() const override { return true; } // Show in main menu to provide weather debugging UI + bool SupportsVR() override { return true; } + bool IsCore() const override { return true; } + std::string_view GetCategory() const override { return "Debug"; } + bool IsInMenu() const override { return true; } // Show in main menu to provide weather debugging UI
89-100: Minor optimization opportunity in GetDisplayName.The function could be slightly optimized by avoiding redundant null checks and string length calculations.
static std::string GetDisplayName(const RE::TESWeather* weather) { const char* name = weather->GetName(); - if (name && strlen(name) > 0) { + if (name && *name != '\0') { return std::string(name); } const char* editorID = weather->GetFormEditorID(); - if (editorID && strlen(editorID) > 0) { + if (editorID && *editorID != '\0') { return std::string(editorID); } return std::to_string(weather->GetFormID()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/WeatherPicker.h(1 hunks)
🔇 Additional comments (3)
src/Features/WeatherPicker.h (3)
74-76: Previous magic number issue has been resolved.The magic numbers 0x7F and 0x40 have been properly replaced with descriptive named constants. The implementation correctly addresses the previous review concern.
78-86: Previous duplicate declaration issue has been resolved.The duplicate static variable declarations mentioned in previous reviews have been eliminated. Only one set of declarations remains, properly organized.
102-109: Previous performance issue with WeatherNameComparator has been resolved.The comparator now efficiently uses the static
GetDisplayNamehelper function instead of recreating lambdas, which addresses the previous performance concern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/WeatherPicker.cpp (1)
82-85: Consider extracting lambda to member function for better testability.The
shouldEnableInteractiveElementslambda could be extracted to a private static member function to improve testability and potentially reuse in other contexts.+private: + static bool ShouldEnableInteractiveElements() { + return (Menu::GetSingleton()->ShouldSwallowInput() || + (globals::game::ui && globals::game::ui->IsMenuOpen(RE::CursorMenu::MENU_NAME))); + } + void WeatherPicker::RenderWeatherDetailsWindow(bool* open) { // ... existing code ... - // Enable interactive elements when a menu is open - auto shouldEnableInteractiveElements = []() -> bool { - return (Menu::GetSingleton()->ShouldSwallowInput() || - (globals::game::ui && globals::game::ui->IsMenuOpen(RE::CursorMenu::MENU_NAME))); - }; - - RenderCoreWeatherDetails(shouldEnableInteractiveElements()); + RenderCoreWeatherDetails(ShouldEnableInteractiveElements());
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Features/WeatherPicker.cpp(1 hunks)src/Features/WeatherPicker.h(1 hunks)src/Menu.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Menu.h
🔇 Additional comments (7)
src/Features/WeatherPicker.cpp (3)
594-634: Well-implemented filtering logic with proper bit manipulation.The weather filtering logic correctly handles:
- All filters selected case
- Individual weather flag matching
- Special "None" filter for unclassified weathers
- Proper masking of tracked vs untracked flags
The bit manipulation and flag checking logic is sound and handles edge cases appropriately.
326-326: Use std::abs for floating-point values.Similar to the previous issue flagged in past reviews,
std::abs()should be used for floating-point values instead ofabs().- if (std::abs(rawDifference) < 30.0f) { + if (std::fabs(rawDifference) < 30.0f) {Also check line 328 for the same issue:
- } else if (std::abs(rawDifference) > 150.0f) { + } else if (std::fabs(rawDifference) > 150.0f) {Likely an incorrect or invalid review comment.
699-703: ```bash
#!/bin/bashCheck if WeatherPicker.cpp includes the precompiled header PCH.h
rg -n "#include.*PCH.h" src/Features/WeatherPicker.cpp
Show the first 20 lines of WeatherPicker.cpp to inspect include directives
echo "---- src/Features/WeatherPicker.cpp (first 20 lines) ----"
head -n 20 src/Features/WeatherPicker.cpp</details> <details> <summary>src/Features/WeatherPicker.h (4)</summary> `5-26`: **Excellent class structure and interface design.** The class demonstrates several good practices: - Proper singleton pattern implementation - Clear virtual method overrides with appropriate metadata - Good categorization as "Debug" feature marked as core - Clean separation of concerns in method organization The VR support, core status, and menu visibility flags are appropriately set for a weather debugging tool. --- `71-76`: **Well-defined constants improve code clarity.** The named constants effectively replace the magic numbers flagged in previous reviews: - `WIND_DIRECTION_OFFSET` provides clear context for the 30.5f value - `ALL_WEATHER_FLAGS` and `UNCLASSIFIED_FLAG` make the bit manipulation logic much more readable - Comments clearly explain the bit patterns --- `78-86`: **Clean static state management with proper initialization.** The static variables are well-organized and properly initialized: - Uses `static inline` for header-defined variables - Logical grouping of related state - Appropriate default values (all filters enabled, acceleration on) - Good naming conventions The previous duplicate declaration issues have been resolved. --- `92-98`: **Improved comparator performance using static helper function.** The `WeatherNameComparator` now correctly uses the static `GetDisplayName` function instead of recreating a lambda on each comparison. This addresses the performance concern raised in previous reviews and provides better efficiency during sorting operations. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Features/WeatherPicker.cpp (1)
855-855: Consider using hexadecimal format for FormID display.FormIDs are conventionally displayed in hexadecimal format for better readability and consistency with game tools.
- return std::to_string(weather->GetFormID()); + return fmt::format("0x{:08X}", weather->GetFormID());Note: If fmt library is not available, use:
char buffer[11]; std::snprintf(buffer, sizeof(buffer), "0x%08X", weather->GetFormID()); return std::string(buffer);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/WeatherPicker.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (10)
src/Features/WeatherPicker.cpp (10)
11-24: Well-structured feature summary.The feature description and key features are clear and informative, providing good documentation for users.
215-217: Correct use of unsigned type for color values.Good fix - using std::uint8_t for color components (0-255 range) is the appropriate type.
541-541: Correct use of std::abs for floating-point values.Good - using std::abs for floating-point arithmetic as recommended in the previous review.
133-354: Excellent refactoring into focused helper functions.Great job breaking down the large DisplayWeatherInfo function into smaller, focused helpers (DisplayWeatherBasicInfo, DisplayPrecipitationInfo, DisplayLightningInfo, DisplayWindInfo) as suggested in the previous review. This improves maintainability and follows the single responsibility principle.
356-508: Well-structured separation of weather UI components.Good implementation of the suggested refactoring - RenderCoreWeatherDetails now delegates to focused functions (RenderWeatherControls and RenderWeatherInformationDisplay), improving code organization and maintainability.
Also applies to: 510-545, 547-568
173-173: Verify GET_INSTANCE_MEMBER macro usage and consider alternatives.The code uses GET_INSTANCE_MEMBER macro which appears to access private/protected members. This approach might be fragile if the game's internal structures change.
#!/bin/bash # Description: Find GET_INSTANCE_MEMBER macro definition and usage patterns # Search for the macro definition rg "define GET_INSTANCE_MEMBER" --type h # Find other usages to understand the pattern rg "GET_INSTANCE_MEMBER" --type cpp -A 2 -B 2Consider using public APIs if available, or document why direct member access is necessary.
321-321: Verify WIND_DIRECTION_OFFSET constant definition.The code uses WIND_DIRECTION_OFFSET for wind direction calculations. Ensure this constant is properly defined and documented to explain its purpose.
#!/bin/bash # Description: Find WIND_DIRECTION_OFFSET definition and its value # Search for the constant definition rg "WIND_DIRECTION_OFFSET" --type cpp --type h -B 2 -A 2 # Check if it's in the Utils/Game.h since wind utilities might be there fd "Game.h" src/Utils --exec grep -n "WIND_DIRECTION_OFFSET" {} \;Also applies to: 323-323
367-367: Verify constant definitions for weather flags.The code correctly uses named constants (ALL_WEATHER_FLAGS, UNCLASSIFIED_FLAG) instead of magic numbers. Ensure these constants are properly defined in the header file.
#!/bin/bash # Description: Verify the weather flag constants are defined # Search for constant definitions in header files fd -e h "WeatherPicker" --exec grep -E "(ALL_WEATHER_FLAGS|UNCLASSIFIED_FLAG)" {} \; # Also check if they might be defined elsewhere rg "ALL_WEATHER_FLAGS|UNCLASSIFIED_FLAG" --type hAlso applies to: 601-601, 410-410, 611-611
572-574: ```shell
#!/bin/bashLocate WeatherPicker header files (.h and .hpp)
headers=$(fd WeatherPicker -t f -e h -e hpp)
echo "Found header files:"
echo "$headers"
echoSearch for static member variable declarations in those headers
echo "Searching for static member declarations:"
grep -HnE 'static\s+.*s_weathersLoaded|static\s+.*s_allWeathers|static\s+.*s_filteredWeathers|static\s+.*s_selectedWeatherIdx|static\s+.*s_weatherFlagFilter|static\s+.*s_lastWeatherFlagFilter|static\s+.*s_accelerateWeatherChange|static\s+.*s_cachedLastWeather' $headers || echo "No static member declarations found for those variables."--- `587-587`: I’d like to see the rest of `GetDisplayName` to confirm how it handles null/empty names and whether we need a guard for a null `weather` pointer: ```shell #!/bin/bash # Show full GetDisplayName implementation rg -n "WeatherPicker::GetDisplayName" -A 20 src/Features/WeatherPicker.cpp
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Utils/UI.cpp (1)
500-535: Consider adding bounds validation for threshold configuration.The function assumes thresholds in
config.thresholdsare properly sorted in ascending order. Consider adding validation or documentation to ensure correct usage.void DrawColorCodedValue( const std::string& label, float valueToCheck, const std::string& valueStr, const ColorCodedValueConfig& config, bool useBullet) { + // Validate that thresholds are sorted (debug builds only) + #ifdef _DEBUG + for (size_t i = 1; i < config.thresholds.size(); ++i) { + if (config.thresholds[i-1].threshold > config.thresholds[i].threshold) { + logger::warn("DrawColorCodedValue: Thresholds are not sorted in ascending order"); + break; + } + } + #endif + // Display label
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Feature.cpp(2 hunks)src/Globals.cpp(3 hunks)src/Globals.h(2 hunks)src/Menu.cpp(5 hunks)src/Menu.h(2 hunks)src/Utils/UI.cpp(1 hunks)src/Utils/UI.h(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Feature.cpp
- src/Globals.cpp
- src/Globals.h
- src/Menu.cpp
- src/Menu.h
- src/Utils/UI.h
⏰ Context from checks skipped due to timeout of 90000ms (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 (3)
src/Utils/UI.cpp (3)
473-498: LGTM! Well-designed color coding helpers.The static helper methods provide a clean API for creating color-coded value configurations. The use of theme colors ensures UI consistency, and the threshold logic is intuitive (low values = gray/disabled, high values = red for bad or green for good).
537-549: LGTM! Simple and effective multi-line tooltip implementation.The function handles optional per-line coloring correctly with proper bounds checking. The implementation is straightforward and serves its purpose well.
410-471: Potential null pointer dereference in non-collapsible path.The function parameter
isExpandedis only validated whenisCollapsibleis true, but the parameter could be null whenisCollapsibleis false. Consider adding parameter validation at the function start.bool DrawSectionHeader(const char* sectionName, bool useWhiteText, bool isCollapsible, bool* isExpanded) { + if (isCollapsible && !isExpanded) { + logger::warn("DrawSectionHeader: isExpanded cannot be null when isCollapsible is true"); + return false; + } + bool stateChanged = false;Likely an incorrect or invalid review comment.
3 participants
Core feature marked for inclusion in main Community Shaders distribution.
Supports both SE and VR platforms.
Summary by CodeRabbit