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

Effect blending styles #3877

Closed
wants to merge 41 commits into from
Closed

Effect blending styles #3877

wants to merge 41 commits into from

Conversation

blazoncek
Copy link
Collaborator

Implements 14 different transitions/blends between different effects/modes.

Alternative to #3669 by @tkadauke with less memory fragmentation and utilisation.
Implemented using clipping rectangles which may not produce same results as original PR (#3669).

- alternative to #3669
@blazoncek
Copy link
Collaborator Author

I'd love to hear your thoughts @tkadauke.

@tkadauke
Copy link

tkadauke commented Apr 3, 2024

I certainly like how it's less complex :)

I'm out of town this week, but can test on the weekend. If this produces roughly the same results as my PR, I would definitely prefer it over mine.

@blazoncek
Copy link
Collaborator Author

@willmmiles could you take a look as well?

@willmmiles
Copy link
Collaborator

OK, will do!

Copy link
Collaborator

@willmmiles willmmiles left a comment

Choose a reason for hiding this comment

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

Other notes: with this patch, when I set blending mode to "fade", changing color settings fades from one to the next. However, if I change the blending mode, changing colors just hard switches over - the fade is disabled but I don't get the new transition style. I think we should be consistent and apply the transition style to color transitions as well, not just effect mode changes.

wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
seg.setClippingRect(w - dw, w, 0, h);
break;
case BLEND_STYLE_PINCH_OUT: // corners
seg.setClippingRect((w + dw)/2, (w - dw)/2, (h + dh)/2, (h - dh)/2); // inverted!!
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recommend making the inverted case an explicit parameter to setClippingRect. You can still encode it internally, but it'd make the API easier to follow

wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
@blazoncek
Copy link
Collaborator Author

However, if I change the blending mode, changing colors just hard switches over - the fade is disabled but I don't get the new transition style.

This is what it is supposed to be as old effect has to run with its color and new effect with a new set of colors (or palette).
If you are using 1D strip then only to 6 transition/blend modes are available. However if you do not see "Swipe left" or "Swipe right" or "Fairy Dust" transitions, then something is amiss.

@willmmiles
Copy link
Collaborator

willmmiles commented Apr 6, 2024

However, if I change the blending mode, changing colors just hard switches over - the fade is disabled but I don't get the new transition style.

This is what it is supposed to be as old effect has to run with its color and new effect with a new set of colors (or palette). If you are using 1D strip then only to 6 transition/blend modes are available. However if you do not see "Swipe left" or "Swipe right" or "Fairy Dust" transitions, then something is amiss.

I definitely don't -- the other transition styles are only applied if seg.mode != tmpMode, not when any other parameter (colors, palette, etc.) is changed.

Honestly we might want to map out the API - how do fadeTransition, modeBlending, blendingStyle and transitionDelay (which might be 0 aka no transition) all inter-relate for each type of change (mode change, non-mode parameter change)? It might be cleaner to drop fadeTransition and modeBlending entirely in favor of blendingStyle and using transitionDelay== 0 to indicate "no transition", since at least some clients have adopted that approach anyways.

@willmmiles
Copy link
Collaborator

On another axis: if we wanted to completely integrate fading and other styles, instead of isPixelClipped we could have a pixelBlendAlpha call that could return any value from 0 (100% old) to 255 (100% new). This could allow some more effect styles that combine approaches, and the infrastructure could also be used in the future to implement multi-effect overlays.

@blazoncek
Copy link
Collaborator Author

Transitions or blending or fading, whatever you want to call it, progressively evolved from simple two-color blend and on/off fade into what it is now. The first iteration even didn't have universal (across all segments) color blending and was limited to first N segments.

So, a lot of flags are legacy based and could be integrated into transitionDelay as ultimately if that is 0, no transitions/blending/fading is performed and it (blending) does not take much (well, at least the amount it did in the past) CPU cycles.

The problem with transitionDelay is that it is a bit convoluted. There is also transitionDelayDefault and jsonTransitionOnce and perhaps some other hidden logic somewhere.

AFAIK the fadeTransition is used for on/off transition (entirely separate process) and color transition/blend, modeBlend just enables/disables effect transitions/blending (independent of color blending) and blendingStyle determines the kind of effect blend/transition which is new in this PR.

@tkadauke
Copy link

tkadauke commented Apr 8, 2024

Ok, so I finally got a chance to test this. As I said, I like how simple this is, and that it doesn't use 8 extra bytes per pixel, like my PR. That said, there are some gaps.

  • As @willmmiles pointed out, only effect transitions are using the selected blend style. I think the real magic comes out when color/palette/brightness changes also trigger the transition. This would also make it much easier to test the transitions for correctness. It's hard to be sure, but I suspect that both "Pinch-out" and "Inside-out" are not working as intended, since I saw some abrupt brightness changes.
  • Push Left/Push Right for 1D transitions aren't implemented. I think that wouldn't be too hard to add. In addition to clipping, you'd add a translation step to both the old and the new effect, which simply moves the rendered pixel by an offset (or x,y vector).
  • In my PR, only transitions that work with the current strip style (1D vs 2D) are shown in the UI.
  • Palette changes happen abruptly here. When I switch from solid white to Aurora using "Fairy Dust", I see some pixels fade out completely, until at the very end the palette switches to green from one frame to the next.
  • Related to the above, if I disable "Effect Blending" in the LED settings, the UI still shows the blend selector (which I think it shouldn't), but the good news is that it actually is disabled.

I also noticed that as with my own PR, toggling the power state doesn't trigger transitions.

If I were to go from here, I'd also consider using masks for transitions. This would help simplify the Fairy Dust blend mode, and would remove the special case in the isPixelClipped() method. Masks could either be one bit per pixel, or a number from 0 to 255 to enable alpha blending, similar to what @willmmiles suggested above.

@willmmiles
Copy link
Collaborator

willmmiles commented Apr 8, 2024

If I were to go from here, I'd also consider using masks for transitions. This would help simplify the Fairy Dust blend mode, and would remove the special case in the isPixelClipped() method. Masks could either be one bit per pixel, or a number from 0 to 255 to enable alpha blending, similar to what @willmmiles suggested above.

Heh, great minds think alike! I thought about it but stopped short of suggesting full on alpha mask generation. I had the crazy idea of using some of the existing FX code to generate an alpha mask to blend other FX. The real coup would be tying it to some of the audio reactive code, I think it'd make some really neat composite effects..

As a core feature, though, it runs in to the code/CPU vs RAM tradeoff -- my understanding is that RAM is at a premium for many existing use cases, so contiguous space for extra buffers on the scale of pixel counts can't be assumed to be available. The "Fairy Dust" implementation in this PR is a good example, it trades up doing a bunch of extra calculations for every render (something like 3x shift, 3x xor, 4x multiply, 2x divide -- all twice per pixel!) to avoid needing to allocate (and worse, manage!) a temporary buffer of pixel status. It's difficult to strike a balance between supporting all features on large installations vs faster implementations when space permits - I don't envy @blazoncek for having to make those tough calls! My $0.02 would be ship this approach now, and keep the idea of blending mask buffers for the next iteration.

@blazoncek
Copy link
Collaborator Author

I've tried masks (& layers) but failed. Those were too memory and CPU intensive. I would welcome any attempt to make those work without greatly affecting stability or performance.

I know that limitation of clipping approach is its lack to perform certain "blends/transitions". I am ok with that as long as all blend/transition variants run well on every supported ESP. I also chose to ignore "transitions" for color or palette change (I've fixed that locally by reverting to blend) as it needlessly (IMO) runs effect function twice reducing FPS (which can be seen on certain effects quite obviously).

To elaborate on memory: Experiment with parallel I2S (code in bus_wrapper.h) and you'll see that even ESP32 with its 320kB soon falls short of RAM (with more than a couple of 100 pixels). So if you thought that we only need to consider 8266, think again.

As for Push variants of transitions: I was implementing from my memory of a single test of @tkadauke implementation and really didn't notice what the "push" effect does differently than swipe. Thinking again it is now clear what "push" does. Pushing on 1D could be implemented using "offset" capability, but this does not exist in 2D. Though idea isn't bad, it may only be obvious with very long transition times (>3s) as otherwise changes happen to quickly to really notice.

I know there are many desires on how and what to implement but, as stated many times, MCU is a limited device in many ways and compromises have to be made.

BTW I managed to remove a whole lot of code dealing with fadeTransition, modeBlend and strip.paletteFade and slightly reduced complexity of brightness fading in my local branch. I'll push those into this PR when a few more tests are done.

@blazoncek blazoncek marked this pull request as draft April 8, 2024 07:48
- transitions always enabled (use delay 0 to disable)
- optimisation in on/off fade
- fix for palette/color blend when blending style is not fade
- various tweaks and optimisations
wled00/FX.cpp Outdated Show resolved Hide resolved
@blazoncek blazoncek marked this pull request as ready for review April 14, 2024 13:47
@blazoncek blazoncek self-assigned this Apr 21, 2024
@tkadauke
Copy link

This is amazing. I am out of town today and tomorrow but happy to give this a try on Thursday if that sounds ok to you.

@blazoncek
Copy link
Collaborator Author

Take your time. It may not be perfect still and I would be happy with help in debugging.

blazoncek and others added 11 commits August 17, 2024 15:24
- copies the source segment
- brightness of segment is relative to source segment
- optionally shifts the color hue
- invert, transpose, mirror work
- if source or targets do not match in size, smallest size is copied
- unused pixels fade to black (allows overlapping segments)
- if invalid source ID is set, segment just fades to black
- added a rgb2hsv conversion function as the fastled variant is inaccurate and buggy

note: 1D to 2D and vice versa is not supported
target segment now also copies the source settings (mirror, grouping, spacing, transpose) so the target looks exactly like the source. inverting can still be set independently.
note: the bug in line 214 of FX_2Dfcn.cpp missing `start` needs to be fixed for it to work properly in 2D.
- added color adjust function with support for hue, lightness and brightness
- colors can now be adjusted in hue, saturation and brightness
- added  `getRenderedPixelXY()` function (credit @willmmiles)
- source is now accurately copied, even 1D->2D is possible
- fix for segment offset not being zero in 2D in `getRenderedPixelXY()`
- added checkmark to copy source grouping/spacing (may remove again)
- tested many different scenarios, everything seems to work
- removed 'copy grouping/spacing' option
- added 'Switch axis' instead (in 2D -> 1D copy this chooses x or y axis to be copied)
- optimized for code size and speed by not using CRGB (where possible) and not returning struct in `rgb2hsv()`
@blazoncek
Copy link
Collaborator Author

Is there still any interest in this or do we abandon and close the PR?

@willmmiles
Copy link
Collaborator

I think this is a valuable feature. Other than correctness testing, what else needs to be done for it?

@blazoncek
Copy link
Collaborator Author

Other than correctness testing, what else needs to be done for it?

It just needs to be tested if there are any bugs or behaviour that is unexpected/unintended. I've seen occasion where transition was interrupted (and stopped). But only when switching effects or other parameters during an existing (long) transition. Unfortunately I could not catch it nor consistently reproduce.

@blazoncek
Copy link
Collaborator Author

@DedeHai what did you do!?!?

@blazoncek
Copy link
Collaborator Author

Closing this PR as @DedeHai made unintentional push with irrelevant changes.
Will also delete branch and push it again from my local copy.

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.

4 participants