Consolidate classic FX #4573
Consolidate classic FX #4573
Conversation
- Dissolve Random - Dual Larson Scanner - Sinelon Dual - Sinelon Rainbow - Ripple Rainbow - Solid Glitter - Dynamic Smooth
WalkthroughConsolidates many FX variants into canonical base modes (adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX.h (1)
255-264: Consider grouping 2D effects together.While the 2D effects are well-named and properly sequenced, consider grouping all 2D effects together in the code for better organization and maintainability. This would make it easier to:
- Find and manage 2D effects
- Add new 2D effects in the future
- Maintain clear separation between 1D and 2D effects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
wled00/FX.cpp(39 hunks)wled00/FX.h(5 hunks)
🔇 Additional comments (10)
wled00/FX.h (2)
146-252: Well-documented effect mode consolidation!The commented-out effects with their replacement suggestions are clear and consistent. The migration paths align perfectly with the PR objectives.
183-225: Helpful alternative effect suggestions!Good practice to suggest modern particle system alternatives while keeping the original effects. This helps users discover newer, potentially more feature-rich implementations.
wled00/FX.cpp (8)
403-408: LGTM! Scan effect now supports dual mode.The addition of
check3parameter for dual mode successfully consolidates the functionality of the removedmode_dual_scan.
473-494: LGTM! Theater chase effect now supports animation and theatre modes.The addition of
check1for animation andcheck3for theatre mode successfully consolidates multiple theater chase effects into one.
510-537: LGTM! Running lights effect now supports moving, dual, and saw modes.The addition of
check1for moving,check2for dual, andcheck3for saw mode successfully consolidates multiple running effects into one.
631-648: LGTM! New sparkle effect with moving and overlay options.The new
mode_sparklefunction withcheck1for moving andcheck2for overlay provides a good base implementation for sparkle effects.
3173-3176: LGTM! Candle effect now supports multi-candle mode.The addition of
check3for multi-candle mode successfully consolidates the functionality of the removedmode_candle_multi.
3025-3031: LGTM! Sinelon effect now supports rainbow and dual modes.The addition of
check1for rainbow andcheck2for dual mode successfully consolidates the functionality of the removedmode_sinelon_rainbowandmode_sinelon_dual.
10046-10160: LGTM! Effect registrations updated to reflect consolidation.The following effects have been correctly commented out as they are now consolidated into other effects:
- FX_MODE_DUAL_SCAN (consolidated into Scan with check3)
- FX_MODE_THEATER_CHASE_RAINBOW (consolidated into Theater with check)
- FX_MODE_SAW (consolidated into Running with check3)
- FX_MODE_DISSOLVE_RANDOM (consolidated into Dissolve with check1)
- FX_MODE_RUNNING_COLOR
- FX_MODE_RUNNING_DUAL (consolidated into Running with check2)
- FX_MODE_DUAL_LARSON_SCANNER
- FX_MODE_METEOR_SMOOTH (merged with mode_meteor)
- FX_MODE_SOLID_GLITTER (consolidated into Glitter)
- FX_MODE_SINELON_DUAL (consolidated into Sinelon with check2)
- FX_MODE_SINELON_RAINBOW (consolidated into Sinelon with check1)
- FX_MODE_RIPPLE_RAINBOW (consolidated into Ripple with check1)
- FX_MODE_CANDLE_MULTI (consolidated into Candle with check3)
- FX_MODE_DYNAMIC_SMOOTH (consolidated into Dynamic with check3)
3085-3091: LGTM! Solid Glitter effect correctly removed.The
mode_solid_glitterfunction has been correctly commented out as its functionality is now part of themode_glittereffect.
|
I've move away from trying to track all changes in the changelog.md file, but for breaking changes like, it's probably our best place to record these changes, so it's then easy to find when we put out the release notes |
I usually did it once in a while by checking commit messages and writing down changes. |
| return running_base(true); | ||
| } | ||
| static const char _data_FX_MODE_SAW[] PROGMEM = "Saw@!,Width;!,!;!"; | ||
| static const char _data_FX_MODE_RUNNING_LIGHTS[] PROGMEM = "Running@!,Width,,,,Rainbow,Dual,Saw;L,!,R;!"; |
There was a problem hiding this comment.
The first checkbox doesn't produce any rainbows. In the code, the value is called moving, but it doesn't have anything to do with movement either. It's color_from_palette's wrap argument (which isn't a super self-explanatory name to a user either...). Both the variable and the UI string should be renamed.
There was a problem hiding this comment.
Moving in the instance of palette means if the palette itself is moving (i.e. hue of the palette is changing).
Some palettes wrap at the end and some don't. This "moving" parameter tells that palette should wrap as it is changing so that transition from ent to start is smooth.
Hmm. Not the best explanation, I know.
There was a problem hiding this comment.
the reason it is named 'rainbow' is legacy: some FX use(d) this exactly for that: moving the palette, creating a rainbow effect when using rainbow palette (i.e. colowheel). I agree that is now a bit nonsensical since many palettes are available, I am open to suggestions for renaming all "move palette" checkmarks into something more meaningful. in other places, such behaviour is also called "move" or "shift".
|
this has been pending for a while. are there any objections to removing consolidated FX? It is a breaking change to existing presets. IMHO 0.16 will bring so many new things (layered segments, PS, transitions) it seems like a good point in time to some de-cluttering of the effect list. |
|
I approve but I think other team members need to be ok with it. |
|
Is it worth implementing a compatibility parser for old presets? ie. a shim function that translates the old FX ids into new ones? |
|
Are you thinking of a separate application that will modify existing presets? |
|
That could have been and continue to be useful tool, but I believe it's much too complex to be part of the embedded wled software. Not to forget that it can never drop any code, it will just grow larger over time. The user would also need to specify from which version of wled to which other version to convert, because that isn't saved in the preset itelf, and simply judging from IDs that are currently unused doesn't work, because they get re-used for new effects over time (at least that is what happened in the past). Such a tool could do more than just replacing IDs. For example it could reset effect options that were unused and set arbitrarily in older versions, then later became used, to either their new defaults or to a value that closely matches the old behavior. But it might also be a pain to keep up to date. |
|
I suppose I'm uncertain from the description what we are trying to achieve. If we think we have effects that are redundant because you can achieve the same result by just changing the parameters of another effect, then yes we should consolidate Alternatively, if we are just looking to reduce the code size but maintain the same list of effect names, then we refactor so we maintain the effect but it's implementation is calling the common code with the right arguments set |
|
It is the former, some effects are in fact duplicates if we allow extended parameters (custom1, custom2, custom3, etc) to be used. I would vote for a separate app to modify existing presets if someone is willing to make one. |
|
I was thinking of a little bit of shim code at parse time - the list in the description above is short enough that I don't think it'd cost much. Ideally I'd put that kind of thing in for handling a 0.15->0.16 migration, and then remove in 0.17. |
|
@netmindz the goal is not to save on flash but to remove redundant code and to clean up the FX list that has now grown quite large. @willmmiles I would also opt for a built-in function rather than a conversion tool as this change might affect a lot of peoples setups (like christmas light setups) where an automatic conversion will save them a lot of hassle. It would mean we must not re-use the freed-up IDs in 0.16 at least. |
|
We could follow the same policy -- 0.16 deprecates the IDs, mapping them internally to the new implementation; 0.17 leaves them unallocated; 0.18 permits them to be re-used. That said, in the future I'd like to get rid of numeric IDs altogether. We already pay the code space for storing name strings, and not having to worry about ID contention makes it easier and safer to write and support FX usermods. |
|
It would be ideal if there was an external parser (eg a webapp) to detect deprecated or changed preset config |
If you can lend a hand, we'd be grateful. |
|
I would have preferred this being merged before adding new effects like #4682. |
|
since current path forward is to reserve the freed up ID's to be able to write parser for existing presets, it does not really matter when this is merged. |
|
However, PRs age really poorly, so if you want to merge something, sooner is much better than later ;) |
|
I think it's a good thing to have numeric ID for an effect, I'm not a fan of reusing an ID, you would never do that on a database. Moving to uint16 has implications for WLED sync and DMX |
the idea is also to shorten the FX list, yes. There is a working html that lets you select/unselect which FX are in the list but I have misplaced the discord link to it. @dosipod may know where to find it. Is moving to uint16 even an option with regards to backwards compatibility? |
done. This is progressing well I think, the removed FX are now fully compatible with old presets and the user gets informed, at least if they are using the UI. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wled00/FX.cpp (1)
526-559: Guard against SEGLEN==0 in Running Lights (modulo-by-zero risk).mode_running_lights computes palIdx using
(i+counter)%SEGLENwithout a length guard. Other effects useif (SEGLEN <= 1) return mode_static();to avoid division-by-zero and edge handling. Add the same guard here.Apply this diff:
static uint16_t mode_running_lights() { - const bool dual = SEGMENT.check2; + if (SEGLEN <= 1) return mode_static(); + const bool dual = SEGMENT.check2; unsigned x_scale = SEGMENT.intensity >> 2; uint32_t counter = (strip.now * SEGMENT.speed) >> 9; const bool moving = SEGMENT.check1; @@ - unsigned palIdx = moving ? (i+counter)%SEGLEN : i; + unsigned palIdx = moving ? ((i + (counter % SEGLEN)) % SEGLEN) : i;
♻️ Duplicate comments (2)
wled00/FX.h (1)
172-172: Fix duplicate word typo: "with with" should be "with".Multiple comments still contain the duplicate word "with with" instead of "with". This was flagged in previous reviews but remains unfixed.
Apply this diff to fix the typos:
-//#define FX_MODE_SAW 16 // removed in 0.16, use Running with with check 3 +//#define FX_MODE_SAW 16 // removed in 0.16, use Running with check 3-//#define FX_MODE_DISSOLVE_RANDOM 19 // removed in 0.16, use Dissolve with with check 1 +//#define FX_MODE_DISSOLVE_RANDOM 19 // removed in 0.16, use Dissolve with check 1-//#define FX_MODE_RUNNING_DUAL 52 // removed in 0.16, use Running with with check 2 +//#define FX_MODE_RUNNING_DUAL 52 // removed in 0.16, use Running with check 2-//#define FX_MODE_METEOR_SMOOTH 77 // removed in 0.16, use Meteor with with check 3 +//#define FX_MODE_METEOR_SMOOTH 77 // removed in 0.16, use Meteor with check 3Also applies to: 175-175, 208-208, 233-233
wled00/FX.cpp (1)
520-521: UI flag naming consistency (“Rainbow” vs. “Move palette”).Across effects, check1 often toggles palette motion (legacy “rainbow”). Prior feedback asked for clearer wording (e.g., “Move palette” / “Wrap palette”). Consider aligning UI strings where check1 means palette motion to reduce confusion.
See previous discussion by TripleWhy/blazoncek about renaming “rainbow/moving” to a clearer term (move/shift palette).
Also applies to: 212-212, 520-520, 559-559
🧹 Nitpick comments (2)
wled00/FX.h (1)
274-274: Inconsistent spacing: "check1" should be "check 1".For consistency with other replacement guidance comments (lines 167, 170, 180, 182, 193, etc.), use a space between "check" and the number.
Apply this diff:
-//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Dynamic with check1 +//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Dynamic with check 1wled00/FX.cpp (1)
10681-10683: Duplicate addEffect() entries for POPCORN and DRIP.POP/DRIP are added twice (Lines 10676–10677 and 10681–10682). The second calls no-op due to reservation checks but add noise. Remove the duplicates.
- addEffect(FX_MODE_POPCORN, &mode_popcorn, _data_FX_MODE_POPCORN); - addEffect(FX_MODE_DRIP, &mode_drip, _data_FX_MODE_DRIP); + // duplicates removed (already added above)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX.cpp(60 hunks)wled00/FX.h(6 hunks)wled00/FX_fcn.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- wled00/FX_fcn.cpp
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/FX.h
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.hwled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX.hwled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (4)
wled00/wled_math.cpp (2)
sin8_t(82-86)sin8_t(82-82)wled00/colors.cpp (2)
hsv2rgb(297-326)hsv2rgb(297-297)wled00/fcn_declare.h (6)
hw_random8(435-435)hw_random8(436-436)hw_random8(437-437)hw_random16(432-432)hw_random16(433-433)hw_random16(434-434)wled00/util.cpp (2)
beatsin8_t(413-421)beatsin8_t(413-413)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
🔇 Additional comments (10)
wled00/FX.h (1)
277-286: LGTM! 2D effect definitions properly activated.The activation of previously reserved 2D effect IDs (FX_MODE_2DSPACESHIPS through FX_MODE_2DWAVINGCELL) is appropriate and aligns with the PR's consolidation objectives. The definitions follow the established pattern and numbering sequence.
wled00/FX.cpp (9)
182-213: Blink consolidation looks good.On/off timing and new palette “cycle” flag are correct; one-frame-on safeguard is retained. UI metadata matches behavior.
419-447: Scan: dual variant integration LGTM.Back-and-forth math with size and mirrored drawing is correct; bounds stay within [0, SEGLEN). Background uses color2 and dual uses palette index 2 when present.
598-642: Dissolve: random-to-primary/palette flag integration LGTM.Random color toggle via check1 and to/from secondary handling are correct; final buffer writeback fixes #4401 intact.
647-665: New Sparkle: overlay and moving background behaviors are coherent.
- Overlay off: background via palette (option 1).
- Overlay on: leave existing frame; flash one pixel.
- “Move” uses time-based palette offset.
Please confirm UI string “Overlay” matches existing conventions used in Glitter family (consistency helps end users).
672-718: Flash/Hyper Sparkle: background animation refactor LGTM.Adopts the same “moving background” semantics as Sparkle; frame pacing preserved.
1129-1147: Larson Scanner: palette motion and dual/bi-delay flags integrated correctly.
- Moving background (check3) applied per-index and time.
- Dual mirroring via check1; delays via custom sliders remain.
1491-1508: Tri-color Chase ordering fix LGTM.Setting color from 3 slots with clear precedence is correct; keeps consistent wrap.
2380-2454: Ripple: background choice + blur integration is sound.
- check1 toggles rainbow background and hue progression.
- custom1 controls blur; 1D/2D branches preserve previous behavior.
6187-6256: Candle: multi variant consolidation LGTM.
- Multi-candle per-LED state allocated only when needed (check3 and SEGLEN>1).
- Single/ multi paths share the same target/self-damping logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX.h (1)
274-274: Inconsistent spacing in check flag reference.The comment uses "check1" without a space, while other removal comments consistently use "check N" format (e.g., "check 1", "check 2", "check 3").
Apply this diff for consistency:
-//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Dynamic with check1 +//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Dynamic with check 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.h(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/FX.h
🧠 Learnings (3)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.h
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (esp32dev)
🔇 Additional comments (3)
wled00/FX.h (3)
167-274: LGTM! Clear consolidation with preserved IDs.The legacy effect removals follow a consistent pattern with clear migration guidance. Keeping the commented-out definitions with their IDs prevents immediate reuse, which aligns with the PR's intent to ease migration and avoid downgrade issues.
277-286: LGTM! 2D effects properly defined.The newly enabled 2D effects (FX_MODE_2DSPACESHIPS through FX_MODE_2DWAVINGCELL) fill the ID range 118-127 and expand the public API surface. The alternative PS effect comments provide helpful migration guidance where overlaps exist.
381-381: LGTM! MODE_COUNT correctly set.MODE_COUNT = 218 correctly represents the highest FX_MODE_ ID + 1 (FX_MODE_PARTICLEGALAXY = 217), accounting for gaps from removed effects. This aligns with WLED's convention for array bounds and range checking.
Based on learnings.
|
(Posted this on discord but also here for the record): maybe you have already a solution but could this work:
|
|
Do you plan to block the old FX IDs from being re-used, forever?
About versioning:
|
The idea is to block them in 0.16 and hopefully have a better solution in 0.17 and remove the mapping code again. The main issue with moving away from 8bit FX numbers is backwards compatibility with external control software / sync / HA etc. there are some ideas we discussed but nothing is decided. |
|
Well then, see above: make sure there is some way for 0.17 to tell if the preset came from 0.16, or something older |
|
@TripleWhy , I might have created a mess by having both discord and github for discussion 🙈 But on your remark: does it block fx_ids to be reused: Answer: no, but there can be only one fx_id with the classic bit set to yes. Each category (fc_id) could have the same fx_id as long as for these the classic bit is false The only requirement is you enforce that the combination <fc_id, classic bit,fx_id> is unique for an effect now and forever |
|
As FX ID change is one time issue for existing presets (now or in the future) I would not bother with any workaround or auxiliary tools. Once FX is removed or replaced it is up to the (annoyed) user to fix it. However, there have to be a clear and explicit instructions for each and every FX in question. There are at least 18 FX that can be removed (and replaced by other effects that may need modification) according to my count. One thing that hurts me still is the fact that there was a time where I've reordered (and grouped) effects when I ported Soundreactive to upstream. Unfortunately I was overruled and new effects kept their IDs from Soundreactive and no reordering was done. Changing FX ID to 16 bit (or some other form) is a non-trivial task that will break backwards compatibility. Consider sync with devices from 0.10 onwards, which currently works as designed with all versions. My 2c. |
|
Add Wipe/Sweep(Random) to the list. |
it is easiest to just have an auxiliary tool in 0.16 (and 0.16 only) to do it automatically as writing up clear instructions will almost take the same amount of time than writing a tool. I agree on all other points.
syncing will only work with non-removed FX IDs, so this PR is a breaking change in that regard unless we keep the IDs reserved forever. |
Sync will still work and original (non replaced/removed) effects will still play. Replaced effects will play incorrect effec, but will still play. You can also add Chase effects to the list, albeit with a bit of twist. Some used SEGCOLOR(1) for background and some SEGCOLOR(0). Code available in my fork. |
- also simplified get_random_wheel_index()
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
wled00/FX.h (6)
172-172: Fix typo: "with with" should be "with".The comment contains the duplicate word "with with" instead of "with".
Apply this diff to fix the typo:
-//#define FX_MODE_SAW 16 // removed in 0.16, use Running with with check 3 +//#define FX_MODE_SAW 16 // removed in 0.16, use Running with check 3
175-175: Fix typo: "with with" should be "with".The comment contains the duplicate word "with with" instead of "with".
Apply this diff to fix the typo:
-//#define FX_MODE_DISSOLVE_RANDOM 19 // removed in 0.16, use Dissolve with with check 1 +//#define FX_MODE_DISSOLVE_RANDOM 19 // removed in 0.16, use Dissolve with check 1
208-208: Fix typo: "with with" should be "with".The comment contains the duplicate word "with with" instead of "with".
Apply this diff to fix the typo:
-//#define FX_MODE_RUNNING_DUAL 52 // removed in 0.16, use Running with with check 2 +//#define FX_MODE_RUNNING_DUAL 52 // removed in 0.16, use Running with check 2
233-233: Fix typo: "with with" should be "with".The comment contains the duplicate word "with with" instead of "with".
Apply this diff to fix the typo:
-//#define FX_MODE_METEOR_SMOOTH 77 // removed in 0.16, use Meteor with with check 3 +//#define FX_MODE_METEOR_SMOOTH 77 // removed in 0.16, use Meteor with check 3
274-274: Fix typo: "Danamic" should be "Dynamic".The comment contains a misspelling of the effect name.
Apply this diff:
-//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Danamic with check1 +//#define FX_MODE_DYNAMIC_SMOOTH 117 // removed in 0.16, use Dynamic with check1
244-244: Fix typo: "Sparker" should be "Sparkler".The comment references "PS Sparker" but the actual effect name is "PS Sparkler" (as seen in FX_MODE_PSSPARKLER at line 369).
Apply this diff:
-#define FX_MODE_GLITTER 87 // optional in 0.16, use PS Sparker with s=255, i=0, c1&c2=0, c3 = speed, add to background overlay +#define FX_MODE_GLITTER 87 // optional in 0.16, use PS Sparkler with s=255, i=0, c1&c2=0, c3 = speed, add to background overlay
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
10618-10620: Glitter only added when particle systems are disabledCurrently, FX_MODE_GLITTER is added only inside WLED_PS_DONT_REPLACE_FX. With PS enabled, Glitter disappears, which may contradict the PR intent (“Solid Glitter → Glitter (alternative: PS Sparkler)”). If Glitter should remain available regardless of PS, add it outside the guard.
If keeping Glitter in both builds is desired, add this near other 1D non-audio effects (next to Sparkle):
+ addEffect(FX_MODE_GLITTER, &mode_glitter, _data_FX_MODE_GLITTER);Please confirm intended availability of Glitter in PS builds.
3041-3048: Leftover TODO in Glitter (“Overlay” option naming/need)There’s a TODO to verify/remove/rename “Overlay”. Decide and clean up before release to avoid confusing UX strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wled00/FX.cpp(57 hunks)wled00/FX.h(6 hunks)wled00/FX_fcn.cpp(1 hunks)wled00/util.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/util.cppwled00/FX_fcn.cppwled00/FX.cpp
wled00/**/!(html_*)*.h
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for non-generated C++ header files (.h)
Files:
wled00/FX.h
🧠 Learnings (5)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.hwled00/FX.cpp
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX.hwled00/FX_fcn.cppwled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (2)
wled00/util.cpp (1)
wled00/fcn_declare.h (3)
hw_random8(435-435)hw_random8(436-436)hw_random8(437-437)
wled00/FX.cpp (4)
wled00/wled_math.cpp (2)
sin8_t(82-86)sin8_t(82-82)wled00/colors.cpp (2)
hsv2rgb(297-326)hsv2rgb(297-297)wled00/fcn_declare.h (6)
hw_random8(435-435)hw_random8(436-436)hw_random8(437-437)hw_random16(432-432)hw_random16(433-433)hw_random16(434-434)wled00/util.cpp (2)
beatsin8_t(413-421)beatsin8_t(413-413)
🔇 Additional comments (3)
wled00/FX_fcn.cpp (1)
2049-2167: LGTM! Legacy effect remapping is well-structured.The
remapLegacyEffectfunction correctly maps deprecated effect IDs to their current equivalents with appropriate flag settings. The logic aligns with the removal comments in FX.h, and the warning propagation ensures users are notified of deprecated usage.The indentation follows the 2-space standard as specified in the coding guidelines.
wled00/FX.h (2)
160-274: Verify MODE_COUNT after effect removals.Multiple legacy effect IDs have been commented out with removal notes. Since MODE_COUNT (line 381) represents the highest FX_MODE ID + 1, it should still be correct at 218 as the highest active ID (FX_MODE_PARTICLEGALAXY = 217) hasn't changed.
However, ensure that the gap created by commented-out IDs is intentional and won't cause issues with:
- Effect ID arrays or lookups
- Preset compatibility
- External integrations expecting sequential IDs
Based on learnings: MODE_COUNT represents the highest FX_MODE ID + 1, not the total count of active effects, so gaps from removed effects are expected.
277-286: LGTM! 2D effects properly defined.The previously placeholder FX_MODE_118 through FX_MODE_127 have been properly defined as active 2D effects (FX_MODE_2DSPACESHIPS through FX_MODE_2DWAVINGCELL). This fills the gap in effect IDs and makes these effects available.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wled00/FX.cpp (2)
2342-2416: Potential OOB writes in Ripple when drawing wave flanksInside the 1D branch, left/right indices can go negative or exceed SEGLEN (“TODO” noted). Add bounds checks before setPixelColor to prevent out-of-range access on small segments/high propagation.
Apply this diff in the inner loop:
- SEGMENT.setPixelColor(left + v, color_blend(SEGMENT.getPixelColor(left + v), col, mag)); // TODO - SEGMENT.setPixelColor(right - v, color_blend(SEGMENT.getPixelColor(right - v), col, mag)); // TODO + int posL = left + v; + int posR = right - v; + if (posL >= 0 && posL < (int)SEGLEN) + SEGMENT.setPixelColor(posL, color_blend(SEGMENT.getPixelColor(posL), col, mag)); + if (posR >= 0 && posR < (int)SEGLEN) + SEGMENT.setPixelColor(posR, color_blend(SEGMENT.getPixelColor(posR), col, mag));
9402-9405: Duplicate init call in PS Fireworks 1DinitParticleSystem1D() is called twice inside each other. This is redundant and can mask allocation issues.
Apply this diff:
- if (!initParticleSystem1D(PartSys, 4, 150, 4, true)) // init advanced particle system - if (!initParticleSystem1D(PartSys, 4, 150, 4, true)) // init advanced particle system - return mode_static(); // allocation failed or is single pixel + if (!initParticleSystem1D(PartSys, 4, 150, 4, true)) // init advanced particle system + return mode_static(); // allocation failed or is single pixel
♻️ Duplicate comments (2)
wled00/FX.cpp (2)
10614-10618: SPARKLE registration under PS guard only — duplicates resolvedPreviously flagged duplicate addEffect() for SPARKLE is gone from the unguarded list and only present here. Good cleanup.
10626-10628: POPCORN/DRIP duplicate registrations are no longer presentEach is added once. This addresses the prior duplication concern.
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
488-521: Running Lights: options work; consider clearer UI labelcheck1 toggles palette motion (“Rainbow”), check2=Dual, check3=Saw. Implementation is fine. Suggest renaming the metadata label “Rainbow” to “Animate palette” (or “Move palette”) for clarity across effects, per prior discussion.
3059-3067: Commented-out Solid Glitter codeThis block is fully commented. Consider removing dead code to reduce noise and avoid confusion now that Glitter covers the use case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(55 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
wled00/**/*.cpp
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use 2-space indentation for C++ source files (.cpp)
Files:
wled00/FX.cpp
🧠 Learnings (5)
📓 Common learnings
Learnt from: DedeHai
PR: wled/WLED#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.
📚 Learning: 2025-08-26T11:51:21.817Z
Learnt from: DedeHai
PR: wled/WLED#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.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-09-02T19:42:57.475Z
Learnt from: freakintoddles2
PR: wled/WLED#4904
File: wled00/FX.cpp:3915-3930
Timestamp: 2025-09-02T19:42:57.475Z
Learning: WLED wled00/FX.cpp::mode_percent — For time-based options (% of Minute/Hour/Day), behavior is clock-like: at rollover (elapsed==0) intentionally show SEGLEN lit for one tick to represent completion, then move to 1 LED at the next second. This is by design (not a progress bar) and should not be “fixed”.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-15T09:59:52.720Z
Learnt from: netmindz
PR: wled/WLED#4728
File: wled00/FX.h:378-378
Timestamp: 2025-06-15T09:59:52.720Z
Learning: In WLED's FX.h, MODE_COUNT represents the highest FX_MODE_ ID + 1, not the total count of FX_MODE_ definitions. This is because some effects may have been removed but their IDs are not reused, creating gaps in the numbering sequence. MODE_COUNT is used for array bounds and range checking.
Applied to files:
wled00/FX.cpp
📚 Learning: 2025-06-07T15:58:42.579Z
Learnt from: willmmiles
PR: wled/WLED#4712
File: wled00/FX_fcn.cpp:992-993
Timestamp: 2025-06-07T15:58:42.579Z
Learning: In WLED segments, the backing pixel buffer is always allocated to the full physical segment size, but effect rendering only uses a subset when mirror or grouping are enabled. For 2D segments, this subset is vWidth() * vHeight(), and for 1D segments it's vLength(). Color processing operations like fade_out should iterate over this rendering subset, not the full allocated buffer.
Applied to files:
wled00/FX.cpp
🧬 Code graph analysis (1)
wled00/FX.cpp (4)
wled00/wled_math.cpp (2)
sin8_t(82-86)sin8_t(82-82)wled00/colors.cpp (2)
hsv2rgb(297-326)hsv2rgb(297-297)wled00/fcn_declare.h (6)
hw_random8(435-435)hw_random8(436-436)hw_random8(437-437)hw_random16(432-432)hw_random16(433-433)hw_random16(434-434)wled00/util.cpp (2)
beatsin8_t(413-421)beatsin8_t(413-413)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: wled_build / Build Environments (usermods)
🔇 Additional comments (16)
wled00/FX.cpp (16)
182-212: Consolidated Blink variants look goodUses check1 for palette motion vs gradient and preserves initial-on frame. Clean and consistent.
218-266: Wipe/Sweep/random consolidation is solidcheck1=Random, check2=Sweep with clear metadata. Progress and color-state handling LGTM.
381-409: Scan with Dual option LGTMcheck3 enables dual scan; fill/paint order and size calc are correct. Good fallback for short segments.
451-481: Theater/Chase unification‘theater’ vs ‘chase’ selection via check3 works, animation via check1 is clear. No issues.
562-604: Dissolve Random mapped via check1Logic is tidy; random color choice vs primary/palette controlled via check1. Good memory allocation and safe fallback.
609-627: Sparkle: Move/Overlay flags LGTMcheck1 moves palette; check2 overlays. Seeding/random-led selection is correct. Metadata matches behavior.
633-652: Sparkle Dark variant consolidationBackground handling and flash cadence updates look correct. No issues.
659-680: Sparkle+ variantHigher burst intensity with proper timing. No concerns.
686-714: Strobe Mega uses palette motion flagcheck1 “Move” usage is consistent across effects. Timing and count logic LGTM.
1091-1109: Larson Scanner: Move/Bi-delay optionsNew move (check3), dual (check1), and bi-delay (check2) options integrate well; rendering order is sound.
1453-1471: Tri-color Chase tidyColor order logic is clearer; no issues found.
1481-1512: ICU background fill and spacingBackground fill moved to SEGCOLOR(1); eye spacing via intensity is correct. Behavior matches description.
1751-1777: Lightning cleanupBackground fill made explicit; blink and delay logic reads clean. All good.
3148-3218: Candle: Multi mode integrationData allocation guarded; per-pixel state and single-candle fallback are correct. Good use of intensity/ speed shaping; no issues.
959-965: Chase Flash: Animated background flagcheck1 “Animate BG” behavior is clear and consistent; timing remains unchanged. LGTM.
9582-9582: PS Sparkler metadataNew controls (Glow/Saturation/Blur/Sparklers/Slide/Bounce/Large) align with implementation. Looks good.
| } else | ||
| { | ||
| SEGMENT.setPixelColor(index, back? col0 : col1); | ||
| if (i == ledIndex) SEGMENT.setPixelColor(index, color_blend(back? col0 : col1, back? col1 : col0, uint8_t(rem))); |
There was a problem hiding this comment.
Why did you remove smooth transition?
There was a problem hiding this comment.
because it acts on one single pixel for about 0.2s and I could not see any transition from that, even on lowest speed. So either this effect was slower initially or I did not test it right.
|
This is my (current) list of effect IDs that can be freed (some already are), by slightly modifying other existing effects. #define FX_MODE_COLOR_WIPE_RANDOM 4 // candidate for removal (was Wipe; use Wipe with check 3)
#define FX_MODE_COLOR_SWEEP 6 // candidate for removal (was Sweep; use Wipe with check 1)
#define FX_MODE_DUAL_SCAN 11 // candidate for removal (use Scan with check 1)
#define FX_MODE_THEATER_CHASE_RAINBOW 14 // candidate for removal (was Theater Rainbow; use Theater with check 3)
#define FX_MODE_SAW 16 // candidate for removal (was Saw; use Running Lights with check 2)
#define FX_MODE_DISSOLVE_RANDOM 19 // candidate for removal (was Dissolve random; use Dissolve with with check 3)
#define FX_MODE_STROBE 23 // candidate for removal (was Strobe; use Blink with with check 2)
#define FX_MODE_STROBE_RAINBOW 24 // candidate for removal (was Strobe Rainbow; use Blink with with check 1 & check 2)
#define FX_MODE_BLINK_RAINBOW 26 // candidate for removal (was Blink Rainbow; use Blink with with check 1)
#define FX_MODE_CHASE_RANDOM 29 // candidate for removal (was Chase Random; use Chase with check 3)
#define FX_MODE_CHASE_RAINBOW 30 // candidate for removal (was Chase Rainbow; use Chase with check 1 & check 3)
#define FX_MODE_CHASE_RAINBOW_WHITE 33 // candidate for removal (was Chase Rainbow White; use Chase with check 1 & check 2 & check 3)
#define FX_MODE_COLOR_SWEEP_RANDOM 36 // candidate for removal (was Sweep Random; use Wipe with check 1 & check 3)
#define FX_MODE_RUNNING_COLOR 37 // candidate for removal (use Theater)
#define FX_MODE_LOADING 47 // candidate for removal (use Gradient with check 1)
#define FX_MODE_RUNNING_DUAL 52 // candidate for removal (use Running)
#define FX_MODE_HALLOWEEN 53 // removed in 0.14!
#define FX_MODE_DUAL_LARSON_SCANNER 60 // candidate for removal (use Scanner with with check 1)
#define FX_MODE_METEOR_SMOOTH 77 // candidate for removal (use Meteor with check 1)
#define FX_MODE_TWINKLECAT 81 // candidate for removal (use Twinklefox with check 3)
#define FX_MODE_SPOTS_FADE 86 // candidate for removal (use Spots with check 1)
#define FX_MODE_SINELON_DUAL 93 // candidate for removal (use sinelon)
#define FX_MODE_SINELON_RAINBOW 94 // candidate for removal (use sinelon)
#define FX_MODE_RIPPLE_RAINBOW 99 // candidate for removal (use ripple)
#define FX_MODE_CANDLE_MULTI 102 // candidate for removal (use candle with multi select)
#define FX_MODE_SOLID_GLITTER 103 // candidate for removal (use glitter)
#define FX_MODE_PHASEDNOISE 109 // candidate for removal (use Phased with check 1)
#define FX_MODE_DYNAMIC_SMOOTH 117 // candidate for removal (check3 in dynamic) |
|
Unnecessary breaking changes impacting everyone's presets that feature any affected effect is not acceptable if this is just about an artificial restriction to always keep effect ID as single byte |
|
Duplication of the effect code, while maintaining the effect ID and expected behaviour is good software design and therefore perfectly ok |
Playing the wrong effect is not working! |
|
I think we can rework the implementation a little here so that we don't cause breaking changes between versions. When calling a legacy effect, we shouldn't change our effectID, but rather delegate the implementation to the new function, that way we still sync the original ID, therefore keeping compatibility @DedeHai If/how we can do that while still having the nice feature of the warning, without too much boilerplate code checking for first frame, might take some thought |
I don't understand what you mean with this comments sorry. Surely weather or not IDs were kept consistent between forks or assigned a new ID does affect the number of IDs used? The current system doesn't apply any significance to the ID as we list effects alphabetically, not by ID so there is no grouping based on ID |
|
From what I have gathered, everyone was on the same page in the end, including yourself. So is there any reason why you closed the PR? |
|
Sorry, didn't mean to close, miss tapped |
8 participants
This PR removes some of the classic FX "aliases" and moves them into the main FX to be used with checkmarks.
cherry picked from @blazoncek's branch, updated, cleaned and tested.
Removed effects:
Summary by CodeRabbit
New Features
Refactor
Chores