Fixed animations introducing or removing objects #2396

marcin-serwin · 2021-12-25T02:14:37Z

Overview: What does this pull request change?

:class:.ShowPassingFlash now removes objects when the animation is finished
Added introducer keyword argument to :class:.Animation analogous to remover
Updated :class:.Graph vertex addition handling

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Code:

class IntroducersInSuccession(Scene):
    def construct(self):
        mobjs = [Square() for _ in range(11)]
        VGroup(*mobjs[:6]).arrange().shift(2 * UP)
        VGroup(*mobjs[6:]).arrange().shift(2 * DOWN)
        self.play(
            Succession(
                Circumscribe(mobjs[0]),
                ShowSubmobjectsOneByOne(mobjs[1]),
                Create(mobjs[2]),
                FadeIn(mobjs[3]),
                Write(mobjs[4]),
                Unwrite(mobjs[5]),
                GrowFromCenter(mobjs[6]),
                ShowPassingFlash(mobjs[7]),
                SpinInFromNothing(mobjs[8]),
                FadeOut(mobjs[9]),
                DrawBorderThenFill(mobjs[10]),
            )
        )

Before:

IntroducersInSuccession.mp4

After:

IntroducersInSuccession.mp4

Reviewer Checklist

The PR title is descriptive enough for the changelog, and the PR is labeled correctly
If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
If applicable: newly added functions and classes are tested

behackl · 2021-12-31T01:24:57Z

I like the semantics of this approach! In particular, the new method for preparing the scene seems useful. I'll have to think a bit more about this and how it interacts with the already present machinery for adding mobjects (the moving_mobjects and the Scene.add_mobjects_from_animations stuff), but in general this looks very clean.

For now, thank you!

behackl

