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

added function to generate random palette based on harmonic color theory #3729

Merged
merged 11 commits into from
Feb 6, 2024

Conversation

DedeHai
Copy link
Collaborator

@DedeHai DedeHai commented Jan 27, 2024

looks really nice :)

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 27, 2024

not sure if adding #include "FastLED.h" fcn_declare.h is the correct way

@blazoncek
Copy link
Collaborator

not sure if adding #include "FastLED.h" fcn_declare.h is the correct way

Better to place it into "wled.h"

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.

Thank you very much. Code looks good.
Will test ASAP.

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

Tested on ESP32 and it performs ok.
There is a slight FPS drop when using FPS >80 and several segments. Not sure if it is related to the change.

@blazoncek
Copy link
Collaborator

@DedeHai please re-test after my change.

@DedeHai DedeHai force-pushed the harmonic-random-palette-generator branch from 895b19b to 2659055 Compare January 29, 2024 19:48
@blazoncek
Copy link
Collaborator

Do not force push.

wled00/FX_fcn.cpp Outdated Show resolved Hide resolved
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've had a second look after yesterday's testing, which involved monitoring palette transitions for several minutes.

I must say that random palettes didn't look any more pleasing than the old ones. Perhaps the issue lies in randomisation of palette entries but I cannot say as I am no color theory specialist. What I would expect from this PR is to add (apart from greater complexity) a more visually pleasing result (best observed using Palette or Rainbow effects).

There are some other issues addressed separately as in-line comments.

P.S. And please follow the guidelines from Wiki on how to manage PRs. I've made some changes yesterday which were overwritten by force-pushing your updates.

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

DedeHai commented Jan 30, 2024

I've had a second look after yesterday's testing, which involved monitoring palette transitions for several minutes.

I must say that random palettes didn't look any more pleasing than the old ones. Perhaps the issue lies in randomisation of palette entries but I cannot say as I am no color theory specialist. What I would expect from this PR is to add (apart from greater complexity) a more visually pleasing result (best observed using Palette or Rainbow effects).

There are some other issues addressed separately as in-line comments.

P.S. And please follow the guidelines from Wiki on how to manage PRs. I've made some changes yesterday which were overwritten by force-pushing your updates.

I disagree. I did compare it to the original and it is much better. but that is taste I guess. Not saying it could not be improved more but it is a start. maybe get some other's opinions?

@blazoncek
Copy link
Collaborator

I disagree. I did compare it to the original and it is much better. but that is taste I guess. Not saying it could not be improved more but it is a start. maybe get some other's opinions?

What about auric color pair palettes, complementary color palettes, tertiary color palettes, etc? Not randomised as seen in this code, but still random as in random base hue. That's what I was thinking of.

I would love to see other people feedback as well. We should ask beta testers on Discord for help.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 30, 2024

What about auric color pair palettes, complementary color palettes, tertiary color palettes, etc?

that IS exactly what it does. I dont get what you mean...

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

that IS exactly what it does. I dont get what you mean...

I do not know what randomness ESP took yesterday, but today palettes look less "jumpy" and more pleasing.
So I have to take that back.

-added fully random palette function ('the old way', currently just used for initialization)
-changed randomness values to make it a little less random
-added 10% chance for pastel color palette
-now using swap() from std library for shuffeling
-changed function name
-moved update check from loadPalette() to handleRandomPalette() saving CPU cycles
@softhack007
Copy link
Collaborator

softhack007 commented Jan 30, 2024

I disagree. I did compare it to the original and it is much better. but that is taste I guess. Not saying it could not be improved more but it is a start. maybe get some other's opinions?

I found some time yesterday to try out the changes (still without gamma correction => code from Sunday). I think the new random palettes do look nice 👍 just sometimes they seems to be a bit too dark, especially when you run at 50% or less global brightness.

Compared to the old random palettes, they are more colourful and fewer pastel colours (pale horses). However for me they also lack any "white".

As the results are so different, why not let users decide? It means we could have two random palettes in the list, one "classic" or simple, and the other one "harmonic". What do you think?

wled00/colors.cpp Outdated Show resolved Hide resolved
@softhack007
Copy link
Collaborator

softhack007 commented Jan 30, 2024

@blazoncek @DedeHai I'm wondering why this PR - which is about adding a new (super 😎 ) way of making random palettes - is also modifying the transitions code?

I have to admit that I did not read through your long discussion... just wouldn't it be better to separate the topics, and move changes to transitions into another PR?

@blazoncek
Copy link
Collaborator

I'm wondering why this PR - which is about adding a new (super 😎 ) way of making random palettes - is also modifying the transitions code?

@softhack007 where did you spot that? As far as I can see it only touches random palettes.

@DedeHai this now looks much better. 👏

