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 transitions #3311

Merged
merged 19 commits into from
Sep 5, 2023
Merged

Effect transitions #3311

merged 19 commits into from
Sep 5, 2023

Conversation

Aircoookie
Copy link
Owner

by @blazoncek

Enables smooth blending from the previous effect if the effect is changed.

Copy link
Owner Author

@Aircoookie Aircoookie left a comment

Choose a reason for hiding this comment

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

Transitions look amazing!

@@ -381,6 +381,26 @@ typedef struct Segment {
byte *data; // effect data pointer
static uint16_t maxWidth, maxHeight; // these define matrix width & height (max. segment dimensions)

typedef struct TemporarySegmentData {
Copy link
Owner Author

@Aircoookie Aircoookie Aug 9, 2023

Choose a reason for hiding this comment

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

This is not bad at all, so this is just a suggestion, but wouldn't it be more maintainable to make TemporarySegmentData an anonymous struct CoreSegmentData and use it both within Segment and Transition? All the members could still be accessed like usual (e.g. seg.speed) since the anonymous struct has no name. Copying could probably also be simplified from how it is implemented now in saveSegenv() as the whole struct could be copied in one line (not sure if it is possible to do that with anonymous structs though).

Oh, and _briT, _cctT and _modeT could be added to the struct too, instead of being in the transition (although that would use 4 bytes more per transition currently due to alignment, so it can stay as is for the time being).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did experiment with that but it is not easy to use anonymous struct in two places.
_bri, ... were initially in the structure but I was having issues with restoring values so I moved them back out of the struct.

@@ -470,6 +557,7 @@ void Segment::setMode(uint8_t fx, bool loadDefaults) {
sOpt = extractModeDefaults(fx, "mY"); if (sOpt >= 0) mirror_y = (bool)sOpt; // NOTE: setting this option is a risky business
sOpt = extractModeDefaults(fx, "pal"); if (sOpt >= 0) setPalette(sOpt); //else setPalette(0);
}
markForReset();
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is a good change, resetting mid-transition like in main led to quite a few issues (new effect allocating data just for it to reset and re-allocate mid-transition), so I will probably change it there too for b4. Should probably be un-commented in WS2812FX::setMode as well.

@@ -1079,13 +1170,15 @@ void WS2812FX::service() {
_segment_index = 0;
Segment::handleRandomPalette(); // move it into for loop when each segment has individual random palette
for (segment &seg : _segments) {
if (!seg.isActive()) continue;
Copy link
Owner Author

Choose a reason for hiding this comment

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

Continuing before seg.resetIfRequired() causes segment data to not be freed if the segment is deleted. Could this be the cause of the leak you observed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch.

@blazoncek blazoncek mentioned this pull request Aug 9, 2023
@blazoncek blazoncek linked an issue Aug 9, 2023 that may be closed by this pull request
@blazoncek blazoncek marked this pull request as ready for review August 12, 2023 19:39
- breaking change
- remove leading 0 checkmark
- add reverse scroll checkmark
- add vertical scroll if text fits into segment (intensity ==0 or ==255)
- rotated characters
- leading 0 check added to short texts (i.e. #DDMM0)
Fixes #3322
@blazoncek blazoncek linked an issue Aug 21, 2023 that may be closed by this pull request
@softhack007
Copy link
Collaborator

@blazoncek I've tested the new code on my "portable 16x8" equipment - looks good 👍

I just noticed a problem with the "Matrix" effect when "global LEDs buffer" is disabled. Only the top row is changing, all other rows stay dark. Not sure if this is related to fx-blending, or maybe its a general bug. Will re-test with my bigger setup in the next days, to see if I can confirm the problem on a second ESP32.

@blazoncek
Copy link
Collaborator

I just noticed a problem with the "Matrix" effect when "global LEDs buffer" is disabled.

For fun create a gap file (to disable a few LEDs) and observe the outcome. 😁

Matrix is flawed. In many ways.

@blazoncek
Copy link
Collaborator

Oh, and BTW all of my units now use fx-blending branch, from tiniest ESP01 to overclocked ESP32. All blend effects nicely.

@softhack007
Copy link
Collaborator

For fun create a gap file (to disable a few LEDs) and observe the outcome. 😁

I'll try that 😁

Just took a quick look at the Matrix effect code, and it seems it will 100% fail without absolutely lossless getPixelColor().

WLED/wled00/FX.cpp

Lines 5136 to 5137 in ba1b6f3

CRGB pix = SEGMENT.getPixelColorXY(col, row);
if (pix == spawnColor) {

Maybe let's consider Matrix a hopeless case, and get back to it once our universes have imploded, or when one of us is totally bored 😝 whatever comes first ;-)

@blazoncek
Copy link
Collaborator

Maybe let's consider Matrix a hopeless case, and get back to it once our universes have imploded, or when one of us is totally bored 😝 whatever comes first ;-)

I want the lady in red and a steak. 😉

@blazoncek
Copy link
Collaborator

@softhack007 as @Aircoookie is currently busy do you agree to merge this and then create a new beta release?

@softhack007 softhack007 merged commit ddbe883 into main Sep 5, 2023
22 checks passed
@blazoncek blazoncek deleted the fx-blending branch September 6, 2023 15:09
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.

Scrolling text direction enhancment Effect transitions
4 participants