Rotating 2D Palette effect

[TripleWhy]

I implemented a new effect that is similar to Palette/Rainbow:

palette.webm

If you like it too, let's talk a bit about it:

Compatibility

I noticed that the Rainbow effect is almost a perfect superset of the Palette effect, so I replaced the Palette effect with my new effect. It provides similar functionality, but is not fully backwards compatible. So it might be better to make this a new effect ("Palette 2D" maybe?). I wanted to leave that judgement up to you.

Breaking changes are:

1. It is now a 2D effect and will therefore ignore a configured 1D FX expansion.
2. This new effect is segment-aware and will assume that a segmented 1D setup is actually a physical matrix, like you would have set it up prior to 0.14. So 1D setups with segments will now look differently.
3. The same speed input will result in slower palette shifting.
4. Compared to Rainbow (not Palette), the size parameter works differently and produces a different scaling for each possible input.

Parameter Description

I'm going to describe what each parameter does here, so you can come up with better names if you don't like mine:

1. Shift (uint8):
   1. In static shift mode, this will determine how far the palette is "shifted". A shift of 0 will display the palette as it is, a shift of 128 will start at the middle of the palette until it hits the end, then start from the beginning again.
   2. In animated shift mode, this will determine how fast the palette is shifted.
2. Size (uint8): Size will scale the palette, similar to the Size parameter in the Rainbow effect.
3. Rotation (uint8):
   1. In static rotation mode, this will determine the angle of the rotation. 0 is 0°, 256 is 360°.
   2. In animated rotation mode, this will determine the rotation speed.
4. Animate Shift (bool): This will switch between static and animated shift mode, so either animate palette shifting, or keep it static.
5. Animate Rotation (bool): This will switch between static and animated rotation mode, so either animate rotation, or keep it static.
6. Physical Square (bool): See next section.

Anamorphic Mode

For the setup I want to use this effect with, I have a set of LEDs that roughly cover a square area, but their numbers are not a square. There is a greater distance between LEDs in y than in x direction. So my setup is anamorphic, if you want.

Therefore, the Physical Square setting changes between assuming all physical LEDs are arranged in an equally spaced grid, and assuming the spacing is different in one direction than the other. There are other ways to look at this (and is interesting for equally spaced non-square setups as well) but this way of thinking made sense to me mathematically, when I implemented it.

This setting has no effect when the matrix has the same number of LEDs in x and y direction.

Here are two images at 45° that have identical settings apart from Physical Square.
Non-square mode:

Square mode:

2D Emulation

Motivation

I have a setup with 6 strips of 115 LEDs each, they light my ceiling. The distance between the strips is roughly 70 cm, while the distance between LEDs on a strip is about 3 cm. I have been using an ESP8266 so far, starting from WLED 0.13. I managed to put some rainbows on it, both along the strip direction and orthogonal to it, but I always wanted a diagonal rainbow, too.

With WLED 0.14 came matrix setups, but my poor 8266 becomes unresponsive immediately when I try to set it up with the above dimensions. I'm planning to replace it with an ESP32, on which that is no issue.

But during the development of my effect, I realized that is is entirely possible to achieve the same result with my existing 1D setup.

1D Matrix Setup

Prior to 0.14, I assume this button in the segment setup was used to configure matrix displays:

For me, this results in the following segmentation, which I have been using.

Click to expand

Obviously this is not the only way you can use segments. My 2D emulation assumes this kind of setup. It simply might not look correctly in other scenarios.

How it works

Now that we assume a certain anatomy of the setup, we can derive matrix dimensions from the number and length's of segments. We can also derive the y coordinate in the matrix from the current segment's index. Finally, the x coordinate is just the LED index in the current segment.

Laid out in one line, like the peek preview in the web UI does, a 45° rotation using color_wheel with the setup described above looks like this:

If we reverse every second segment, line them up vertically instead of horizontally, and scale them a bit (through the magic of gimp, or through the magic of having the LEDs physically ordered like that) we get this:

Notes

• If the segment setup is different or does not represent a matrix, this might not look correctly.
• I don't iterate over segments. If a segment is missing or configured differently, it will simply be missing/look different from the picture above.
• Using this 2D emulation results in one call per line instead of one call per matrix (as it would be with a 2D setup).
• The only way (I found) to distinguish between 1D setups, and 2D setups with 1D segments is reading strip.isMatrix. Other effects use this information too, so I though it was fine.

Floating Point Math

