Consolidate classic FX #4573
Consolidate classic FX #4573
Conversation
- Dissolve Random - Dual Larson Scanner - Sinelon Dual - Sinelon Rainbow - Ripple Rainbow - Solid Glitter - Dynamic Smooth
WalkthroughThis pull request updates the FX module by removing several legacy LED effect modes and their corresponding exported declarations. In the source file, multiple outdated effect functions are removed while a new Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 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