Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
added function to generate random palette based on harmonic color theory #3729
Changes from 7 commits
a9bcc75
bccc97d
12e2116
e114b84
f5ed757
2659055
ef6fe43
ca05aa8
e0f89be
66d9e8c
41e51bb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.