You will notice that my code is actually two versions, one for floating point math, one for integer math. I decided to use the float version on ESP32 for the better results, and the int version on ESP8266 for better performance.

I had to use two 64-bit multiplications in int mode to stay within the int range. It might be possible to get rid of them by halfing all other int sizes. But the int version already produces slightly lower quality results anyway, and I didn't want to further decrease quality, so I didn't try. (And besides the 64 bit multiplication appears to be only 2x slower than other int multiplications.)

The int version is a bit faster on the ESP32 and more so in the ESP8266 (which doesn't have an FPU I think?). I made some measurements, but I'm not sure what version of sin_t/cos_t they used. If you are interested in numbers, I can take some new measurements.

(The sin_t implementation is relevant because I when benchmarked different sin implementations, the Taylor version was about 24x (ESP32) / 11x (ESP8266) slower than the Arduino version. But there are only two trigonometry computations per call, so it might not matter that much. Btw. builds without the Taylor version are not only faster, but smaller too.)

Other Thoughts

This transformation of the Palette effect from 1D to 2D might be interesting as a general 1D FX expansion. However it would need parameters. I didn't see any expansions that have parameters at the moment, so I didn't look into that.

[blazoncek]

I like it though I do not know if replacing original mode_palette is the best way.
Because of metadata changes of slider descriptions.

You should make sure it behaves exactly the same as the original function did in 1D mode (even on 1D segments on matrix).

[dosipod]

Looks good on 32x16 and it would act as the old effect if the "Animation Rotation" is not checked and having
three sliders is nice , works well with segments mirror and revers for psychedelic effect .

[TripleWhy]

You should make sure it behaves exactly the same as the original function did in 1D mode

It does not. (Differences are listed above.) I'll update the PR to be a new effect instead of replacing an old one.
I do think however that having too many effects that are too similar doesn't do you any favors from a user point of view

[blazoncek]

You should make sure it behaves exactly the same as the original function did in 1D mode

It does not. (Differences are listed above.) I'll update the PR to be a new effect instead of replacing an old one. I do think however that having too many effects that are too similar doesn't do you any favors from a user point of view

Then don't rush. Please make sure that effect behaves very much like the old one on strip and 1D segment.
It does not need to be 100% same but very close. Otherwise people can complain.

I think it is worth the effort to have it in one effect as you suggest instead of introducing a new one.

[TripleWhy]

I see. I believe it to be similar enough. But I'm probably not the one who will receive possible complaints either ^^
So I'd leave the judgement up to you guys.

If I were to add a new effect: How would I handle its ID? The IDs seem grouped. I would think the effect belongs to the first group, but there aren't any gaps before the next group starts. Would I be shifting all IDs, put my effect at the end regardless of logical grouping, re-use a removed ID, or put it in the first group but using an ID from after the last group?

[blazoncek]

As far as IDs go, I was over-voted and effect IDs from SoundReactive prevailed. So they are all messed mixed up.

Use existing gaps.

[blazoncek]

My second review is intended to clarify what I meant by saying that "using rotation on 1D" may be a good thing and could yield a nice enhancement to the Palette effect.

First off, it seems to me that you may have a few misconceptions regarding segment use. Also, effect function should never interact directly with anything strip related (with a few notable exceptions that are there due to legacy reasons, such as strip.now).

What I'd do is the following:

• use square canvas of max(width,height)
• always perform palette paint in square 2D
• intersect segment with the canvas (at appropriate shift from top or left depending on the segment dimension)
• use intersection to paint segment

This could be optimised to reduce the painting only on the longest dimension to reduce the resources needed.

[blazoncek]

I do not get it. How is this supposed to work?

getSegmentsNum() will return the size of underlying vector which will have the size of the largest encountered segment count. I.e. I create can 10 segments then delete all but one (and adjust its dimensions) and the number returned by getSegmentsNum() will return 10, not 1. Segments are not removed from vector unless low memory condition occurs.
If you need usable segments then use getActiveSegmentsNum().

[TripleWhy]

Its not like on the fly segment changes work particularly great anyway. You can only be sure of what the will actually look like if you store the config and reboot the device. So I'm not concerned with the vector being too long. I don't care about missing segments either, those segments will just not be filled by my effect.

Using getActiveSegmentsNum() however would be incorrect, as I care for existing segments, not those that happen to be on. I also want to compare the segment ID to it, so the segment count must be greater than the segment ID. The segment count is my assumed matrix height for 2D emulation.

[blazoncek]

getActiveSegments() counts valid segments, not the segments that are on. There is no function to count on segments. Valid segments are segments that have length()>0. To put it the other way: active segments are visible in UI, inactive segments are not.

[TripleWhy]

Short update, just because I have the impression that you are moving rather quickly (even during the week):
On the weekend, I will address the issues, add a more detailed description of what I wanted to achieve with the 1D/segmentID stuff, and think about rotating 1D segments again.

[blazoncek]

Short update, just because I have the impression that you are moving rather quickly (even during the week)

Life is too short. There is no time to slack. And you can always skip on sleep. 😉

I've updated the effect to work with is2D() and removes dependency on segment IDs and it should rotate in 1D. Will push shortly after a bit more testing.

[blazoncek]

Code tested and works beautifully. 2D or 1D or both at the same time.
It does not rely on segment position though.

[TripleWhy]

Would you mind sharing the code so that I don't have to start from scratch?

[blazoncek]

Would you mind sharing the code so that I don't have to start from scratch?

Of course, I'll push directly to this PR.

[blazoncek]

Verdict?

[TripleWhy]

Didn't try it yet, and probably won't today. At first glance I like most of the formatting changes, but I believe you removed some of the functionality I was going for. But as I said, I'm going to write a more detailed description about that tomorrow ^^

[TripleWhy]

Updated description to include a new "2D Emulation" section. Hopefully that gets across my intentions with strip.isMatrix, strip.getCurrSegmentId(), and getSegmentsNum().

After trying your version, without looking into why:

• vertical 1D segments seem to work
• horizontal 1D segments work only partially
• 2D emulation is broken
• the maximum bounds are now incorrect, which should result in imperfect results in 2D setups as well
• your updated default parameters make the new effect less similar to the original effect

So, assuming you don't have any fundamental issues with implementing 2D emulation, I'm going to have another look at 1D segments, but I would like to not adopt some of your changes. If you want, I can add some review comments on the lines I don't like.

[blazoncek]

@TripleWhy I've now thoroughly analysed code and tested (the adapted) version which includes your requirements and my modifications. I will take some of my comments back. They were premature without thorough code analysis.

Indeed it is necessary to use strip.isMatrix and it is ok to use getCurrentSegmentId() (in combination with getActiveSegmensNum() instead) but only if it is optional. I will push the updates for you to re-analyse and confirm or reject.

My personal opinion regarding the use of getCurrentSegmentId() is still valid (it may not be apparent to the casual user why there is a shift). I have tested the effect on my roof borderline (comprised of several segments) and the result looked awful compared to the old behaviour. Segments were off and did not fit together like they did of old.

[blazoncek]

After a lengthy discussion (on Discord) and thorough testing the updated Palette effect (from this PR) gets green light from my side (with a few minor adjustments that yet need to be done). The default behaviour of new effect may differ from original default behaviour if used from within existing presets (created prior to 0.13) so the change will need to be marked a BREAKING change.

Discussion also uncovered that original Palette (mode_palette) and Rainbow (mode_rainbow_cycle) effects produced almost the same result. The only real difference was in speed (Rainbow is faster and does not stop at 0) and behaviour when Default palette is used.

I would recommend to update Rainbow effect (slow it down and stop animation at speed 0) so that it could mimic behaviour of the old Palette effect if selected palette is not a Default one. So the impact would be less critical.

The other option is to make this a new effect but then we'd have 3 effects capable of producing exactly the same output depending on the set parameters which I'd like to avoid.

@Aircoookie I'd like your opinion on the matter.

[DedeHai]

@TripleWhy we got a complaint on discord that this effect changed compared to the old behaviour. Do you remember what the reason was, this was not made to be equal to the old one? in the initial comments of this PR that was explicitly requested by @blazoncek
I have checked and only minor changes are needed to make it the same as the old 1D version:

• if speed slider is at zero, shift should stop
• shift should be zero if set to zero (i.e. remove the -128), centering the slider exactly to get the old behaviour is tedious
• animated shift should be -= and not +=, the direction is now inverted compared to the old one.
  I have tested these changes and could make a PR for a fix if you have no objections

edit:
I forgot: as previously discussed, rotation should also be disabled by default

[blazoncek]

@DedeHai the change was approved from my side and was documented in changelog as well as release notes for 0.15.0-b1.
The reasoning being it only requires users to update preset if it exists, otherwise they are free to experiment with the settings.

If you do change behaviour make sure you will add description to the changelog as it will become another breaking change (for users of 0.15).

[TripleWhy]

Opinion: You will get complaints no matter what you do, even if you don't do anything. Without changes, you can never improve. If you want to really lock into one exact behavior, you should lock into one exact version. That's what we do in software development every day.

About the bullet points:

if speed slider is at zero, shift should stop

No it should not. There is a dedicated checkbox for that now. Stopping at 0 would be bad for the following reasons:

• it introduces two ways of doing the same thing
• it adds more branches and complexity to the code
• it means that the math used isn't the same for all values of the slider
• it removes one slow animation speed that is currently possible

The difference between slow animation speed steps is much larger than the difference in fast animation speeds, so removing one slow speed is actually quite significant.
Personally, I am also more interested in slow animations than fast ones, to make stuff look classier instead of flashier. (Also it doesn't give you epilepsy looking at it extended periods of time.)

shift should be zero if set to zero (i.e. remove the -128)

I really don't care where the 0 point is, but @blazoncek did, so he introduced the - 128 here. (I'm assuming this line is what you are talking about?)

I believe that was motivated by a combination of where the default slider position is and what the most likely position for an existing preset was going to be.

Unfortunately, there is no way to not break some existing presets if new parameters are introduced, because presets save all parameters, even if they are unused or invisible at the time of saving. So if you load them after adding a parameter, the stored value is somewhat arbitrary.

animated shift should be -= and not +=, the direction is now inverted compared to the old one.

I don't think that is true, I believe I kept that as close to the original as possible. But I'm not sure anymore. In any case, using subtractions with unsigned integers is a really bad idea if you can avoid it.

What you can do however is rotate the effect by 180° and achieve the same thing, no need to change the code for that. As for what is the default or loaded from old presets... see the previous discussion point.

edit:

as previously discussed, rotation should also be disabled by default

I also don't care about the defaults 🤷

[DedeHai]

thanks for the insights. I think changing it (as close as possible) to the old behaviour prior to 0.15 release will give less requests from users, right now this already comes up like every other week: mostly confusion about the rotation but the recent question was about why it is now shifted. I guess there are far fewer users on beta than on 14.x so changing it now would be the last opportunity.

No it should not. There is a dedicated checkbox for that now. Stopping at 0 would be bad for the following reasons

fair point. should keep it as is. my bullet points were just the changes for EXACT match of the old effect.

animated shift should be -= and not +=, the direction is now inverted compared to the old one.

I don't think that is true,

it is, I just tested that (the old code does - offset). I agree on the signed integer, using rotation is probably better so by default the rotation should be 180° to make it the same as in 14.x. I will check how that can be done, if it can't then its up to the user.

Unfortunately, there is no way to not break some existing presets if new parameters are introduced, because presets save all parameters, even if they are unused or invisible at the time of saving. So if you load them after adding a parameter, the stored value is somewhat arbitrary.

I am sure there will be some complaints either way ;)
People use this effect on 1D strips to display the selected palette, when playing with sliders, it is hard to center it, especially on a mobile device, so having the palette unshifted at 0 is better IMHO and also more compatible with old presets where zero=stop and no offset.

