Improvements to pinwheel algorithm, should work on any size and should also be faster #4185
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -637,37 +637,31 @@ uint16_t IRAM_ATTR_YN Segment::nrOfVStrips() const { | |
|
|
||
| // Constants for mapping mode "Pinwheel" | ||
| #ifndef WLED_DISABLE_2D | ||
| constexpr int Fixed_Shift = 15; // fixpoint scaling factor (15bit for fraction) | ||
|
|
||
| // Pinwheel helper function: matrix dimensions to number of rays | ||
| static inline int getPinwheelSteps(int vW, int vH) { | ||
| int maxXY = max(vW, vH); | ||
| return maxXY * 5; // theoretical value sqrt(2) * pi (1 pixel step in the corner) Note: (maxXY * 9)/2 works on most sizes but not all, there is room for speed optimization here | ||
| } | ||
| // Pinwheel helper function: pixel index to radians | ||
| static float getPinwheelAngle(int i, int vW, int vH) { | ||
| int steps = getPinwheelSteps(vW, vH) - 1; // -1 to make the angle larger so the wheel has no gap | ||
| return float(i) * (DEG_TO_RAD * 360) / steps; | ||
| } | ||
|
|
||
| static void setPinwheelParameters(int i, int vW, int vH, unsigned& posx, unsigned& posy, int& inc_x, int& inc_y, int& maxX, int& maxY) { | ||
| float angleRad = getPinwheelAngle(i, vW, vH); // angle in radians | ||
| float cosVal = cos_t(angleRad); | ||
| float sinVal = sin_t(angleRad); | ||
| posx = (vW - 1) << (Fixed_Shift - 1); // X starting position = center (in fixed point) | ||
| posy = (vH - 1) << (Fixed_Shift - 1); // Y starting position = center (in fixed point) | ||
| inc_x = cosVal * (1 << Fixed_Shift); // X increment per step (fixed point) | ||
| inc_y = sinVal * (1 << Fixed_Shift); // Y increment per step (fixed point) | ||
|
There was a problem hiding this comment. Same comment as below - generally it's better to avoid shifting signed integers. I would propose to stay with my original implementation, using the scaling factor instead of shifting values around. You could even translate one into the other: There was a problem hiding this comment. Update: this page from Microsoft explains the problem: https://learn.microsoft.com/en-us/cpp/cpp/left-shift-and-right-shift-operators-input-and-output?view=msvc-170 I think that c++20 (the c++ language definition from 2020) finally ended this by stating "Right-shift on signed integral types is an arithmetic right shift, which performs sign-extension.". But for older compilers, I would still not rely on "what happens when I shift a negative value". Some more explanation is here: https://stackoverflow.com/questions/7522346/right-shift-and-signed-integer There was a problem hiding this comment. I read shifts easier than large numbers. There was a problem hiding this comment. Then maybe |
||
| inc_x = inc_x >> 1; // reduce to 50% to avoid pixel holes | ||
| inc_y = inc_y >> 1; // note: this increases the number of loops to be run but the loops are fast, it appears like a good tradeoff | ||
| maxX = vW << Fixed_Shift; // X edge in fixedpoint | ||
| maxY = vH << Fixed_Shift; // Y edge in fixedpoint | ||
| } | ||
| #endif | ||
|
|
||
|
|
@@ -689,7 +683,7 @@ uint16_t IRAM_ATTR Segment::virtualLength() const { | |
| vLen = sqrt16(vH*vH + vW*vW); // use diagonal | ||
| break; | ||
| case M12_sPinwheel: | ||
| vLen = getPinwheelSteps(vW, vH); | ||
| break; | ||
| default: | ||
| vLen = vW * vH; // use all pixels from segment | ||
|
|
@@ -763,47 +757,38 @@ void IRAM_ATTR_YN Segment::setPixelColor(int i, uint32_t col) | |
| for (int y = 0; y < i; y++) setPixelColorXY(i, y, col); | ||
| break; | ||
| case M12_sPinwheel: { | ||
| unsigned posx, posy; // unsigned so negative numbers overflow to > maxXY to save negative checking | ||
| int inc_x, inc_y, maxX, maxY; | ||
| setPinwheelParameters(i, vW, vH, posx, posy, inc_x, inc_y, maxX, maxY); | ||
| unsigned totalSteps = getPinwheelSteps(vW, vH); | ||
| static int pixelsdrawn; | ||
| /* | ||
| Note on the skipping algorithm: | ||
| - in the center, the angle is way too small for efficient drawing, pixels get overdrawn a lot, making it slow | ||
| - tracking the radius and deciding when to actually draw pixels significantly reduces the number of overdraws | ||
| - the number of angular steps required for a certain radius is in theory 2*pi*radius*sqrt(2) (worst case, in the corner) | ||
| - we can exploit that and only draw rays that are larger than a minimum step size (need to account for rounding errors) | ||
| */ | ||
| // avoid re-painting the same pixel | ||
| int lastX = INT_MIN; // impossible position | ||
| int lastY = INT_MIN; | ||
| unsigned currentR = 0; // current radius in "pixels" | ||
| // draw ray until we hit any edge | ||
| while ((posx < maxX) && (posy < maxY)) { | ||
| int x = posx >> Fixed_Shift; | ||
|
There was a problem hiding this comment. Did you check that shift works properly on signed integer? The reason I initially used multiplication and division is: shifting a signed integer is undefined (or compiler defined) behaviour - depending on architecture you might end with either an arithmetic shift (preserving sign) or with a logical shift (deleting sign). There was a problem hiding this comment. that is good info. I did notice a difference in negative and positive values (negative having less pixel holes) I will try with multiplications again. There was a problem hiding this comment. Sorry if I sounded a bit like "lecturing". It's a lesson I've learned some time ago, after spending days to understand why negative values suddenly changed sign..... There was a problem hiding this comment.
This could also be an issue with rounding - negative values "jump" to a different side when cutting away the fraction part. So possibly there are more "off by 1" cases for negative values. |
||
| int y = posy >> Fixed_Shift; | ||
| if (x != lastX || y != lastY) { // only paint if pixel position is different | ||
| currentR++; | ||
| int requiredsteps = currentR * 8; // empirically found value (R is a rough estimation, always >= actual R) | ||
|
There was a problem hiding this comment. @Brandon502 can you try with There was a problem hiding this comment. Tried 9, 10, and 12. I haven't seen any holes using 12 up to 64x64. There was a problem hiding this comment. the larger the number, the more overdraws and lower FPS, there are two more parameters to tweak, I tried a few options comparing holes vs FPS and current state was my best finding on 32x32 but did not check many other sizes |
||
| int skipsteps = totalSteps/requiredsteps; | ||
| if(!skipsteps || i % (skipsteps) == 0) // check if pixel would 'overdraw' | ||
| { | ||
| setPixelColorXY(x, y, col); | ||
| } | ||
| } | ||
| lastX = x; | ||
| lastY = y; | ||
| posx += inc_x; // advance to next position | ||
| posy += inc_y; | ||
| } | ||
| break; | ||
|
|
@@ -929,27 +914,17 @@ uint32_t IRAM_ATTR_YN Segment::getPixelColor(int i) const | |
| break; | ||
| case M12_sPinwheel: | ||
| // not 100% accurate, returns pixel at outer edge | ||
| unsigned posx, posy; // unsigned so negative numbers overflow to > maxXY to save negative checking | ||
| int inc_x, inc_y, maxX, maxY; | ||
| setPinwheelParameters(i, vW, vH, posx, posy, inc_x, inc_y, maxX, maxY); | ||
|
|
||
| // trace ray from center until we hit any edge - to avoid rounding problems, we use the same method as in setPixelColor | ||
| int x = INT_MIN; | ||
| int y = INT_MIN; | ||
| while ((posx < maxX) && (posy < maxY)) { | ||
| // scale down to integer (compiler will replace division with appropriate bitshift) -> not guaranteed | ||
| x = posx >> Fixed_Shift; | ||
| y = posy >> Fixed_Shift; | ||
| // advance to next position | ||
| posx += inc_x; | ||
| posy += inc_y; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Brandon502 you can tweak the 5 here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we learned when we were optimizing before was that Esp32 and S3 performed differently with rounding. I was able to have lower number of rays on my setup with a esp32 but @softhack007 had holes in his S3 setup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I remember.
Actually I could test with my 128x64 (hub75) -S3 setup once you have good parameters - just tell me where to tweak, and give me a few days (a lot of work@job coming this week).