Soap FX optimization #4543
Soap FX optimization #4543
Conversation
- saves ~500 bytes of flash - slight speed improvement
DedeHai
added
effect
optimization
|
Please use new lambda XY() as it simplifies review and prevents errors. |
|
it does, just not in the first commit ;) |
wled00/FX.cpp
Outdated
| } | ||
| CRGB PixelA = CRGB::Black; | ||
| if ((zD >= 0) && (zD < colrow)) PixelA = isRow ? SEGMENT.getPixelColorXY(zD, i) : SEGMENT.getPixelColorXY(i, zD); | ||
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[isRow ? (abs(zD)%cols) + i*cols : i + (abs(zD)%rows)*cols] * 3); |
There was a problem hiding this comment.
I was referring to this where XY() is used to get the index of array.
There was a problem hiding this comment.
I did that as a speed improvement, but can revert back.
- slight increase in code size, speed is the same but better readability.
- moved local variables into function - made coordinates an array - amplitude can now be changed by user (default setting is a slight increase to original which cannot be avoided without complicated logic or default slider setting)
|
I was thinking about this to improve readability: static void soapPixels(bool isRows, uint32_t *pixels, uint8_t *noise3d) {
const int cols = SEG_W;
const int rows = SEG_H;
const auto XY = [&](int x, int y) { return (x%cols) + (y%rows) * cols; };
const int hor = isRows ? rows : cols;
const int ver = isRows ? cols : rows;
const int amplitude = max(1, ((ver >= 16) ? (ver-8)/8 : 1) * (1 + SEGMENT.custom1) >> 6);
CRGBW ledsbuff[ver];
for (int y = 0; y < hor; y++) {
int amount = ((int)noise3d[isRows ? y*cols : y] - 128) * 2 * amplitude + (128 - SEGMENT.custom2)*2;
int delta = abs(amount) >> 8;
int fraction = abs(amount) & 255;
for (int x = 0; x < ver; x++) {
int zD;
int zF;
if (amount < 0) {
zD = x - delta;
zF = zD - 1;
} else {
zD = x + delta;
zF = zD + 1;
}
const int indxA = isRows ? XY(zD,y) : XY(y,zD);
const int indxB = isRows ? XY(zF,y) : XY(y,zF);
const int noisA = isRows ? XY(abs(zD),y) : XY(y,abs(zD));
const int noisB = isRows ? XY(abs(zF),y) : XY(y,abs(zF));
CRGB PixelA;
CRGB PixelB;
if ((zD >= 0) && (zD < ver)) PixelA = pixels[indxA];
else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[noisA]*3);
if ((zF >= 0) && (zF < ver)) PixelB = pixels[indxB];
else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[noisB]*3);
ledsbuff[x] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction)));
}
for (int x = 0; x < ver; x++) {
if (isRows) SEGMENT.setPixelColorXY(x, y, pixels[XY(x,y)] = ledsbuff[x]);
else SEGMENT.setPixelColorXY(y, x, pixels[XY(y,x)] = ledsbuff[x]);
}
}
}Where |
|
A local buffer would also help with speed, so your call. I think you should be able to push to this PR (or merge this and update later?) calling the variables hor/ver and the indices x/y even though they get flipped around (hor beeing actually vertical y coordinates and y actually being x direction if doing columns) may be more confusing than helpful. |
|
I hope I have not ruined it. This produced smallest footprint on my system. |
wled00/FX.cpp
Outdated
| const auto abs = [](int x) { return x<0 ? -x : x; }; | ||
| const int tRC = isRow ? rows : cols; // transpose if isRow | ||
| const int tCR = isRow ? cols : rows; // transpose if isRow | ||
| const int amplitude = 2 * ((tCR >= 16) ? (tCR-8) : 8) / (1 + ((255 - SEGMENT.custom1) >> 5)); |
There was a problem hiding this comment.
I had >> 5 in the beginning but found it matches less with the original look on default setting. if you don't mind that, this is better.
edit: I tested this and it does change the look as well as the original logic, on high custom1 values it looks very weird. maybe you can keep my logic using max(1, xxxxx) as it will match better with the initial
amplitude = (cols >= 16) ? (cols-8)/8 : 1;
There was a problem hiding this comment.
Go ahead, I did not have much time to compare now-and-then but from the top of my head this was "original" if custom1==0.
I would avoid max() or min() nd just try to find a sweet spot.
There was a problem hiding this comment.
I find this the best compromise:
const int amplitude = 1 + tCR / ((33 - SEGMENT.custom3) >> 1));And is close to original source from @St3p40 while using slider to boost amplitude.
It may require default value of 4-8 to be equal to original.
NOTE: changed custom1 to custom3 as there is no need for 255 values.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe update expands the functionality of FX.cpp by introducing a variety of new effect modes, including standard, audio-reactive, and 2D matrix-based displays. The changes involve adding new effect functions with associated metadata, reorganizing the effect data structures for improved performance and maintainability, and incorporating additional utility functions to support the new visualizations. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WS2812FX
participant EffectFunction
User->>WS2812FX: Select new effect mode
WS2812FX->>WS2812FX: Update effect function pointer based on selection
WS2812FX->>EffectFunction: Invoke selected effect function
EffectFunction-->>WS2812FX: Return computed LED pattern
WS2812FX-->>User: Update LED output with new pattern
✨ 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: 1
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
7523-7570: Improved mode_2Dsoap implementationThe mode_2Dsoap function has been enhanced with:
- Better memory allocation handling
- More efficient data structure usage
- Clearer variable naming
- Proper const correctness
The changes improve both performance and maintainability while preserving the original functionality.
Consider adding comments explaining the magic numbers used in the calculations:
// Scale factor for noise coordinates const uint32_t scale32_x = 160000U/cols; const uint32_t scale32_y = 160000U/rows;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🔇 Additional comments (5)
wled00/FX.cpp (5)
7571-7571: Updated mode metadata stringThe mode metadata string has been updated to reflect the new parameters and defaults.
7900-7901: Added new 2D effectsTwo new 2D effects have been added:
- Octopus effect
- Soap effect
The effects are properly integrated into the effects list and follow the established patterns.
7902-7903: Added Waving Cell effectThe Waving Cell effect has been added with proper metadata and implementation.
7904-7905: Added Akemi effect with audio supportThe Akemi effect has been added with audio visualization capabilities and proper metadata.
7906-7907: End of effect definitionsThe file properly closes the effect definitions section and maintains code organization.
wled00/FX.cpp
Outdated
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | ||
| // adapted for WLED by @blazoncek | ||
| // adapted for WLED by @blazoncek, optimization by @dedehai | ||
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | ||
| const int cols = SEG_W; | ||
| const int rows = SEG_H; | ||
| const auto XY = [&](int x, int y) { return x + y * cols; }; | ||
| const auto abs = [](int x) { return x<0 ? -x : x; }; | ||
| const int tRC = isRow ? rows : cols; // transpose if isRow | ||
| const int tCR = isRow ? cols : rows; // transpose if isRow | ||
| const int amplitude = 2 * ((tCR >= 16) ? (tCR-8) : 8) / (1 + ((255 - SEGMENT.custom1) >> 5)); | ||
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | ||
|
|
||
| CRGB ledsbuff[tCR]; | ||
|
|
||
| for (int i = 0; i < tRC; i++) { | ||
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | ||
| int delta = abs(amount) >> 8; | ||
| int fraction = abs(amount) & 255; | ||
| for (int j = 0; j < tCR; j++) { | ||
| int zD, zF; | ||
| if (amount < 0) { | ||
| zD = j - delta; | ||
| zF = zD - 1; | ||
| } else { | ||
| zD = j + delta; | ||
| zF = zD + 1; | ||
| } | ||
| int yA = abs(zD); | ||
| int yB = abs(zF); | ||
| int xA = i; | ||
| int xB = i; | ||
| if (isRow) { | ||
| std::swap(xA,yA); | ||
| std::swap(xB,yB); | ||
| } | ||
| const int indxA = XY(xA,yA); | ||
| const int indxB = XY(xB,yB); | ||
| CRGB PixelA; | ||
| CRGB PixelB; | ||
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | ||
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | ||
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | ||
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | ||
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | ||
| } | ||
| for (int j = 0; j < tCR; j++) { | ||
| CRGB c = ledsbuff[j]; | ||
| if (isRow) std::swap(j,i); | ||
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | ||
| if (isRow) std::swap(j,i); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Significant optimization of soapPixels function
The soapPixels function has been optimized and refactored to:
- Use a lambda function for XY coordinate calculation
- Add helper lambda for absolute value calculation
- Improve readability by using more descriptive variable names
- Reduce code duplication by extracting common calculations
- Use const references where possible to avoid copies
Consider further optimizing by:
- Using std::abs instead of custom abs lambda
- Making the function static since it's only used internally
- Using constexpr for compile-time constants
- const auto abs = [](int x) { return x<0 ? -x : x; };
+ using std::abs;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | |
| // adapted for WLED by @blazoncek | |
| // adapted for WLED by @blazoncek, optimization by @dedehai | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return x + y * cols; }; | |
| const auto abs = [](int x) { return x<0 ? -x : x; }; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = 2 * ((tCR >= 16) ? (tCR-8) : 8) / (1 + ((255 - SEGMENT.custom1) >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD); | |
| int yB = abs(zF); | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA,yA); | |
| std::swap(xB,yB); | |
| } | |
| const int indxA = XY(xA,yA); | |
| const int indxB = XY(xB,yB); | |
| CRGB PixelA; | |
| CRGB PixelB; | |
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | |
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | |
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | |
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | |
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j,i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | |
| if (isRow) std::swap(j,i); | |
| } | |
| } | |
| } | |
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | |
| // adapted for WLED by @blazoncek, optimization by @dedehai | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return x + y * cols; }; | |
| using std::abs; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = 2 * ((tCR >= 16) ? (tCR-8) : 8) / (1 + ((255 - SEGMENT.custom1) >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD); | |
| int yB = abs(zF); | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA, yA); | |
| std::swap(xB, yB); | |
| } | |
| const int indxA = XY(xA, yA); | |
| const int indxB = XY(xB, yB); | |
| CRGB PixelA; | |
| CRGB PixelB; | |
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | |
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA] * 3); | |
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | |
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB] * 3); | |
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j, i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i, j)] = c); | |
| if (isRow) std::swap(j, i); | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
abs lambda produces smaller code than std::abs
There was a problem hiding this comment.
@blazoncek Ah, thank you for the correction! I'll retract my suggestion about std::abs.
✏️ Learnings added
Learnt from: blazoncek
PR: wled-dev/WLED#4543
File: wled00/FX.cpp:7470-7522
Timestamp: 2025-02-10T06:05:54.253Z
Learning: In WLED, using a lambda for abs() function (e.g. [](int x){return x<0 ? -x : x;}) produces smaller code size compared to std::abs and is preferred for optimization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wled00/FX.cpp (3)
7524-7570: Improved mode_2Dsoap implementationThe mode_2Dsoap function has been optimized with:
- Better memory allocation handling
- Clearer variable naming
- Improved data structure organization
- Added const correctness
Consider further improvements:
- const size_t segSize = SEGMENT.width() * SEGMENT.height(); - const size_t dataSize = segSize * (sizeof(uint8_t) + sizeof(CRGB)); + // Pre-calculate sizes to avoid repeated multiplication + const size_t segSize = static_cast<size_t>(cols) * rows; + const size_t dataSize = segSize * (sizeof(uint8_t) + sizeof(CRGB)); - uint8_t *noise3d = reinterpret_cast<uint8_t*>(SEGENV.data); - CRGB *pixels = reinterpret_cast<CRGB*>(SEGENV.data + segSize * sizeof(uint8_t)); + // Use structured binding for cleaner data access + struct SoapData { + uint8_t noise3d[]; + CRGB pixels[]; + uint32_t coords[3]; + }; + auto* data = reinterpret_cast<SoapData*>(SEGENV.data);
7577-7629: Optimized mode_2Doctopus implementationThe mode_2Doctopus function has been improved with:
- Better memory management
- Cleaner coordinate calculations
- Improved palette handling
- Added const correctness
Consider adding:
+ // Pre-calculate common values + const int C_X = (cols / 2) + ((SEGMENT.custom1 - 128)*cols)/255; + const int C_Y = (rows / 2) + ((SEGMENT.custom2 - 128)*rows)/255; + const float t = strip.now * (SEGMENT.speed + 1) / 8.0f; + // Cache palette colors + CRGB colorCache[256]; + for (int i = 0; i < 256; i++) { + colorCache[i] = ColorFromPalette(SEGPALETTE, i); + }
7635-7656: Optimized mode_2Dwavingcell implementationThe function could be optimized by:
- uint32_t t = (strip.now*(SEGMENT.speed + 1))>>3; - uint32_t aX = SEGMENT.custom1/16 + 9; - uint32_t aY = SEGMENT.custom2/16 + 1; - uint32_t aZ = SEGMENT.custom3 + 1; + // Pre-calculate constants + const uint32_t t = (strip.now*(SEGMENT.speed + 1))>>3; + const uint32_t aX = SEGMENT.custom1/16 + 9; + const uint32_t aY = SEGMENT.custom2/16 + 1; + const uint32_t aZ = SEGMENT.custom3 + 1; + + // Cache palette colors + CRGB colorCache[256]; + for (int i = 0; i < 256; i++) { + colorCache[i] = ColorFromPalette(SEGPALETTE, i); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
📓 Learnings (1)
wled00/FX.cpp (1)
undefined
<retrieved_learning>
Learnt from: blazoncek
PR: #4543
File: wled00/FX.cpp:7470-7522
Timestamp: 2025-02-10T06:05:54.513Z
Learning: In WLED, using a lambda for abs() function (e.g. [](int x){return x<0 ? -x : x;}) produces smaller code size compared to std::abs and is preferred for optimization.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_8MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m_160)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
🔇 Additional comments (3)
wled00/FX.cpp (3)
7573-7574: Improved code documentationGood addition of source attribution and credit to original author.
7669-7686: Improved addEffect implementationGood implementation of the addEffect function with:
- Proper bounds checking
- Clear error handling
- Support for finding empty slots
- Protection against overwriting existing effects
7688-7907: Comprehensive effect registrationWell-organized setupEffectData function that:
- Properly initializes all effects
- Groups effects logically (1D, 2D, audio)
- Uses clear naming conventions
- Maintains proper ordering
wled00/FX.cpp
Outdated
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | ||
| // adapted for WLED by @blazoncek | ||
| // adapted for WLED by @blazoncek, optimization by @dedehai | ||
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | ||
| const int cols = SEG_W; | ||
| const int rows = SEG_H; | ||
| const auto XY = [&](int x, int y) { return (x%cols) + (y%rows) * cols; }; | ||
| const auto abs = [](int x) { return x<0 ? -x : x; }; | ||
| const int tRC = isRow ? rows : cols; // transpose if isRow | ||
| const int tCR = isRow ? cols : rows; // transpose if isRow | ||
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | ||
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | ||
|
|
||
| CRGB ledsbuff[tCR]; | ||
|
|
||
| for (int i = 0; i < tRC; i++) { | ||
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | ||
| int delta = abs(amount) >> 8; | ||
| int fraction = abs(amount) & 255; | ||
| for (int j = 0; j < tCR; j++) { | ||
| int zD, zF; | ||
| if (amount < 0) { | ||
| zD = j - delta; | ||
| zF = zD - 1; | ||
| } else { | ||
| zD = j + delta; | ||
| zF = zD + 1; | ||
| } | ||
| int yA = abs(zD); | ||
| int yB = abs(zF); | ||
| int xA = i; | ||
| int xB = i; | ||
| if (isRow) { | ||
| std::swap(xA,yA); | ||
| std::swap(xB,yB); | ||
| } | ||
| const int indxA = XY(xA,yA); | ||
| const int indxB = XY(xB,yB); | ||
| CRGB PixelA; | ||
| CRGB PixelB; | ||
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | ||
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | ||
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | ||
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | ||
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | ||
| } | ||
| for (int j = 0; j < tCR; j++) { | ||
| CRGB c = ledsbuff[j]; | ||
| if (isRow) std::swap(j,i); | ||
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | ||
| if (isRow) std::swap(j,i); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Significant optimization of soapPixels function
The soapPixels function has been optimized and refactored to:
- Use a lambda function for XY coordinate calculation
- Add helper lambda for absolute value calculation
- Improve readability by using more descriptive variable names
- Reduce code duplication by extracting common calculations
- Use const references where possible to avoid copies
The function could be further optimized by:
- Using a local buffer for intermediate results to avoid repeated pixel reads/writes
- Avoiding repeated XY calculations by caching results
- Reducing branching in the inner loop
Here's a suggested optimization:
- CRGB PixelA;
- CRGB PixelB;
- if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA];
- else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3);
- if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB];
- else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3);
- ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction)));
+ // Cache palette lookups
+ CRGB PixelA = (zD >= 0 && zD < tCR) ? pixels[indxA] : ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3);
+ CRGB PixelB = (zF >= 0 && zF < tCR) ? pixels[indxB] : ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3);
+
+ // Pre-calculate scale factors
+ uint8_t scaleA = ease8InOutApprox(255 - fraction);
+ uint8_t scaleB = ease8InOutApprox(fraction);
+
+ // Blend in one step
+ ledsbuff[j] = PixelA.nscale8(scaleA) + PixelB.nscale8(scaleB);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | |
| // adapted for WLED by @blazoncek | |
| // adapted for WLED by @blazoncek, optimization by @dedehai | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return (x%cols) + (y%rows) * cols; }; | |
| const auto abs = [](int x) { return x<0 ? -x : x; }; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD); | |
| int yB = abs(zF); | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA,yA); | |
| std::swap(xB,yB); | |
| } | |
| const int indxA = XY(xA,yA); | |
| const int indxB = XY(xB,yB); | |
| CRGB PixelA; | |
| CRGB PixelB; | |
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | |
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | |
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | |
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | |
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j,i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | |
| if (isRow) std::swap(j,i); | |
| } | |
| } | |
| } | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return (x % cols) + (y % rows) * cols; }; | |
| const auto abs = [](int x) { return x < 0 ? -x : x; }; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i * cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD); | |
| int yB = abs(zF); | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA, yA); | |
| std::swap(xB, yB); | |
| } | |
| const int indxA = XY(xA, yA); | |
| const int indxB = XY(xB, yB); | |
| // Cache palette lookups | |
| CRGB PixelA = (zD >= 0 && zD < tCR) ? pixels[indxA] : ColorFromPalette(SEGPALETTE, ~noise3d[indxA] * 3); | |
| CRGB PixelB = (zF >= 0 && zF < tCR) ? pixels[indxB] : ColorFromPalette(SEGPALETTE, ~noise3d[indxB] * 3); | |
| // Pre-calculate scale factors | |
| uint8_t scaleA = ease8InOutApprox(255 - fraction); | |
| uint8_t scaleB = ease8InOutApprox(fraction); | |
| // Blend in one step | |
| ledsbuff[j] = PixelA.nscale8(scaleA) + PixelB.nscale8(scaleB); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j, i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i, j)] = c); | |
| if (isRow) std::swap(j, i); | |
| } | |
| } | |
| } |
wled00/FX.cpp
Outdated
| @@ -7472,11 +7472,11 @@ static const char _data_FX_MODE_2DDISTORTIONWAVES[] PROGMEM = "Distortion Waves@ | |||
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |||
| const int cols = SEG_W; | |||
| const int rows = SEG_H; | |||
| const auto XY = [&](int x, int y) { return x + y * cols; }; | |||
| const auto XY = [&](int x, int y) { return (x%cols) + (y%rows) * cols; }; | |||
There was a problem hiding this comment.
I was too zealous with optimisations. Apparently. 😄
Though I saw glitches but couldn't pin-point where I introduced them.
To keep optimisation, could xA & xB be modulo-ed instead?
There was a problem hiding this comment.
probably (which is what I did in the orignal code, before bringing XY back) I will check.
edit:
I just threw this code in this morning before work to fix the appearance, needs more testing.
There was a problem hiding this comment.
This will do it:
int yA = abs(zD)%tCR;
int yB = abs(zF)%tCR;There was a problem hiding this comment.
Though I saw glitches but couldn't pin-point where I introduced them.
I saw the exact same glitches previously while optimizing but could not remember how I fixed them so I asked chatGPT to compare the two versions and the changed XY function popped up, which I had completely missed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
wled00/FX.cpp (1)
7573-7574: Add missing documentation for mode_2Dsoap parametersThe mode description string should include documentation for the custom parameters.
-static const char _data_FX_MODE_2DSOAP[] PROGMEM = "Soap@!,Smoothness,Density;;!;2;pal=11"; +static const char _data_FX_MODE_2DSOAP[] PROGMEM = "Soap@Speed,Smoothness,Density;;!;2;pal=11";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX.cpp(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
wled00/FX.cpp (1)
undefined
<retrieved_learning>
Learnt from: blazoncek
PR: #4543
File: wled00/FX.cpp:7470-7522
Timestamp: 2025-02-10T06:05:54.513Z
Learning: In WLED, using a lambda for abs() function (e.g. [](int x){return x<0 ? -x : x;}) produces smaller code size compared to std::abs and is preferred for optimization.
</retrieved_learning>
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- GitHub Check: wled_build / Build Enviornments (esp32s3_4M_qspi)
- GitHub Check: wled_build / Build Enviornments (esp32s3dev_16MB_opi)
- GitHub Check: wled_build / Build Enviornments (esp32c3dev)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp32dev_V4)
- GitHub Check: wled_build / Build Enviornments (esp32dev)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full_160)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
🔇 Additional comments (1)
wled00/FX.cpp (1)
7524-7570: LGTM! Well structured mode_2Dsoap functionThe mode_2Dsoap function is well organized with:
- Proper allocation checks
- Clear variable naming
- Efficient data structure reuse
- Good use of helper functions
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | ||
| // adapted for WLED by @blazoncek | ||
| // adapted for WLED by @blazoncek, optimization by @dedehai | ||
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | ||
| const int cols = SEG_W; | ||
| const int rows = SEG_H; | ||
| const auto XY = [&](int x, int y) { return x + y * cols; }; | ||
| const auto abs = [](int x) { return x<0 ? -x : x; }; | ||
| const int tRC = isRow ? rows : cols; // transpose if isRow | ||
| const int tCR = isRow ? cols : rows; // transpose if isRow | ||
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | ||
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | ||
|
|
||
| CRGB ledsbuff[tCR]; | ||
|
|
||
| for (int i = 0; i < tRC; i++) { | ||
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | ||
| int delta = abs(amount) >> 8; | ||
| int fraction = abs(amount) & 255; | ||
| for (int j = 0; j < tCR; j++) { | ||
| int zD, zF; | ||
| if (amount < 0) { | ||
| zD = j - delta; | ||
| zF = zD - 1; | ||
| } else { | ||
| zD = j + delta; | ||
| zF = zD + 1; | ||
| } | ||
| int yA = abs(zD)%tCR; | ||
| int yB = abs(zF)%tCR; | ||
| int xA = i; | ||
| int xB = i; | ||
| if (isRow) { | ||
| std::swap(xA,yA); | ||
| std::swap(xB,yB); | ||
| } | ||
| const int indxA = XY(xA,yA); | ||
| const int indxB = XY(xB,yB); | ||
| CRGB PixelA; | ||
| CRGB PixelB; | ||
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | ||
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | ||
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | ||
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | ||
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | ||
| } | ||
| for (int j = 0; j < tCR; j++) { | ||
| CRGB c = ledsbuff[j]; | ||
| if (isRow) std::swap(j,i); | ||
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | ||
| if (isRow) std::swap(j,i); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Significant optimization of soapPixels function
The soapPixels function has been optimized and refactored to:
- Use a lambda function for XY coordinate calculation
- Add helper lambda for absolute value calculation
- Improve readability by using more descriptive variable names
- Reduce code duplication by extracting common calculations
- Use const references where possible to avoid copies
The function could be further optimized by:
- Using a local buffer for intermediate results to avoid repeated pixel reads/writes
- Avoiding repeated XY calculations by caching results
- Reducing branching in the inner loop
Here's a suggested optimization:
- CRGB PixelA;
- CRGB PixelB;
- if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA];
- else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3);
- if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB];
- else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3);
- ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction)));
+ // Cache palette lookups
+ CRGB PixelA = (zD >= 0 && zD < tCR) ? pixels[indxA] : ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3);
+ CRGB PixelB = (zF >= 0 && zF < tCR) ? pixels[indxB] : ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3);
+
+ // Pre-calculate scale factors
+ uint8_t scaleA = ease8InOutApprox(255 - fraction);
+ uint8_t scaleB = ease8InOutApprox(fraction);
+
+ // Blend in one step
+ ledsbuff[j] = PixelA.nscale8(scaleA) + PixelB.nscale8(scaleB);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | |
| // adapted for WLED by @blazoncek | |
| // adapted for WLED by @blazoncek, optimization by @dedehai | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return x + y * cols; }; | |
| const auto abs = [](int x) { return x<0 ? -x : x; }; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i*cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD)%tCR; | |
| int yB = abs(zF)%tCR; | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA,yA); | |
| std::swap(xB,yB); | |
| } | |
| const int indxA = XY(xA,yA); | |
| const int indxB = XY(xB,yB); | |
| CRGB PixelA; | |
| CRGB PixelB; | |
| if ((zD >= 0) && (zD < tCR)) PixelA = pixels[indxA]; | |
| else PixelA = ColorFromPalette(SEGPALETTE, ~noise3d[indxA]*3); | |
| if ((zF >= 0) && (zF < tCR)) PixelB = pixels[indxB]; | |
| else PixelB = ColorFromPalette(SEGPALETTE, ~noise3d[indxB]*3); | |
| ledsbuff[j] = (PixelA.nscale8(ease8InOutApprox(255 - fraction))) + (PixelB.nscale8(ease8InOutApprox(fraction))); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j,i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i,j)] = c); | |
| if (isRow) std::swap(j,i); | |
| } | |
| } | |
| } | |
| //Idea from https://www.youtube.com/watch?v=DiHBgITrZck&ab_channel=StefanPetrick | |
| // adapted for WLED by @blazoncek, optimization by @dedehai | |
| static void soapPixels(bool isRow, uint8_t *noise3d, CRGB *pixels) { | |
| const int cols = SEG_W; | |
| const int rows = SEG_H; | |
| const auto XY = [&](int x, int y) { return x + y * cols; }; | |
| const auto abs = [](int x) { return x < 0 ? -x : x; }; | |
| const int tRC = isRow ? rows : cols; // transpose if isRow | |
| const int tCR = isRow ? cols : rows; // transpose if isRow | |
| const int amplitude = max(1, (tCR - 8) >> 3) * (1 + (SEGMENT.custom1 >> 5)); | |
| const int shift = 0; //(128 - SEGMENT.custom2)*2; | |
| CRGB ledsbuff[tCR]; | |
| for (int i = 0; i < tRC; i++) { | |
| int amount = ((int)noise3d[isRow ? i * cols : i] - 128) * amplitude + shift; // use first row/column: XY(0,i)/XY(i,0) | |
| int delta = abs(amount) >> 8; | |
| int fraction = abs(amount) & 255; | |
| for (int j = 0; j < tCR; j++) { | |
| int zD, zF; | |
| if (amount < 0) { | |
| zD = j - delta; | |
| zF = zD - 1; | |
| } else { | |
| zD = j + delta; | |
| zF = zD + 1; | |
| } | |
| int yA = abs(zD) % tCR; | |
| int yB = abs(zF) % tCR; | |
| int xA = i; | |
| int xB = i; | |
| if (isRow) { | |
| std::swap(xA, yA); | |
| std::swap(xB, yB); | |
| } | |
| const int indxA = XY(xA, yA); | |
| const int indxB = XY(xB, yB); | |
| // Cache palette lookups | |
| CRGB PixelA = (zD >= 0 && zD < tCR) ? pixels[indxA] : ColorFromPalette(SEGPALETTE, ~noise3d[indxA] * 3); | |
| CRGB PixelB = (zF >= 0 && zF < tCR) ? pixels[indxB] : ColorFromPalette(SEGPALETTE, ~noise3d[indxB] * 3); | |
| // Pre-calculate scale factors | |
| uint8_t scaleA = ease8InOutApprox(255 - fraction); | |
| uint8_t scaleB = ease8InOutApprox(fraction); | |
| // Blend in one step | |
| ledsbuff[j] = PixelA.nscale8(scaleA) + PixelB.nscale8(scaleB); | |
| } | |
| for (int j = 0; j < tCR; j++) { | |
| CRGB c = ledsbuff[j]; | |
| if (isRow) std::swap(j, i); | |
| SEGMENT.setPixelColorXY(i, j, pixels[XY(i, j)] = c); | |
| if (isRow) std::swap(j, i); | |
| } | |
| } | |
| } |
|
that AI needs to be put on a leash ;) that said, nothing more to add from my side, I briefly tested it on 32x32 and it looks good now. IMHO ready to merge. |
Summary by CodeRabbit