[TripleWhy]

it is hard to center it, especially on a mobile device

Very true. Should there be a reset to defaults button maybe?

[TripleWhy]

having the palette unshifted at 0 is better IMHO

That just means not applying the 128 offset, right? I agree with that, but I don't care as much as @blazoncek did about adding it ^^

This doesn't however mean that the direction of the shift should be inverted when you slide the slider, which is what I thought - vs + was about. In any case, I also don't care much about the direction.
If you need a tie breaker, I would propose that when looking at a flat LED strip with indexes in ascending order from left to right, the pattern should look like it moves to the right if you move the slider to the right.

[DedeHai]

I will play with this FX on 1D and 2D some more and try to make changes to best fit all. what I found when quickly trying to revert the direction using the rotation slider in 1D: I could never get it to go the other way but it does it when animating the rotation. Will try to figure that one out without breaking the 2D version.

Very true. Should there be a reset to defaults button maybe?

that already is possible, just a bit of a workaround: select a different effect, then select palette again. maybe a reset button would be a nice addition, I have no opinion about that.

[TripleWhy]

Hm... blazoncek did talk about limiting the (non-animated?) rotation angle for more precision, because you can transpose segments to get the missing angles. I don't remember if that was implemented, but your comment makes it sound like it was

[DedeHai]

that makes a lot of sense, but only in 2D :)