Ant and PacMan effects #4536
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:
|
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
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
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#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.
blazoncek
left a comment
There was a problem hiding this comment.
I would like you to also remove changes to package-lock.json and reuse removed effects IDs (marked in the source) for new effects.
Otherwise the code is clean and readable, no comments there.
that is by design. if you want to fix it, there are two ways:
|
I think this is the cleanest and least confusing way to do it, so I'll do this for the next version. Thx. |
|
Hi @DedeHai and @blazoncek I just pushed the latest version. I've fixed all of the bugs that I know about and looked at everything you have mentioned, except the floats. I see a lot of other effects with floats and I just did my effects the same as those, since I don't know how to not use floats. Thanks for all of your help! Both effects look really good now. |
|
Hi @DedeHai and @blazoncek I just pushed the latest version. I had found a crashing issue in PacMan when using the White Dots option on a 2D matrix. It's fixed now. I think both effects are done now and we can merge this branch into the main WLED (I think that is the correct terminology). Please take a look when you have time. If you think it's good, let me know what I need to do. Thx! |
|
I need to look at it once more, I think the pacman effect is ok to merge, I am not too sure about the ants effect, I need to test it some more and check the code. As mentioned before, the ant effect may be better done using the PS: potentially shorter code, much more rendering options (colors, size of ants etc.) |
Ok, let me know about the Ants code after you've had time to review it. Thanks! |
…ler68/WLED-AC into pr-ant-and-pacman-modes
|
The everyXLeds code is not looking right on my 141 LED strip, so I'm going to play around with it. |
ups, there is a missing bracket: |
Yes, that was it. Thx! New version pushed. Looks very optimized now. |
|
I tested the ant effect yesterday and while the effect itself looks ok, the code is a mess. |
I basically used the code from the rolling_balls FX and made modifications to it. Last night I started looking at your PS code, so maybe I will try to convert it. Not sure how that will go since I'm not a coding expert. If that doesn't go well, I'll move it to the user-effects usermod. |
|
you can start looking at |
|
Ok, thx. I'll take a look at that soon. |
|
Hi @DedeHai and @blazoncek I haven't had any time to work on this for almost 2 months, but am hoping to this weekend or next weekend. Life and family keep getting in my way. :-) There are 3 things that I want to get done:
Which one should I do first? Do I need to close this PR (since I will probably remove the Ants FX) and then create a new branch and PR? |
|
I'd try to figure out what's causing memory drain 1st. |
Ok, I'll do that first. My published code on GitHub is the same as my local "rolled back" code that I tried to merge/squash on 5/25/2025 in the custom-fx channel on the WLED discord. It said that local code was already up-to-date with my published code. So maybe I need to go back further and checkout an older commit. I have a big gitlog saved as a text file. Or the reflog if either of those would help. Or would it be easier to create a new repository by cloning the latest code from WLED AC? |
|
@blazoncek and @DedeHai I installed SourceTree in case that will help us figure out how to get my published code back to a point where I can help find the memory error. I've attached a screenshot of my main branch's history. In May I tried going back to a good commit (b19ec2c), but my published code on GitHub was up-to-date with my local branch, so I need to go further back. Is it up-to-date with my PR branch because of the highlighted/selected branch merge (b9e7c2e)? |
|
|
Closed this PR and will open a new one for just the PacMan effect. |
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