Magma FX in the user_fx usermod #5360
Conversation
WalkthroughAdds a new 2D magma/lava visual effect: procedural magma rendering, lava-bomb particle simulation, per-frame state and slider handling, palette/wrap support, and registers the new mode with the FX system. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@usermods/user_fx/user_fx.cpp`:
- Line 200: The code allows particleCount (forced to a minimum of 2) to exceed
the allocated buffer when MAGMA_MAX_PARTICLES = width / 2 is 0 or 1; fix by
ensuring MAGMA_MAX_PARTICLES is clamped to a safe minimum (e.g., >=2) or by
returning early for too-narrow widths before allocating/using particles, and
also ensure particleCount is always min(particleCount, MAGMA_MAX_PARTICLES);
update the definitions/usages of MAGMA_MAX_PARTICLES and particleCount in
user_fx.cpp accordingly.
- Around line 188-191: The condition for drawing the anti-aliased sub-pixel for
w2 is wrong: it checks yf - 1 >= 0 (yf is fractional in [0,1) so this is always
false) instead of checking the pixel index; update the bounds check in the block
that calls SEGMENT.addPixelColorXY(xi, yFlipped - 1, pcolor.scale8(w2)) to use
yFlipped - 1 >= 0 (matching the check used for the w3 path) so the w2
contribution can be drawn when within bounds.
- Line 136: The uint8_t overflow occurs because idx = i * 4 wraps when i ≥ 64;
update the declarations used in both drawLavaBombs and the mode_2D_magma
initialization loop so that both i and idx are uint16_t (instead of uint8_t for
either variable) to avoid wrapping for larger particle counts, and ensure any
subsequent indexing or arithmetic using i/idx uses the new 16-bit types (i.e.,
change the loop variable and the idx variable declarations where they appear).
- Line 149: The condition if (posY < height / 8 - 1 || posX < 0 || posX >=
width) can underflow when height < 8 because height is unsigned; change the
logic to avoid unsigned wrap-around by computing a signed threshold (e.g., int
thresh = (int)height / 8 - 1; if (thresh < 0) thresh = 0;) and then use if (posY
< thresh || posX < 0 || posX >= width) (reference variables posY, height, width
and the original conditional).
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
11-11: Potential macro redefinition conflict withPALETTE_SOLID_WRAP.This macro is also defined in
FX.cpp. If both translation units are compiled independently this is fine, but if any shared header ever defines it, you'll get a redefinition warning. Consider guarding with#ifndef.Suggested guard
-#define PALETTE_SOLID_WRAP (paletteBlend == 1 || paletteBlend == 3) +#ifndef PALETTE_SOLID_WRAP + `#define` PALETTE_SOLID_WRAP (paletteBlend == 1 || paletteBlend == 3) +#endif
196-288: Consider adding a 2D minimum-size guard.Several calculations assume a minimum matrix size (e.g.,
width >= 4for particles,height >= 8for respawn thresholds). A single early guard would prevent edge-case issues across the function.Example
if (!strip.isMatrix || !SEGMENT.is2D()) return mode_static(); + if (SEG_W < 4 || SEG_H < 8) return mode_static(); // matrix too small for magma effect
|
unfortunately, the original effect is not license compatible. So unless you get explicit permission by the original author this can not be added. |
|
Thanks @DedeHai I will try to get explicit permission. If not, I will close this PR. |
|
you can try to ping them directly in this PR |
|
@DmytroKorniienko @kostyamat Hi! Both of you worked on an LED pattern/effect (Magma) that I found on soulmatelights.com and I was hoping you would give me permission to use the code in the WLED project. May I have your permission to do so? Also, do you have contact information for SottNick? Some of the code was originally from them. I have changed a lot of the code, but there are still some pieces of it that use all three of your code. Thank you for your consideration. And thank you for your coding! I love the Magma effect (obviously since I want to put it into WLED)! Regards, Bob Loeffler (Arizona, USA) |
You can freely use any my code of owned projects, no objections from my side |
At the time, all permissions from all possible contributors were obtained, all copyrights in the code were indicated in the comments of the code, and our project was published under the GPL license, which allows its free use, under the terms of the GPL. All contributors who gave consent to the use of their ideas or code in our project, in one way or another, were informed and agreed to the publication of ideas, code, etc. as part of our project under the GPL license. Therefore, on my behalf, I think I have every right to give you consent, which includes cross-consent of all contributors. |
I would like to note that in order to get the maximum visual effect from most of the patterns in our project, you should focus on porting the subpixel engine to WLED, which we developed and finalized. And the actual code should be taken from the development branch, not from Master. Master is very outdated. Unfortunately, the war that Russia is waging against our country has changed priorities, and besides the fact that the development branch is almost ready for release and is quite stable, there are still several reasons that postponed the release. Therefore, we are not against it if there are significant improvements in WLED based on our project. Life goes on, and we do not adhere to the "dog on the hay" policy. |
@kostyamat thanks for the permission, WLED is under EUPL so slightly less restrictive but very similar, EUPL into GPL is possible, GPL into EUPL only with explicit consent. |
You will find many more interesting LED patterns in our project https://github.com/DmytroKorniienko/FireLamp_EmbUI, and it is there that they are debugged and have the most correct appearance and code. Soulmate was used by us, as a rule, for small sketches and testing ideas, and the pattern code was worked out on real devices with real matrices, where it was brought to the ideal (to our taste). As I already noted, the code should be taken from the dev branch, where a) more patterns; b) subpixel graphics and existing patterns have undergone the most correct changes and additions. And yes, you and other participants in the WLED project have full permission for all our improvements. The only condition is - do not forget to indicate authorship. *** And in addition, if WLED ever (I hope not) suddenly becomes a closed commercial project - all permits should be considered automatically revoked. |
|
@DmytroKorniienko @kostyamat Thank you both for the permission! We appreciate it. I will take a look at the code in your dev branch. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@usermods/user_fx/user_fx.cpp`:
- Line 166: Remove the unused local variable assignment "float velocityY =
particleData[idx + 3];" to eliminate the dead code and compiler warning; locate
the assignment in the particle processing block where "particleData" and "idx"
are used (same scope as the existing "velocityX" or other particle fields) and
simply delete the line, or if the Y velocity is intended to be used later,
replace the unused variable with appropriate usage instead.
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
125-125: Usehw_random8(2)instead ofrandom8(2)for consistency.The rest of this file (lines 58, 61, 155, 169, 245) uses the WLED
hw_random8family.random8is a FastLED alias. Based on learnings, FastLED aliases should be replaced with proper WLED implementations.Proposed fix
- uint8_t noise = perlin8(i * magmaDeltaValue, (j + ff_y_int + random8(2)) * magmaDeltaHue, ff_z_int); + uint8_t noise = perlin8(i * magmaDeltaValue, (j + ff_y_int + hw_random8(2)) * magmaDeltaHue, ff_z_int);
197-197: Nit: extra semicolon.
FX_FALLBACK_STATIC;;has a redundant trailing semicolon after the macro expansion.- if (!strip.isMatrix || !SEGMENT.is2D()) FX_FALLBACK_STATIC;; // not a 2D set-up + if (!strip.isMatrix || !SEGMENT.is2D()) FX_FALLBACK_STATIC; // not a 2D set-up
| int16_t yi = (int16_t)posY; | ||
|
|
||
| if (xi >= 0 && xi < width && yi >= 0 && yi < height) { | ||
| float velocityY = particleData[idx + 3]; |
There was a problem hiding this comment.
Unused variable velocityY.
velocityY is assigned but never read. Remove it to avoid compiler warnings and dead code.
Proposed fix
- float velocityY = particleData[idx + 3];
-
- // Get a random color from the current palette
+ // Get a random color from the current palette📝 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.
| float velocityY = particleData[idx + 3]; | |
| // Get a random color from the current palette |
🤖 Prompt for AI Agents
In `@usermods/user_fx/user_fx.cpp` at line 166, Remove the unused local variable
assignment "float velocityY = particleData[idx + 3];" to eliminate the dead code
and compiler warning; locate the assignment in the particle processing block
where "particleData" and "idx" are used (same scope as the existing "velocityX"
or other particle fields) and simply delete the line, or if the Y velocity is
intended to be used later, replace the unused variable with appropriate usage
instead.
5 participants
This PR will add the Magma effect into the new user_fx usermod instead of FX.cpp
Summary by CodeRabbit