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

Timed animation #67

Merged
merged 8 commits into from Aug 7, 2023
Merged

Timed animation #67

merged 8 commits into from Aug 7, 2023

Conversation

gamblor21
Copy link
Member

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).

@gamblor21
Copy link
Member Author

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?

@tannewt tannewt requested a review from kattni September 29, 2020 21:54
@jposada202020
Copy link
Contributor

@kattni I think this is read, could you take a look thanks :)

@evaherrada evaherrada changed the base branch from master to main June 7, 2021 17:38
Copy link
Contributor

@kattni kattni left a comment

Choose a reason for hiding this comment

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

@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.

@kattni
Copy link
Contributor

kattni commented Oct 31, 2021

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?

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 volume.py file from this PR.

@rhooper
Copy link

rhooper commented Oct 31, 2021

@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.

@gamblor21
Copy link
Member Author

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.

@FoamyGuy
Copy link
Contributor

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.

@gamblor21
Copy link
Member Author

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.

@kattni
Copy link
Contributor

kattni commented Apr 25, 2022

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.

Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

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

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.

@FoamyGuy FoamyGuy mentioned this pull request Jun 30, 2023
@FoamyGuy
Copy link
Contributor

FoamyGuy commented Aug 7, 2023

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.

@FoamyGuy FoamyGuy merged commit ed8e0ba into adafruit:main Aug 7, 2023
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Aug 7, 2023
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.

None yet

5 participants