FX improvements and cleanup #4145

DedeHai · 2024-09-15T07:45:36Z

This has been on my todo list for a long time, finally done.
It adds some new functionality to 2D FX (blurring / smearing / palette support) and saves on code size (merged 1D FX)
Note: Polar Lights is now slightly different as no palette matches the original effect palette

Waving Cell: Improved with higher temporal resolution (smoother at lower speeds) and added additional mode setting and optional blurring
Merged puddles and puddlepeak
Merged Gravcenter, Gravcentric, Gravfreq and Gravimeter (saves 1.2k of flash)
Merged meteor and meteor smooth
Renamed police_base into mode_two_dots as that was just an alias
Changed default of Palette effect: Animate rotation is now unchecked by default (it looks weird and confusing in 1D as a default)
Added or enhanced blurring of
- DNA
- DNA Spiral
- Drift
- Drift Rose
- Crazy Bees
- Ripple
- Colored Bursts
- Frizzles
- Lissajous
- Sindots
- Spaceships
Added palette support to
- Crazy Bees
- Polar Lights
- Drift Rose
Some code cleanup (removed unused / commented stuff)
Moved dev info for AR to the top so ist easier to find as a reference, also added link to KB there

- Waving Cell: Improved with higher temporal resolution (smoother at lower speeds) and added additional mode setting and optional blurring - Merged puddles and puddlepeak - Merged Gravcenter, Gravcentric, Gravfreq and Gravimeter (saves 1.2k of flash) - Merged meteor and meteor smooth - Renamed police_base into mode_two_dots as that was just an alias - Changed default of Palette effect: Animate rotation is now unchecked by default (it looks weird and confusing in 1D as a default) - Added or enhanced blurring of - DNA - DNA Spiral - Drift - Drift Rose - Crazy Bees - Ripple - Colored Bursts - Frizzles - Lissajous - Sindots - Spaceships - Added palette support to - Crazy Bees - Polar Lights - Drift Rose - Some code cleanup (removed unused / commented stuff) - Renamed police_base into mode_two_dots (was just an alias not used by anything else) - Moved dev info for AR to the top so ist easier to find as a reference, also added link to KB there

blazoncek · 2024-09-15T12:51:53Z

@ewoudwijma @netmindz would you care to take a look as some effects were created by you.

Changed default of Palette effect: Animate rotation is now unchecked by default (it looks weird and confusing in 1D as a default)

This will need to be checked with @TripleWhy

blazoncek

I do welcome the changes and see great value in optimisations, but...

Please avoid breaking existing default behaviour of effects unless strictly necessary. It is not all about speed when you have 10.000+ users.
There are also some trailing spaces and formatting glitches to be fixed.

wled00/FX.cpp

blazoncek · 2024-09-16T05:15:04Z

wled00/FX.cpp

@@ -5021,10 +5024,11 @@ uint16_t mode_2DDNASpiral() {               // By: ldirko  https://editor.soulma
      SEGMENT.setPixelColorXY(x1, i, WHITE);
    }
  }
+  SEGMENT.blur(SEGMENT.custom1 >> (!SEGMENT.check1), SEGMENT.check1); 


This will cause different looking effect on existing presets. Please consider backwards compatibility.

in FX where I added blur, the default slider position is set to 0, checks are set to 0.
that will change behaviour on users that have 'default fx parameters' unused. if that is a restriction, none of the FX can be extended (ever).

blazoncek · 2024-09-16T05:15:41Z

wled00/FX.cpp

