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

Bugfix: allow new-style method animation to be used in animation groups #906

Merged

Conversation

behackl
Copy link
Member

@behackl behackl commented Jan 4, 2021

Motivation

New-style method animations (using the .animate syntax) can't currently be included in AnimationGroup or Succession.

Overview / Explanation for Changes

With this change, animations in the list are checked whether they are a _AnimationBuilder -- and if so, the corresponding animation is built and added to self.animations.

Oneline Summary of Changes

- Bugfix: allow new-style method animation to be used in animation groups (:pr:`PR NUMBER HERE`)

Testing Status

Added a test for AnimationGroup and Succession, passing locally.

Acknowledgements

Reviewer Checklist

  • Newly added functions/classes are either private or have a docstring
  • Newly added functions/classes have tests added and (optional) examples in the docs
  • Newly added documentation builds, looks correctly formatted, and adds no additional build warnings
  • The oneline summary has been included in the wiki

@behackl behackl added pr:bugfix Bug fix for use in PRs solving a specific issue:bug pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! labels Jan 4, 2021
@friedkeenan
Copy link
Member

friedkeenan commented Jan 4, 2021

Perhaps there should be a single function to handle this stuff that could be used in AnimationGroup and in Scene, something like

def handle_animation(anim):
    if isinstance(anim, _AnimationBuilder):
        return anim.build()

    if isinstance(anim, Animation):
        return anim

    raise TypeError(...)

@behackl
Copy link
Member Author

behackl commented Jan 5, 2021

dd54e1f now delegates building of an animation from an animation factory to a dedicated prepare_animation function.

Copy link
Member

@friedkeenan friedkeenan 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, just have a couple not very important nitpicks

manim/animation/composition.py Outdated Show resolved Hide resolved
manim/animation/composition.py Outdated Show resolved Hide resolved
behackl and others added 2 commits January 6, 2021 03:21
Co-authored-by: friedkeenan <friedkeenan@protonmail.com>
@behackl behackl added this to the Release v0.2.1 milestone Jan 9, 2021
@behackl behackl merged commit c25b89c into ManimCommunity:master Jan 9, 2021
@behackl behackl deleted the animationbuilder-animationgroup branch January 9, 2021 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:bugfix Bug fix for use in PRs solving a specific issue:bug pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants