Add new effect: PS Galaxy #4682
Conversation
WalkthroughA new 2D particle effect called "Particle Galaxy" has been introduced, featuring spiraling particles with configurable parameters. The effect is implemented in the core source file, registered in the effect setup, and assigned a unique identifier in the header file. The total mode count is also updated to reflect the addition. Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)wled00/FX.cppTip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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: 3
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
8992-8996:custom1/check3interaction silently overrides particle sizeWhen
SEGMENT.check3is true the code forcibly maps anycustom1value to either0or1, effectively discarding sizes > 1.
Because UI sliders still allow 0-255 this can confuse users and feels like a bug.Consider using a separate custom parameter for the “trace” toggle or honour the original size while still enabling motion-blur, e.g.:
-if (SEGMENT.check3) - particlesize = SEGMENT.custom1 ? 1 : 0; +if (SEGMENT.check3 && particlesize <= 1) // only coerce when really needed + particlesize = particlesize ? 1 : 0;Please confirm if clamping to {0,1} is intentional UX.
9057-9057: Metadata string: placeholder fields are confusing
"PS Galaxy@!,!,Size,,Color,,Starfield,Trace;..."contains empty parameter slots (!,!,,,) which show as unnamed sliders in the UI.Consider either:
- Filling them with meaningful names (e.g. “Speed”, “Intensity”), or
- Removing trailing commas to hide them.
This improves UX consistency with other particle effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(2 hunks)wled00/FX.h(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Environments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp32c3dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full)
- GitHub Check: wled_build / Build Environments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Environments (nodemcuv2_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
- GitHub Check: wled_build / Build Environments (nodemcuv2)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Environments (esp8266_2m_160)
🔇 Additional comments (3)
wled00/FX.h (2)
356-356: New effect identifier registration
The macroFX_MODE_PARTICLEGALAXYis correctly inserted with ID217, followingFX_MODE_PS1DSPRINGY (216). Ensure this ID is unique and doesn’t collide with any other mode IDs.Please verify in
FX.cpp’ssetupEffectData()(or equivalent) thatmode_particlegalaxyis registered using this identifier, e.g.:addEffect(FX_MODE_PARTICLEGALAXY, mode_particlegalaxy, "Particle Galaxy");
357-357: Update MODE_COUNT to reflect new mode
TheMODE_COUNTmacro has been incremented from217to218, which correctly accounts for the added effect. Confirm that any arrays, loops, or UI elements that rely onMODE_COUNTproperly accommodate the new size.wled00/FX.cpp (1)
8970-8983: Memory-leak / double-alloc defence
initParticleSystem2Dallocates and stores the pointer inSEGENV.data, butPartSysis also a local pointer.
If the function fails midway (e.g. allocation succeeded but a subsequent call path returnsmode_static()), the allocated object might be leaked.Verify that
initParticleSystem2D()internally frees on failure or add a clean-up path before returning.
|
Are any of the coderabbit suggestions valid/useful ? |
|
the last one is actually a good one. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wled00/FX.cpp (1)
9006-9018: 🛠️ Refactor suggestionOptimize by moving invariant calculations outside the particle loop
The calculation of centerx/centery and the mode-specific settings are invariant for each frame but are recalculated for every particle. Moving these outside the particle loop would improve performance, especially on resource-constrained devices like ESP8266.
+ // move alive particles in a spiral motion (or almost straight in fast starfield mode) + int32_t centerx = PartSys->maxX >> 1; // center of matrix in subpixel coordinates + int32_t centery = PartSys->maxY >> 1; + if (SEGMENT.check2) { // starfield mode + PartSys->setKillOutOfBounds(true); + PartSys->sources[0].var = 7; // emiting variation + PartSys->sources[0].source.x = centerx; // set emitter to center + PartSys->sources[0].source.y = centery; + } + else { + PartSys->setKillOutOfBounds(false); + PartSys->sources[0].var = 1; // emiting variation + } + for (uint32_t i = 0; i < PartSys->usedParticles; i++) { //check all particles if (PartSys->particles[i].ttl == 0) continue; //skip dead particles // (dx/dy): vector pointing from particle to center - int32_t centerx = PartSys->maxX >> 1; // center of matrix in subpixel coordinates - int32_t centery = PartSys->maxY >> 1; - if (SEGMENT.check2) { // starfield mode - PartSys->setKillOutOfBounds(true); - PartSys->sources[0].var = 7; // emiting variation - PartSys->sources[0].source.x = centerx; // set emitter to center - PartSys->sources[0].source.y = centery; - } - else { - PartSys->setKillOutOfBounds(false); - PartSys->sources[0].var = 1; // emiting variation - }
🧹 Nitpick comments (2)
wled00/FX.cpp (2)
9039-9041: Use consistent type definitions for spiral calculationsYou're using
intfor spiral velocity calculations, while the rest of the code usesint32_t. For consistency and to prevent potential issues with large values (especially from the<< 9shift), useint32_tthroughout.- int vxc = (dx << 9) / (distance - 19); // subtract value from distance to make the pull-in force a bit stronger (helps on faster speeds) - int vyc = (dy << 9) / (distance - 19); + int32_t vxc = (dx << 9) / (distance - 19); // subtract value from distance to make the pull-in force a bit stronger (helps on faster speeds) + int32_t vyc = (dy << 9) / (distance - 19);
9052-9055: Define behavior for intermediate custom3 valuesThe color mapping logic only explicitly handles custom3 values of 0 and 31, leaving the behavior for values between 1-30 undefined. Consider adding handling for intermediate values or clarifying the current behavior in a comment.
if(SEGMENT.custom3 == 31) // color by age but mapped to 1024 as particles have a long life, since age is random, this gives more or less random colors PartSys->particles[i].hue = PartSys->particles[i].ttl >> 2; else if(SEGMENT.custom3 == 0) // color by distance PartSys->particles[i].hue = map(distance, 20, (PartSys->maxX + PartSys->maxY) >> 2, 0, 180); // color by distance to center + else // for intermediate values 1-30, inherit hue from source with small variations + PartSys->particles[i].hue = PartSys->sources[0].source.hue + (hw_random8() & 0x0F) - 7; // +/- 7 variation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/FX.cpp (2)
undefined
<retrieved_learning>
Learnt from: DedeHai
PR: #4682
File: wled00/FX.cpp:8997-9005
Timestamp: 2025-05-09T18:43:15.327Z
Learning: In the WLED codebase, SEGMENT.custom3 is always constrained to the range 0-31 and will not exceed this range.
</retrieved_learning>
<retrieved_learning>
Learnt from: DedeHai
PR: #4682
File: wled00/FX.cpp:9016-9039
Timestamp: 2025-05-09T18:48:21.248Z
Learning: WLED does not support matrices large enough (≥1024 px) for 32-bit overflow in dxdx + dydy to be a practical concern in particle system effects.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: wled_build / Build Environments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Environments (usermods)
- GitHub Check: wled_build / Build Environments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Environments (esp32_wrover)
- GitHub Check: wled_build / Build Environments (esp32_eth)
- GitHub Check: wled_build / Build Environments (lolin_s2_mini)
- GitHub Check: wled_build / Build Environments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Environments (esp32dev_V4)
- GitHub Check: wled_build / Build Environments (esp32dev)
- GitHub Check: wled_build / Build Environments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Environments (esp8266_2m)
🔇 Additional comments (1)
wled00/FX.cpp (1)
10762-10762: LGTM! Effect registration looks good.The new effect is properly registered in the effect setup function.
|
ok, fixed it. ready to merge. |
2 participants
VID_20250508_185054_1.mp4
Summary by CodeRabbit