Open
Conversation
… during transition much nicer look and no more color inaccuracies
Member
|
Not tried it, but sounds exactly what one of my customers wanted from my lamps |
Collaborator
Author
|
it also was requested somewhere before, I saw it more than once but could not find the references. In a nutshell this is the "sunrise FX" but instead of a fixed sunrise pattern, it allows to use any palette/color as a starting point and as a destination. I am not sure it is very easy to use though: if you just call it once it does nothing but initialize, so you have to use it either in a playlist or in a "chain of presets" |
Contributor
WalkthroughThis PR introduces a new "Slow Transition" effect that enables gradual palette transitions over extended durations with configurable timing. It also modifies the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wled00/FX.cpp`:
- Around line 10830-10836: The transition uses the sync-adjusted clock
(strip.now) causing jumps; change all uses of strip.now in this sunrise-like
effect to the real-time millis() clock: when initializing the timer set
*startTime = millis(); compute current = millis(); then use elapsed = current -
*startTime (with uint32_t arithmetic) to calculate expectedSteps (leave
totalSteps and duration logic unchanged). Replace references to strip.now in the
startTime assignment and the elapsed calculation so this mode mirrors
mode_sunrise() behavior and is not affected by strip time sync adjustments.
- Around line 10797-10801: The preset handoff currently repeatedly calls
applyPreset() each render after the fade completes; modify the handoff code that
uses slow_transition_data (SlowTransitionData / slow_transition_data) so it sets
a one-shot latch (e.g., a boolean flag) when the transition finishes, invokes
applyPreset() only once, and immediately return from the render path after
performing that single handoff to avoid continued rendering with stale data;
apply the same one-shot latch + early-return pattern to the other two handoff
blocks referenced (the similar blocks around the other locations).
- Around line 10839-10849: The sweep branch (when SEGMENT.check2 is true) jumps
*stepsDone to expectedSteps and only applies the final microstep, losing
intermediate palette-entry updates; change the logic in the block using
*stepsDone, expectedSteps, data->currentPalette, data->startPalette,
data->endPalette, CRGB/CRGBW and color_blend so that you advance step-by-step
for any skipped microsteps: iterate from the previous stepsDone+1 up to
expectedSteps and for each step compute i = step % 16 and blendAmount = step /
16 and update data->currentPalette[i] accordingly (instead of applying only the
final step), then set *stepsDone = expectedSteps after the loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c313c770-8556-4149-b475-3255b2f43376
📒 Files selected for processing (3)
wled00/FX.cppwled00/FX.hwled00/FX_fcn.cpp
Comment on lines
+10797
to
+10801
| typedef struct SlowTransitionData { | ||
| CRGBPalette16 startPalette; // initial palette | ||
| CRGBPalette16 currentPalette; // blended palette for current frame, need permanent storage so we can start from this if target changes mid transition | ||
| CRGBPalette16 endPalette; // target palette | ||
| } slow_transition_data; |
Contributor
There was a problem hiding this comment.
Make the preset handoff one-shot and stop rendering after it.
Once the fade completes, this path can invoke applyPreset() on every render until something else resets the effect, and it then keeps drawing with the pre-preset data snapshot after mutating segment state. Add a latch and return immediately after the handoff.
Suggested fix
typedef struct SlowTransitionData {
CRGBPalette16 startPalette; // initial palette
CRGBPalette16 currentPalette; // blended palette for current frame, need permanent storage so we can start from this if target changes mid transition
CRGBPalette16 endPalette; // target palette
+ bool presetApplied; // ensure applyPreset() only runs once per completed transition
} slow_transition_data;
@@
if (changed || SEGMENT.call == 0) {
if (SEGMENT.call == 0) {
//data->presetapplied = false;
data->startPalette = SEGPALETTE;
data->currentPalette = SEGPALETTE;
data->endPalette = SEGPALETTE;
*stepsDone = 0xFFFF; // set to max, fading will start once a change is detected
}
else {
data->startPalette = data->currentPalette;
data->endPalette = SEGPALETTE;
*stepsDone = 0; // reset counter
}
+ data->presetApplied = false;
*startSpeed = SEGMENT.speed;
*startTime = strip.now; // set start time
}
@@
if (*stepsDone >= totalSteps) {
// transition complete, apply preset set by intensity slider (if any)
data->currentPalette = data->endPalette; // set to end palette (sweep may not have set all entries)
- if (SEGMENT.intensity > 0) {
+ if (SEGMENT.intensity > 0 && !data->presetApplied) {
uint8_t targetPreset = SEGMENT.intensity;
+ data->presetApplied = true;
applyPreset(targetPreset, CALL_MODE_DIRECT_CHANGE); // apply preset (if it exists)
+ return;
}
}
}Also applies to: 10816-10829, 10856-10869
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 10797 - 10801, The preset handoff currently
repeatedly calls applyPreset() each render after the fade completes; modify the
handoff code that uses slow_transition_data (SlowTransitionData /
slow_transition_data) so it sets a one-shot latch (e.g., a boolean flag) when
the transition finishes, invokes applyPreset() only once, and immediately return
from the render path after performing that single handoff to avoid continued
rendering with stale data; apply the same one-shot latch + early-return pattern
to the other two handoff blocks referenced (the similar blocks around the other
locations).
Comment on lines
+10830
to
+10836
| *startTime = strip.now; // set start time | ||
| } | ||
|
|
||
| uint32_t totalSteps = SEGMENT.check2 ? 16 * 255 : 255; | ||
| uint32_t duration = (SEGMENT.speed == 0) ? 10000 : (uint32_t)SEGMENT.speed * 60000; // 10s if zero (good for testing), otherwise map 1-255 to 1-255 minutes | ||
| uint32_t elapsed = strip.now - *startTime; // note: will overflow after ~50 days if just left alone (edge case unhandled) | ||
| uint32_t expectedSteps = (uint64_t)elapsed * totalSteps / duration; |
Contributor
There was a problem hiding this comment.
Use millis() for the transition timer.
strip.now is the sync-adjusted animation clock in WLED. That is fine for frame-synced FX, but this mode measures real minutes, so a sync jump can fast-forward or rewind the transition. mode_sunrise() already avoids that in this file for the same reason.
Suggested fix
- *startTime = strip.now; // set start time
+ *startTime = millis(); // keep minute-scale timing independent from FX sync
}
uint32_t totalSteps = SEGMENT.check2 ? 16 * 255 : 255;
uint32_t duration = (SEGMENT.speed == 0) ? 10000 : (uint32_t)SEGMENT.speed * 60000; // 10s if zero (good for testing), otherwise map 1-255 to 1-255 minutes
- uint32_t elapsed = strip.now - *startTime; // note: will overflow after ~50 days if just left alone (edge case unhandled)
+ uint32_t elapsed = millis() - *startTime;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 10830 - 10836, The transition uses the
sync-adjusted clock (strip.now) causing jumps; change all uses of strip.now in
this sunrise-like effect to the real-time millis() clock: when initializing the
timer set *startTime = millis(); compute current = millis(); then use elapsed =
current - *startTime (with uint32_t arithmetic) to calculate expectedSteps
(leave totalSteps and duration logic unchanged). Replace references to strip.now
in the startTime assignment and the elapsed calculation so this mode mirrors
mode_sunrise() behavior and is not affected by strip time sync adjustments.
Comment on lines
+10839
to
+10849
| if (*stepsDone > expectedSteps) | ||
| *stepsDone = expectedSteps;// in case sweep was disabled mid transition | ||
|
|
||
| if (*stepsDone < expectedSteps) { | ||
| *stepsDone = expectedSteps; // jump to expected steps to make sure timing is correct (need up to 4080 frames, at 20fps that is ~200 seconds) | ||
| if (SEGMENT.check2) { | ||
| // sweep: one palette entry at a time | ||
| uint8_t i = *stepsDone % 16; | ||
| uint8_t blendAmount = *stepsDone / 16; | ||
| data->currentPalette[i] = CRGB(color_blend(CRGBW(data->startPalette[i]), CRGBW(data->endPalette[i]), blendAmount)); | ||
| } else { |
Contributor
There was a problem hiding this comment.
Sweep mode drops skipped microsteps.
When SEGMENT.check2 is enabled, the transition has 4080 microsteps. expectedSteps can advance by more than one between renders, but this branch jumps straight to the final value and applies only the last microstep, so the skipped palette-entry updates are lost and the sweep no longer matches elapsed time.
Suggested fix
- if (*stepsDone > expectedSteps)
- *stepsDone = expectedSteps;// in case sweep was disabled mid transition
-
- if (*stepsDone < expectedSteps) {
- *stepsDone = expectedSteps; // jump to expected steps to make sure timing is correct (need up to 4080 frames, at 20fps that is ~200 seconds)
+ if (*stepsDone != expectedSteps) {
if (SEGMENT.check2) {
- // sweep: one palette entry at a time
- uint8_t i = *stepsDone % 16;
- uint8_t blendAmount = *stepsDone / 16;
- data->currentPalette[i] = CRGB(color_blend(CRGBW(data->startPalette[i]), CRGBW(data->endPalette[i]), blendAmount));
+ while (*stepsDone < expectedSteps) {
+ ++(*stepsDone);
+ uint8_t i = *stepsDone % 16;
+ uint8_t blendAmount = *stepsDone / 16;
+ data->currentPalette[i] = CRGB(color_blend(CRGBW(data->startPalette[i]), CRGBW(data->endPalette[i]), blendAmount));
+ }
} else {
+ *stepsDone = expectedSteps;
// full palette at once
uint8_t blendAmount = (uint8_t)*stepsDone;
for (uint8_t i = 0; i < 16; i++) {
data->currentPalette[i] = CRGB(color_blend(CRGBW(data->startPalette[i]), CRGBW(data->endPalette[i]), blendAmount));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 10839 - 10849, The sweep branch (when
SEGMENT.check2 is true) jumps *stepsDone to expectedSteps and only applies the
final microstep, losing intermediate palette-entry updates; change the logic in
the block using *stepsDone, expectedSteps, data->currentPalette,
data->startPalette, data->endPalette, CRGB/CRGBW and color_blend so that you
advance step-by-step for any skipped microsteps: iterate from the previous
stepsDone+1 up to expectedSteps and for each step compute i = step % 16 and
blendAmount = step / 16 and update data->currentPalette[i] accordingly (instead
of applying only the final step), then set *stepsDone = expectedSteps after the
loop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A new effect that is purely made for very slow transitions, up to 255 minutes.
It also allows to call a preset after the transition is done by setting intensity slider to the preset number to be applied after the transition has finished.
Using segment layering, this also allows it to be a "mask" although that takes a bit of tinkering with the preset json.
One scenario possible with this effect would be this:
The FX only transitions palettes (colors if palette 0 or any color derived palette is used), brightness slider transitions normally i.e. fast. If a color/palette is changed mid transition, it starts a new transition towards that color with the current state as a starting point so transitions never jump.
p.s.
I also remvoed the return value from
Segment::loadPalette()` as the target palette is passed by reference and the return value is never even used anymore.Fixes #5375
Summary by CodeRabbit