WLED icon indicating copy to clipboard operation
WLED copied to clipboard

FastLED CRGB[] (leds) support

Open Aircoookie opened this issue 2 years ago • 8 comments

Is your feature request related to a problem? Please describe. It is some effort for users to port effects written for the FastLED library to WLED.

Describe the solution you'd like Add support for using the CRGB leds[] array from within WLED effect functions, calling setPixelColor() and getPixelColor() automatically via [] operator override

Describe alternatives you've considered Current WLED effect code flavor and upcoming custom FX scripting does not make it easier to port FastLED effects.

Additional context https://stackoverflow.com/questions/37043078/c-overloading-array-operator

Aircoookie avatar May 08 '22 11:05 Aircoookie

leds[] array is in fact double-buffering. It can be global/permanent or local/effect specific (temporary).

Unfortunately most often leds[] represent RGB array stripping away W support WLED has.

Still, using leds[], global or local, can benefit effect porting since LED/RGB data is retained (losslessly) between invocations of the effect function which is it's most common use.

Actually there is no need to overload [] operator since we only need an index to leds[]. Currently most 2D implementations use XY() function that produces an index from X and Y coordinates of a pixel. This requires a knowlege of pixel position on a strip in relation to the layout of the matrix in case global leds[] is used which can be cumbersome. Using local leds[] removes that requirement but requires knowing segment dimensions (which are available since segment is a member class of a strip!). In such case XY() only needs to know width of a segment.

I have implemented 2D support for WLED that uses local leds[] array in a sample 2D effect bundled with it.

blazoncek avatar May 08 '22 14:05 blazoncek

Of course leds[] in its standard form is double buffering. This overload idea would allow for using leds[] without using double buffering or allocating extra memory. I think double buffering is essential to have, especially with 2D where you often have to limit brightness, but I'm not yet sure whether it should occur on the effect or bus level:

Effect level benefits:

  • Only use double buffering if effect requires it
  • Less convoluted to implement

Bus level benefits:

  • Less memory fragmentation
  • Most consistent, also allows for memory of e.g. live/Individual LED control if turned off or brightness adjusted
  • Can be user configured depending on available memory

Aircoookie avatar May 08 '22 14:05 Aircoookie

Sorry, I misunderstood the original post. 😄

Overloading leds[XY(x,y)] with a setPixelColor/getPixelColor is a nice idea. Though, rarely needed since leds[] serves its own purpose IMO. If using global leds[], strip.show() could do the setPixelColor and getPixelColor could be replaced to return leds[] (since it is used in handful effects). If using local leds[] a for() loop is needed to do setPixelColor at the end of effect function, which can be shortened as SEGMENT.show().

blazoncek avatar May 08 '22 16:05 blazoncek

I have been porting FastLED effects (25 currently) to WLED using dynamic leds[] array and it only takes 8 lines of code to make simple effects work.

blazoncek avatar May 19 '22 17:05 blazoncek

One more note regading global leds[].

WLED handles color in uint32_t whereas CRGB is effectively uint24_t and as such discarding/loosing W channel.

If leds[] is implemented as a global to allow benefits from above and to serve as a double buffer it will mean sacrificing W channel for portability. This may not be desirable in every circumstance.

The additional benefit of global leds[] may be (depending on effect implementation) support for overlapping segmens and blendinng their effects.

blazoncek avatar Jun 29 '22 06:06 blazoncek

@ewoudwijma has started work on this here.

So the recommendation would be to overload [] and =operators to support setPixelColor and getPixelColor

blazoncek avatar Aug 01 '22 09:08 blazoncek

Eventually it turned out we removed leds from effect functions all together and only have leds under the hood in sPC and gPC. So using fastled code into WLED requires changing to sPC and gPC. Good thing is gPC is not applying brightness on earlier sPC (due to leds array under the hood).

Question is if this is enough if we still need to add leds overloads? I personally think not as we keep code more straightforward by only allowing sPC and gPC. What do you think?

ewoudwijma avatar Aug 31 '22 10:08 ewoudwijma

Although I would prefer WLED way of writing effects (using setPixelColor() & getPixelColor()) most people familiar with FastLED will find it awkward. Unfortunately it is true that converting FastLED effect for WLED is not really straightforward and leds[] alone will not make it possible to port anything but simplest effects.

blazoncek avatar Sep 01 '22 08:09 blazoncek

Double buffering done in #3280

blazoncek avatar Aug 05 '23 20:08 blazoncek

@Aircoookie do we keep this open? I know setPixelColor/getPixelColoris not real leds[] substitute but IMO adding C++ overloaded operators will be too much work for too little effect as FastLED effects cannot all be directly ported to WLED.

blazoncek avatar Nov 16 '23 15:11 blazoncek

My opinion is, let's close it.

Even if we can use some tricks to implement an "array index" operator, it won't allow to do all the fancy things that are possible with FastLED leds[].

  • setPixelColor/getPixelColor has some performance penalties, whereas fastLEDs leds[] are the quickest way to do things
  • our sPC/gPC is lossy, unless you use global buffering
  • I doubt that we could support patterns like memmove( &(leds[4]), &(leds[48]), 42 * sizeof(leds[0]))

softhack007 avatar Nov 17 '23 20:11 softhack007

Just a thought: If we ditch global brightness then we no longer need to care about lossy pixel restoration. And we can play with pixel arrays directly.

blazoncek avatar Nov 17 '23 21:11 blazoncek