Repository navigation
2D effect bugfixes for large panels - #5908
Open
softhack007 wants to merge 10 commits into
Open
softhack007 wants to merge 10 commits into
softhack007 wants to merge 10 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 12 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main changes: bug fixes for 2D effects on large panels. 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.
- 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.
Member
Author
|
@coderabbitai for future PR's that touch 2D effects (fx.cpp or usermods), please remember to always check that the effect math is stable up to 256x256 pixels. Consider 24x16, 32x24, 32x32, and 48x40 as typical 2D sizes when using ws2812b panels. HUB75 panels are typically 64x64, up to 4 panels can be chained for a larger fixture. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. @softhack007 Thanks for the guidance. The review criteria are:
✏️ Learnings added
You are interacting with an AI system. |
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
2 participants
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