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

feat: add repeat-from animation mode #40

Merged
merged 1 commit into from Mar 2, 2022
Merged

Conversation

cristicbz
Copy link
Contributor

Adds AnimationMode::RepeatFrom(index) which repeats from a given index rather than going all the way back to zero. This super handy for simple animations like e.g. a torch being lit then staying lit, or a missile lighting up its motor and then looping.

Because AnimationMode is non_exhaustive I believe this is not a breaking change.

I considered getting rid of AnimiationMode::Repeat and using Animation::RepeatFrom(0) in fn repeat(self), but we can't do this without a breaking change.

I was wondering, why even expose the AnimationMode enum? It looks to me like it's not actually returned or taken as argument by any APIs. Could it be private to the crate? It would make it easier to make changes like this, which generalise an existing animation mode without it being a breaking change.

@jcornaz
Copy link
Owner

jcornaz commented Mar 2, 2022

Adds AnimationMode::RepeatFrom(index) which repeats from a given index rather than going all the way back to zero. This super handy for simple animations like e.g. a torch being lit then staying lit, or a missile lighting up its motor and then looping.

sounds good to me, thanks for the contribution 👍

Because AnimationMode is non_exhaustive I believe this is not a breaking change.

indeed, it is not a breaking change.

I considered getting rid of AnimiationMode::Repeat and using Animation::RepeatFrom(0) in fn repeat(self), but we can't do this without a breaking change.

I was wondering, why even expose the AnimationMode enum? It looks to me like it's not actually returned or taken as argument by any APIs. Could it be private to the crate? It would make it easier to make changes like this, which generalise an existing animation mode without it being a breaking change.

Yeah, I made the mistake of making it public even though that wasn't needed. But we can go for the solution you suggest without making a breaking change. here is how:

  1. Create a new enum which is pub(crate). You may simply name it Mode. (no need to use non_exhaustive if it is private)
  2. Change internal implementation to use the new enum.
  3. Deprecate the now unused old public enum AnimationMode.

You may do that in a different PR if you wish. Or you may do it here, as you prefer.

Let me know if you don't have time or don't want to do that refactoring, in that case, I'll merge as-is and refactor myself later.

@jcornaz jcornaz self-assigned this Mar 2, 2022
@jcornaz jcornaz self-requested a review March 2, 2022 08:42
@jcornaz jcornaz added the enhancement New feature or request label Mar 2, 2022
@jcornaz jcornaz changed the title feat: add repeat from animation mode feat: add repeat-from animation mode Mar 2, 2022
@cristicbz cristicbz force-pushed the repeat-from branch 2 times, most recently from 631485d to c3ed369 Compare March 2, 2022 16:43
@cristicbz
Copy link
Contributor Author

cristicbz commented Mar 2, 2022

@jcornaz Ok, great, didn't want to go hacking on your codebase before checking with you on the design! Just push-forced a commit that does the refactor you described---the old changes didn't really make sense as the first commit so I replaced the commit instead.

I can have a go at splitting up into two commits instead: refactor first, add RepeatFrom second, but it's a bit of a faff and weren't sure if you cared enough.

Copy link
Owner

@jcornaz jcornaz left a comment

Choose a reason for hiding this comment

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

looks good to me thanks!

@jcornaz jcornaz merged commit 586859c into jcornaz:main Mar 2, 2022
@cristicbz cristicbz deleted the repeat-from branch March 2, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants