Effect blending styles #3877
Effect blending styles #3877
Conversation
- alternative to #3669
|
I'd love to hear your thoughts @tkadauke. |
|
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. |
|
@willmmiles could you take a look as well? |
|
OK, will do! |
There was a problem hiding this comment.
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
| 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!! |
There was a problem hiding this comment.
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
| if (len < 2) return false; | ||
| unsigned shuffled = hashInt(i) % len; | ||
| unsigned pos = (shuffled * 0xFFFFU) / len; | ||
| return progress() <= pos; |
There was a problem hiding this comment.
This needs to be return (progress() <= pos) ^ _modeBlend;, otherwise we're using the same mask for old and new.
| if (!invert && iInside) return _modeBlend; | ||
| if ( invert && !iInside) return _modeBlend; | ||
| return !_modeBlend; |
There was a problem hiding this comment.
Consider using xor here - return !iInside ^ invert ^ _modeBlend;
There was a problem hiding this comment.
Logic was never my best friend.
There was a problem hiding this comment.
Lol fair enough! Xor in particular is the weird one I rarely use. In this context, xor can be thought of as a "conditional invert", ie. a ^ b == if (b) return !a; else return a;
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). |
I definitely don't -- the other transition styles are only applied if Honestly we might want to map out the API - how do |
|
On another axis: if we wanted to completely integrate fading and other styles, instead of |
|
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 The problem with AFAIK the |
|
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.
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 |
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. |
|
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 |
- 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
| @@ -1211,28 +1211,29 @@ static const char _data_FX_MODE_COMET[] PROGMEM = "Lighthouse@!,Fade rate;!,!;!" | |||
| */ | |||
| uint16_t mode_fireworks() { | |||
| if (SEGLEN == 1) return mode_static(); | |||
| const uint16_t width = SEGMENT.is2D() ? SEGMENT.virtualWidth() : SEGMENT.virtualLength(); | |||
| const uint16_t height = SEGMENT.virtualHeight(); | |||
| const unsigned width = SEGMENT.is2D() ? SEGMENT.virtualWidth() : SEGMENT.virtualLength(); | |||
There was a problem hiding this comment.
FFS! These changes were unintended for PR. 🤦♂️
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).