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

FX: Waterfall, Matripix & Dissolve fix #4428

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

blazoncek
Copy link
Collaborator

@blazoncek blazoncek commented Dec 25, 2024

  • for Arc expansion
  • or gaps
  • or ledmaps with missing pixels

Fixes #4416
Fixes #4401

@blazoncek blazoncek changed the title FX: Waterfall and Matripix fix FX: Waterfall, Matripix & Dissolve fix Dec 25, 2024
@netmindz netmindz added this to the 0.15.1 candidate milestone Dec 26, 2024
@netmindz netmindz added the bug label Dec 26, 2024
@softhack007
Copy link
Collaborator

softhack007 commented Dec 27, 2024

I'm not sure that this is a good approach for fixing the problem.

Its basicially a way to implement setUpLeds() without implementing it ;-) many audioreactive effects are doing a "shift left" or "shift out", and I guess it's just a matter of time until the next user comes up with a combination of matrix dimensions and 2D expand mode where the same fix would be needed in another effect.

  • duplication of code
  • the fix would need to be removed again once we implement segment level LEDs buffering
  • 8266 might run into memory problems (yeah it would also happen with a real "setUpLeds()" implemented in the segment class)
  • it's not strictly necessary to use the memory book keeping of SEGENV.allocateData(dataSize) for this purpose.

On the positive side,

  • this proposed fix shows a way to solve the problem in a generic manner. Maybe we should still separate out the buffer allocation/reallocation into a separate function segment::setUpLedsV2() and add a user option to disabled segment buffers - in case that memory gets tight. getpixelcolor/ setPixelColor could detect that a buffer was allocated, and handle buffer operations transparently - so effect authors just need to add SEGENV.setUpLedsV2() nothing more.
  • the fix will work with arbitrary ledMaps and even with gapMaps.

@blazoncek
Copy link
Collaborator Author

Well, do as you please. 🤷‍♂️ I proposed a solution that works, has least impact and adds very little to code size.

If you call pixels[] code duplication, well, you have to store values somewhere. gPC() seems a waste of resources in such case, setUpLeds() or not. This is by far the fastest approach (as data is only allocated once ad will only grow if needed).

Tested on ESP8266 with up to 512 pixels (which is maximum recomended for it).

@blazoncek
Copy link
Collaborator Author

Oh, one more thing. allocateData() is intended fot that particular purpose - persistent effect data - no matter in what form that data is. Pixels or otherwise.

setUpLeds() was used because audio effects were mostly ported FastLED effects that barely fit into WLED framework (and hence the issues we cope with now, remember Matrix?) and relied on leds[] array. leds[] did not know of ledmaps or gaps. But we got rid of leds[] since then (for good or bad).

IMO any effect that needs persistent data (pixels or other) should utilise allocateData() and use well known fallback if running out of memory (we are under a lot less pressure since @willmmiles optimised AWS and other stuff). And, yes I know it, that would require updating some effects.

There is a getPixelColor() on the NPB roadmap that will return unmodified value but that will still not solve gaps in LED layout.

@DedeHai
Copy link
Collaborator

DedeHai commented Dec 28, 2024

IMHO this is a good fix for the issue at hand. Implementing local buffers i.e. setUpLeds() (which I am not familar with) sounds like something that requires a lot of code-rework and out of scope for the moment.

@blazoncek
Copy link
Collaborator Author

FYI there are several more effects that will not work correctly with gaps (and/or 2D expansion).

@DedeHai I invented setUpLeds() in one of the early prototypes of 0.14 when WLED still relied only on NPB getPixelColor() to retrieve previous frame's pixel values. gPC was not accurate and lead to color distortion at lower brightness. Audio effects (from SR fork) were written using FastLED and relied heavily on FastLED's leds[] array which did not exist in AC. With setUpLeds() a local approximation was created so code like leds[x]=leds[x-1] could work as expected. Unfortunately RAM was scarce and code still needed to be modified so in later versions (after MM split off) me and @Aircoookie modified that to use global buffer allocated on bus level. This change was never ported to MM.

Unfortunately bus knows nothing about gaps (as it is working on physical layer) and hence some effects can't behave properly if they rely on previous pixel values that need to be moved around.
Handling gaps needs to happen on the layer where they are introduced (strip) or higher up. Handling them in effect function works fastest but requires the most amount of RAM. It also introduces issues when you try to blend/overlap two segments. I have written elsewhere that effects have to be rewritten if they are to support overlapping or the change needs to go into strip logic to handle overlapping segments. The latter may prove challenging.

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