@@ -5048,13 +5052,13 @@ uint16_t mode_2DDrift() {              // By: Stepko   https://editor.soulmateli
    int mySin = sin_t(angle) * i;
    int myCos = cos_t(angle) * i;
    SEGMENT.setPixelColorXY(colsCenter + mySin, rowsCenter + myCos, ColorFromPalette(SEGPALETTE, (i * 20) + t_20, 255, LINEARBLEND));
-    if (SEGMENT.check1) SEGMENT.setPixelColorXY(colsCenter + myCos, rowsCenter + mySin, ColorFromPalette(SEGPALETTE, (i * 20) + t_20, 255, LINEARBLEND));
+    if (SEGMENT.check2) SEGMENT.setPixelColorXY(colsCenter + myCos, rowsCenter + mySin, ColorFromPalette(SEGPALETTE, (i * 20) + t_20, 255, LINEARBLEND));


This will cause different looking effect on existing presets. Please consider backwards compatibility.

blazoncek · 2024-09-16T05:17:04Z

wled00/FX.cpp

@@ -5100,17 +5104,17 @@ uint16_t mode_2DFrizzles(void) {                 // By: Stepko https://editor.so
  const int cols = SEGMENT.virtualWidth();
  const int rows = SEGMENT.virtualHeight();

-  SEGMENT.fadeToBlackBy(16);
+  SEGMENT.fadeToBlackBy(18);


This will cause different looking effectand may case playlists to no longer function as intended. Please consider backwards compatibility.

wled00/FX.cpp

blazoncek · 2024-09-16T05:21:53Z

wled00/FX.cpp

-      SEGMENT.addPixelColorXY(bee[i].aimX, bee[i].aimY + 1, CHSV(bee[i].hue, 255, 255));
-      SEGMENT.addPixelColorXY(bee[i].aimX - 1, bee[i].aimY, CHSV(bee[i].hue, 255, 255));
-      SEGMENT.addPixelColorXY(bee[i].aimX, bee[i].aimY - 1, CHSV(bee[i].hue, 255, 255));
+      CRGB flowerCcolor = ColorFromPalette(SEGPALETTE,bee[i].hue);


Why not use SEGMENT.color_from_palette() to keep using WLED native pixels and behaviour?

Regarding ColorFromPalette() and SEGMENT.color_from_palette().

The former was used in FastLED effects ported to WLED with minimal effort (or to keep changes simple). Unfortunately this does not adhere to WLED's "default" behaviour for palettes and color, unless specifically crafted within effect. So I would prefer to use SEGMENT.color_from_palette() wherever possible if there are changes to an effect function.

generally agreed.
color_from_palette does some additional checks and calculations, plus converts to 32bits, then FX may convert it back to CRGB, then setPixelColor converts it again to 32bit. That makes FX slow that call this for every pixel so I don't see it as a good solution. I have some basic ideas on how to fix this with CRGBW (basically overload some functions to accept RGB and RGBW and calculate the fastest way possible).
Once there is a good solution, this will be a simple search and replace but that will have to wait for round2 cleanup.

then FX may convert it back to CRGB,

This (and other manipulations like that) will need to be removed or converted. color_from_palette() is there for a reason, to ensure consistent behaviour of "Default" options.

wled00/FX.cpp

DedeHai · 2024-09-16T05:43:27Z

I did not consider backward compatibility to be so important but you are right, will revert default behaviour back to what it was before.
edit:
should meteor_smooth be kept as an alias or is removing it ok?

blazoncek · 2024-09-16T06:13:03Z

should meteor_smooth be kept as an alias or is removing it ok

As we removed some effects in 0.13 and then again in 0.14 I think we can do that again in favour of the possibility to finally merge in PS effects. I would really like to see those (at least 2D) in.

wled00/FX.cpp

TripleWhy · 2024-09-15T14:21:08Z

wled00/FX.cpp

  unsigned width = ((SEGLEN*(SEGMENT.intensity+1))>>9); //max width is half the strip
  if (!width) width = 1;
+  if (!SEGMENT.check2) SEGMENT.fill(SEGCOLOR(2));
+  uint32_t color1 = SEGCOLOR(0);
+  uint32_t color2 = (SEGCOLOR(1) == SEGCOLOR(2)) ? color1 : SEGCOLOR(1);


Does anyone else find this logic strange? ^^

Not a change introduced this PR, but I never noticed that before. I would prefer a more straightforward approach, even if that may break a few existing configurations

Legacy.
If colors 2 & 3 match, use color 1 for both "dots". Color 3 is background. Color 3 (AKA background) can be omitted if "Overlay" is selected.
Unfortunately a poor choice of variable names if that is what you are referring to. 😁

wled00/FX.cpp

TripleWhy · 2024-09-16T07:50:07Z

wled00/FX.cpp

+// Gravcenter effects By Andrew Tuline.
+// Gravcenter base function for Gravcenter (0), Gravcentric (1), Gravimeter (2), Gravfreq (3) (merged by @dedehai)
+
+uint16_t mode_gravcenter_base(unsigned mode) {               


After having read through this function now:

While I do appreciate an effort to generalize things, is that really the best approach for these effects? How much of the code is actually shared in this new version. Not that much if we are honest. So I don't think this particular approach makes too much sense in this case.

An alternative approach could be to move the portions of the code that are actually shared/generalizable into gravcenter helper functions, then call those from the specific functions. Less interleaving, no runtime checks, clear readable code path from top to bottom.

well, merging them like this saves 1.2k of flash so actually it is quite some shared code.
while I agree this could be made prettier I don't think seperating it in a different way would improve the readability.
the runtime checks are negligible, that is only done once per frame.

wled00/FX.cpp

- reverted blurring to old values (where applicable) - added 'Traffic Light' palette (originally defined in Polar Lights FX) - Bugfix in Meteor: starting point was not the same before merge - made blurring in ripple_base a passing argument instead of a bool - Code cleanup

netmindz · 2024-09-17T09:21:33Z

@ewoudwijma @netmindz would you care to take a look as some effects were created by you.

Changed default of Palette effect: Animate rotation is now unchecked by default (it looks weird and confusing in 1D as a default)

This will need to be checked with @TripleWhy

I don't think any of those were added by me, but I can try and take a look at this PR in the next few days

wled00/FX.cpp

TripleWhy · 2024-09-17T10:42:20Z

wled00/FX.cpp


  return FRAMETIME;
 } // mode_2DSindots()
-static const char _data_FX_MODE_2DSINDOTS[] PROGMEM = "Sindots@!,Dot distance,Fade rate,Blur;;!;2;c2=64";
+static const char _data_FX_MODE_2DSINDOTS[] PROGMEM = "Sindots@!,Dot distance,Fade rate,Blur,,Extra Blur;;!;2;";


call it smear maybe? that would be the same as in other effects where you added it

I am not happy with that name either... but smear is a different behaviour (it blurs without fading underlaying colors, causing a motion blurring or 'smear'). Well screw it, I will just update it to use smear (did not succeed in that before but I have an Idea)

uses fire palette by default, looks is 95% identical. also the /cols should be /rows or it will look different depending on matrix dimensions (brightness changes, also there are weird lines if width != height)

no penalty in size of `Segment` struct (if it is kept just at the place I added it, compiler does not optimize struct alignment order and just adds padding if not aligned, there it falls into a padding)

checking 'gradient' applies the selected palette as a gradient, default palette still does color gradient.

DedeHai · 2024-09-19T18:10:54Z

last two commits (pal0 override and palette gradient for scrolling text) are just suggestions for review/comment.

blazoncek · 2024-09-20T18:55:10Z

wled00/FX_fcn.cpp

@@ -213,24 +213,12 @@ CRGBPalette16 &Segment::loadPalette(CRGBPalette16 &targetPalette, uint8_t pal) {
  if (pal < 245 && pal > GRADIENT_PALETTE_COUNT+13) pal = 0;
  if (pal > 245 && (strip.customPalettes.size() == 0 || 255U-pal > strip.customPalettes.size()-1)) pal = 0; // TODO remove strip dependency by moving customPalettes out of strip
  //default palette. Differs depending on effect
-  if (pal == 0) switch (mode) {


The unfortunate consequence of this removal is that when used from existing preset, there will be no "default" palette to choose from. So, unless user chooses the effect in UI (with defined "default" palette from metadata) it will not have "default" palette defined. Once user selects it from UI the palette will be defined.

This is then inconsistent behaviour.

the default palette gets set when FX is loaded.
I don't fully understand in what scenario the behaviour would differ (with the latest bugfix). Can you check latest commit changes and give me a description of a test case I can try?

Take, for example, effect "Fire 2012". Its default platte is "Fire" (35) even if user selects palette "Default" (0).

If I have an existing preset with Fire 2012 it will most likely have ”pal”:0 as its selected palette. But after removal of this code this will mean "Party" palette until user manually selects Fire 2012 from UI which will populate its default palette to 35.

Steps to reproduce:

use 0.14 to create preset of Fire 2012 with default values (i.e. palette chosen is Default)

update to this PR

restart ESP

select preset with Fire 2012 effect

it will display with Party colors instead of Fire

select another preset or effect

now select Fire 2012 in Effects tab, palette will change to Fire

select another preset or effect

select preset with Fire 2012

it will display with Fire palette.

Got it thx, I will try to fix this.

ok, fixed it. can confirm your test scenario now works.

- _default_palette now always gets properly set to party colors (in that case the FX does not have a 'pal' argument and sOpt is -1) - added initialization to zero for _default_palette (reason to crash)

- 'pal' FX option is always extracted when chaning an FX, _default_palette variable is thus always set in setMode()

blazoncek

This is now a little better but still does not cover HTTP API.
FYI palette with ID 255 is first custom palette.

wled00/FX_fcn.cpp

DedeHai · 2024-09-22T10:46:09Z

This is now a little better but still does not cover HTTP API.

a little hint which function handles this?

blazoncek · 2024-09-22T10:53:04Z

a little hint which function handles this?

set.cpp ; handleSet()

DedeHai · 2024-09-22T11:14:53Z

handleSet() calls seg.setMode() which takes care of setting the default palette. what am I missing?

blazoncek · 2024-09-22T11:31:03Z

what am I missing?

Then you are all set. I do not know every line of code from the top of my head, but I do now a few relations that may be hidden.

blazoncek · 2024-10-02T13:25:16Z

Ah, disregard. 🤦

DedeHai added 2 commits September 15, 2024 09:33

Merge remote-tracking branch 'upstream/0_15' into 0_15_FX_improvements

0af6889

blazoncek requested a review from softhack007 September 15, 2024 12:48

bugfix

2db67f3

blazoncek requested changes Sep 16, 2024

View reviewed changes

TripleWhy reviewed Sep 16, 2024

View reviewed changes

TripleWhy reviewed Sep 17, 2024

View reviewed changes

DedeHai added 4 commits September 17, 2024 17:27

Removed local palette from Firenoise (and fixed a bug there)

d82ad48

uses fire palette by default, looks is 95% identical. also the /cols should be /rows or it will look different depending on matrix dimensions (brightness changes, also there are weird lines if width != height)

smear added to sindots

5a46e93

Added _default_palette parameter so pass FX setting to pal0

6cd500b

no penalty in size of `Segment` struct (if it is kept just at the place I added it, compiler does not optimize struct alignment order and just adds padding if not aligned, there it falls into a padding)

Gradient Palette support for scrolling text

c98679f

checking 'gradient' applies the selected palette as a gradient, default palette still does color gradient.

Merge remote-tracking branch 'upstream/0_15' into 0_15_FX_improvements

9413827

blazoncek reviewed Sep 20, 2024

View reviewed changes

DedeHai added 2 commits September 20, 2024 21:42

bugfixes

bb6b37d

- _default_palette now always gets properly set to party colors (in that case the FX does not have a 'pal' argument and sOpt is -1) - added initialization to zero for _default_palette (reason to crash)

Fix for correct _default_palette loading on existing presets

123ca5b

- 'pal' FX option is always extracted when chaning an FX, _default_palette variable is thus always set in setMode()

blazoncek requested changes Sep 22, 2024

View reviewed changes

wled00/FX_fcn.cpp Show resolved Hide resolved

wled00/FX_fcn.cpp Outdated Show resolved Hide resolved

bugfix

f1f947d

softhack007 added Awaiting testing in progress This pull request is currently being reviewed and, if necessary, modified labels Sep 28, 2024

softhack007 added this to the 0.15.1 candidate milestone Sep 28, 2024

blazoncek approved these changes Oct 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FX improvements and cleanup #4145

FX improvements and cleanup #4145

DedeHai commented Sep 15, 2024 •

edited

Loading

blazoncek commented Sep 15, 2024

blazoncek left a comment

blazoncek Sep 16, 2024

DedeHai Sep 16, 2024

blazoncek Sep 16, 2024

blazoncek Sep 16, 2024

blazoncek Sep 16, 2024

blazoncek Sep 16, 2024

DedeHai Sep 16, 2024 •

edited

Loading

blazoncek Sep 17, 2024

DedeHai commented Sep 16, 2024 •

edited

Loading

blazoncek commented Sep 16, 2024

TripleWhy Sep 15, 2024

blazoncek Sep 16, 2024

TripleWhy Sep 16, 2024

DedeHai Sep 16, 2024

netmindz commented Sep 17, 2024

TripleWhy Sep 17, 2024

DedeHai Sep 17, 2024

DedeHai commented Sep 19, 2024 •

edited

Loading

blazoncek Sep 20, 2024

DedeHai Sep 20, 2024

blazoncek Sep 21, 2024

DedeHai Sep 21, 2024 •

edited

Loading

DedeHai Sep 22, 2024

blazoncek left a comment

DedeHai commented Sep 22, 2024

blazoncek commented Sep 22, 2024

DedeHai commented Sep 22, 2024 •

edited

Loading

blazoncek commented Sep 22, 2024

blazoncek commented Oct 2, 2024 •

edited

Loading

FX improvements and cleanup #4145

Are you sure you want to change the base?

FX improvements and cleanup #4145

Conversation

DedeHai commented Sep 15, 2024 • edited Loading

blazoncek commented Sep 15, 2024

blazoncek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DedeHai Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DedeHai commented Sep 16, 2024 • edited Loading

blazoncek commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netmindz commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DedeHai commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DedeHai Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blazoncek left a comment

Choose a reason for hiding this comment

DedeHai commented Sep 22, 2024

blazoncek commented Sep 22, 2024

DedeHai commented Sep 22, 2024 • edited Loading

blazoncek commented Sep 22, 2024

blazoncek commented Oct 2, 2024 • edited Loading

DedeHai commented Sep 15, 2024 •

edited

Loading

DedeHai Sep 16, 2024 •

edited

Loading

DedeHai commented Sep 16, 2024 •

edited

Loading

DedeHai commented Sep 19, 2024 •

edited

Loading

DedeHai Sep 21, 2024 •

edited

Loading

DedeHai commented Sep 22, 2024 •

edited

Loading

blazoncek commented Oct 2, 2024 •

edited

Loading