Adding perlin noise replacement for fastled functions #4594
Conversation
needs a proper scaling analysis of all steps in all resolutions to minimize errors.
WalkthroughThe changes update the noise generation methods across multiple modules. In the audio reactive component and various LED effect modules, calls to Changes
Possibly related PRs
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
wled00/FX.cpp (2)
6224-6224: Minor suggestion regarding type usage.Storing
sizeof(float)in anunsignedworks, but typicallysize_tis used for sizes. This is non-blocking.
6648-6648: Consider clarifying the expression with parentheses.
strip.now * SEGMENT.speed/64 * SEGLEN/255can be read in multiple ways due to left-to-right associativity. Adding parentheses would help avoid confusion.wled00/fcn_declare.h (1)
517-525: Add brief usage notes for new Perlin noise prototypes.
These new function declarations look logically consistent and match the changes elsewhere in the PR.
However, consider documenting the parameter ranges and expected outputs (especially foris16bitin the_rawvariations) to help maintain clarity for future developers.wled00/util.cpp (5)
622-625: Add short overview of newly added Perlin functions.
The comment block reference is good, but consider briefly summarizing the main differences from typical noise functions, including performance rationale and approximate numeric range.
626-626: Prefer a compile-time constant over a macro.
Usingconstexpr int PERLIN_SHIFT = 1;instead of#definecan help with type safety and debugging.-#define PERLIN_SHIFT 1 +constexpr int PERLIN_SHIFT = 1;
637-645: Validate hashing approach for 1D gradient.
This approach looks correct for single-dimension Perlin. Ensure that the prime multipliers align well with your random/hashing goals, and confirm no collisions degrade the noise distribution.Would you like a test snippet or additional analysis to confirm uniform distribution?
664-669: Confirm numeric stability insmoothstep.
This simplified cubic polynomial is standard, but the bit shifts are quite tight. If you find any banding artifacts, consider minor adjustments or an alternate polynomial.
694-724:perlin2D_rawmatches 1D logic well.
Everything appears consistent. Check for any off-by-one or wrap-around issues if using extremely high x, y values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
usermods/audioreactive/audio_reactive.cpp(1 hunks)wled00/FX.cpp(30 hunks)wled00/fcn_declare.h(1 hunks)wled00/util.cpp(2 hunks)wled00/wled.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- wled00/wled.cpp
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: wled_build / Build Enviornments (usermods)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp32c3dev)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_160)
🔇 Additional comments (39)
usermods/audioreactive/audio_reactive.cpp (1)
872-872: Improved performance by replacing inoise8 with perlin8 for simulated audio.This change aligns with the PR's objective of replacing FastLED's inoise functions with faster Perlin noise implementations. For simulated microphone input, using perlin8() should provide better performance while maintaining similar random noise patterns needed for audio simulation.
wled00/FX.cpp (28)
2186-2186: Looks good.No significant concerns regarding shifting from
inoise8toperlin8.
2206-2206: Double-check potential wrap-around insin8_t(noise * 3).When multiplying
noiseby 3, the value can exceed 255 and wrap around. If wrap-around behavior is intentional, no change is needed. Otherwise, consider clamping or casting to ensure the result stays within the expected range.
2224-2224: Implementation appears consistent.Replacing
inoise16withperlin16is straightforward here.
2245-2245: No issues detected.Transition to
perlin16with a right shift for the final 8-bit value looks appropriate.
2260-2260: Confirm expected range for color palette index.Unlike other places where
perlin16(...) >> 8is used, this call passes the full 16-bit value intocolor_from_palette()without shifting. If that’s intentional, please verify that the function uses the entire 16-bit range as intended. Otherwise, consider shifting or casting.
4122-4122: Change is acceptable.Using
perlin8(...)/16formodValto randomize the wave is a fine adaptation.
4191-4191: Implementation looks correct.Noise-based index generation is consistent with the rest of the code.
4802-4802: No concerns here.
perlin16usage with mapped output aligns with the intended effect logic.
5060-5060: No issues noted.Simple two-argument
perlin8call looks fine.
5453-5457: Transition to newperlin8()calls is clear.These random offsets for generating mapped coordinates appear intentional.
5513-5513: Seems straightforward.
perlin8(x * scale, y * scale, strip.now / (16 - SEGMENT.speed/16))is consistent with other usage patterns.
5535-5538: Change is valid.Three-argument calls to
perlin8for separate axes are standard here.
5583-5583: Change looks good.New noise-based palette indexing is consistent with other segments.
5700-5701: Conversion logic is acceptable.Replacing
inoise8_rawwithperlin8and applying an offset/division is a normal approach for adjusting the noise range.
6238-6245: Verify float-to-int conversions in indexing.
abs8(u1 - j * sinus)suggests an implicit float → int cast. Confirm that truncation is intended before taking% cols. If so, explicitly cast to avoid ambiguity.
6394-6398: Implementation looks fine.Incorporation of
perlin8with audio reactivity handling is done consistently.
6476-6476: No issues identified.Simultaneous use of multiple Perlin inputs appears deliberate.
6498-6498: All good.Mirrors the gravity logic from the preceding chunk.
6620-6620: Looks correct.Noise-based indexing for sound-based expansions is consistent.
6679-6679: No concerns.Logic is coherent with other perlin8 usage.
7095-7095: Implementation is consistent.Noise-based location mapping for the new bin approach is fine.
7545-7545: Implementation is sound.Combining
perlin16 >> 8into 8-bit data is standard.
8050-8050: No concerns with applying wind force.Shifting the
perlin8(...)result by 7 after scaling is consistent with the rest of the code.
8059-8059: Change is appropriate.
perlin8()for turbulence is a standard approach in these effects.
8276-8277: Approved.Gravity-based randomization using Perlin noise is in line with the rest of this PR.
8348-8352: Potential overflow inint8_tfields.
baseheightand additionalperlin8calls can sum up to 510, exceeding the range ofint8_t. If both are up to 255, the result will overflow. Consider clamping or using a larger type, e.g.int16_t.Apply this diff if you want to cast and clamp:
- int8_t xslope = (baseheight + (int16_t)perlin8(xnoise - 10, ynoise, SEGENV.aux0)); - int8_t yslope = (baseheight + (int16_t)perlin8(xnoise, ynoise - 10, SEGENV.aux0)); + int16_t xslope = baseheight + perlin8(xnoise - 10, ynoise, SEGENV.aux0); + int16_t yslope = baseheight + perlin8(xnoise, ynoise - 10, SEGENV.aux0); + if (xslope < -128) xslope = -128; + else if (xslope > 127) xslope = 127; + if (yslope < -128) yslope = -128; + else if (yslope > 127) yslope = 127;
9720-9720: No issues present.Noise-based
xgravitylogic is consistent with the rest of the effect design.
10072-10072: Change is acceptable.Adding a randomized hue offset from Perlin noise is consistent with the approach in other modes.
wled00/util.cpp (10)
509-509: Confirm output range suitability.
Replacinginoise8withperlin8can subtly alter the output distribution. Ensure the resulting effect still aligns with the target audio visualization, as Perlin noise might differ slightly at extremes.
514-514: Validate consistency with previous visual/audio effects.
Similar to line 509, confirm that the changes introduced byperlin8preserve the intended effect behavior under different timing or input parameters.
628-635: Check for boundary cases inhashToGradient.
The function is concise and efficient. Performance seems strong for a small integer-based approach. Just make sure extreme corner cases (e.g. large input hash) don’t cause unexpected intermediate overflows.
647-653: Cross-check potential integer overflow ingradient2D.
The function accounts for two coordinates; it appears you are hashing them together with distinct prime factors. The logic is consistent with the 1D approach. No immediate issues seen.
655-662: Hash combination ingradient3D.
Using multiple prime-based shifts to incorporate x, y, z is a solid strategy. The scale factor in the return statement (line 661) looks correct for typical Perlin usage.
671-674: Check inline usage inlerpPerlin.
The linear interpolation approach is clear. Having it inlined helps performance. No immediate concerns.
676-692: Wrap logic inperlin1D_rawcarefully.
Theis16bitbranching is effective for controlling wrap-around in small coordinate spaces. Be sure all call sites pass the correct boolean and expect the unscaled raw range.
726-770:perlin3D_rawcomplexity.
The logic is more involved, but it follows the same approach as the 1D and 2D implementations. The large arithmetic is carefully bit-shifted. Keep an eye on performance if used in tight loops, but overall this should be acceptable.
772-783: Ensure scaling constants remain consistent inperlin16.
These scale factors (e.g.1159,1537,1731) appear carefully chosen. If you further tweak the hashing or random steps, re-verify these constants to preserve the approximate output range.
785-795: Double-check final offsets inperlin8.
After scaling down to 8 bits, the offset constants (e.g.+ 32769) ensure the range stays within 0–255. Encouraging thorough real-world testing to verify no unexpected boundary saturations occur.
|
I would have preferred if you'd use |
|
the final goal is to replace fastled, then it does not make sense to me to keep the legacy function names. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
wled00/FX.cpp (5)
2206-2206: Confirm shift-by-8 logic aligns with desired color range.
This usage discards the lower 8 bits from the 16-bit Perlin output. Ensure this is producing the intended intensity curve.
2224-2224: Avoid using magic number 4223 for Z-coordinate.
Consider extracting4223into a named constant for clarity.- unsigned noise = perlin16(real_x, 0, 4223) >> 8; + static const uint16_t PERLIN_Z_BASE = 4223; + unsigned noise = perlin16(real_x, 0, PERLIN_Z_BASE) >> 8;
4122-4122: Replace repeated expression(i*10 + i*10)withi*20.
For clarity, consider simplifying the multiplication.- if (moder == 1) modVal = (perlin8(i*10 + i*10) /16); + if (moder == 1) modVal = (perlin8(i*20) /16);
6399-6402: Add parentheses to clarify operator precedence.
Using parentheses can help ensure the correct order of multiplication and division, especially with(1 + SEGMENT.intensity/64)* perlin8(...) / 2.- unsigned thisVal = (1 + SEGMENT.intensity/64) * perlin8(i * 45 , t , t)/2; + unsigned thisVal = ((1 + (SEGMENT.intensity/64)) * perlin8(i * 45, t, t)) / 2;
6653-6653: Check arithmetic order when multiplying bySEGLEN/255.
Potential goals might be better served by careful ordering or casting.- unsigned index = perlin8(i*SEGMENT.speed/64, strip.now*SEGMENT.speed/64*SEGLEN/255); + unsigned index = perlin8((i*SEGMENT.speed)/64, (strip.now*SEGMENT.speed/64)*(SEGLEN/255));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wled00/FX.cpp(28 hunks)wled00/fcn_declare.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: wled_build / Build Enviornments (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (usermods)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32c3dev)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
- GitHub Check: wled_build / Build Enviornments (nodemcuv2)
🔇 Additional comments (27)
wled00/FX.cpp (23)
2186-2186: Check for potential overflow with large SEGLEN.
IfSEGLENis large, theni * SEGLENmight overflow the parameter's expected range. Verify if wrap-around is the intended effect.
2245-2245: Looks consistent with the other Perlin calls.
The shift-by-8 follows the established pattern of reducing 16-bit noise to 8-bit.
2260-2260: Check consistency with other>> 8cases.
Other code samples shift the 16-bit result down. Verify if retaining the full 16-bit range is intentional here.
4191-4191: Replacement ofinoise8withperlin8is straightforward.
Everything looks good here; logic and parameters remain consistent.
4802-4802: EnsureSEGMENT.speednever reaches 260 or higher.
Because(260 - SEGMENT.speed)is used in division, verify that it doesn’t become zero in corner cases.
5060-5060: No immediate issues with this change.
The switch frominoise8toperlin8appears consistent, and the indexing logic remains intact.
5453-5457: Double-check float vs integer usage inperlin8calls.
strip.now * speedis a float expression, butperlin8typically expects integer parameters. Confirm that the cast or truncation is acceptable.
5513-5513: Check integer division by(16 - SEGMENT.speed/16).
Ensure that(16 - SEGMENT.speed/16)never goes to zero and is used as intended when speed is high.
5535-5538: Potential risk for small denominators.
(256 - SEGMENT.speed)and similar expressions can become small. Confirm that it never becomes zero or negative.
5583-5583: Check_speedfor zero-safety.
(SEGENV.step / _speed)could divide by zero if_speedis 0. Verify_speedis guaranteed nonzero.
5700-5701: Verify the new scaling logic.
Previously, noise was halved. Now you subtract 127 and scale by/3. Ensure these changes still produce the intended “bump” effect.
6481-6481: Change frominoise8toperlin8is correctly implemented.
No issues spotted with parameter usage.
6503-6503: Change frominoise8toperlin8is correctly implemented.
No issues spotted with parameter usage.
6625-6625: Straightforward substitution toperlin8.
Calculation forvolumeSmth+ offsets remains consistent.
6684-6684: All good here.
Noise usage and palette application remain functionally consistent.
7100-7100: Looks fine for mapping.
Just ensure the mapped range [7500..58000] is correct for largeSEGLEN.
7550-7550: Use of>> 8is consistent with the other 16-bit noise calls.
No issues spotted in scaling or palette usage.
8055-8055: Centering noise around zero.
Casting toint16_tand subtracting 127 is a valid approach to get signed wind speed from noise.
8064-8064: Same pattern of re-centering noise.
Implementation looks consistent with the standard approach.
8281-8282: Gravity anchored by Perlin noise.
Logic is consistent; no issues spotted.
8353-8357: Particle forces from Perlin noise.
The usage of additional Perlin calls for slopes is stable.
9725-9725: Simple shift frominoise8toperlin8.
No concerns with subtracting 128 to anchorxgravity.
10077-10077: Beware of largemidsvalues.
When multiplied and then shifted by 9, the final hue increment could overflow ifmidsorperlin8output is high. Verify that’s acceptable.wled00/fcn_declare.h (4)
489-490: Good addition of FastLED compatibility aliases.The inclusion of these legacy aliases (
inoise8->perlin8andinoise16->perlin16) provides backward compatibility with existing code that uses FastLED's noise functions, making the transition smoother.
519-521: Well-structured raw Perlin noise function declarations.The raw Perlin noise function declarations provide a good foundation for the noise generation system. The
is16bitparameter allows for flexibility in output precision, and the 32-bit input parameters provide sufficient precision for complex noise patterns.
522-524: Good 16-bit Perlin noise API with multiple dimensionality options.The 16-bit Perlin noise functions provide higher precision output and support for 1D, 2D, and 3D noise generation, which offers flexibility for various visual effects.
525-527: Appropriate 8-bit Perlin noise functions with performance considerations.The 8-bit functions use 16-bit input parameters (as opposed to 32-bit in the 16-bit variants), which is a good balance between precision and performance for 8-bit output scenarios.
|
did anyone have a chance to test this? |
|
If you are wanting others to test, I think it would be helpful if you could add some notes as what/how to test Otherwise let's merge in and see if anyone complains 😜 |
|
that is what I was trying with the
part in the commit comment :) I mostly tested on 2D FX and those look fine to me. I was hoping someone could run it on a different setup with a different perspective as I may be biased from all the earlier tests. |
|
I would say the graphs are more accurate way to compare than looking at the effects and they look close enough to my eyes |
I created integer-math based perlin noise functions to replace the 8-bit optimized fastled inoise functions.
They execute about twice as fast and use no look-up tables. The functions are slightly different from the originals but I tried to match them as good as possible. Here is a comparison of the outputs:
saves about 2.7k of flash as a bonus.
These FX use perlin noise, please check if you can spot obvious differences. They are very subtle and I can only spot them in a direct side-by-side comparison.
Summary by CodeRabbit
New Features
Refactor
Chores