Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand1D Arc - No holes #171

Merged
merged 2 commits into from
Oct 17, 2024
Merged

Expand1D Arc - No holes #171

merged 2 commits into from
Oct 17, 2024

Conversation

Brandon502
Copy link

@Brandon502 Brandon502 commented Oct 16, 2024

Only matripix is not working correctly on arc. Effect starts at the end of the strip and use gPC to fill to beginning.
@softhack007
Copy link
Collaborator

@Brandon502 thanks for your PR 😃

Did you see a performance impact when including the corners?

@Brandon502
Copy link
Author

Brandon502 commented Oct 17, 2024

@Brandon502 thanks for your PR 😃

Did you see a performance impact when including the corners?

Performance is slightly worse since the big gap was filled, but nothing really noticeable. Here's some rough numbers from a HUB75 test on esp32.

Size Expand1D FPS
64x64 Arc no holes 50
64x64 Arc holes 60
64x32 Arc no holes 80
64x32 Arc holes 84
64x64 Arc no holes mirr/rev 50
64x64 Circle 33

Upstream v15 I believe works the same way, but doesn't include the rounding to fill the final corner pixels or gPC improvements.

The only effect that I have found that doesn't work due to gPC was matripix on most sizes. If this is approved would likely want to change the default expand1D for that effect to something besides arc.

@softhack007
Copy link
Collaborator

thanks, let's merge. I guess that @troyhacks will love this 👍

@softhack007 softhack007 merged commit 2ac354a into MoonModules:mdev Oct 17, 2024
36 checks passed
vLen = max(vW,vH); // get the longest dimension
break;
case M12_pArc:
vLen = sqrt16(vW * vW + vH * vH);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sqrt16 parameter will overflow if we go above 32767 pixels (182x182). But for the next few years we'll be safe - max_pixels is around 18k right now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24,576 pixels is my current record. 😁

@troyhacks
Copy link
Collaborator

thanks, let's merge. I guess that @troyhacks will love this 👍

Absolutely love this. Thanks, @Brandon502 - great contribution! 💯

troyhacks pushed a commit to troyhacks/WLED that referenced this pull request Oct 18, 2024
Expand1D Arc - No holes, and filling the complete panel
troyhacks pushed a commit to troyhacks/WLED that referenced this pull request Oct 18, 2024
Expand1D Arc - No holes, and filling the complete panel
@troyhacks
Copy link
Collaborator

@Brandon502 and @softhack007 - it seems there's a little weirdness with this on testing.

Corners.Pxl.20241018.041008767.mp4

The now-filled corners are "pulling away" too quickly - this is on my 192x96 matrix, so it might be less noticeable on a smaller setup?

@Brandon502
Copy link
Author

@Brandon502 and @softhack007 - it seems there's a little weirdness with this on testing.
Corners.Pxl.20241018.041008767.mp4

The now-filled corners are "pulling away" too quickly - this is on my 192x96 matrix, so it might be less noticeable on a smaller setup?

Seeing the same thing on 64x32, didn't notice this before. This is because of gPC not being accurate enough. I think I know a better way that will be more accurate. I'll have to try and test it tomorrow.

@troyhacks
Copy link
Collaborator

Another issue, works well in all situations except with just a mirror or mirror+reverse on X.

PXL_20241018_120010711.jpg

PXL_20241018_120135685.jpg

@Brandon502
Copy link
Author

Brandon502 commented Oct 18, 2024

@troyhacks Thanks for testing it out. I don't think I can add changes to this PR since it was merged, but here's a fix. Brandon502@32c9546 Likely a better way to do it, but I wasn't able to figure it out.
@softhack007 I tried storing vLen so it doesn't have to calculate sqrt so many times, but didn't see performance gain. Might help other chips though.

Edit: Combined with Brandon502@945312d noticed a missing pixel on certain ratios. Should be fixed now.

@Brandon502
Copy link
Author

@softhack007 is it a known bug or intended behavior that you can not have a single column/row when segmenting 2D? 2 wide works but 1 goes blank on Solid/other effects.

@softhack007
Copy link
Collaborator

@softhack007 is it a known bug or intended behavior that you can not have a single column/row when segmenting 2D

@Brandon502 that's a known bug currently - I had to use is2D() because sometimes single column effects crash - root cause still unknown.

The other known problem is that setting a "custom bus start" in led settings does not work. Some people try this to have the same output on several strips. I'll have to invent something here, because some fastpath optimizations assume that exactly one output led get used when setting a pixel on "bus" level.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 18, 2024

but here's a fix. Brandon502@32c9546
Likely a better way to do it, but I wasn't able to figure it out.

Thanks 🥇 I'll add that to mdev soon.

@softhack007 I tried storing vLen so it doesn't have to calculate sqrt calculate sqrt so many times, but didn't see performance gain. Might help other chips though.

Makes sense - actually a single "setPixelColor' still requires 500-1000 cpu cycles, so saving like 20 cycles for sqrt() won't make a noticeable difference.

Edit: Combined with Brandon502@945312d7ed9ac8716772497cbc490af9feaefd54 noticed a missing pixel on certain ratios

I think it's the same type of problem that we have with pinwheel - there is no exact 1:1 mapping between "virtual" and "physical" pixels so sometimes gPC cannot return exactly the pixel that was set.

I'll look into adding a pixel buffer for 1d2d mapping - this will cost us 1-2kb RAM at max, but we'll never have to worry about the problem again. And the buffer could live in PSRAM on -S3 and -S2.

@Brandon502
Copy link
Author

I think removing the round up part is what messed my first fix commit up. The round up usually just fills the corner pixel, but on certain ratios it is multiple pixels. Added it back in and made gPC default to return corner if the round up made it go off the matrix. Seems to fix every size and all gPC effects work. With Pinwheel a buffer definitely helps since gPC seems impossible to make perfect. I've been using my pinwheel2 branch for a while now and everything seems to be performing better and no more holes.

@softhack007
Copy link
Collaborator

softhack007 commented Oct 19, 2024

@Brandon502 @troyhacks I've committed the fix to mdev, please test.

@softhack007 I tried storing vLen so it doesn't have to calculate sqrt calculate sqrt so many times, but didn't see performance gain.

I've just added virtualLength() to the "fastpath" caching in Segment::startFrame().
It seems there was already another caching in place: SEGLEN (used in many effects) expands to strip._virtualSegmentLength. That could explain your observation that caching vLen does not improve performance.

@Brandon502
Copy link
Author

@softhack007 Looks like the speed up effect is still happening in the corners when not using mirror options. Changing line 1245 on gPC to float minradius = float(i) - 0.1f; instead of -.5f removes/reduces that noticeable jump and seems like all other functionality remains. Think this happened because I based gPC off drawArc and not the simple segement version.

@troyhacks
Copy link
Collaborator

troyhacks commented Oct 22, 2024

I'm still getting (small) corner gaps like the photos above in the same situations, but it's much improved.

Also at 192x96, so maybe not noticeable with smaller panels.

I can live with it - I mostly want full circles. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants