refactor(weather-editor): use existing functions for lock retarget#2153
Conversation
📝 WalkthroughWalkthroughRefactored weather selection handling in the editor by replacing direct assignment to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/WeatherEditor.cpp (1)
683-690:⚠️ Potential issue | 🟡 MinorAvoid double-applying weather when retargeting an active lock.
This refactor introduced a behavioral change. The previous code directly assigned
editorWindow->lockedWeather = selectedWeather;, but now callseditorWindow->LockWeather(selectedWeather), which appliesForceWeather()again. This causes:
- Accelerated path:
ForceWeather()called twice in succession- Non-accelerated path:
SetWeather()(transition) followed immediately byForceWeather()(cancels transition)Since the PR states "no functionality changes are intended," consider adding a
RetargetLockedWeather()helper that updates only the locked target without re-applying the weather, or revert to direct assignment if the lock is already active.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/WeatherEditor.cpp` around lines 683 - 690, The change calls editorWindow->LockWeather(selectedWeather) when the lock is already active, which re-applies ForceWeather/SetWeather and alters behavior; instead add or use a RetargetLockedWeather helper that only updates the lock target (e.g., editorWindow->lockedWeather) without invoking ForceWeather/SetWeather, and call that when editorWindow->IsWeatherLocked() is true (or revert to direct assignment to editorWindow->lockedWeather) so s_accelerateWeatherChange, sky->ForceWeather and sky->SetWeather are not invoked a second time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Features/WeatherEditor.cpp`:
- Around line 683-690: The change calls
editorWindow->LockWeather(selectedWeather) when the lock is already active,
which re-applies ForceWeather/SetWeather and alters behavior; instead add or use
a RetargetLockedWeather helper that only updates the lock target (e.g.,
editorWindow->lockedWeather) without invoking ForceWeather/SetWeather, and call
that when editorWindow->IsWeatherLocked() is true (or revert to direct
assignment to editorWindow->lockedWeather) so s_accelerateWeatherChange,
sky->ForceWeather and sky->SetWeather are not invoked a second time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c609949c-7848-41f5-aa5f-659bb0f43dc4
📒 Files selected for processing (1)
src/Features/WeatherEditor.cpp
|
✅ A pre-release build is available for this PR: |
|
the AI comment may be valid, but double applying is not that big of a deal since it only triggers on the button press once. |
…ommunity-shaders#2153) (cherry picked from commit 36c1b0f)
In the previous PR I missed that I could reuse a function I made earlier.
No changed functionality.
Summary by CodeRabbit