Timed animation #67
Timed animation #67
Conversation
|
I'm not sure why but somehow putting a PR for this branch too included the changes I had with the other PR branch. Not sure if I should just delete the extra "volume.py" file here? |
|
@kattni I think this is read, could you take a look thanks :) |
There was a problem hiding this comment.
@gamblor21 Hey! We're taking a look at this now. You submitted it before we started using the reuse licensing. Please update the licensing along with any possible suggestions we may have regarding the code.
I would suggest running it through pre-commit to make sure it fits with our current standards.
Regardless of which PR is merged first, it will likely cause merge conflicts to have that file in here, as well as causing issues if any changes are needed on the other PR. I would suggest deleting the |
|
@gamblor21 i have been thinking about this PR. It has reminded me that I feel like the current auto-advance API is a bit convoluted. Instead of a new sequence class, I feel like having Animation instances be in control of their advancement behaviour would be better. Thus refactoring Animation, AnimationGroup, and AnimationSequence would make it all much more flexible and less complicated to use. I can be more specific if you agree. |
|
Just a note that I have not forgot about this PR and hope to take a look at the comments soon. @rhooper I'm not 100% sure what you mean changing the advancement but you know this area better then I do. My main use of this was to run have short animations run repeatedly for a period of time that was synced to music playing. E.g. Rainbow affect for 15 seconds then key change and start a blinking pattern. I am willing to look at refactoring things if there is a better way to do so for the future. |
|
Checking in on these, @gamblor21 you still have these new animation PRs on your radar? I think it would be great to include an example script illustrating the new animations when they are added as well. |
They are still on my radar (I have browser tabs open to the PRs). I will try to make time to take a look at them in the next couple weeks or so. |
|
I believe part of the concern is that we're trying to work on keeping this library a bit slimmer. However, on some level, adding more animations isn't a huge issue since they're in separate modules already. @rhooper plans to make some changes to this library over the weekend, in the core and helper areas, if there is time to be had. She can also take another look at these animation PRs as well while looking into the rest of it. |
There was a problem hiding this comment.
I added commits to tweak a few things:
- merge from main
- fix the license syntax to comply with re-use check
- added examples in the repo for both new types of functionality: volume animation, and timed sequence
I tested both of the new functionalities successfully with the supplied examples. The timed sequence one on a Feather S3 TFT with Neopixel Featherwing, and the Volume animation one with a PyPortal and Neopixel strip.
I think it's easiest to just keep both changes in this PR and close the other one in favor of this one. The new classes are separated enough to make evaluation of them seperately easy.
This is passing actions now, and looks good to me at this point.
@kattni or @rhooper is it okay to merge this one as-is for now? Any refactoring or further changes could still come in a follow-up PR at any time.
|
I gave this another run and look over. Going to merge now. We can have followup if ever there is a desire to make further changes. |
Updating https://github.com/adafruit/Adafruit_CircuitPython_LED_Animation to 2.7.3 from 2.7.2: > Merge pull request adafruit/Adafruit_CircuitPython_LED_Animation#67 from gamblor21/timed-animation Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries
A animation sequence that lets you specify how long each animation runs, individually. Allows for more fine grained control of the cycle of animations (good for syncing to music).