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

Rotating 2D Palette effect #3683

Merged
merged 4 commits into from Jan 27, 2024
Merged

Conversation

TripleWhy
Copy link

@TripleWhy TripleWhy commented Jan 14, 2024

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:
non-square mode

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:
Repeat until the end

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

Click to expand

segments

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:
line

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:
2D emulation

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.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

wled00/FX.cpp Show resolved Hide resolved
@dosipod
Copy link

dosipod commented Jan 14, 2024

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
Copy link
Author

TripleWhy commented Jan 14, 2024

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Collaborator

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.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wled00/FX.cpp Show resolved Hide resolved
wled00/FX.cpp Outdated
#endif
const bool isMatrix = strip.isMatrix;
const int cols = SEGMENT.virtualWidth();
const int rows = isMatrix ? SEGMENT.virtualHeight() : strip.getSegmentsNum();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@TripleWhy
Copy link
Author

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

@blazoncek
Copy link
Collaborator

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
Copy link
Collaborator

Verdict?

@TripleWhy
Copy link
Author

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
Copy link
Author

TripleWhy commented Jan 20, 2024

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
Copy link
Collaborator

@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
Copy link
Collaborator

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.

@blazoncek blazoncek self-assigned this Jan 22, 2024
blazoncek and others added 2 commits January 22, 2024 19:38
Limit rotation to +-90 deg (swapping sin/cos & limit angle)
Shift palette shift
@blazoncek blazoncek merged commit a71c910 into Aircoookie:0_15 Jan 27, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants