Skip to content

Fixes issue in Dissolve when a palette has same color pixel as the current background color when Complete is enabled#5453

Open
BobLoeffler68 wants to merge 2 commits intowled:mainfrom
BobLoeffler68:dissolve-fixes
Open

Fixes issue in Dissolve when a palette has same color pixel as the current background color when Complete is enabled#5453
BobLoeffler68 wants to merge 2 commits intowled:mainfrom
BobLoeffler68:dissolve-fixes

Conversation

@BobLoeffler68
Copy link
Copy Markdown
Contributor

@BobLoeffler68 BobLoeffler68 commented Mar 26, 2026

When you select the Dissolve effect, select a palette, and enable the Complete checkbox, the effect should fill in the LEDs with the colors from the palette. Then, after all of them have been filled in ("completed"), it will wait the normal delay time and then dissolve all of them. But if the selected palette contains the same color as you have selected as the background color (SEGCOLOR(1)), then the Complete process will never be able to complete, so it looks like the effect is hung up or paused. Example palettes that cause this issue are the Fire and Ice Fire palettes if you have black selected for your background color since those palettes contain black, CRGB(0,0,0).

Summary by CodeRabbit

  • Bug Fixes
    • Resolved issues in the dissolve effect where transitions could produce invisible or incorrect pixels by preventing matching with the background color and adding an early safeguard for empty segments so allocation and spawning behave correctly.

…rrent background color when Complete mode is enabled
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 26, 2026

Walkthrough

Added an early bailout in dissolve() when segment length is <1 and adjusted dissolve-to-primary/palette spawning so the computed spawn color is stored in a temporary uint32_t c; if SEGMENT.check2 is enabled and c would equal the background marker (SEGCOLOR(1)), a low bit is flipped (c ^= 0x00000001) before writing to pixels[i].

Changes

Cohort / File(s) Summary
Dissolve effect logic
wled00/FX.cpp
Inserted early fallback when SEGLEN < 1 before allocation sizing. In the dissolve-to-primary/palette branch, store target color in uint32_t c and, when SEGMENT.check2 is set and c == SEGCOLOR(1), flip a low bit (c ^= 0x00000001) prior to assigning pixels[i]. Replaces prior direct assignment that only checked pixels[i] == SEGCOLOR(1).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • blazoncek
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately describes the specific bug being fixed: when Dissolve's Complete option is enabled and a palette contains colors matching the background color, those pixels are now properly handled to prevent the effect from appearing hung.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@wled00/FX.cpp`:
- Around line 706-708: The workaround that flips the low bit of c (c ^=
0x00000001) should only run in Complete mode; change the condition to only
modify c when it equals SEGCOLOR(1) AND the segment is in Complete mode (use
SEGMENT.check2 as the flag referenced in this logic). In other words, leave the
palette/fill selection using SEGCOLOR(0) and SEGMENT.color_from_palette(...)
unchanged, but replace the unconditional "if (c == SEGCOLOR(1)) c ^=
0x00000001;" with a guarded check "if (c == SEGCOLOR(1) && SEGMENT.check2) ..."
so identical fg/bg cases remain a true no-op when not in Complete mode.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: bf8b5910-7fe2-491c-a6ed-d48b24c8e078

📥 Commits

Reviewing files that changed from the base of the PR and between 78ecd38 and 6516c60.

📒 Files selected for processing (1)
  • wled00/FX.cpp

Comment thread wled00/FX.cpp
@DedeHai
Copy link
Copy Markdown
Collaborator

DedeHai commented Mar 27, 2026

good find, I overlooked this situation when making the update.

@BobLoeffler68
Copy link
Copy Markdown
Contributor Author

@coderabbitai Please add the Effect label to this PR.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check available labels in the repository
gh label list --repo wled/WLED --limit 50 | grep -i effect

# Add the "Effect" label to PR `#5453`
gh pr edit 5453 --repo wled/WLED --add-label "Effect"

Length of output: 269


@BobLoeffler68 The label "effect" does exist in the repository, but the command to add it to the PR is blocked due to workflow restriction settings. You'll need to add it manually via the GitHub UI — on the right-hand side of the PR page, click the Labels gear icon and select effect.


🧠 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: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:28.661Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

