Tweak the Christmas animation effect to be less harsh on the eyes #7648
Conversation
There was a problem hiding this comment.
Tested the changes, and they look good!
However, could you add the new setting to the documentation, and update the settings in the documents?
https://github.com/qmk/qmk_firmware/blob/master/docs/feature_rgblight.md#effect-and-animation-settings
|
Also, the only downside I see to this change is that it definitely increases the size of the code for this animation, and causes a number of boards to be too large. That's not really an issue with this PR, though. But if you have ideas for optimizing the code, it may be nice to include here. Otherwise, no worries. |
|
Awesome, gonna fix the stuff mentioned here, then it should be ready to merge 😇 |
There was a problem hiding this comment.
Okay, I know this is a bit weird of a request, but instead of using pow, could you just handle the math in the function?
It makes it a bit more readable, and most importantly, it's like a 2k difference in firmware size. (this being why it's weird). Whatever pow does, it's NOT friendly for AVR. And since most of our boards use AVR ....
|
Also, a good board to test with is the singa. It seems to compile the largest. |
|
Replaced the pow now, and made some other tweaks, but for some reason, the firmware is still to large (for the singa). Could it be that the EDIT: well yes, without any float in the code, we gain a cool 1200 bytes. Still not enough, but close. |
|
@Maxr1998 AVRs don't have an FPU, so doing float math is very expensive both flash-wise and computationally. |
|
Some more changes, but still a bit less than 400 bytes over. With the whole function body commented out, I only have 100 bytes remaining on the Singa. Actually haven't got any ideas left how I could compress my code to 1/5th of its current size 🙈. More suggestions? |
|
You can get a much more accurate representation of the travis build locally with Travis shows |
|
Interesting, the docker build actually passes for me locally. So, does that mean the PR is mergeable then? Still any ideas how to strip some bytes from the binaries? |
|
Technically yes, the PR would be mergeable as all the boards are now building. Though the question I would pose is, do we want to accept an increase in firmware of 340 bytes for what is arguably the worse animation out of the bunch. Personally, I wouldn't care if the animation was just flat out removed... |
|
maybe make it disabled out by default? |
|
@zvecr yikes. Well, I kinda like it, so maybe really make it disabled by default, as in, remove it from |
|
Yeah disabled by default could work (which doesn't have to be done within this PR). Lets see what some of the other collaborators say on the situation. |
|
I’d be alright with removing it from default |
|
Ditto |
|
I have created this issue to further discuss what should be the default:) |
|
@yiancar thanks for creating the issue, this seems to be a good idea generally. However, I think that should be handled inside another PR. I can do a quick draft tomorrow, removing some of the more complex/specific animations from the default already. |
|
Forgot about this a bit, but I added a draft for the changes in #7680. Feel free to discuss those over there. |
|
Thank you for your contribution! |
Maxr1998
force-pushed
the
christmas_tweaks
branch
from
48a981b to
00088c9
Compare
|
@tzarc done 😄 |
|
Also tested my changes, obviously. Now I'm kinda in a Christmas mood 🤔 🎅 |
Perfect outcome! ;) Thanks for the submission, merged! |
) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
…k#7648) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
…k#7648) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
…k#7648) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
…k#7648) * Tweak the Christmas animation effect to be less harsh on the eyes * Further improve the tweaked Christmas animation code - Use constants where it makes sense - Instead of complicated math, use a static variable to keep track if it's animating from or to red - Don't use pow (but a simple macro instead) - Using floating point math is necessary for the fraction in the cubic bezier function to work * Update docs for the tweaked Christmas animation effect * Further improve memory usage - Don't use floats, but 32 bit ints instead (where needed) - Replace limits.h with constant * Fix typo
Description
I found that the Christmas animation is a bit harsh on the eyes, jumping directly from red to green and back, and I thought it would be a good idea to make the effect a bit smoother (especially it's Christmas we're talking about, a rather calm & cozy holiday).
So, my idea generally was to, instead of "jumping" between the colors, I use a cubic-bezier formula to animate over the colors in between (e.g. orange, yellow), which obviously is pretty easily done by animating the hue.
The more complicated thing was to tweak these curves to a) actually have a smooth animation and b) make the yellow not stand out as much, that's why I also animated the value.
I tested my changes, and I think the animation looks much better now, but there are still a few problems with my code.
RGBLIGHT_EFFECT_CHRISTMAS_INTERVALby a lot, essentially breaking all setups customizing that value.get_interval_timewith different speed modes which would probably be better suited for that.So, it might also be a good idea to, instead of changing the existing effect, add a completely new one, thus leaving more choice to the users and avoiding any breaking changes.
I'd be interested to hear what you think, and I recommend to just try out the updated effect 😀
Types of Changes
Checklist