FX: Waterfall, Matripix & Dissolve fix

[blazoncek]

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

Fixes #4416
Fixes #4401

[softhack007]

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]

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]

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]

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]

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.

[netmindz]

Can one of the reviewers please approve if this it ready to merge (and cherry pick to the release branch) ?

[softhack007]

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.

Yes I agree - while i don't find this solution is future-proof, its the only thing that we have right now.
Implementing local buffers i.e. setUpLeds() would be better imho, but it will definitely be a rework that has wider consequences.

So fine for me to merge this into the next release.

[netmindz]

There were some conflicts merging back to 0_15_0 so it might be best for you to check the fix is still correct @blazoncek

[blazoncek]

Looks ok.