@@ -466,6 +456,21 @@ CRGBPalette16 IRAM_ATTR &Segment::currentPalette(CRGBPalette16 &targetPalette, u
void Segment::handleRandomPalette() {
// just do a blend; if the palettes are identical it will just compare 48 bytes (same as _randomPalette == _newRandomPalette)
// this will slowly blend _newRandomPalette into _randomPalette every 15ms or 8ms (depending on MIN_SHOW_DELAY)
// if palette transitions is enabled, blend it according to Transition Time (if longer than minimum given by service calls)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@blazoncek I was referring to this part (modifying transitions/blending)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah. This refers to "respect transition time" while blending new random palette into old/displayable random palette.

My (or original) implementation relied on periodic (with constant period) calls to handleRandomPalette() to do the blend, (due to bug) this happened rather quickly (and not consistent between different ESPs)

The bug has been mitigated and the blend was consistent but still dependent on FPS. This change (although I'm not very fond of it) somehow mitigates that.
Reality is that it would need a separate timer and be independently controllable.

Copy link
Collaborator

@softhack007 softhack007 Jan 31, 2024

Choose a reason for hiding this comment

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

Thanks for explaining 👍
Indeed transition/blending time should - in best case - not depend on framerates, but use millis() or other timers so it always looks smooth, and same time will be needed no matter how high or low users set their "target fps".

As its "your" code, you're the best person to know if a second PR is needed or not.

Copy link
Collaborator

@softhack007 softhack007 Jan 31, 2024

Choose a reason for hiding this comment

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

@blazoncek another thought:
We could measure "time elapsed" (in millis) since the last blend step. then calculate how many steps correspond to elapsed time (using 40 FPS as the baseline for changes). Put the blending into a loop. If steps needed > 0, perform several blends at once. OFC the blending code must still run at a high rate so it won't show visual stuttering.
This is similar to how audioreactive mitigates "hickups" in userloop activity, in order to stabilize audio filtering.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@softhack007 I already added in something like that but I think it needs to be polished a little more. It is not a question of coding it but more a question of how it should behave.
My opinion is, that the user should be given the option to set the blend speed, which I added in by tying it to 'Transition Speed' but IMHO this should be a separate value. Next question is then, does this blend speed control random palette only or is it also used for normal palette blending or will that still depend on transition time. Adding too many options is also bad for usability, it should be somehow logical which value controls what. Right now (without my changes but with @blazoncek fix for random blend) the Transition Time (which also controls palette blending) can be set to 5000ms for example, but random palettes will still blend at default speed, which is ~150 frames. To me as a user this is quite inconsistent.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are discussing it then let me try to explain "Transition Time" as far as blending palettes go. "Transition Time" is the time taken to switch (smoothly without visible steps) from one palette to the other. Random palette is exactly that - a single randomly generated palette. A single palette which is constantly changing so no transition is triggered when random change happen. If you want to consider this random change to be considered as a trigger for "Transition Time" then the approach taken above works until transition time is shorter than minimum time needed for full blend of two palettes (255 iterations) this is the inconsistency I am not fond of. I am not saying it will not work or that it may impact users much, I just want for things to be logical.

User can specify the amount of time between random changes (between 1s and 65535s), we could reuse that time to determine the time needed to transition from one random palette to the other without introducing yet another parameter or rely on "Transition Time". The connection can be linear, logarithmic or some other non-linear function. I.e. if a user specifies random palette time of 30s then the actual transition could take 1/3rd of time (or 10 seconds). This correlation will ensure no conflicts arise from improper user selection. I am open to suggestions regarding this approach.

In any case I'm ok with proposed solution taking strip.getTransition() into account.

Copy link
Collaborator Author

@DedeHai DedeHai Feb 1, 2024

Choose a reason for hiding this comment

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

I completely agree, to me the current solution (without this PR) with frame based random palette blends is inconsistent as well. If I set transitions to a slow 10s then change between palettes I get a smooth transition. If I switch to random palette, suddenly transitions are much faster.
I just try to imagine differen use cases and what a user would expect. In my case, I use it for 2D matrix displays and ambient lighting. For the display case, a frame based 'static' change would be fine. For ambient not so much (as I mentioned before), here I would want slow transitions or at least have some control over it.
Then I see lots of people using it for quite flashy displays, mostly sound reactive stuff. I think here a fast transition would be expected if transition time is set low, even if the random change is set to say 5 minutes, so tying the random transition time to the change interval may be a bad idea. These are the scenarios I was thinking about.
There is no 'one fits all' solution I think without adding another config parameter.
The currently proposed solution in this PR may be the best compromise. Some people may not like it, some may find it much better, most probably don't even care or notice ;)
The most consistent approach would be to adjust the random palette blend so it (approximately) fits the transition time, even if that is set lower than the current frame based transition. I could do that by adjusting the amount of blending done in 'handleRandomPalette' but keep the approach to just call that function once every frame.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Jan 31, 2024

I found some time yesterday to try out the changes (still without gamma correction => code from Sunday). I think the new random palettes do look nice 👍 just sometimes they seems to be a bit too dark, especially when you run at 50% or less global brightness.

Compared to the old random palettes, they are more colourful and fewer pastel colours (pale horses). However for me they also lack any "white".

As the results are so different, why not let users decide? It means we could have two random palettes in the list, one "classic" or simple, and the other one "harmonic". What do you think?

I tweaked it already a little, made it a tad brighter and upon request from @blazoncek added a 50% chance of hue shuffeling, giving it a more 'ranbowy' look and added a 10% chance to generate a 'pastel' palette (experimental, not sure if this should be kept).
There is only so much 'color theory' I can pack in there and still keep it simple enough, the rest is personal taste. You like more white, another more dark, another wants more pastel, another more rainbow etc. However, the parameters are easy to tweak but it may be hard to find a set of parameters we all can agree on :) but I will do my best and I agree with you about 'more white'.
In the latest commit I added the 'fully random palette' back in as a function (which I needed to initialize) so it would be real easy now to have a boolean/tick in the LED config that activates 'harmonic random palette' or leaves it the way it was as some users may be used to that and like it more.

DedeHai and others added 3 commits February 1, 2024 20:06
added more white, changed function return value of fully random palette function
@blazoncek
Copy link
Collaborator

@DedeHai please verify if my latest changes broke anything. If not, I am merging as is.

@DedeHai
Copy link
Collaborator Author

DedeHai commented Feb 6, 2024

@DedeHai please verify if my latest changes broke anything. If not, I am merging as is.

thx for the code cleanup. I see no issues, works just like before.

@blazoncek blazoncek merged commit 0ab2d18 into Aircoookie:0_15 Feb 6, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants