Dissolve Plus: new effect that extends the features of the Dissolve effect#5440
Open
BobLoeffler68 wants to merge 5 commits intowled:mainfrom
Open
Dissolve Plus: new effect that extends the features of the Dissolve effect#5440BobLoeffler68 wants to merge 5 commits intowled:mainfrom
BobLoeffler68 wants to merge 5 commits intowled:mainfrom
Conversation
Contributor
|
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:
WalkthroughAdds a new WLED user FX Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1347-1350: The code uses strip.getMainSegment().palette when
deciding between color_wheel and color_from_palette inside the SEGMENT.check1
block, causing the choice to follow another segment; change the palette lookup
to use the current segment's palette (SEGMENT.palette) instead of
strip.getMainSegment().palette in the pId assignment so the wheel-vs-palette
decision is local to the segment (update the uint8_t pId = ... line).
- Around line 1323-1333: The code enters a "freezeForever" state (lastOneMode &&
SEGMENT.custom2 == 255) but still redraws the frozen frame every tick by running
the loop that calls SEGMENT.setPixelColor(i, pixels[i]); to fix this, detect
freezeForever and skip the redraw loop entirely (return early) when true so the
renderer is not updated; update the block around SEGENV.step / lastOneDelay to
set SEGENV.step and phase as before but, if freezeForever, do not execute the
for-loop that writes pixels[] via SEGMENT.setPixelColor and simply return to
avoid overwriting presets/playlist changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44034dfd-1b4e-41ae-92ee-8c06cd878dbe
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
- Will now use SEGMENT.palette to get the palette id, not the main segment’s palette. - and Last one delay == 255 was still redrawing the frozen frame every tick. - Also, if a palette is selected that has colors in it that are the same as the background color, the "last one" LED wasn't displaying sometimes, so I made that a little better by selecting a different random LED. It's still possible that it will be an LED with a similar color as the background, but there is no way to completely prevent that from happening.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1307-1321: When handling the Last one toggle in the block that
compares DISSOLVE_PREV_LAST_ONE and lastOneMode, also handle the false->true
inverse: if lastOneMode is now false and the current phase is one of the
survivor-only phases (check PHASE_FILL_SURVIVOR and PHASE_PAUSE_SURVIVOR), reset
the effect back to the base dissolve state by calling SET_PHASE(PHASE_DISSOLVE),
clear any survivor-specific state such as SEGENV.step and SET_DONE(0) and ensure
SEGENV.aux1 or related indices are reinitialized as needed, then call
SET_PREV_LAST_ONE(lastOneMode ? 1 : 0); this ensures survivor-only phases are
normalized when the Last one checkbox is cleared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e61ea8bf-29cd-4b62-9e0f-afbcf9f234bf
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
…ses when Last one is turned off.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
1359-1371: Minor formatting issue on line 1365.The closing brace should be on its own line, not on the same line as the assignment statement.
🧹 Proposed fix
if (SEGMENT.check1) { uint8_t pId = SEGMENT.palette; uint32_t c = (pId == 0) ? SEGMENT.color_wheel(hw_random8()) : SEGMENT.color_from_palette(hw_random16(SEGLEN), true, PALETTE_SOLID_WRAP, 0); if (c == SEGCOLOR(1)) c ^= 0x00000001; // flip the last bit to make sure it is slightly different than the background color - pixels[i] = c; } + pixels[i] = c; + } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1359 - 1371, In the filling branch inside user_fx.cpp (the block using filling, pixels[i], SEGCOLOR and SEGMENT.check1), move the closing brace that currently shares the line with the assignment "pixels[i] = c;" so that the brace is on its own line; locate the inner if (SEGMENT.check1) { ... pixels[i] = c; } and ensure the '}' that closes that if is placed on its own line before the else, preserving existing indentation and leaving the subsequent else and break lines unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1359-1371: In the filling branch inside user_fx.cpp (the block
using filling, pixels[i], SEGCOLOR and SEGMENT.check1), move the closing brace
that currently shares the line with the assignment "pixels[i] = c;" so that the
brace is on its own line; locate the inner if (SEGMENT.check1) { ... pixels[i] =
c; } and ensure the '}' that closes that if is placed on its own line before the
else, preserving existing indentation and leaving the subsequent else and break
lines unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e515ddf9-8200-4aab-a0a7-12abc502ea31
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
usermods/user_fx/user_fx.cpp (1)
1317-1323:⚠️ Potential issue | 🟠 MajorNormalize
PHASE_PAUSE_SURVIVORwhenLast oneis cleared.This branch still leaves the effect in
PHASE_PAUSE_SURVIVOR. Line 1330 then keeps honoring the survivor-only pause, and withcustom2 == 255it can hold the frozen one-pixel frame for hundreds of ticks after checkbox 3 is already off.💡 Proposed fix
- } else if (DISSOLVE_PHASE == PHASE_FILL_SURVIVOR) { + } else if (DISSOLVE_PHASE == PHASE_FILL_SURVIVOR || DISSOLVE_PHASE == PHASE_PAUSE_SURVIVOR) { SET_PHASE(PHASE_FILL); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1317 - 1323, When clearing the "last one" survivor state you must also normalize a survivor-only pause into the global pause; in the same branch that handles DISSOLVE_PHASE == PHASE_DISSOLVE_SURVIVOR / PHASE_FILL_SURVIVOR add a check for DISSOLVE_PHASE == PHASE_PAUSE_SURVIVOR and call SET_PHASE(PHASE_PAUSE) so PHASE_PAUSE_SURVIVOR is not left active (use the existing DISSOLVE_PHASE, PHASE_PAUSE_SURVIVOR, SET_PHASE and PHASE_PAUSE symbols to locate and update the code).
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
1309-1316: Drop the retry loop after resettingpixels[].After Line 1310 every entry is
SEGCOLOR(1), so thisdo/whilecannot succeed early and always burns up toSEGLENrandom draws before exiting. A singleSEGENV.aux1 = hw_random16(SEGLEN);is enough here.♻️ Proposed simplification
- unsigned attempts = 0; - do { - SEGENV.aux1 = hw_random16(SEGLEN); - attempts++; - } while (pixels[SEGENV.aux1] == SEGCOLOR(1) && attempts < SEGLEN); + SEGENV.aux1 = hw_random16(SEGLEN);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 1309 - 1316, The retry loop that fills SEGENV.aux1 with hw_random16(SEGLEN) after pixels[] is reset is redundant; replace the do/while loop in the lastOneMode branch with a single assignment SEGENV.aux1 = hw_random16(SEGLEN); (leave the surrounding SET_PHASE(PHASE_DISSOLVE) and pixels fill intact) and remove the attempts counter and loop logic so only one random draw occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1383-1388: The survivor pixel is being restored using a
position-mapped palette color (SEGMENT.color_from_palette(..., true,...)) which
forces a deterministic color in Random mode; change the generation to use the
same random/wheel selection as the rest of the fill by calling
SEGMENT.color_from_palette with the position-mapping flag set to false (i.e.,
replace the second argument true with false when computing c for
pixels[SEGENV.aux1]), preserving the existing c == SEGCOLOR(1) toggle and
assignment logic.
---
Duplicate comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1317-1323: When clearing the "last one" survivor state you must
also normalize a survivor-only pause into the global pause; in the same branch
that handles DISSOLVE_PHASE == PHASE_DISSOLVE_SURVIVOR / PHASE_FILL_SURVIVOR add
a check for DISSOLVE_PHASE == PHASE_PAUSE_SURVIVOR and call
SET_PHASE(PHASE_PAUSE) so PHASE_PAUSE_SURVIVOR is not left active (use the
existing DISSOLVE_PHASE, PHASE_PAUSE_SURVIVOR, SET_PHASE and PHASE_PAUSE symbols
to locate and update the code).
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1309-1316: The retry loop that fills SEGENV.aux1 with
hw_random16(SEGLEN) after pixels[] is reset is redundant; replace the do/while
loop in the lastOneMode branch with a single assignment SEGENV.aux1 =
hw_random16(SEGLEN); (leave the surrounding SET_PHASE(PHASE_DISSOLVE) and pixels
fill intact) and remove the attempts counter and loop logic so only one random
draw occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49dd4e47-7b03-412e-8b2b-8bb881c765aa
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
…vor pixel when Random checkbox is selected. Also we were filling all pixels with the background color when the last one mode was changed to true, but we don't want that.
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 `@usermods/user_fx/user_fx.cpp`:
- Around line 1286-1289: In mode_dissolveplus, guard against SEGLEN == 0 before
allocating or using it as a random/index range: add an early exit (e.g. call
FX_FALLBACK_STATIC or return) if SEGLEN is zero so we never call
SEGENV.allocateData/reinterpret_cast or perform random/index operations with a
zero range; reference SEGLEN, the mode_dissolveplus function,
SEGENV.allocateData and the pixels pointer when making the change.
- Around line 1287-1290: Store and use a persistent background color snapshot
instead of reading SEGCOLOR(1) directly from the live state: when
allocating/initializing the pixel buffer (after SEGENV.allocateData and setting
pixels via SEGENV.data) capture uint32_t bgColor = SEGCOLOR(1) and use bgColor
in all dissolved-sentinel comparisons and writes (places referencing pixels[],
SEGCOLOR(1), and in Complete). Additionally, when SEGCOLOR(1) actually changes
at runtime, normalize the buffer by scanning pixels[] and mapping any entries
equal to the old bgColor to the new SEGCOLOR(1) so previously dissolved pixels
remain correctly marked; update the stored bgColor afterwards.
- Around line 1301-1305: The survivor index (SEGENV.aux1) is being chosen from
the full virtual range via hw_random16(SEGLEN), which can pick unmapped slots
and produce an invisible "last one"; update all places that set SEGENV.aux1
(e.g., the blocks around SET_PHASE(PHASE_DISSOLVE)/lines referencing
hw_random16(SEGLEN)) to pick from the segment's mapped/visible LED indices only.
Concretely, gather the segment's mapped indices (skip mapping entries equal to
0xFFFF as used by WS2812FX::show()) or iterate the mapping to build a list of
valid physical indices, then choose a random entry from that list and store its
virtual index in SEGENV.aux1 (preserving lastOneMode/SET_PREV_LAST_ONE and
SET_DONE behavior). Ensure the same mapped-only selection logic is applied to
every occurrence noted (around lines ~1302, ~1311-1315, ~1383-1394, ~1427-1431)
so the survivor is always visible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d44e6d96-5a98-4886-b4a8-fb30b24534cc
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
Comment on lines
+1286
to
+1289
| static void mode_dissolveplus(void) { | ||
| unsigned dataSize = sizeof(uint32_t) * SEGLEN; | ||
| if (!SEGENV.allocateData(dataSize)) FX_FALLBACK_STATIC; //allocation failed | ||
| uint32_t* pixels = reinterpret_cast<uint32_t*>(SEGENV.data); |
Contributor
There was a problem hiding this comment.
Guard SEGLEN == 0 before using it as a random/index range.
Unlike the other 1D effects in this file, this effect never bails out for an empty segment. Line 1302, Line 1313, Line 1355, and Line 1429 then use SEGLEN as a random range, and the later buffer accesses become undefined when the segment length is zero.
🛡️ Proposed fix
static void mode_dissolveplus(void) {
+ if (SEGLEN < 1) FX_FALLBACK_STATIC;
unsigned dataSize = sizeof(uint32_t) * SEGLEN;
if (!SEGENV.allocateData(dataSize)) FX_FALLBACK_STATIC; //allocation failed
uint32_t* pixels = reinterpret_cast<uint32_t*>(SEGENV.data);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/user_fx/user_fx.cpp` around lines 1286 - 1289, In mode_dissolveplus,
guard against SEGLEN == 0 before allocating or using it as a random/index range:
add an early exit (e.g. call FX_FALLBACK_STATIC or return) if SEGLEN is zero so
we never call SEGENV.allocateData/reinterpret_cast or perform random/index
operations with a zero range; reference SEGLEN, the mode_dissolveplus function,
SEGENV.allocateData and the pixels pointer when making the change.
Comment on lines
+1287
to
+1290
| unsigned dataSize = sizeof(uint32_t) * SEGLEN; | ||
| if (!SEGENV.allocateData(dataSize)) FX_FALLBACK_STATIC; //allocation failed | ||
| uint32_t* pixels = reinterpret_cast<uint32_t*>(SEGENV.data); | ||
|
|
Contributor
There was a problem hiding this comment.
Persist the previous background color instead of treating the live one as state.
pixels[] stores literal colors, but Line 1359, Line 1374, and Lines 1403-1405 use the current SEGCOLOR(1) as the sentinel for "dissolved". If color 2 changes while the effect is running, previously dissolved pixels stop matching, so Complete can finish early and dissolve passes start repainting stale-background pixels instead of only active ones.
🎯 One way to normalize the buffer on background-color changes
- unsigned dataSize = sizeof(uint32_t) * SEGLEN;
+ unsigned dataSize = sizeof(uint32_t) * (SEGLEN + 1);
if (!SEGENV.allocateData(dataSize)) FX_FALLBACK_STATIC; //allocation failed
uint32_t* pixels = reinterpret_cast<uint32_t*>(SEGENV.data);
+ uint32_t& storedBg = pixels[SEGLEN];
if (SEGENV.call == 0) {
for (unsigned i = 0; i < SEGLEN; i++) pixels[i] = SEGCOLOR(1);
+ storedBg = SEGCOLOR(1);
SET_PHASE(PHASE_DISSOLVE);
SEGENV.aux1 = hw_random16(SEGLEN);
SET_DONE(0);
SET_PREV_LAST_ONE(lastOneMode ? 1 : 0);
+ } else if (storedBg != SEGCOLOR(1)) {
+ for (unsigned i = 0; i < SEGLEN; i++) {
+ if (pixels[i] == storedBg) pixels[i] = SEGCOLOR(1);
+ }
+ storedBg = SEGCOLOR(1);
}Also applies to: 1299-1305, 1358-1375, 1397-1406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/user_fx/user_fx.cpp` around lines 1287 - 1290, Store and use a
persistent background color snapshot instead of reading SEGCOLOR(1) directly
from the live state: when allocating/initializing the pixel buffer (after
SEGENV.allocateData and setting pixels via SEGENV.data) capture uint32_t bgColor
= SEGCOLOR(1) and use bgColor in all dissolved-sentinel comparisons and writes
(places referencing pixels[], SEGCOLOR(1), and in Complete). Additionally, when
SEGCOLOR(1) actually changes at runtime, normalize the buffer by scanning
pixels[] and mapping any entries equal to the old bgColor to the new SEGCOLOR(1)
so previously dissolved pixels remain correctly marked; update the stored
bgColor afterwards.
Comment on lines
+1301
to
+1305
| SET_PHASE(PHASE_DISSOLVE); | ||
| SEGENV.aux1 = hw_random16(SEGLEN); | ||
| SET_DONE(0); | ||
| SET_PREV_LAST_ONE(lastOneMode ? 1 : 0); | ||
| } |
Contributor
There was a problem hiding this comment.
Choose the survivor from mapped LEDs only.
Line 1302, Line 1313, and Line 1429 sample SEGENV.aux1 from the full virtual range, and Line 1384 assumes that index is visible. On segments with mapping gaps, the "last one" can land on an unmapped slot, so the pause/freeze phase ends up with no visible LED lit even though the internal state thinks there is a survivor.
Based on learnings: unmapped 0xFFFF mapping entries are skipped in WS2812FX::show(), so selecting a gap index here leaves the survivor non-visible.
Also applies to: 1311-1315, 1383-1394, 1427-1431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@usermods/user_fx/user_fx.cpp` around lines 1301 - 1305, The survivor index
(SEGENV.aux1) is being chosen from the full virtual range via
hw_random16(SEGLEN), which can pick unmapped slots and produce an invisible
"last one"; update all places that set SEGENV.aux1 (e.g., the blocks around
SET_PHASE(PHASE_DISSOLVE)/lines referencing hw_random16(SEGLEN)) to pick from
the segment's mapped/visible LED indices only. Concretely, gather the segment's
mapped indices (skip mapping entries equal to 0xFFFF as used by
WS2812FX::show()) or iterate the mapping to build a list of valid physical
indices, then choose a random entry from that list and store its virtual index
in SEGENV.aux1 (preserving lastOneMode/SET_PREV_LAST_ONE and SET_DONE behavior).
Ensure the same mapped-only selection logic is applied to every occurrence noted
(around lines ~1302, ~1311-1315, ~1383-1394, ~1427-1431) so the survivor is
always visible.
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
1 participant
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.
This PR will add the Dissolve effect into the user_fx usermod (instead of FX.cpp)
This effect extends the features of the original Dissolve effect by:
A couple of these features were requested in #1044
Summary by CodeRabbit