Learnt from: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.

Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FXparticleSystem.cpp:598-606
Timestamp: 2026-03-09T19:16:30.985Z
Learning: In WLED's FXparticleSystem.cpp, the particle system (both 1D and 2D) is intentionally RGB-only and does not process or output the white channel. The `fast_color_scaleAdd()` function discards the W byte by design. Even though `CRGBW` is used internally for color representation (e.g., `baseRGB`, palette lookups via `ColorFromPalette`), the W channel is never rendered. Do not flag the missing W channel handling in particle system rendering paths as a bug.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/FX_fcn.cpp:1380-1399
Timestamp: 2026-03-21T17:47:53.826Z
Learning: In WLED's stencil blend mode (case 16 in `WS2812FX::blendSegment()`, `wled00/FX_fcn.cpp`), the transparency key is intentionally hardcoded to black (`t ? t : b`), not the segment's background color (`colors[1]`). This is a deliberate design choice to avoid additional parameters and overhead. It is the user's responsibility to not use a non-black background in combination with stencil mode. Do not flag this as a bug in future reviews.

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.

@BobLoeffler68
Copy link
Copy Markdown
Contributor Author

@coderabbitai I do not have permissions to add labels, so I do not have the Labels gear icon on this PR page.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 25, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

No problem! Let me try to add the label directly for you.


🏁 Script executed:

#!/bin/bash
# Try adding the "effect" label to PR `#5453` via GitHub CLI
gh pr edit 5453 --repo wled/WLED --add-label "effect"
echo "Exit code: $?"

Length of output: 266


@BobLoeffler68 Unfortunately, the command to add labels is blocked by workflow restriction settings in the WLED repository, so I'm unable to add it programmatically either.

Since you don't have label permissions and I'm restricted as well, the best path is to ask a repository maintainer to add the "effect" label. I can see that @DedeHai has already commented on your PR — you could ask them (or another WLED maintainer) to apply the label for you.


🧠 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: softhack007
Repo: wled/WLED PR: 5443
File: wled00/FX_fcn.cpp:1277-1277
Timestamp: 2026-03-24T12:10:32.630Z
Learning: In WLED's `WS2812FX::service()` (wled00/FX_fcn.cpp), the old condition `|| (doShow && seg.mode == FX_MODE_STATIC)` was an **inclusion** guard — it caused FX_MODE_STATIC to render only when another segment had already set doShow=true. It did NOT skip or protect FX_MODE_STATIC from rendering. The PR `#5443` simplification removes this condition, meaning FX_MODE_STATIC now renders on every `timeToShow` tick uniformly. This is intentional and not a regression. Do not flag FX_MODE_STATIC special-casing as missing in future reviews of this function.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1304-1313
Timestamp: 2026-03-25T07:03:35.475Z
Learning: In WLED `mode_dissolveplus` (usermods/user_fx/user_fx.cpp), using `hw_random16(SEGLEN)` to select the survivor pixel index is correct and safe for this 1D-only effect. The 0xFFFF unmapped-entry concern from the physical bus mapping does not apply to 1D segments because virtual indices 0..SEGLEN-1 always map to valid physical LEDs without gaps. Do not flag this as a bug in future reviews of 1D effects.

Learnt from: softhack007
Repo: wled/WLED PR: 5457
File: usermods/zigbee_rgb_light/usermod_zigbee_rgb_light.h:0-0
Timestamp: 2026-03-31T17:31:01.023Z
Learning: In WLED PR `#5457` (zigbee_rgb_light usermod): The WLED_MAX_DIGITAL_CHANNELS=0 build flag used in the esp32c6_zigbee environment is a temporary workaround for rmt_tx_wait_all_done() timeout spam when the Zigbee/802.15.4 stack is active. The root cause is under investigation and is likely related to Zigbee light-sleep (CONFIG_PM_ENABLE) disrupting RMT's internal time base, or ISR latency due to cache-disable during flash ops — NOT the 802.15.4 radio "sharing" the RMT peripheral (they are separate hardware). Because a proper fix (rmt_enable()/rmt_disable() PM-lock wrapping, allow_pd=0, CONFIG_RMT_TX_ISR_CACHE_SAFE) may eliminate the need to disable digital channels entirely, do NOT add a compile-time `#error` guard requiring WLED_MAX_DIGITAL_CHANNELS=0; doing so would prematurely bake in a constraint that may be lifted once the investigation concludes.

