Perlinscape FX (2D) in the user_fx usermod#5587
Open
BobLoeffler68 wants to merge 8 commits intowled:mainfrom
Open
Perlinscape FX (2D) in the user_fx usermod#5587BobLoeffler68 wants to merge 8 commits intowled:mainfrom
BobLoeffler68 wants to merge 8 commits intowled:mainfrom
Conversation
softhack007
added
usermod
Member
|
@BobLoeffler68 I see "0 files changed" - is this expected? 
|
softhack007
added
the
waiting for feedback
Contributor
Author
Hmmmmm... I'm not sure what is happening. I can't even compile it anymore as it says platform_wled_default doesn't exist in the [common] section of platformio.ini Something strange is happening. |
Contributor
WalkthroughA new 2D "Perlinscape" user FX effect was added. It renders a Perlin-noise landscape on a 2D LED matrix with configurable palette or RGB output, zoom/intensity and XY multipliers, optional rotation, and optional randomized X/Y direction flips. Changes2D Perlinscape Effect
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
1335-1336: 💤 Low valueConsider
uint_fast8_tfor hot-path loop counters.The nested per-pixel loops use
byte(uint8_t). For hot-path performance, the coding guidelines suggestuint_fast8_t/uint_fast16_tfor loop counters, though effect functions have diverse contributor styles.♻️ Optional refactor
- for (byte x = 0; x < width; x++) { - for (byte y = 0; y < height; y++) { + for (uint_fast8_t x = 0; x < width; x++) { + for (uint_fast8_t y = 0; y < height; y++) {As per coding guidelines: Use
uint_fast16_t/uint_fast8_tin hot-path code (per-frame/per-pixel loops).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1335 - 1336, The per-pixel nested loops use the typedef byte (uint8_t) for counters x and y which can be suboptimal in hot-path rendering; change the loop counter types from byte to uint_fast8_t (or uint_fast16_t if width/height can exceed 255) for the loops that iterate over width and height (the variables x and y in the nested for loops) to follow the coding guideline for per-frame/per-pixel hot-path performance, ensuring any dependent uses/overloads accept the updated integer type.
1261-1354: 💤 Low valueAdd AI-generated code markers if applicable.
The PR description mentions "additional features (and help from Claude)" and the code comment references "help from Claude", but there are no
// AI:/// AI: endmarkers in the code. If any contiguous blocks were AI-generated, please mark them per the coding guidelines.Example format:
// AI: below section was generated by an AI ... AI-generated code ... // AI: endAs per coding guidelines: Mark AI-generated code blocks with
// AI: below section was generated by an AI/// AI: endcomments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1261 - 1354, The function mode_2D_perlinscape and its top comment reference “help from Claude”; if any contiguous portion of this function or surrounding blocks was AI-generated, wrap that exact contiguous block with the required markers (add a line "// AI: below section was generated by an AI" before the block and "// AI: end" after it); if no part of mode_2D_perlinscape or its comments was AI-generated, add a brief single-line clarifying comment near the top (e.g., immediately above the function comment) stating that no AI-generated code was used; locate code by the symbol mode_2D_perlinscape and the header comment mentioning Claude to apply the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1329-1330: Replace the standard library calls
cosf(angle)/sinf(angle) with the project's approximations: call
cos_approx(angle) to assign cosA and sin_approx(angle) to assign sinA; update
the expressions where cosA and sinA are set (look for the assignments to cosA
and sinA that currently use cosf/sinf with the variable angle) so they use
cos_approx(angle) and sin_approx(angle) respectively to comply with the WLED C++
coding guideline.
- Line 1277: The allocation is oversized because only five floats (offX, offY,
stepX, stepY, prevT) are used: update the SEGENV.allocateData call to allocate
exactly 5 * sizeof(float) bytes (remove the + width * height), and ensure no
other code in this scope relies on the removed buffer; locate the call to
SEGENV.allocateData and the subsequent accesses to offX/offY/stepX/stepY/prevT
to verify they're unchanged.
---
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1335-1336: The per-pixel nested loops use the typedef byte
(uint8_t) for counters x and y which can be suboptimal in hot-path rendering;
change the loop counter types from byte to uint_fast8_t (or uint_fast16_t if
width/height can exceed 255) for the loops that iterate over width and height
(the variables x and y in the nested for loops) to follow the coding guideline
for per-frame/per-pixel hot-path performance, ensuring any dependent
uses/overloads accept the updated integer type.
- Around line 1261-1354: The function mode_2D_perlinscape and its top comment
reference “help from Claude”; if any contiguous portion of this function or
surrounding blocks was AI-generated, wrap that exact contiguous block with the
required markers (add a line "// AI: below section was generated by an AI"
before the block and "// AI: end" after it); if no part of mode_2D_perlinscape
or its comments was AI-generated, add a brief single-line clarifying comment
near the top (e.g., immediately above the function comment) stating that no
AI-generated code was used; locate code by the symbol mode_2D_perlinscape and
the header comment mentioning Claude to apply the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 71a00cb6-14af-4164-b12e-ec1652c75822
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
…- use cos_approx() and sin_approx() instead of cosf and sinf
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
usermods/user_fx/user_fx.cpp (2)
1337-1338: ⚡ Quick winConsider using
uint16_tfor loop variables to support larger matrices.The loop variables
xandyare declared asbyte(uint8_t), which limits them to 0-255. While typical WLED matrices are smaller, usinguint16_twould future-proof the code for larger matrices and match the return types ofSEG_WandSEG_H.🔧 Proposed fix
- for (byte x = 0; x < width; x++) { - for (byte y = 0; y < height; y++) { + for (uint16_t x = 0; x < width; x++) { + for (uint16_t y = 0; y < height; y++) { float rx = cosA * (x - cx) - sinA * (y - cy) + cx; float ry = sinA * (x - cx) + cosA * (y - cy) + cy;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` around lines 1337 - 1338, Loop indices x and y are declared as byte (uint8_t), which restricts matrix sizes; change their types to uint16_t in the nested loops that iterate over SEG_W and SEG_H (replace the declarations of x and y in the for loops) so they match the larger return types and support bigger matrices, and update any nearby usages/expressions that assume byte to prevent implicit truncation.
1277-1277: 💤 Low valueRemove commented-out old code.
Lines 1277, 1329, and 1330 contain the old buggy code that has been fixed. These commented lines should be removed to keep the codebase clean.
🧹 Proposed cleanup
const uint16_t width = SEG_W; const uint16_t height = SEG_H; -// if (!SEGENV.allocateData(5 * sizeof(float) + width * height)) FX_FALLBACK_STATIC; if (!SEGENV.allocateData(5 * sizeof(float))) FX_FALLBACK_STATIC;if (SEGMENT.check2) { float angle = strip.now / 5000.0f; -// cosA = cosf(angle); -// sinA = sinf(angle); cosA = cos_approx(angle); sinA = sin_approx(angle); }Also applies to: 1329-1330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@usermods/user_fx/user_fx.cpp` at line 1277, Remove the leftover commented-out old code that calls SEGENV.allocateData and uses FX_FALLBACK_STATIC; locate occurrences of the commented line containing "SEGENV.allocateData(5 * sizeof(float) + width * height)) FX_FALLBACK_STATIC" and the related commented lines and delete them so the fixed implementation remains clean and no obsolete code is left in user_fx.cpp (search for SEGENV.allocateData and FX_FALLBACK_STATIC to find all instances).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@usermods/user_fx/user_fx.cpp`:
- Around line 1337-1338: Loop indices x and y are declared as byte (uint8_t),
which restricts matrix sizes; change their types to uint16_t in the nested loops
that iterate over SEG_W and SEG_H (replace the declarations of x and y in the
for loops) so they match the larger return types and support bigger matrices,
and update any nearby usages/expressions that assume byte to prevent implicit
truncation.
- Line 1277: Remove the leftover commented-out old code that calls
SEGENV.allocateData and uses FX_FALLBACK_STATIC; locate occurrences of the
commented line containing "SEGENV.allocateData(5 * sizeof(float) + width *
height)) FX_FALLBACK_STATIC" and the related commented lines and delete them so
the fixed implementation remains clean and no obsolete code is left in
user_fx.cpp (search for SEGENV.allocateData and FX_FALLBACK_STATIC to find all
instances).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 61a0d9c7-5bb0-44d1-843b-e4ee11aef293
📒 Files selected for processing (1)
usermods/user_fx/user_fx.cpp
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.
This PR adds the 2D Perlinscape effect into the new user_fx usermod instead of FX.cpp
Example videos:
80007897461__00E4D193-7401-4ED9-9F74-DBF3C8C172A7.mov
^-- Default settings and Rotation
IMG_6838.mov
^-- Using the Hult 64 palette
Untitled.1.mp4
^-- Hult 64 palette and mirrored X
Untitled.mp4
^-- Candy palette and Rotation
Summary by CodeRabbit