Overall, I like these changes – thanks again for your efforts! There are some things I am somewhat unhappy with (like the fact that this makes it less easy to understand when in an Animation's life cycle mobjects are added to the scene), but I think the fact that this PR makes Animations more symmetric with the introducer keyword arg on the one hand, and the setup_scene method on the other hand, outweighs my concerns.

It would be good to have another dev look over this / approve it before it is merged.

behackl · 2022-01-06T13:18:25Z

manim/animation/composition.py

@@ -112,6 +117,7 @@ def interpolate(self, alpha: float) -> None:
 class Succession(AnimationGroup):
    def __init__(self, *animations: Animation, lag_ratio: float = 1, **kwargs) -> None:
        super().__init__(*animations, lag_ratio=lag_ratio, **kwargs)
+        self.scene = None


I see why this is necessary, but I don't quite like it.

I think this was needed only because some tests didn't call setup_scene before begin. I've updated them and it's no longer necessary.

behackl · 2022-01-06T17:56:32Z

manim/mobject/graph.py

@@ -497,7 +497,7 @@ def __getitem__(self: "Graph", v: Hashable) -> "Mobject":
    def __repr__(self: "Graph") -> str:
        return f"Graph on {len(self.vertices)} vertices and {len(self.edges)} edges"

-    def _add_vertex(
+    def _create_vertex(


I like these changes to Graph vertex creation, regardless of the changed overridden animation. I do have to admit: I don't like the amount of bending of the Animation framework that needs to be done to make custom overrides of vertex creation work. Maybe it is not worth keeping the @override_animate idea around.

Darylgolden

LGTM in general, just some questions and comments.

Darylgolden · 2022-01-19T12:56:02Z

manim/animation/animation.py

        if self.is_remover():
            scene.remove(self.mobject)

+    def setup_scene(self, scene: "Scene") -> None:


Shouldn't this be prefaced with a single underscore to mark it as only for internal use?

Darylgolden · 2022-01-19T12:56:45Z

manim/animation/animation.py

@@ -434,6 +456,16 @@ def is_remover(self) -> bool:
        """
        return self.remover

+    def is_introducer(self) -> bool:


Given that this method only returns the value of an attribute, is it really needed?

It's made this way to mirror the is_remover method.

Darylgolden · 2022-01-19T12:59:54Z

manim/animation/animation.py

        self.suspend_mobject_updating: bool = suspend_mobject_updating
        self.lag_ratio: float = lag_ratio
+        self._on_finish: Callable[["Scene"], None] = _on_finish


I think it's better to mark typehints with the actual class rather than a string.

Darylgolden · 2022-01-19T13:00:50Z

manim/mobject/graph.py

-            The mobject to be used as the vertex. Overrides all other
-            vertex customization options.
-        """
+    ) -> Tuple[Hashable, np.ndarray, dict, "Mobject"]:


Typehint, see above

Darylgolden · 2022-01-19T13:01:12Z

manim/mobject/graph.py

+        vertex: Hashable,
+        position: np.ndarray,
+        vertex_config: dict,
+        vertex_mobject: "Mobject",


Typehints, see above

for more information, see https://pre-commit.ci

Darylgolden

LGTM, thank you!

marcin-serwin marked this pull request as draft December 25, 2021 03:16

marcin-serwin marked this pull request as ready for review December 25, 2021 14:07

behackl self-requested a review December 31, 2021 01:25

behackl added the enhancement Additions and improvements in general label Dec 31, 2021

behackl approved these changes Jan 6, 2022

View reviewed changes

CountingBeeps mentioned this pull request Jan 6, 2022

Prepare new release, v0.14.0 #2411

Merged

marcin-serwin force-pushed the circumscribe-bug branch from 2d33b57 to aef14f3 Compare January 7, 2022 10:33

Darylgolden requested changes Jan 19, 2022

View reviewed changes

marcin-serwin added 8 commits January 20, 2022 17:17

Replace finish with clean_up_from_scene in ShowPassingFlash

d2a50b9

Add introducer flag to animations

5ed918d

Mark some animations as introducers

b3ad8e7

Handle animating graph

de73b24

Update tests

4edf4e3

Update tests to provide mocked scene

f6ce8b5

Make setup_scene internal

4ed29a8

Replace string annotation with new typehints

7a77ee6

marcin-serwin force-pushed the circumscribe-bug branch from aef14f3 to 7a77ee6 Compare January 20, 2022 16:23

[pre-commit.ci] auto fixes from pre-commit.com hooks

26ac33b

for more information, see https://pre-commit.ci

Darylgolden approved these changes Jan 22, 2022

View reviewed changes

Darylgolden changed the title ~~Fix animations introducing or removing objects~~ Fixed animations introducing or removing objects Jan 22, 2022

Darylgolden added 2 commits January 22, 2022 15:50

Merge branch 'main' into circumscribe-bug

74fdc65

Merge branch 'main' into circumscribe-bug

a0306e4

Darylgolden merged commit a96ad3e into ManimCommunity:main Jan 23, 2022

JeroSQ mentioned this pull request Feb 20, 2022

Circumscribe shows shape after animation if run_time is passed outside the Circumscribe instance and is different than 1 #2563

Closed

behackl mentioned this pull request Feb 28, 2022

Mobject Ordering Regression in 0.15 #2585

Closed

ad-chaos mentioned this pull request Mar 6, 2022

Succession() act incorrectly when display Mobjs successively #1256

Closed

Laifsyn mentioned this pull request Oct 10, 2024

Call .finish() for every animation in AnimationGroup on finish #3951

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed animations introducing or removing objects #2396

Fixed animations introducing or removing objects #2396

marcin-serwin commented Dec 25, 2021 •

edited by Darylgolden

Loading

behackl commented Dec 31, 2021

behackl left a comment

behackl Jan 6, 2022

marcin-serwin Jan 7, 2022

behackl Jan 6, 2022

Darylgolden left a comment

Darylgolden Jan 19, 2022

Darylgolden Jan 19, 2022

marcin-serwin Jan 20, 2022

Darylgolden Jan 19, 2022

Darylgolden Jan 19, 2022

Darylgolden Jan 19, 2022

Darylgolden left a comment

Fixed animations introducing or removing objects #2396

Fixed animations introducing or removing objects #2396

Conversation

marcin-serwin commented Dec 25, 2021 • edited by Darylgolden Loading

Overview: What does this pull request change?

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

behackl commented Dec 31, 2021

behackl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darylgolden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darylgolden left a comment

Choose a reason for hiding this comment

marcin-serwin commented Dec 25, 2021 •

edited by Darylgolden

Loading