Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new 2D magma/lava effect: procedural magma rendering, lava-bomb particle simulation, per-frame state and slider handling, palette wrap support, effect registration, and merge-conflict markers present in the diff. 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
|
@BobLoeffler68 looks ok to me, just give me the green light to merge. |
|
@DedeHai I just resolved the two conflicts in this one. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
usermods/user_fx/user_fx.cpp (1)
217-218: Use a collision-free key for settings change detection.The additive checksum can collide (different slider combinations with the same sum), so reinitialization may be skipped incorrectly.
Proposed refactor
- uint32_t settingssum = SEGMENT.speed + SEGMENT.intensity + SEGMENT.custom1 + SEGMENT.custom2; - bool settingsChanged = (*settingsSumPtr != settingssum); + uint32_t settingsKey = + (uint32_t)SEGMENT.speed | + ((uint32_t)SEGMENT.intensity << 8) | + ((uint32_t)SEGMENT.custom1 << 16) | + ((uint32_t)SEGMENT.custom2 << 24); + bool settingsChanged = (*settingsSumPtr != settingsKey); ... - *settingsSumPtr = settingssum; + *settingsSumPtr = settingsKey;Also applies to: 254-254
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@usermods/user_fx/user_fx.cpp` around lines 217 - 218, The current additive checksum (settingssum) can collide; instead build a collision-free composite key by packing the four slider bytes into a single 32-bit value (e.g., key = (SEGMENT.speed<<24) | (SEGMENT.intensity<<16) | (SEGMENT.custom1<<8) | SEGMENT.custom2) and use that for the comparison against *settingsSumPtr (replace settingssum with this packed key wherever settingsSumPtr is used, including the other occurrence noted). Ensure you cast the SEGMENT fields to uint32_t before shifting to avoid overflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Line 102: There are unresolved merge-conflict markers ('<<<<<<<', '=======',
'>>>>>>>') left in user_fx.cpp that break compilation and drop effect
registration; remove all conflict tokens and reconcile the two sides so both
intended effects and their registrations remain, ensuring the effect
initialization/registration block (the effect registration code around the
current conflict) is merged cleanly and any duplicated or missing calls are
fixed so both effects are registered exactly once.
- Line 15: Remove the duplicate macro definition PALETTE_SOLID_WRAP that
references the global paletteBlend; keep the correct definition that uses
strip.paletteBlend, and update the file to only define PALETTE_SOLID_WRAP as
(strip.paletteBlend == 1 || strip.paletteBlend == 3) so all uses of
PALETTE_SOLID_WRAP (e.g., where it's referenced alongside functions/methods
handling palettes and effects) consistently use the strip member instead of the
unsynchronized global paletteBlend.
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 217-218: The current additive checksum (settingssum) can collide;
instead build a collision-free composite key by packing the four slider bytes
into a single 32-bit value (e.g., key = (SEGMENT.speed<<24) |
(SEGMENT.intensity<<16) | (SEGMENT.custom1<<8) | SEGMENT.custom2) and use that
for the comparison against *settingsSumPtr (replace settingssum with this packed
key wherever settingsSumPtr is used, including the other occurrence noted).
Ensure you cast the SEGMENT fields to uint32_t before shifting to avoid
overflow.
5 participants
This PR will add the Magma effect into the new user_fx usermod instead of FX.cpp
Summary by CodeRabbit