Initial commit. Added Ant and PacMan effects #4536
Conversation
|
Thanks. |
|
I like the general idea of this effect and the animation is well done.
maybe I have some more points after testing :) |
How do I revert changes to those two files in Github Desktop? |
Thanks!
Yes, I knew changing the FRAMETIME would be ugly because it slows down the FPS rate, but I couldn't figure out how to do it any other way. Do you have a suggestion of how to do it after looking at the code?
Ah, good idea with the user-selectable yellow dots. The speed is selectable with that first slider, but that is doing it by changing the FRAMETIME. I need a suggestion for a better way to do it. :-( Yes, white dot spacing would be good to make it more obvious that PacMan is eating the dots.
That would be awesome! I am planning on changing the direction that they start moving. Right now they move from LED 0 to SEGLEN, so I will add an option checkbox to start them moving from SEGLEN to 0. Thanks! |
you need to render every frame, meaning rendering the same frame again if time has not advanced enough between calls.
that can be done by selecting "reverse" on the segment as well |
Rendering every frame is still a new concept to me. :-) Thanks.
Oh, that's right! I won't worry about that one and will check that off of my list. :-) |
|
I tested the two effects, I found this:
|
wled00/FX.cpp
Outdated
|
|
||
| antSize = map(SEGMENT.custom1, 0, 255, 1, 5); // the size/length of each ant is user selectable (1 to 5 pixels) with a slider | ||
|
|
||
| uint8_t bgColorIdx = map(SEGMENT.custom2, 0, 255, 0, 5); // background color based on the what the user selects from the Background Color slider |
There was a problem hiding this comment.
you could use one of the WLED colors, COLOR1 I think is usually used for background, that way any color can be selected by the user.
There was a problem hiding this comment.
True, but the way I did it allows the user to have 3 different colored ants instead of only 2. I guess we can see what the users want after they play with it a little.
There was a problem hiding this comment.
I used a palette and got a lot more than 3 ant colors
There was a problem hiding this comment.
Let me chime in.
You can use color_from_palette() to select color from selected palette or predefined color slot.
uint32_t fgColor = SEGMENT.color_from_palette(i, (true|false), (true|false), 0, [bri]);
uint32_t bgColor = SEGMENT.color_from_palette(i, (true|false), (true|false), 1, [bri]);If user selects "default" palette, color slot will be used otherwise palette entry will be used.
You can also use, in a similar fashion, SEGMENT.color_wheel(index).
There was a problem hiding this comment.
Let me chime in. You can use
color_from_palette()to select color from selected palette or predefined color slot.uint32_t fgColor = SEGMENT.color_from_palette(i, (true|false), (true|false), 0, [bri]); uint32_t bgColor = SEGMENT.color_from_palette(i, (true|false), (true|false), 1, [bri]);If user selects "default" palette, color slot will be used otherwise palette entry will be used.
You can also use, in a similar fashion,
SEGMENT.color_wheel(index).
Can the user select the 3 specific colors that they want? Or will it be random colors? A random option would be nice, but I also want them to be able to select 3 specific colors of their choice.
There was a problem hiding this comment.
you can do all that: there is the "auto generated" palettes from 2 or 3 colors, so if you choose the correct "hue" index it will use the three selected colors. Or make it a checkmark to use colors instead of palette or use color_wheel() which I think does either use colors or palette.
you can use one of the checkmarks as well to do "random colors" and then just assign a random hue index instead of fixed ones for more variation.
Just look at those functions (color from palette and colorwheel) and experiment.
There was a problem hiding this comment.
Color slots are just slots for stored color values. How you use them is up to you.
I have merely provided information how you can use best of both worlds: color slots and palettes.
The magic word is "Default palette". Default palette behaves a bit differently than other palettes as it is not necessarily a palette (as shown above). It can be color slot 0, 1 or 2 depending on how you choose to use it.
There was a problem hiding this comment.
you can do all that: there is the "auto generated" palettes from 2 or 3 colors, so if you choose the correct "hue" index it will use the three selected colors. Or make it a checkmark to use colors instead of palette or use color_wheel() which I think does either use colors or palette. you can use one of the checkmarks as well to do "random colors" and then just assign a random hue index instead of fixed ones for more variation. Just look at those functions (color from palette and colorwheel) and experiment.
Ok, thx.
There was a problem hiding this comment.
Color slots are just slots for stored color values. How you use them is up to you. I have merely provided information how you can use best of both worlds: color slots and palettes.
The magic word is "Default palette". Default palette behaves a bit differently than other palettes as it is not necessarily a palette (as shown above). It can be color slot 0, 1 or 2 depending on how you choose to use it.
Ok, thx. Lots to think about. :-)
| ants[i].height = thisHeight; | ||
| } | ||
| // check if reached past the end of the strip. If so, wrap around. | ||
| if (thisHeight >= 1.0f && ants[i].velocity > 0.0f) { |
There was a problem hiding this comment.
in general, this effect may also be done with my particle system, the code would be much simpler as the system will handle bounce between ants and wrap around, just need to handle the confused ants "manually". you can take a look at the "PS pinball" effect to see if you like that look.
There was a problem hiding this comment.
Ok, thanks for the suggestion. I'll look at that effect and code.
Thanks for testing!
It should work if you set the Transition to 0.
An ant can "merge" with another ant of the same color, so it looks like it disappears, but it's just using the same LEDs as another ant. That is probably what you are seeing. I haven't seen any actually disappear or shrink. |
Do not assume such approach. The effect has to handle everything. |
Good point. Is there a way to set the transition to 0 in the code? |
no, you need to handle transitions in the FX. |
There is, but you don't do that from effect. You have to handle transitions (if needed at all). |
I don't use Github Desktop app but here are some instructions for git: Basically you check-out a file from previous commit. And then commit it again. NEVER FORCE PUSH YOUR CHANGES! |
I think I did the reverts correctly, but I can't tell from the messages that came back. WLED-AC is my working directory. C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git status C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- package-lock.json C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- platformio.ini C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git status C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git commit -m "Reverted 2 files" Does it look like I did it correctly? I also tried the full file path of the two files when the checkouts didn't give me any feedback: C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- C:\Users\boblo\Documents\GitHub\repos\WLED-AC\package-lock.json C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- C:\Users\boblo\Documents\GitHub\repos\WLED-AC\platformio.ini I have not done a push yet after running the commands above. |
|
|
So these should've worked, right? C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- C:\Users\boblo\Documents\GitHub\repos\WLED-AC\package-lock.json C:\Users\boblo\Documents\GitHub\repos\WLED-AC>git checkout 34d0f17 -- C:\Users\boblo\Documents\GitHub\repos\WLED-AC\platformio.ini I saw several websites that said that you can use just the filename of the file if you were running the command from the working directory. But I'll use the full file path from now on. Thanks. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request updates the project configuration and LED effect implementations. In platformio.ini, the default environment is changed to esp32dev and the upload speed increased. In the WLED effect files, two new effect modes—"ants" and "pacman"—are added, the "rolling_balls" effect is modified, and "mode_racers" is removed. Additionally, the header file is updated to define new effect mode constants and adjust the total mode count. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Controller as WLED Controller
participant Data as Effect Data Setup
participant LED as LED Strip
User->>Controller: Select LED effect mode
Controller->>Data: Load effect data (including new modes)
alt "Ants" effect selected
Controller->>Controller: Execute mode_ants()
else "Pacman" effect selected
Controller->>Controller: Execute mode_pacman()
else Other effect selected
Controller->>Controller: Execute existing effect
end
Controller->>LED: Update LED display
🪧 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 (6)
wled00/FX.cpp (6)
3023-3024: Consider using a more descriptive struct nameThe struct name
RollingBallis being used for both rolling balls and ants effects. Consider renaming it to something more generic likeMovingObjector creating separate structs for better code clarity.-typedef struct RollingBall { // used for rolling_balls and Ants modes +typedef struct MovingObject { // used for rolling_balls and Ants modes
3133-3136: Consider using constexpr for compile-time constantsThe magic numbers used for array sizes and limits could be defined as constexpr values for better maintainability.
- uint32_t bgcolor = BLACK; - uint8_t antSize = 1; - const uint16_t maxNumAnts = 32; // 255/16 + 1 ( * 2 ?????) - uint16_t dataSize = sizeof(rball_t) * maxNumAnts; + static constexpr uint16_t MAX_ANTS = 32; // Maximum number of ants + static constexpr uint8_t DEFAULT_ANT_SIZE = 1; + uint32_t bgcolor = BLACK; + uint8_t antSize = DEFAULT_ANT_SIZE; + uint16_t dataSize = sizeof(rball_t) * MAX_ANTS;
3243-3247: Consider using bit fields to reduce memory usageThe PacManChars struct could use bit fields to reduce memory usage since some fields don't need full bytes.
typedef struct PacManChars { - uint16_t pos; - uint8_t size; - uint32_t color; + uint16_t pos; // Position needs full range + uint8_t size:4; // Size likely won't exceed 15 + uint32_t color; // Color needs full 32 bits } pacmancharacters_t;
3386-3388: Consider using a constant for timing valueThe hardcoded timing value could be defined as a constant for better maintainability.
+ static constexpr uint16_t PACMAN_FRAME_TIME = 20; - return 20 + ((22 * (uint32_t)(255 - SEGMENT.speed)) / SEGLEN); // FRAMETIME and FRAMETIME_FIXED were too fast + return PACMAN_FRAME_TIME + ((22 * (uint32_t)(255 - SEGMENT.speed)) / SEGLEN); // Custom timing for smooth animation
7944-7965: Consider adding bounds checking for effect IDsThe addEffect function could benefit from additional bounds checking and error handling.
uint8_t WS2812FX::addEffect(uint8_t id, mode_ptr mode_fn, const char *mode_name) { + if (mode_fn == nullptr || mode_name == nullptr) return 255; // Invalid parameters + if (id == 255) { // find empty slot for (size_t i=1; i<_mode.size(); i++) if (_modeData[i] == _data_RESERVED) { id = i; break; } }
7967-7975: Consider using std::vector::reserve for performancePre-allocating vector capacity could improve performance during initialization.
void WS2812FX::setupEffectData() { + _mode.reserve(_modeCount); // Pre-allocate capacity + _modeData.reserve(_modeCount); // Pre-allocate capacity // Solid must be first! (assuming vector is empty upon call to setup) _mode.push_back(&mode_static); _modeData.push_back(_data_FX_MODE_STATIC);
📜 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 (3)
platformio.ini(2 hunks)wled00/FX.cpp(3 hunks)wled00/FX.h(1 hunks)
🔇 Additional comments (4)
wled00/FX.h (2)
329-329: Mode count updated correctly.The
MODE_COUNTis correctly updated to 189 to account for the two new effect modes (FX_MODE_ANTSandFX_MODE_PACMAN).
325-327: New effect modes added.The addition of
FX_MODE_ANTSandFX_MODE_PACMANeffects aligns with the PR objectives. However,FX_MODE_RACERSis commented out, which suggests it was initially planned but removed.Let's verify if there are any references to the commented-out mode:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for any references to FX_MODE_RACERS in the codebase rg "FX_MODE_RACERS|mode_racers"Length of output: 171
FX_MODE_RACERS confirmed as intentionally disabled.
The search results show that all references to FX_MODE_RACERS—both in wled00/FX.h and wled00/FX.cpp—are commented out. This indicates that the removal/replacement of FX_MODE_RACERS was deliberate. The addition of FX_MODE_ANTS and FX_MODE_PACMAN, along with the updated mode count, aligns well with the PR objectives.platformio.ini (2)
13-14: Default Environment Configuration Change
The changes here narrow the default board selection to only esp32dev by commenting out the original multi-board list and then setting “default_envs = esp32dev.” Given the community feedback requesting a reversion of unintended configuration changes, please confirm if this restriction is intentional. Limiting the environments could reduce testing flexibility, especially as the effects were originally noted for potential issues on other ESP chips.Also applies to: 21-21
130-130: Upload Speed Parameter Update
The upload_speed setting has been increased from 115200 to 921600. While a higher upload speed can accelerate firmware deployment on ESP32, it might introduce compatibility issues on other boards. Considering prior feedback about reverting some changes in platformio.ini, please verify whether this adjustment is needed solely for the new visual effects or if it should remain unchanged to support broader hardware compatibility.
Ants ---- - Made the number of ants more dynamically user-selectable. - The size of ants can now be up to 20 pixels wide. - Changed random() and random16() to hw_random() and hw_random16(). - Fixed a few suggestions by CodeRabbit TODO: - Foreground and background color selection needs to be improved. The current way of setting the background color is very clunky (with slider wled-dev#4). PacMan ------ - constexpr instead of const - return mode_static() on short segments (15 or less pixels) - made use of numGhosts intead of hard-coded integers - now using aux0 for binary flags (DIRECTION_FLAG and GHOSTSBLUE_FLAG) - now using aux1 for the timer tick count - reworked timing to draw moving characters and power dot - now returning FRAMETIME correctly TODO: - Speed adjustments? - Transition from previous FX is not looking good. It is supposed to wipe out any pixels by making them black. Then draw whitish dots every other pixel if the White Dots checkbox is enabled. But there are usually colored pixels still displayed from the previous FX.
wled00/FX.cpp
Outdated
| @@ -3124,38 +3124,27 @@ static const char _data_FX_MODE_ROLLINGBALLS[] PROGMEM = "Rolling Balls@!,# of b | |||
| * First slider is for the ants' speed. | |||
| * Second slider is for the # of ants. | |||
| * Third slider is for the Ants' size. | |||
| * | |||
| * Checkbox1 is for using the palettes (enabled) or the color slots (disabled) | |||
There was a problem hiding this comment.
in other FX, this is also done using something like if(palette==0) which is the "default" palette at the top.
I found that this was happening when one of the ant colors was the same as the background color (e.g. a black ant on a black background). You couldn't see that there was an ant there, until a different colored ant moved over it and then you would see just one or two pixels. I fixed this so that the ant color will never be the background color if the user is using the color slots. But it can still happen if they use a palette that has the same color as the background. I don't think it will be a problem most of the time. |
I added two new effects: Ant and PacMan Please take a look at them and test them and make any suggestions. I'm a noob at this, so I will probably code these in the most inefficient manner. :-) I don't know if the speeds are going to be good on other esp chips (e.g. ESP8266). I've only tested on an ESP32 WROOM 32D so far. Both effects are pretty rough right now and experts could make them so much better, I would think. I want to add at least another feature to reverse the direction of the PacMan effect.
Summary by CodeRabbit