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 (
|
![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#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.
|
Hi @DedeHai I've cleaned up the PacMan code and it's looking much better, but I still have one big issue. I added the ability to select up to 5 power dots, but it's not working correctly. If I hard-code numPowerDots in line 3325, it works fine. But when I allow the user to select the number of power dots with the intensity slider (line 3324), it doesn't work correctly. No more than 3 power dots are displayed even if I select 4 or 5, and the power dots are not put in the correct LEDs. Can you take a look at it? I just pushed the latest code to my branch. Thanks. |
|
looks a lot cleaner, I think you are getting the hang of it. I can check whats up with the dots. |
I really, really appreciate your help and guidance! |
|
fixed it. I did get a crash at one point when changing number of dots near the end of the animation, no idea why. |
Thanks! I did find a new bug with the dynamic setting of the number of power dots. After approximately half of the power dots are eaten while the setting is set at the default (50% of the slider), change it to 100% and you will see the new powerdots in the pixesl that have already been eaten. I can fix that by adding "&& character[i+5].pos > character[PACMAN].topPos" to line 3423: // now draw the power dots in the segment only if they have not been eaten yet But PacMan will still start chasing the ghosts at that pixel because line 3407 is placing the powerdot there, even though we can't see it because of my change above. I tried doing the following, but then none of the powerdots are set/displayed (besides the last one). :-( // update power dot positions: can change if user selects a different number of power dots Any ideas? And do we want to use maxPowerDots in there? Or should it be numPowerDots? I played with both but it didn't seem to help with this bug. |
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. |
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