Repository navigation
2D effect bugfixes for large panels - #5908
Open
softhack007 wants to merge 16 commits into
Open
softhack007 wants to merge 16 commits into
softhack007 wants to merge 16 commits into
Conversation
* Crazy Bee: keep bees on track by avoiding int8 math * DNA Spiral: fix effect freeze when width >= 128 * Colored Bursts: Use full panel when width >= 128 * Rotozoomer: Use full panel when width >= 128
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThe changes add Changes2D effect arithmetic
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly summarizes the main change: bug fixes for 2D effects on large panels. It is concise and specific. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
softhack007
commented
Oct 15, 2026
DedeHai
reviewed
Oct 15, 2026
as suggested by @DedeHai
* adjusted scaling parameters to avoid going out-of-bounds * limit effect speed on large panels (was depending on pixel count) * parameter tuning for larger panels, to make the effect look similar to smaller sizes
* solves undefined behavior from mixing signed and unsigned math * replace abs8() with abs() * parameter tuning to create a bigger sun an large fixtures * previous look preserved up to 32x32 pixels
DedeHai
reviewed
Oct 16, 2026
the "black center" was never showing in larger panels => parameters tuned so the effect looks better on >32x32. The values used come from experimenting - no idea how the math really works ....
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
forgot to replace this - the inner loop should use pixelCount instead of SEGENV.step. Slightly adjusted speedScale to compensate for _slower_ effect motion.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
as suggested by the rabbit
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
avoid an effect freeze when the effect was temporarily suspended, for example by switching the "power icon" off and then back on
prevent effect freeze when timebase jumps
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
prevents short delay before effects start
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Future support:For dimension 257, cols - 1 or rows - 1 is 256, but beatsin8_t converts highest to uint8_t, producing 0. Limit cols/rows to the maximum that beatsin8_t can handle. NB: replacing beatsin8_t with beatsin16_t did not work, it makes the effect look very boring.
Member
Author
|
Side note: width = 256 or height = 256 is possible in theorie, however it confuses the 2D panel setup. Event width=255 and offset=1 won't work; it causes 2D setup and segments to fall back into 1D mode. This might get fixed in a future PR. For now the practical upper limit in WLED is 240 pixels width/height, achievable with 15 ws2812b panels of 16x16 pixels (alternatively: 7 HUB75 panels 32x32 = 224), or with a mixed HUB75/ws2812b panel setup. |
DedeHai
reviewed
Oct 21, 2026
| unsigned _scale = map(SEGMENT.intensity, 0, 255, 30, adjScale); | ||
| int _speed = map(SEGMENT.speed, 0, 255, 128, 16); | ||
| // size-based mappings | ||
| float adjustHeight = (rows > 32) ? 12.0f * 32.0f / rows : mapf(max(rows, 8), 8, 32, 28, 12); // scale height inversely for larger matrices |
Collaborator
There was a problem hiding this comment.
I dont think there is a need to use floats here, should be doable in integer math
Member
Author
There was a problem hiding this comment.
challenge accepted - I'll see if the inner loop can be done with adjustHeight converted to fixed point
Member
Author
There was a problem hiding this comment.
@DedeHai answer is yes - we can get rid of the float calculation in the inner loop. I need to test my idea on hardware, and will push something tomorrow.
DedeHai
approved these changes
Oct 21, 2026
* prevent timing overflow * prevent barHeight overflow * hide "peak dot" when peak fade = 255
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes several problems that were identified in #5903
byteandint8_t+ bigger sun on larger panels (WLED-MM backport)general improvements:
abs8()withabs()where possiblelerp16by8(uint16_t a, uint16_t b, uint8_t frac)to fastled-slimAI assistance: nothing. Just my squishy bio-brains.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes