fix color jump if changing mid transition#5601
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughstartTransition() now preserves visual continuity for in-flight transitions by restoring or blending the old segment from the transition-start state, capturing current blended colors when no old segment is created, and conditionally resetting palette-blend tracking and timers. ChangesMid-Transition State Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
@blazoncek I looked at your commit blazoncek@40d9812 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
DedeHai
added
optimization
|
A credit would be due. Don't you think? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 301-324: When promoting an in-flight fade into a segmentCopy
transition (the branch where segmentCopy && !_t->_oldSegment), refresh the
transition's stored "from" visual state so it reflects the current blended
frame: for each color set _t->_colors[i] to the current blend (use color_blend16
with _t->_colors[i], colors[i], and _t->_progress), and set _t->_bri =
currentBri() and _t->_cct = currentCCT() before restarting _t->_start/_t->_dur
and creating _t->_oldSegment; this ensures beginDraw(), currentBri(), and
currentCCT() animate from the actual mid-fade values rather than the original
start values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d26c0df1-c4fb-45ce-a2c4-775a180bdc4e
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
|
BTW both our fixes are still flawed to some extent. Question is if it is worth investing much time in edge cases. |
|
I am aware of that, I think fixing all edge cases is not worth it ATM |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
301-316:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMid-transition
_tstate not refreshed insegmentCopybranch — color/brightness jump still possible.When this branch upgrades an in-flight FADE transition into a
segmentCopytransition (e.g.,setMode()always passessegmentCopy=trueregardless ofblendingStyle),_t->_start/_t->_durare restarted, but_t->_colors,_t->_bri,_t->_cct, and_t->_paletteare left at the original transition‑start values. After the restart, the new segment'sbeginDraw()(in FADE blending style) blends from the stale_t->_colorsatprog=0, so the first frames render the original start colors before the mode change rather than the current blended frame — i.e. exactly the kind of jump this PR aims to remove, just constrained to the FADE+mode‑change path.The new
else if (_t->_progress > 0)branch (Lines 317‑332) already does this state capture; consider sharing it (or duplicating the snapshot) for thesegmentCopy && !_t->_oldSegmentcase as well so that_t->_colorsreflects the current blended visual before_oldSegmentis set up. If the author considers this an acceptable remaining edge case (per the PR discussion), a brief comment in the source would help future readers.💡 Suggested adjustment
if (isInTransition()) { if (segmentCopy && !_t->_oldSegment) { // already in transition but segment copy requested and not yet created + // capture current blended "from" state so FADE-style mid-transition mode changes don't jump back + if (_t->_progress > 0) { + for (unsigned i = 0; i < NUM_COLORS; i++) _t->_colors[i] = color_blend16(_t->_colors[i], colors[i], _t->_progress); + _t->_bri = currentBri(); + _t->_cct = currentCCT(); + _t->_palette = palette; + } _t->_oldSegment = new(std::nothrow) Segment(*this); // store/copy current segment settings _t->_start = millis(); // restart transition timer _t->_dur = dur; _t->_prevPaletteBlends = 0; // reset palette blends if (_t->_oldSegment) { _t->_oldSegment->palette = _t->_palette; // restore original palette and colors (from start of transition) - // if already partway through a FADE transition, set old segment's colors to current blend to avoid jumping back to original colors - if (_t->_progress > 0) - for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = color_blend16(_t->_colors[i], colors[i], _t->_progress); - else - for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = _t->_colors[i]; + for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = _t->_colors[i];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX_fcn.cpp` around lines 301 - 316, The segmentCopy branch creates _t->_oldSegment and restarts the transition (_t->_start/_t->_dur) but fails to refresh the in-flight transition snapshot variables (_t->_colors, _t->_bri, _t->_cct, _t->_palette), so beginDraw() can still blend from stale start values and cause a visible jump; update that branch in the segmentCopy && !_t->_oldSegment block to capture the current blended state the same way the existing else-if (_t->_progress > 0) branch does (compute current blended colors and brightness/cct/palette based on _t->_progress and assign them into _t->_colors/_t->_bri/_t->_cct/_t->_palette before creating _oldSegment), then proceed to set _t->_oldSegment and reset _t->_start/_t->_dur; keep the existing isActive()/stopTransition() check unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@wled00/FX_fcn.cpp`:
- Around line 301-316: The segmentCopy branch creates _t->_oldSegment and
restarts the transition (_t->_start/_t->_dur) but fails to refresh the in-flight
transition snapshot variables (_t->_colors, _t->_bri, _t->_cct, _t->_palette),
so beginDraw() can still blend from stale start values and cause a visible jump;
update that branch in the segmentCopy && !_t->_oldSegment block to capture the
current blended state the same way the existing else-if (_t->_progress > 0)
branch does (compute current blended colors and brightness/cct/palette based on
_t->_progress and assign them into _t->_colors/_t->_bri/_t->_cct/_t->_palette
before creating _oldSegment), then proceed to set _t->_oldSegment and reset
_t->_start/_t->_dur; keep the existing isActive()/stopTransition() check
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15e914e0-ae92-4d22-9369-a09796751917
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/FX_fcn.cpp (1)
301-316:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFirst branch still leaves
_t->_bri/_t->_cct(and_t->_colors) stale before restarting the transition.The new
else if (_t->_progress > 0)branch correctly refreshes_t->_colors,_t->_bri, and_t->_cctto the current blended values before restarting the timer. The first branch (segmentCopy upgrade of an in-flight transition), however, still bumps_t->_start/_t->_durand resets_t->_prevPaletteBlendswithout touching_t->_colors/_t->_bri/_t->_cct. Concrete case: FADEsetOpacityin flight →setMode(alwayssegmentCopy=true) enters this branch; on the next frameprogress()→0, socurrentBri()returns_t->_bri(the original pre-fade brightness), producing a visible snap-back in opacity (and analogously CCT, and colors viabeginDraw()for the new segment when style is FADE)._t->_oldSegment->colors[]is already set from the current blend, so the old segment renders correctly — only the “from” state used bycurrentBri/CCT()and FADE color blending is stale.The fix is symmetric to what the else-if branch already does — capture the current blended state before restart:
💡 Suggested fix
if (segmentCopy && !_t->_oldSegment) { // already in transition but segment copy requested and not yet created + // capture current blended state as the new "from" so beginDraw()/currentBri()/currentCCT() don't snap back after the restart + if (_t->_progress > 0) { + for (unsigned i = 0; i < NUM_COLORS; i++) _t->_colors[i] = color_blend16(_t->_colors[i], colors[i], _t->_progress); + _t->_bri = currentBri(); + _t->_cct = currentCCT(); + } _t->_oldSegment = new(std::nothrow) Segment(*this); // store/copy current segment settings _t->_start = millis(); // restart transition timer _t->_dur = dur; _t->_prevPaletteBlends = 0; // reset palette blends if (_t->_oldSegment) { _t->_oldSegment->palette = _t->_palette; // restore original palette and colors (from start of transition) - // if already partway through a FADE transition, set old segment's colors to current blend to avoid jumping back to original colors - if (_t->_progress > 0) - for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = color_blend16(_t->_colors[i], colors[i], _t->_progress); - else - for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = _t->_colors[i]; + // _t->_colors now holds the current blended state (updated above), so _oldSegment can mirror it directly + for (unsigned i = 0; i < NUM_COLORS; i++) _t->_oldSegment->colors[i] = _t->_colors[i];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wled00/FX_fcn.cpp` around lines 301 - 316, In the segmentCopy && !_t->_oldSegment branch, also capture and store the current blended "from" state before resetting the transition timer: if _t->_progress > 0, update _t->_colors[i] to color_blend16(_t->_colors[i], colors[i], _t->_progress) for all NUM_COLORS and set _t->_bri and _t->_cct to their current blended values (use the same blending logic as the existing else-if branch) so currentBri()/currentCCT() and FADE blending won’t snap back when _t->_start/_t->_dur are restarted; keep the existing assignments to _t->_oldSegment, _t->_start, _t->_dur and _t->_prevPaletteBlends and preserve the isActive() check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@wled00/FX_fcn.cpp`:
- Around line 301-316: In the segmentCopy && !_t->_oldSegment branch, also
capture and store the current blended "from" state before resetting the
transition timer: if _t->_progress > 0, update _t->_colors[i] to
color_blend16(_t->_colors[i], colors[i], _t->_progress) for all NUM_COLORS and
set _t->_bri and _t->_cct to their current blended values (use the same blending
logic as the existing else-if branch) so currentBri()/currentCCT() and FADE
blending won’t snap back when _t->_start/_t->_dur are restarted; keep the
existing assignments to _t->_oldSegment, _t->_start, _t->_dur and
_t->_prevPaletteBlends and preserve the isActive() check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13c269ca-3bc6-4117-a695-2aa0e8ec174a
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
2 participants
This improves the color changes while a FADE transition is going on.
Summary by CodeRabbit