Additional features for the Dissolve FX#5437
Additional features for the Dissolve FX#5437BobLoeffler68 wants to merge 3 commits intowled:mainfrom
Conversation
…s for both filling and dissolving); existing slider is only for dissolving now. Added a checkbox to select whether one last LED will stay lit ("last one" mode). Added a slider for the delay when only one LED is lit if in last one mode.
In last one mode, if the delay slider is max (255), the effect will not redraw any LEDs, so this can be used with a playlist and physical button to, for example, restart the animation by unchecking checkbox 3 using a preset and then checking it again using another preset. That will redraw the LEDs, stop with only one random LED lit, and then wait until the playlist is run again. This was requested in wled#1044
WalkthroughDissolve effect in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
699-700: Fix the packed-state comment.
DISSOLVE_PREV_LAST_ONEnow lives inSEGENV.aux0, so theaux1note is already misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 699 - 700, Update the packed-state comment in FX.cpp to reflect that DISSOLVE_PREV_LAST_ONE was moved into SEGENV.aux0: state that aux0 now packs the phase/stage of the dissolve/refill process and DISSOLVE_PREV_LAST_ONE, and that aux1 only contains the random survivor pixel index (no longer contains previous last-one mode); update the text near the existing comment referencing DISSOLVE_PREV_LAST_ONE, SEGENV.aux0, and aux1 to avoid the misleading description.
🤖 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 741-745: The code currently reseeds SEGENV.aux1 at the phase-4
exit (where SEGENV.step is reset and SET_PHASE(2)/SET_DONE(0) are called), which
can pick a new survivor index while the previous one is still lit in pixels[],
causing later refill frames to use two seeds; remove the reseed from that exit
and instead assign SEGENV.aux1 at the start of the actual refill logic (the
phase-2 path that processes the refill — the block that previously used aux1 to
light the refill pixel), so the survivor index remains stable until refill
begins; apply the same change for the second identical reseed site as well.
- Around line 754-758: dissolve() now uses SEGMENT.custom1, SEGMENT.custom2,
SEGMENT.check2 and SEGMENT.check3 but only FX_MODE_DISSOLVE exposes those
controls in its metadata; update the metadata entry for FX_MODE_DISSOLVE_RANDOM
to expose the same shared controls so UI can set fill-speed and last-one
behavior for both modes. Locate the metadata definition that mirrors
FX_MODE_DISSOLVE (the entry for FX_MODE_DISSOLVE_RANDOM) and add the same
parameter flags/fields for custom1, custom2, check2 and check3 (same as the
FX_MODE_DISSOLVE metadata) so SEGMENT.custom1/custom2/check2/check3 are editable
for the random variant as well. Ensure you make the same change for the other
occurrence around the referenced second location (near line 836) so both
metadata entries match.
---
Nitpick comments:
In `@wled00/FX.cpp`:
- Around line 699-700: Update the packed-state comment in FX.cpp to reflect that
DISSOLVE_PREV_LAST_ONE was moved into SEGENV.aux0: state that aux0 now packs the
phase/stage of the dissolve/refill process and DISSOLVE_PREV_LAST_ONE, and that
aux1 only contains the random survivor pixel index (no longer contains previous
last-one mode); update the text near the existing comment referencing
DISSOLVE_PREV_LAST_ONE, SEGENV.aux0, and aux1 to avoid the misleading
description.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
741-745:⚠️ Potential issue | 🟠 MajorDon't reseed the survivor before refill starts.
Line 745 picks a new
SEGENV.aux1while the previous survivor is still lit inpixels[]. On the next phase-2 frame, Lines 778-781 light that new index too, so later cycles can refill from two seeds instead of the paused “last one”. Move the reseed into the phase-2 → phase-3 transition instead.Also applies to: 778-781, 810-815
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 741 - 745, SEGENV.aux1 is being reseeded too early (during the phase-1→phase-2 transition), which allows the new seed to be lit while the previous survivor is still present; instead, stop assigning SEGENV.aux1 in the block that sets SET_PHASE(2) (the lines around the SEGENV.step >= lastOneDelay check) and move the hw_random16(SEGLEN) assignment into the phase-2→phase-3 transition handler (the code path that currently advances from phase 2 to phase 3), so the survivor remains the single seed until refill actually begins; apply the same change to the other reseed sites noted (the blocks around the existing lines ~778-781 and ~810-815) so reseeding only happens when entering phase 3.
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
699-700: Align the aux-state comment with the new bit layout.
DISSOLVE_PREV_LAST_ONEnow lives inSEGENV.aux0, so the comment above misdescribes both aux fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 699 - 700, Update the aux-state comment to reflect that DISSOLVE_PREV_LAST_ONE now resides in SEGENV.aux0: describe aux0 as containing the packed phase/stage of the dissolve/refill process and the previous value of lastOneMode (DISSOLVE_PREV_LAST_ONE), and describe aux1 as containing the packed random survivor pixel index (and any remaining bits as applicable); reference SEGENV.aux0, DISSOLVE_PREV_LAST_ONE, aux0 and aux1 so the comment matches the new bit layout.
🤖 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 842-845: The "Random" checkbox is ignored because
mode_dissolve_random always calls dissolve(SEGMENT.color_wheel(hw_random8()));
update mode_dissolve_random to honor SEGMENT.check1: if SEGMENT.check1 is true
call dissolve with a random color (SEGMENT.color_wheel(hw_random8())), otherwise
call dissolve using the segment's configured color (e.g., SEGMENT.colors[0] or
the appropriate primary color accessor), leaving _data_FX_MODE_DISSOLVE_RANDOM
unchanged.
---
Duplicate comments:
In `@wled00/FX.cpp`:
- Around line 741-745: SEGENV.aux1 is being reseeded too early (during the
phase-1→phase-2 transition), which allows the new seed to be lit while the
previous survivor is still present; instead, stop assigning SEGENV.aux1 in the
block that sets SET_PHASE(2) (the lines around the SEGENV.step >= lastOneDelay
check) and move the hw_random16(SEGLEN) assignment into the phase-2→phase-3
transition handler (the code path that currently advances from phase 2 to phase
3), so the survivor remains the single seed until refill actually begins; apply
the same change to the other reseed sites noted (the blocks around the existing
lines ~778-781 and ~810-815) so reseeding only happens when entering phase 3.
---
Nitpick comments:
In `@wled00/FX.cpp`:
- Around line 699-700: Update the aux-state comment to reflect that
DISSOLVE_PREV_LAST_ONE now resides in SEGENV.aux0: describe aux0 as containing
the packed phase/stage of the dissolve/refill process and the previous value of
lastOneMode (DISSOLVE_PREV_LAST_ONE), and describe aux1 as containing the packed
random survivor pixel index (and any remaining bits as applicable); reference
SEGENV.aux0, DISSOLVE_PREV_LAST_ONE, aux0 and aux1 so the comment matches the
new bit layout.
| void mode_dissolve_random(void) { | ||
| dissolve(SEGMENT.color_wheel(hw_random8())); | ||
| } | ||
| static const char _data_FX_MODE_DISSOLVE_RANDOM[] PROGMEM = "Dissolve Rnd@Repeat speed,Dissolve speed;,!;!"; | ||
| static const char _data_FX_MODE_DISSOLVE_RANDOM[] PROGMEM = "Dissolve Rnd@Repeat speed,Dissolve speed,Fill speed,Last one delay,,Random,Complete,Last one;!,!;!;!;o1=1"; |
There was a problem hiding this comment.
Random is a dead control in Dissolve Rnd.
Line 843 always passes a random color to dissolve(), so the checkbox exposed at Line 845 can never change behavior. Either hide that slot for this variant or honor SEGMENT.check1 here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wled00/FX.cpp` around lines 842 - 845, The "Random" checkbox is ignored
because mode_dissolve_random always calls
dissolve(SEGMENT.color_wheel(hw_random8())); update mode_dissolve_random to
honor SEGMENT.check1: if SEGMENT.check1 is true call dissolve with a random
color (SEGMENT.color_wheel(hw_random8())), otherwise call dissolve using the
segment's configured color (e.g., SEGMENT.colors[0] or the appropriate primary
color accessor), leaving _data_FX_MODE_DISSOLVE_RANDOM unchanged.
…rted. That is fixed now. And I made a comment change. Both recommendations by coderabbit.
|
making changes to existing FX is a bit tricky: they need to stay backwards compatible so no breaking changes to existing setups if it can be avoided. I did not yet check the code but generally adding more than a few lines of code is a no-go (unless well justified). |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
841-844:⚠️ Potential issue | 🟡 MinorThe
Randomcheckbox inDissolve Rndis dead.
mode_dissolve_random()always passes a random color intodissolve(), so unchecking the control exposed by_data_FX_MODE_DISSOLVE_RANDOMnever changes behavior. If this variant should stay permanently random, hide that slot instead.💡 Minimal fix
void mode_dissolve_random(void) { - dissolve(SEGMENT.color_wheel(hw_random8())); + dissolve(SEGMENT.check1 ? SEGMENT.color_wheel(hw_random8()) : SEGCOLOR(0)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 841 - 844, mode_dissolve_random always passes a random color to dissolve() so the "Random" checkbox declared in _data_FX_MODE_DISSOLVE_RANDOM does nothing; either 1) respect the checkbox by reading the control/parameter used by other modes (the same parameter index exposed by _data_FX_MODE_DISSOLVE_RANDOM) and call dissolve() with hw_random8() only when that parameter is set, otherwise pass the deterministic color value used by the non-random dissolve mode, or 2) if this variant is intended to be permanently random, remove or hide the "Random" slot from _data_FX_MODE_DISSOLVE_RANDOM so the UI doesn't expose a dead control; implement the chosen approach by editing mode_dissolve_random to check the control flag and choose the color source, or by editing the PROGMEM string _data_FX_MODE_DISSOLVE_RANDOM to remove the Random option.
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
702-707: Replace the packed-state magic numbers with named constants.The raw phase IDs (
0..4) and bit masks inSET_DONE/SET_PREV_LAST_ONEare now referenced across several branches, which makes this state machine harder to audit than it needs to be. A small local enum plus named mask constants would make future tweaks much safer.Based on learnings: In WLED code reviews, when code is modified or added, look for "magic numbers" (hardcoded numeric literals) and suggest replacing them with defined constants when meaningful constants exist.
Also applies to: 753-815
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/FX.cpp` around lines 702 - 707, The packed-state masks and phase values used with SEGENV.aux0 (DISSOLVE_PHASE, DISSOLVE_DONE, DISSOLVE_PREV_LAST_ONE, SET_PHASE, SET_DONE, SET_PREV_LAST_ONE) use hardcoded bit offsets and implicit phase IDs; replace these magic numbers with local named constants and an enum for phase IDs (e.g., enum DissolvePhase { PHASE_INIT=0, PHASE_FADE=1, ... }) and bitmask constants (e.g., DISSOLVE_DONE_MASK = 1<<8, DISSOLVE_PREV_LAST_ONE_MASK = 1<<9) so the getter macros use masks and shifts by name and the setter macros use those named masks and shifts; update all uses across the block (including functions that read/write SEGENV.aux0) to reference the new enum and mask constants instead of raw numeric literals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wled00/FX.cpp`:
- Around line 841-844: mode_dissolve_random always passes a random color to
dissolve() so the "Random" checkbox declared in _data_FX_MODE_DISSOLVE_RANDOM
does nothing; either 1) respect the checkbox by reading the control/parameter
used by other modes (the same parameter index exposed by
_data_FX_MODE_DISSOLVE_RANDOM) and call dissolve() with hw_random8() only when
that parameter is set, otherwise pass the deterministic color value used by the
non-random dissolve mode, or 2) if this variant is intended to be permanently
random, remove or hide the "Random" slot from _data_FX_MODE_DISSOLVE_RANDOM so
the UI doesn't expose a dead control; implement the chosen approach by editing
mode_dissolve_random to check the control flag and choose the color source, or
by editing the PROGMEM string _data_FX_MODE_DISSOLVE_RANDOM to remove the Random
option.
---
Nitpick comments:
In `@wled00/FX.cpp`:
- Around line 702-707: The packed-state masks and phase values used with
SEGENV.aux0 (DISSOLVE_PHASE, DISSOLVE_DONE, DISSOLVE_PREV_LAST_ONE, SET_PHASE,
SET_DONE, SET_PREV_LAST_ONE) use hardcoded bit offsets and implicit phase IDs;
replace these magic numbers with local named constants and an enum for phase IDs
(e.g., enum DissolvePhase { PHASE_INIT=0, PHASE_FADE=1, ... }) and bitmask
constants (e.g., DISSOLVE_DONE_MASK = 1<<8, DISSOLVE_PREV_LAST_ONE_MASK = 1<<9)
so the getter macros use masks and shifts by name and the setter macros use
those named masks and shifts; update all uses across the block (including
functions that read/write SEGENV.aux0) to reference the new enum and mask
constants instead of raw numeric literals.
Yeah, I was aware that it could be tricky, but I think I made it backwards compatible since the previous functions are still there and I'm not setting any defaults that are different than what were originally there. But, if you think this is a problem, I could create it as a new effect in user_fx.cpp |
I think I will just close this PR and create a new one for a new Dissolve effect (e.g. Dissolve Plus) in user_fx. I'll do that in the morning. It's past my bedtime. Thx for feedback... and all of the work you guys are doing! |
|
@coderabbitai review these changes and highlight any breaking changes, highlight changes that will alter existing presets. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me examine the actual code changes in the PR. 🏁 Script executed: Length of output: 7239 🏁 Script executed: Length of output: 778
Breaking Change Analysis for PR
|
| Before | After | |
|---|---|---|
| Fill pixel logic | hw_random8() <= SEGMENT.intensity |
hw_random8() <= SEGMENT.custom1 |
SEGMENT.intensity role |
Controls both fill and dissolve | Controls dissolve only |
SEGMENT.custom1 role |
Unused (always 0 in saved presets) | Controls fill speed |
Since all existing presets have custom1 = 0, the new condition hw_random8() <= 0 is only true ~0.4% of the time. The fill phase will effectively stall — no pixels will turn on — making the effect appear broken on any saved preset.
A backwards-compatible fix would be to treat custom1 == 0 as a fallback to intensity:
uint8_t fillSpeed = SEGMENT.custom1 == 0 ? SEGMENT.intensity : SEGMENT.custom1;
if (hw_random8() <= (filling ? fillSpeed : SEGMENT.intensity)) {🟡 Behavioral Change: "Complete" (check2) Timing Logic
Affects existing presets that use the Complete checkbox.
| Before | After | |
|---|---|---|
SEGENV.step when check2=true and pixels incomplete |
Not incremented (waits for completion first) | Incremented continuously; reset to 0 once complete, then delay runs |
Old behavior: the "Repeat speed" delay only began counting after all pixels had changed. New behavior: the delay timer runs during the transition, then resets and runs again after completion. This effectively doubles the delay for the transition period and changes the visible rhythm of the effect.
🟡 mode_dissolve_random Metadata String Changes
Original: "Dissolve Rnd@Repeat speed,Dissolve speed;,!;!"
New: "Dissolve Rnd@Repeat speed,Dissolve speed,Fill speed,Last one delay,,Random,Complete,Last one;!,!;!;!;o1=1"
- New sliders (
custom1,custom2) and new checkboxes (check1,check2,check3) are added. The fill speed issue above applies here too. o1=1in the defaults field setscheck1(Random) on by default for new instances only — this does not affect existing presets (presets store their own checkbox values).- The color slot
!,!(was,!) is cosmetic only.
✅ Non-Breaking Changes
SEGENV.aux0semantic change (0/1 → packed bit field): Safe, becauseSEGENVis runtime state and is re-initialized whenSEGENV.call == 0.- New phase 4 and state machine: Gated behind
lastOneMode(check3), which isfalsein all existing presets. - Survivor pixel protection logic: Only active when
lastOneModeis true, so existing presets are unaffected.
Summary Table
| Change | Existing Presets Affected? | Severity |
|---|---|---|
custom1 used as fill speed (was 0) |
All Dissolve / Dissolve Rnd presets | 🔴 Critical |
check2 (Complete) step timing |
Presets with Complete enabled |
🟡 Medium |
| Dissolve Rnd: new sliders/checkboxes | All Dissolve Rnd presets (fill speed = 0) | 🟡 Medium |
SEGENV.aux0 reinterpretation |
None (runtime only) | ✅ Safe |
| New phases / lastOneMode logic | None (off by default) | ✅ Safe |
The most pragmatic resolution, as @BobLoeffler68 already suggested, would be to implement this as a new effect (e.g. Dissolve Plus) in user_fx.cpp, which avoids all of the above preset compatibility concerns entirely.
[review]
🧠 Learnings used
Learnt from: DedeHai
Repo: wled/WLED PR: 4798
File: wled00/FX.cpp:7531-7533
Timestamp: 2025-08-26T11:51:21.817Z
Learning: In WLED PR `#4798`, DedeHai confirmed that certain gamma-related calls in FX.cpp/FX_fcn.cpp/particle systems are intentional for effect-level shaping (e.g., brightness curves, TV sim, Pride 2015 pre-mix), distinct from final output gamma. Do not flag or remove these in future reviews; add comments when feasible to clarify intent.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:167-171
Timestamp: 2026-01-13T03:07:20.560Z
Learning: In WLED effects (usermods/user_fx/user_fx.cpp and similar), when storing future timestamps, assignments like `stored_time = now + delay` are correct and handle uint32_t wraparound properly. Only comparison statements need to use subtraction-based elapsed-time checks (e.g., `(now - stored_time) >= delay`). Do not flag assignment statements as needing fixes for wraparound handling.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2025-12-28T14:06:48.772Z
Learning: In WLED post-commit ee9ac947, the rendering pipeline uses per-segment buffers and per-pixel bus updates. Unmapped (0xFFFF) mapping entries are now skipped in WS2812FX::show() (no “clear to black”), which can leave physical gap LEDs with stale/random colors unless they are explicitly cleared. This is a behavior change from pre-0.16 where a full physical buffer was effectively refreshed each frame.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5109
File: wled00/FX.cpp:3174-3343
Timestamp: 2025-11-27T06:33:11.436Z
Learning: WLED Ants effect (wled00/FX.cpp): The author prefers the current velocity initialization using hw_random16(1000, 5000)/5000.0f, resulting in an effective range of ~3.6–10.0 (with VELOCITY_MIN=2.0, VELOCITY_MAX=10.0), and wants the code kept as-is with comments updated to document this behavior. Avoid suggesting changes to span the full 2.0–10.0 range in future reviews.
Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-08T00:57:36.134Z
Learning: In WLED (wled00/cfg.cpp), `deserializeConfig()` is called with an empty JsonObject when cfg.json doesn't exist on fresh install. Any value read without the ArduinoJSON `|` fallback operator or CJSON macro will override correct constructor defaults with 0/null. Known affected values: `strip.setTargetFps(hw_led["fps"])` at line 179 (sets FPS to 0/unlimited instead of WLED_FPS=42). Fix: `strip.setTargetFps(hw_led["fps"] | WLED_FPS)`. The gamma issue (gammaCorrectCol/Bri) had the same root cause and was fixed in commit d1d9dec402 (Jan 2026) using inline `| default` fallbacks.
Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5293
File: usermods/user_fx/user_fx.cpp:149-151
Timestamp: 2026-01-24T18:51:09.740Z
Learning: In WLED's Spinning Wheel effect (usermods/user_fx/user_fx.cpp), the color scale calculation (SEGENV.aux1) must match the position modulo range. When position uses `pos = (pos_fixed >> 16) % SEGLEN`, the color scale must be `aux1 = (255 << 8) / SEGLEN` to distribute the palette evenly across the full position range. Using vstripLen for color scaling while position uses SEGLEN causes palette colors to cycle too quickly and appear incomplete.
Learnt from: DedeHai
Repo: wled/WLED PR: 4923
File: wled00/FX.cpp:4883-4901
Timestamp: 2025-09-12T17:29:43.826Z
Learning: In WLED’s web UI, only one slider value (e.g., SEGMENT.intensity or SEGMENT.custom1) changes at a time; code relying on this may use simplified change guards, though presets/JSON can still update multiple fields atomically.
Learnt from: blazoncek
Repo: wled/WLED PR: 5140
File: wled00/data/settings_time.htm:66-76
Timestamp: 2025-12-01T07:01:16.949Z
Learning: In WLED PR `#5134`, the fix for macros being initialized with the enable bit set only handles new configurations, not existing ones. If there is a bug in timer/macro handling code (e.g., in settings_time.htm), it must be fixed to work correctly for existing configurations as well.
|
@DedeHai and @coderabbitai Thanks for the analysis. I will close this PR and create a new one for adding this as a new effect in user_fx. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
|
Closing this PR and will create a new one to add a new effect called Dissolve Plus in the user_fx usermod. |
This PR will add the following features to the Dissolve effect in FX.cpp
In last one mode, if the delay slider is max (255), the effect will not redraw any LEDs, so this can be used with a playlist and physical button to, for example, restart the animation by unchecking checkbox 3 using a preset and then checking it again using another preset. That will redraw the LEDs, stop with only one random LED lit, and then wait until the playlist is run again.
A couple of these features were requested in #1044 back in 2020.
Summary by CodeRabbit