Learnt from: BobLoeffler68
Repo: wled/WLED PR: 5440
File: usermods/user_fx/user_fx.cpp:1307-1327
Timestamp: 2026-03-24T07:59:28.661Z
Learning: In WLED's Dissolve Plus effect (usermods/user_fx/user_fx.cpp, mode_dissolveplus), when the "Last one" checkbox (check3/lastOneMode) is turned off while the effect is in PHASE_PAUSE_SURVIVOR, the phase is intentionally NOT immediately reset. The pause expires naturally via the normal delay timing (a few hundred ticks), after which the filling phase begins. PHASE_DISSOLVE_SURVIVOR is reset to PHASE_DISSOLVE and PHASE_FILL_SURVIVOR is reset to PHASE_FILL on toggle-off, but PHASE_PAUSE_SURVIVOR is left to expire on its own. Do not flag this as a bug in future reviews.

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: willmmiles
Repo: wled/WLED PR: 5462
File: wled00/json.cpp:1189-1198
Timestamp: 2026-03-30T15:32:08.847Z
Learning: In WLED's `respondModeData()` (wled00/json.cpp), the 256-character `lineBuffer` limit for effect descriptor strings (getModeData) is an intentional constraint that matches the same 256-char limit used ~6 other places in the codebase. It is not new to this PR and should not be flagged as a regression. Lifting it requires a future refactor (better type system or dynamic buffer).

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-04-10T11:41:46.737Z
Learning: In the WLED repository (wled/WLED), trailing-whitespace removals or other pure-whitespace "optimizations" must NOT be intermixed with real code/logic changes in the same PR. Reviewers prefer whitespace cleanup to be submitted in a dedicated, separate PR. Reason: intermixing makes diffs harder to read (obscures actual logic changes) and causes trouble when cherry-picking commits. Always flag PRs that mix whitespace-only edits with functional changes and ask the contributor to separate them.

Learnt from: softhack007
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-03-25T12:25:11.610Z
Learning: In WLED's `Segment::startTransition()` (wled00/FX_fcn.cpp:284), when `isInTransition()` is true and `_t->_oldSegment` already exists, the function silently returns without updating `_t->_start` or `_t->_bri`. This causes a bug where rapid successive on/off toggles during a non-FADE blending transition (e.g., fairy dust) leave the transition clock stale: by the time of the second re-trigger, elapsed time may already exceed `_dur`, so `updateTransitionProgress()` sets `_progress = 0xFFFF` immediately on the next service tick and `stopTransition()` fires — the blending effect never plays. The fix is to always reset `_t->_start = millis()`, `_t->_dur = dur`, and `_t->_bri = currentBri()` (current visible brightness) in the `isInTransition()` branch, regardless of whether `_oldSegment` exists.

Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/json.cpp:310-310
Timestamp: 2026-03-21T18:12:09.437Z
Learning: In WLED's `deserializeSegment()` (wled00/json.cpp), the blend mode field `seg.blendMode` is intentionally written without a post-read clamp (`getVal(elem["bm"], seg.blendMode)`). Out-of-range or unsupported blend mode values are handled safely in `WS2812FX::blendSegment()` (wled00/FX_fcn.cpp), which defaults to mode 0 for any unsupported value via a bounds check against the `BLENDMODES` constant. Do not flag the missing clamp in deserializeSegment as a bug in future reviews.

Learnt from: DedeHai
Repo: wled/WLED PR: 0
File: :0-0
Timestamp: 2026-01-13T21:23:35.514Z
Learning: In WLED, the global `paletteBlend` variable (wled.h:603) and the `WS2812FX::paletteBlend` member (FX.h:940) are duplicates without synchronization code. The global is loaded/saved in cfg.cpp and set via UI in set.cpp, but never copied to the strip member. This is the only such case in the codebase; other settings are either strip-only members (autoSegments, correctWB, cctFromRgb, isMatrix) or global-only (gammaCorrectCol/Bri/Val, blendingStyle).

Learnt from: DedeHai
Repo: wled/WLED PR: 4615
File: wled00/FXparticleSystem.cpp:598-606
Timestamp: 2026-03-09T19:16:30.985Z
Learning: In WLED's FXparticleSystem.cpp, the particle system (both 1D and 2D) is intentionally RGB-only and does not process or output the white channel. The `fast_color_scaleAdd()` function discards the W byte by design. Even though `CRGBW` is used internally for color representation (e.g., `baseRGB`, palette lookups via `ColorFromPalette`), the W channel is never rendered. Do not flag the missing W channel handling in particle system rendering paths as a bug.

Learnt from: DedeHai
Repo: wled/WLED PR: 4889
File: wled00/FX_fcn.cpp:1380-1399
Timestamp: 2026-03-21T17:47:53.826Z
Learning: In WLED's stencil blend mode (case 16 in `WS2812FX::blendSegment()`, `wled00/FX_fcn.cpp`), the transparency key is intentionally hardcoded to black (`t ? t : b`), not the segment's background color (`colors[1]`). This is a deliberate design choice to avoid additional parameters and overhead. It is the user's responsibility to not use a non-black background in combination with stencil mode. Do not flag this as a bug in future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants