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

Problem with Arc option (1d expand FX) #4416

Open
1 task done
mitrokun opened this issue Dec 22, 2024 · 12 comments · May be fixed by #4428
Open
1 task done

Problem with Arc option (1d expand FX) #4416

mitrokun opened this issue Dec 22, 2024 · 12 comments · May be fixed by #4428
Labels

Comments

@mitrokun
Copy link

What happened?

After the 0.15 b4 update to release I got a bug for the 2D matrix.
The arc modifier stopped working adequately for some
sound effects (waterfall, matripix). The diodes light up only at the edge and there is no animation.
I tried a clean installation and installing the night version 0.16, but it did not help.
Can anyone confirm or deny the existence of the problem?

To Reproduce Bug

Enable the Waterfall effect on the 2D matrix (you can use sine instead mic) and compare the animation with "1d expand FX" modifier "corner" and "arc".

Expected Behavior

Animation on the entire matrix area for both options

Install Method

Binary from WLED.me

What version of WLED?

WLED 0.15.0 (build 2412100)

Which microcontroller/board are you seeing the problem on?

ESP32

Relevant log/trace output

No response

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mitrokun mitrokun added the bug label Dec 22, 2024
@mitrokun
Copy link
Author

doc_2024-12-22_18-27-50.mp4

@softhack007
Copy link
Collaborator

softhack007 commented Dec 22, 2024

@mitrokun I think this part of the video is your 2D layout, correct?

So the matrix you have is 15 x 13 pixels, and the segment "reverse X" option is enabled.

image

@softhack007
Copy link
Collaborator

@blazoncek it looks like a commit from you has last modified the "arc" logic e82f38e

any ideas?

@mitrokun
Copy link
Author

mitrokun commented Dec 22, 2024

I think this part of the video is your 2D layout, correct?

yes
image

the segment "reverse X" option is enabled.

You can use any combination of "mirror" and "reverse" settings, the problem still remains. The initial position changes, but the arc waves are missing

@blazoncek
Copy link
Collaborator

any ideas?

Yes.

Reverse is at issue here. Why? Because Waterfall (and some other effects) use getPixelColor() for shifting pixels. As Arc is expanding in circular fashion it is not well suited for rectangular matrices as you have to deal with corners somehow. If you want to fill corner in a circular fashion it needs to be encompased within drawing range (and we know that from Pitagora c^2 = a^2 b^2). But when a (or b) is larger than W (or H) it falls out of draing range.

The change you pointed fixed a complaint that Arc was not filling entire matrix, i.e. corners.

If you want to fix this issue you can either reduce Arc expansion range (and then have reopened original issue when Arc was not filling entire matrix/segment) or fix forementioned effect(s).

@softhack007
Copy link
Collaborator

softhack007 commented Dec 23, 2024

@blazoncek thanks, yes I agree it's most likely the "shifting" (combined with reverse) that does not work in arc with this very small 2D resolution. So basicially it's the same problem that we are fighting in the "pinwheel" expansion - exact getPixelColor is not working sometimes because the reference pixel might actually overlap with other pixels.

I think the best solution would be to add a dedicated pixel buffer for arc and pinwheel, so getPixelColor could retrieve the exact color that was previously sef with setPixelColor.

@blazoncek
Copy link
Collaborator

There is another fix possible which will consume a bit more resources. That is - getting pixel at x,y coordinates instead of x,0 (or y,0).
The problem with missing pixels (gaps or ledmap with -1) will still remain though.
BTW pinwheel has its own issues.

@blazoncek
Copy link
Collaborator

I think the best solution would be to add a dedicated pixel buffer for arc and pinwheel, so getPixelColor could retrieve the exact color that was previously sef with setPixelColor.

I don't think so. That would add too much complexity.
The simplest solution was already in-place way back in first versions of 0.14 - setUpLeds(). Any effect that relies on previous pixel values should allocate its own buffer to maintain those values independent of pixel mapping (gaps or expansion). To (try to) reduce memory fragmentation an if could be added to check if gaps/ledmaps are present or expansion takes place.

I can prepare such PR if you wish.

@softhack007
Copy link
Collaborator

softhack007 commented Dec 23, 2024

That would add too much complexity.
The simplest solution was already in-place way back in first versions of 0.14 - setUpLeds().

I see your point - and a setupLeds() would also be necessary to further improve drawing speed - I was thinking (for long time) that the complications added by segment level buffering are not worth it. However the truth is, today each pixel has to "go down the rabbit hole" until we deliver it at the bus driver level, and each gPC requires the same deep dive.

Experience from MM - and also from particle effects by @DedeHai - is that we can expect around 30% of speedup, plus we will never again need to worry about gaps that are not mapped to any physical pixels.

Maybe setupLeds() 2.0 should be aware of 1D2D mapping, and create a buffer of SEGLEN pixels as this will also help when using 1D2D expand (for any expand that does not map 1:1 to 2d pixels).

Let's continue the topic after Xmas days 👍 (grandpa currently has higher duties 🎅 )

@blazoncek
Copy link
Collaborator

Maybe setupLeds() 2.0 should be aware of 1D2D mapping, and create a buffer of SEGLEN pixels as this will also help when using 1D2D expand (for any expand that does not map 1:1 to 2d pixels).

It does not need to be (explicitly) as virtualLength() deals with that.
The other option is to use effect data buffer (as it was designed for this purpose). Unfortunate consequence may be heap exhaustion in extreme cases or not big enough buffer.

And yes, triple buffering has always been an option. Expensive, though.

@blazoncek
Copy link
Collaborator

@mitrokun while you are at it, please check other effect if anyone else also exhibits similar issue.

BTW I have fix for Waterfall ready.

@mitrokun
Copy link
Author

mitrokun commented Dec 24, 2024

BTW I have fix for Waterfall ready.

Tnx

check other effect

Ок.
Surprisingly, the problem is clearly visible only on the two effects I mentioned at the beginning - waterfall and matripix. And both effects were in my presets🫡.

@blazoncek blazoncek linked a pull request Dec 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants