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

Transition styles #3669

Open
wants to merge 39 commits into
base: 0_15
Choose a base branch
from
Open

Conversation

tkadauke
Copy link

This replaces #3633. See the other PR for discussion.

This seems feature complete for the first round, what's still needed is ESP8622 testing, which I'll do today.

So far, implemented are Fade (which is the original transition style), swipe right, swipe left, outside in, and inside out.
… to allow for effects that modify pixels from the previous frame
@tkadauke
Copy link
Author

Ok makes sense. Effect blending (a.k.a. transition styles) is now disabled by default on ESP8266.

@tkadauke
Copy link
Author

Are there any other changes needed before this can be merged?

@blazoncek
Copy link
Collaborator

Well ...
Honestly I do not like the idea that mode/effect blending is disabled by default on ESP8266. In fact I do not like the idea that old "fade" approach is tied to the new "transition" approach. Even though "fade" may require a few more CPU cycles it works on ESP8266 without issues (apart from possibly lowering FPS on large LED count).

I would much rather see the "transition" approach being independently selectable and entirely removed from ESP8266 code if it cannot run reliably. There's also a question why did you decide to use vectors for transition styles? The number and content are known at compile time so there is no need for vector IMO. There are also unconventional comparisons like != in for loops and some other minor things.

And my personal aspect as a (current) maintainer of the project is the problem of complexity this PR adds to already complex task of rendering modes/effects to segments and strip. Even though I know the workings of the code I can get lost in the new code quite easily.

Do not get me wrong, I do like idea.

@tkadauke
Copy link
Author

Thanks for the feedback, I can definitely change this to fall more in line with your expectations.

Honestly I do not like the idea that mode/effect blending is disabled by default on ESP8266 [...] I would much rather see the "transition" approach being independently selectable and entirely removed from ESP8266 code if it cannot run reliably.

Sorry for the misunderstanding. Happy to change this.

There's also a question why did you decide to use vectors for transition styles?

Just for consistency's sake. Happy to choose another path if that saves memory and/or CPU.

The number and content are known at compile time so there is no need for vector IMO.

At the moment yes, but the argument could be made to remove the 2D transition styles when 2D is disabled.

There are also unconventional comparisons like != in for loops and some other minor things.

I'm trying to follow the local conventions as closely as possible. Happy to change this. If you spot anything else, please let me know in an inline comment.

And my personal aspect as a (current) maintainer of the project is the problem of complexity this PR adds to already complex task of rendering modes/effects to segments and strip. Even though I know the workings of the code I can get lost in the new code quite easily.

I appreciate the sentiment. Anything in particular you would like me to change? I have a suggestion below.

Here's what I'd suggest:

  • I'll revert back to guarding everything behind a compile time flag.
  • I'll decouple transition styles from the previously available effect blending at the compile time level. However, if transition styles are enabled, they take precedence over effect blending.
  • I'll default to effect blending enabled on ESP8266 and transition styles enabled on ESP32. I'll make it so that people can still enable it on ESP8266. After all, I could make it work, the UI was just slow, with or without transition styles compiled in. Maybe it's something else, I might have a bad controller on my hands.
  • I can move all transition style logic out into it's own C++ file (just like FX.cpp is separate from the rest), and merge 1D and 2D rendering into the same functions. For each transition style, I can add a function, like like we have it for effects. Then I can change the call site to call the function through a vector lookup, again just like it's done for effect rendering.

@blazoncek
Copy link
Collaborator

Thank you for understanding. When I get a bit more time I'll try to provide more feedback.
Your proposition sounds reasonable.

@tkadauke
Copy link
Author

Perfect! I'll get started on those changes tonight. Thanks again for your feedback and support!

@tkadauke
Copy link
Author

Ok, so I did all the things that I suggested. In the current version, if you want transition styles, you also have to enable mode blending; disabling mode blending will also disable transition styles. The reason is that a lot of the code required for sophisticated transitions is also necessary for fading between effects.

That said, I tested the following setups:

  • ESP32: effect blending and transition styles turned off. Works.
  • ESP32: effect blending on, transition styles turned off. Works.
  • ESP32: both effect blending and transition styles turned on. Works.
  • ESP8266: effect blending on, transition styles turned off. Works.
  • ESP8266: both effect blending and transition styles turned on. I used a different controller than last time. Works surprisingly well with 300 LEDs. I would still recommend to leave it disabled for now.

@tkadauke
Copy link
Author

@blazoncek, did you have a chance to look at this PR lately?

@blazoncek
Copy link
Collaborator

Unfortunately no, but I intend to.

@blazoncek
Copy link
Collaborator

Just a heads-up that I have not forgotten about this PR.

@blazoncek blazoncek self-assigned this Mar 26, 2024
@blazoncek
Copy link
Collaborator

@tkadauke I have finally found the time to test this but unfortunately found out that it does not work for on/off transitions. It only works for effect change. Is that intended?

I also got a few crashes.

