Pinwheel Rework#4551
Conversation
Optimized pinwheel algorithm. Math and memory optimizations by @DedeHai
WalkthroughThe pull request refactors the pinwheel effect logic within the WS2812FX implementation. It removes several constants previously used for effect dimensions and angles, updates the fixed scale constant from 512 to 16384, and introduces a new helper function to compute pinwheel parameters using Bresenham’s algorithm. Additionally, the pixel color setting method is updated to incorporate the new logic. Changes
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (2)
wled00/FX_fcn.cpp (2)
689-702: Validate scale-safety for angles and expansions.The calculations in
setPinwheelParameters()rely on(i + k) * baseAngle. Ifibecomes unexpectedly large, there could be potential overflow when multiplied bybaseAngle. Consider clamping or validating the range ofito ensure it cannot exceed the pinwheel length.
836-930: Potential stack usage concern and index checks.
uint16_t lineCoords[2][maxLineLength]; sincemaxLineLengthismax(vW, vH) + 2, large 2D matrices could pressure stack memory. A comment or safeguard to prevent overly large arrays might be warranted.- At lines 864-866,
closestEdgeIdx = min(closestEdgeIdx, idx-2);relies on an incrementedidxto be at least 2 after the first iteration. This logic is sound given the immediate increment, but consider an extra defensive check if future changes alter that loop’s indexing mechanism.- At line 900, there is a note “//!! should be long line idx!” clarifying which index to read from. Ensure that the commented hint matches intended usage (using
longLineIdxconsistently) or consider a minor refactor for clarity to avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(3 hunks)
⏰ 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 (lolin_s2_mini)
- GitHub Check: wled_build / Build Enviornments (esp32_eth)
- GitHub Check: wled_build / Build Enviornments (esp8266_2m)
🔇 Additional comments (3)
wled00/FX_fcn.cpp (3)
683-684: Good introduction of a higher fixed-point scale.The larger
Fixed_Scalevalue (16384) increases precision in trigonometric calculations for the pinwheel effect. This should reduce rounding artifacts for 2D transformations.
685-688: Ensure consistent usage of the pinwheel step size.The function
getPinwheelLength()always returns a multiple of 8. While this helps avoid over-drawing, ensure all code paths using pinwheel rays respect this multiple-of-8 strategy and do not silently assume a different step geometry.
1064-1077: Double-check edge tracing in getPixelColor.Using
setPinwheelParameters(i, vW, vH, x, y, cosVal, sinVal, true)to rotate half a ray width is a clever approach. However, the while loop at lines 1069-1071 stops once any edge is reached. Confirm that cases wherevWorvHis 1 or very small matrices still produce an expected result, as the logic could exit immediately with minimal coverage.
|
apart from that minor change request, approved from my side. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wled00/FX_fcn.cpp (1)
836-930: Excellent optimizations with minor improvement suggestions.The implementation is well-optimized with:
- Efficient line drawing using Bresenham's algorithm
- Smart block filling between lines
- Previous ray tracking to avoid redundant drawing
- Proper edge case handling
Consider these minor improvements:
- Use more descriptive variable names (e.g., change 'idx' to 'coordIndex')
- Add more detailed comments about block filling efficiency conditions
- for (int idx = 0; idx < lineLength[longLineIdx] * 2;) { //!! should be long line idx! + for (int coordIndex = 0; coordIndex < lineLength[longLineIdx] * 2;) { // iterate through coordinates (x,y pairs) - int x1 = lineCoords[0][idx]; - int x2 = lineCoords[1][idx++]; - int y1 = lineCoords[0][idx]; - int y2 = lineCoords[1][idx++]; + int x1 = lineCoords[0][coordIndex]; + int x2 = lineCoords[1][coordIndex++]; + int y1 = lineCoords[0][coordIndex]; + int y2 = lineCoords[1][coordIndex++];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wled00/FX_fcn.cpp(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: wled_build / Build Enviornments (esp32_wrover)
- 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 (lolin_s2_mini)
- 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 (esp8266_2m_compat)
- GitHub Check: wled_build / Build Enviornments (esp01_1m_full)
🔇 Additional comments (3)
wled00/FX_fcn.cpp (3)
683-683: LGTM! Improved fixed-point precision.The increase in Fixed_Scale from 512 to 16384 (2^14) provides better precision for fixed-point calculations while still fitting within a 16-bit integer.
685-688: LGTM! Efficient calculation of pinwheel rays.The helper function efficiently calculates the number of rays based on matrix dimensions and ensures it's a multiple of 8 using bitwise operations, which prevents over-drawing.
689-702: LGTM! Well-designed parameter calculation.The helper function effectively:
- Uses fixed-point arithmetic for better precision
- Incorporates cos16_t and sin16_t as requested
- Handles rotation for pixel color reading
3 participants
Previous discussions here: #4185
Pinwheel algorithm optimized. Math and memory optimizations by @DedeHai
May still have a few quirks in untested matrix sizes / effects.
Summary by CodeRabbit