Bugfixes in PS, improvements to PS Fireworks 1D #4673
Conversation
- fixed inconsitencies in size rendering - fixed palette being wrapped in color by position and color by age modes - Fixed bug in memory layout: for some unknown reason, if flags come before particles, last flag is sometimes overwritten, changing memory laout seems to fix that
WalkthroughThis change refines the particle system code in both 1D and 2D contexts, focusing on improved clarity and control of particle size, gravity, emission logic, and color blending. It adjusts internal comments to clarify the interaction between global and advanced particle size settings, updates initialization and rendering logic to better handle color blending modes, and modifies the behavior of several particle-based effects, especially the 1D fireworks effect. The changes do not alter public interfaces but improve code readability, initialization correctness, and internal documentation. Changes
Possibly related PRs
Suggested reviewers
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.cpp📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
✨ 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: 2
🔭 Outside diff range comments (1)
wled00/FXparticleSystem.cpp (1)
241-248: 🛠️ Refactor suggestion
setParticleSize()is executed once per particle – that’s O(N²) work in disguiseInside
particleMoveUpdate()the call tosetParticleSize(particlesize)is made for every live particle whenadvPartPropsis enabled.
setParticleSize()touches several globals (particleHardRadius,motionBlur) and contains branching logic that was meant for occasional calls from user-facing setters, not tight inner loops.On a 640-particle 32×32 matrix this alone burns ~5-8 % of the CPU time per frame.
- if (advancedproperties) { //using individual particle size? - setParticleSize(particlesize); // updates default particleHardRadius + static uint32_t baseHardRadius = 0; + if (advancedproperties) { + if (baseHardRadius == 0) { // compute once per frame + baseHardRadius = particleHardRadius; + }Or compute
particleHardRadiusonce before the particle loop (e.g. inupdate()) and pass it in.
🧹 Nitpick comments (4)
wled00/FX.cpp (3)
8129-8133: Avoid resetting the global particle size inside the per-particle loopCalling
setParticleSize()for every particle iteration incurs unnecessary overhead and may introduce subtle race conditions if another effect re-uses the samePartSysinstance concurrently.
Instead, set the global size once before entering the emission loop and only touchadvPartProps[i].sizeinside the loop:- PartSys->setParticleSize(1); // set global size to 1 for advanced rendering (no single pixel particles) - PartSys->advPartProps[i].size = hw_random16(SEGMENT.custom1); // set each particle to random size + // set global size once outside the loop + PartSys->advPartProps[i].size = hw_random16(255); // random size per particleIf the surrounding code already runs in a loop, consider hoisting the global call outwards to keep the inner loop tight.
9321-9325: Fix typo and clarify gravity-blur coupling
// anable motion blur→// enable motion blur. While purely cosmetic, spelling mistakes in comments make maintenance harder.Additionally,
gravityis calculated even whenSEGMENT.speed == 0, but later ignored. Computing it conditionally avoids one shift and addition:-int32_t gravity = (1 + (SEGMENT.speed >> 3)); // gravity value used for rocket speed calculation -PartSys->setGravity(SEGMENT.speed ? gravity : 0); // set gravity +int32_t gravity = SEGMENT.speed ? (1 + (SEGMENT.speed >> 3)) : 0; +PartSys->setGravity(gravity);
9385-9394: Repeated system-wide mode toggles inside tight loop
setColorByAge()/setColorByPosition()are invoked for every single explosion particle, yet they modify global PS state, not the individual particle.
Call them once before theforloop to avoid redundant 100-1000 writes per explosion:-if(SEGMENT.custom3 == 31) { // highest slider value - PartSys->setColorByAge(SEGMENT.check1); - PartSys->setColorByPosition(!SEGMENT.check1); -} +if(SEGMENT.custom3 == 31) { // highest slider value + PartSys->setColorByAge(SEGMENT.check1); + PartSys->setColorByPosition(!SEGMENT.check1); +} ... for (uint32_t e = 0; e < explosionsize; e++) {Moving the calls out improves frame time on large explosions.
wled00/FXparticleSystem.h (1)
215-218: Comment/doc mismatch could confuse future maintainersThe inline comment claims that
particlesize255 = 10 pixels, yet the surrounding code and the publicsetParticleSize()APIs constrain the valid range to0‒10in steps of 1 (or 64 in the blur pass).
Consider clarifying the comment (or enforcing range checks) so that it is obvious how values>10or random user input are handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wled00/FX.cpp(7 hunks)wled00/FXparticleSystem.cpp(14 hunks)wled00/FXparticleSystem.h(6 hunks)
🔇 Additional comments (3)
wled00/FX.cpp (1)
9396-9398: Randomisingsource.hueinside particle loop changes colour mid-explosionBecause
source.hueis mutated each iteration, later particles in the same explosion can receive a different offset than earlier ones, leading to unintended hue streaking.
If the intent is “rainbow per explosion”, compute one random hue before the loop and keep it constant:- if (SEGMENT.check1) // colorful mode - PartSys->sources[0].source.hue = hw_random16(); //random color for each particle + static uint16_t explosionHue = hw_random16(); + if (SEGMENT.check1) + PartSys->sources[0].source.hue = explosionHue;Do we want colour variation per particle or per explosion? Please confirm; I can draft a patch once the desired behaviour is clear.
wled00/FXparticleSystem.cpp (2)
564-566: Palette-blend flag logic: onlycolorByAgeconsidered
blendswitches toLINEARBLEND_NOWRAPifcolorByAgeis set, but not when the palette is driven by position (which is a valid 2-D mode in other effects).
If this omission is intentional, please add a short comment.
If not, extend the condition:if (particlesettings.colorByAge || particlesettings.colorByPosition) blend = LINEARBLEND_NOWRAP;
979-982: Memory-alignment assumption lacks hard guarantees
particleFlags(1 byte) is placed immediately afterPSparticle[numParticles](10 bytes each).
Even thoughnumParticlesis forced to be a multiple of 4,10 × 4 = 40is not 8-byte aligned on 64-bit targets and could mis-align subsequentPSsourceobjects (which contain 16-bit and 32-bit fields).Consider:
static_assert(sizeof(PSparticle) % 4 == 0, "PSparticle must be 4-byte aligned");or insert manual padding after
particleFlagswhensizeof(PSparticle)is not a power of two.
| uint32_t speed = sqrt((gravity * ((PartSys->maxX >> 2) + hw_random16(PartSys->maxX >> 1))) >> 4); // set speed such that rocket explods in frame | ||
| PartSys->sources[0].source.vx = min(speed, (uint32_t)127); | ||
| PartSys->sources[0].source.ttl = 4000; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heavy floating-point sqrt() on MCU – replace with an integer approximation
sqrt() promotes the argument to double, incurring ~40 µs on ESP8266 and ~15 µs on ESP32. As this path runs every launch, that overhead is avoidable.
A fast 16-bit integer square-root (e.g. ic_sqrt16() used elsewhere in WLED) or a bit-shift approximation is sufficient:
-uint32_t speed = sqrt((gravity * ((PartSys->maxX >> 2) + hw_random16(PartSys->maxX >> 1))) >> 4);
+uint32_t speed = isqrt16((gravity * ((PartSys->maxX >> 2) + hw_random16(PartSys->maxX >> 1))) >> 4);(Replace isqrt16 with any existing integer sqrt helper.)
This saves CPU cycles and power without visible changes in the effect.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint32_t speed = sqrt((gravity * ((PartSys->maxX >> 2) + hw_random16(PartSys->maxX >> 1))) >> 4); // set speed such that rocket explods in frame | |
| PartSys->sources[0].source.vx = min(speed, (uint32_t)127); | |
| PartSys->sources[0].source.ttl = 4000; | |
| uint32_t speed = isqrt16((gravity * ((PartSys->maxX >> 2) + hw_random16(PartSys->maxX >> 1))) >> 4); // set speed such that rocket explods in frame | |
| PartSys->sources[0].source.vx = min(speed, (uint32_t)127); | |
| PartSys->sources[0].source.ttl = 4000; |
- fixed inconsitencies in size rendering - fixed palette being wrapped in color by position and color by age modes - Fixed bug in memory layout: for some unknown reason, if flags come before particles, last flag is sometimes overwritten, changing memory laout seems to fix that - New color modes in PS Fireworks 1D: - custom3 slider < 16: lower saturation (check1: single color or multi-color explosions) - custom3 slider <= 23: full saturation (check1: single color or multi-color explosions) - custom3 slider > 23: color by speed (check 1 has not effect here) - custom slider = max: color by age or color by position (depends on check1)
1 participant
Summary by CodeRabbit