Guru Meditation Error: Core  1 panic'ed (IntegerDivideByZero). Exception was unhandled.
Core 1 register dump:
PC      : 0x4010dec7  PS      : 0x00060b30  A0      : 0x800e9ed4  A1      : 0x3ffb1ec0  
A2      : 0x3ffc3534  A3      : 0x00000000  A4      : 0x00000000  A5      : 0x3ffb3264  
A6      : 0x00000001  A7      : 0x0000ffff  A8      : 0x00000002  A9      : 0x3ffb1ea0  
A10     : 0x00000041  A11     : 0x00050032  A12     : 0x00000001  A13     : 0x000001a5  
A14     : 0x00000001  A15     : 0x00000001  SAR     : 0x0000000a  EXCCAUSE: 0x00000006  
EXCVADDR: 0x00000000  LBEG    : 0x40002390  LEND    : 0x4000239f  LCOUNT  : 0x00000000  

ELF file SHA256: 0000000000000000

Backtrace: 0x4010dec7:0x3ffb1ec0 0x400e9ed1:0x3ffb1ef0 0x40122112:0x3ffb1f50 0x40122266:0x3ffb1f90 0x40145161:0x3ffb1fb0 0x4008d4a2:0x3ffb1fd0
  #0  0x4010dec7:0x3ffb1ec0 in transition_outside_in() at wled00/transition.cpp:238 (discriminator 4)
  #1  0x400e9ed1:0x3ffb1ef0 in WS2812FX::service() at wled00/FX_fcn.cpp:1467
  #2  0x40122112:0x3ffb1f50 in WLED::loop() at wled00/wled.cpp:114
  #3  0x40122266:0x3ffb1f90 in loop() at wled00/wled00.ino:20
  #4  0x40145161:0x3ffb1fb0 in loopTask(void*) at /Users/blaz/.platformio/packages/[email protected]/cores/esp32/main.cpp:23
  #5  0x4008d4a2:0x3ffb1fd0 in vPortTaskWrapper at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/freertos/port.c:355 (discriminator 1)
Guru Meditation Error: Core  1 panic'ed (IntegerDivideByZero). Exception was unhandled.
Core 1 register dump:
PC      : 0x4010e010  PS      : 0x00060930  A0      : 0x800e9ed4  A1      : 0x3ffb1ec0  
A2      : 0x3ffc3534  A3      : 0x0000ffff  A4      : 0x00000000  A5      : 0x00000001  
A6      : 0x00000000  A7      : 0x3ffb3264  A8      : 0x8010e010  A9      : 0x3ffb1ea0  
A10     : 0x0000033e  A11     : 0x00052632  A12     : 0x00000001  A13     : 0x000001a5  
A14     : 0x00000001  A15     : 0x00000001  SAR     : 0x0000000a  EXCCAUSE: 0x00000006  
EXCVADDR: 0x00000000  LBEG    : 0x40002390  LEND    : 0x4000239f  LCOUNT  : 0x00000000  

ELF file SHA256: 0000000000000000

Backtrace: 0x4010e010:0x3ffb1ec0 0x400e9ed1:0x3ffb1ef0 0x40122112:0x3ffb1f50 0x40122266:0x3ffb1f90 0x40145161:0x3ffb1fb0 0x4008d4a2:0x3ffb1fd0
  #0  0x4010e010:0x3ffb1ec0 in transition_inside_out() at wled00/transition.cpp:261 (discriminator 4)
  #1  0x400e9ed1:0x3ffb1ef0 in WS2812FX::service() at wled00/FX_fcn.cpp:1467
  #2  0x40122112:0x3ffb1f50 in WLED::loop() at wled00/wled.cpp:114
  #3  0x40122266:0x3ffb1f90 in loop() at wled00/wled00.ino:20
  #4  0x40145161:0x3ffb1fb0 in loopTask(void*) at /Users/blaz/.platformio/packages/[email protected]/cores/esp32/main.cpp:23
  #5  0x4008d4a2:0x3ffb1fd0 in vPortTaskWrapper at /home/runner/work/esp32-arduino-lib-builder/esp32-arduino-lib-builder/esp-idf/components/freertos/port.c:355 (discriminator 1)

So far I've only tested on ESP32.

@tkadauke
Copy link
Author

Thanks for testing this! I will look into the crashes.

I am aware that on/off transitions aren't implemented. Is that something you'd expect to be covered? If so, I'm happy to work on this.

@blazoncek
Copy link
Collaborator

It would make it consistent and would also satisfy several requests. But ...
It will be hard to implement as on/off transitions do not use segment logic but have their own handling.

@tkadauke
Copy link
Author

Yeah I noticed that. One way to do this is if we have a transition_style != FADE, for turning off, we could first do the transition, and then immediately turn off. We'd do the opposite for turning on.

blazoncek added a commit that referenced this pull request Apr 3, 2024
- alternative to #3669
@blazoncek blazoncek mentioned this pull request Apr 3, 2024
@Huskiefluff
Copy link

Huskiefluff commented Aug 11, 2024

How does one even install this. I see no binaries. This is exactly what i'm looking for. Kind of disappointed Wled doesnt have anything like this already, seems like a basic feature.

@tkadauke
Copy link
Author

@Huskiefluff, thanks for your interest in this feature. This is a development proposal of a feature that I would like to see in WLED. Currently, @blazoncek is working on implementing this in a way that works with all supported platforms than WLED runs on. If I had to guess, it would likely be part of version 0.15 at some point in time.

To get this running today, you'd have to check out the code and compile it for your platform. There are no published binaries, since this PR (and @blazoncek's PR) hasn't been accepted into the main branch yet. If you can connect your controller to your computer via USB, then flashing it should be straightforward. There are plenty of tutorials out there that'll teach you how to do this.

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.

3 participants