Added is_running attribute to :class:~.Animation and made :class:~.AnimationGroup aware of running animations

[behackl]

Motivation

There are two severe issues with how AnimationGroup runs right now (that this PR is concerned with 🙃):

• AnimationGroup.interpolate calls interpolate on all animations included in the group, independent of whether the animation time is within their corresponding start_time/end_time intervals determined by AnimationGroup.build_animations_with_timings.
• AnimationGroup.begin currently causes all included animations to call begin() at the beginning of the group. This causes problems in situations where the same mobject is animated in several different animations in an AnimationGroup.

Explanation for Changes

The first issue is solved in a more or less clean way, for the second I am not really sure that calling begin and finish in the way currently implemented by this PR will work in all situations. If someone more familiar with animations has a better idea, I'd appreciate it.

Testing Status

This fixes the behavior in the example

class SuccessionWorkaround(Scene):
    def construct(self):
        sq = Square(color=color.RED, fill_opacity=0.7)
        anim = AnimationGroup(
            ApplyMethod(sq.shift, LEFT),
            ApplyMethod(sq.shift, RIGHT),
            lag_ratio=1
        )
        self.play(anim)
        self.wait()

as discussed on Discord, and the square first moves left, and then right:

Acknowledgement

• I have read the Contributing Guidelines

[leotrs]

Good stuff.

[leotrs]

Question: an animation only has a starting_object if its begin() method has been called, correct? So this conditional is checking whether the animation is currently running?

If so, we should make note to implement a cleaner way of doing that in the future. Not in this PR tho.

[behackl]

From what I've looked at today, I think there are some animations that don't set starting_object when their begin is called. It might actually be better to refactor this still on this PR: maybe by adding a boolean has_started set to False on Animation.__init__, and to true in all the implementing begin methods?

[leotrs]

Agreed. Suggest is_running.

[leotrs]

I can see why this finish has to go. But if we just delete it, then the parent class' finish method will be called. For extra clarity, we may just want to override finish with a pass so that the parent class is not called.

[behackl]

I haven't really investigated what finish actually does for animations, but it sounds like a good approach.

[behackl]

I've been trying to investigate the two errors in test_indication.py -- and I am really confused and stuck. According to pytest --show_diff, the differences look like this:

for FlashTest and ShowPassingFlashAroundTest, respectively. However, I can't see these fragments when rendering with, e.g., manim test.py ShowPassingFlashAroundTest -ql --disable_caching -s:

What could be broken here that pytest and direct rendering yield different results? Any ideas, @ManimCommunity/tests?

[leotrs]

Looked into it for a bit for I got nothing 🤔

[huguesdevimeux]

Concerning tests :
Control data may be wrong, as they have been generated a looong time ago, at a time where manim was still a newborn, so don't expect them to be 100% accurate and sure.

However, here, I think it's normal if they failed. SowpassingFlash looks like this on master latest :

So, the last frame is in theory empty. This does not match with the generated output of the test (i.e, at the end, there should be nothing, (this is btw what you got)).

For Flash, same,

At the end, there should be only the square

(on a side note, is it supposed to be like this? Isn't the flash thing supposed to be around the square?)

[behackl]

Concerning tests :
Control data may be wrong, as they have been generated a looong time ago, at a time where manim was still a newborn, so don't expect them to be 100% accurate and sure.

Sure, but as you say: in this case, the data seems reasonable to me.

What worries me is that I don't obtain the same fragments (i.e., the Square and the remaining Flash) when rendering with -ql -s, and that I really don't have an explanation for. I thought that this should precisely give the testoutput, no?

(on a side note, is it supposed to be like this? Isn't the flash thing supposed to be around the square?)

I was also wondering about that; I'm not sure. Maybe Flash is mainly supposed to be used with Dot etc.?

[huguesdevimeux]

What worries me is that I don't obtain the same fragments (i.e., the Square and the remaining Flash) when rendering with -ql -s, and that I really don't have an explanation for. I thought that this should precisely give the testoutput, no?

What do you mean? It is probably normal due ot the t value issue #183. (maybe).

I was also wondering about that; I'm not sure. Maybe Flash is mainly supposed to be used with Dot etc.?

Aaaand. I don't know. Probably.

[leotrs]

Flash is located at the center of the Mobject. I don't know if there is a way of changing the radius around the center. But what you got seems to me what is intended.

I agree with @behackl , I think there is something else going on here.

[behackl]

What worries me is that I don't obtain the same fragments (i.e., the Square and the remaining Flash) when rendering with -ql -s, and that I really don't have an explanation for. I thought that this should precisely give the testoutput, no?

What do you mean? It is probably normal due ot the t value issue #183. (maybe).

Look at the images I've posted above: the third one is ShowPassingFlashAroundTest rendered using manim test.py ShowPassingFlashAroundTest -ql --disable_caching -s, and it seems that this is a fully black picture (opening the image with an image editor didn't reveal any differently colored pixels). However, at the same time the output that is rendered within the test and that is shown in the diff above has this weird square fragment in it.

I don't know why rendering it with -ql -s on the one hand and within pytest on the other hand yields different results.

[behackl]

I'll put some more work into this to implement the refactor discussed with @leotrs above -- but I still need help with figuring out what actually causes the tests to fail.

[leotrs]

I just came back to look at this for a sec. I checked out this branch and rendered both ShowPassingFlashAroundTest and FlashTest. The former one is totally empty! And the latter one only has the square, no flash at all.

@behackl, the frame you posted is generated with -s. Did you actually look at the full rendered video? 😅

However, the kicker is that I get the same "generated" and "expected" diffs as @behackl does. This means that when ran with pytest, there is a flash, but when rendering a video or last frame, there is no flash. The plot thickens!

Note: @huguesdevimeux originally posted the result of rendering these on master, not on this branch. Also, he rendered ShowPassingFlashTest, not ShowPassingFlashAroundTest! (These are too easy to mix up - we should change their names eventually.)

[behackl]

I sort of forgot about this PR for a while; I've now incorporated the changes discussed above:

• Animations now have an attribute is_running which is initially False, set to True once Animation.begin() is called, and then set to False again in Animation.finish()
• AnimationGroup should now only deal with those child animations which are currently active; in particular it doesn't begin all child animations at the same time, and it only calls update_mobject for those animations that are currently running (in contrast to all of them all the time).

Ready for review.

[jsonvillanueva]

The changes look good generally, but I think something broke due to recent changes in the library since this has last been looked at... particularly with the following code:

class MyTest(Scene):
    def construct(self):
        s = Square()
        self.play(
            AnimationGroup(
                s.animate.shift(LEFT),
                s.animate.shift(RIGHT),
                lag_ratio=1
            )
        )

MyTest.mp4

Actually, even using the earlier example above:

class MyTest(Scene):
    def construct(self):
        sq = Square(color=color.RED, fill_opacity=0.7)
        anim = AnimationGroup(
            ApplyMethod(sq.shift, LEFT),
            ApplyMethod(sq.shift, RIGHT),
            lag_ratio=1
        )
        self.play(anim)
        self.wait()

MyTest.mp4

[behackl]

The changes look good generally, but I think there's some edge cases to consider.. particularly with the following code:

class MyTest(Scene):
    def construct(self):
        s = Square()
        self.play(
            AnimationGroup(
                s.animate.shift(LEFT),
                s.animate.shift(RIGHT),
                lag_ratio=1
            )
        )

MyTest.mp4

I forgot to update the PR description. The problem with the example there is that multiple animations are applied to the same mobject, which is discouraged in general.

The intention of this PR was that

class AG(Scene):
    def construct(self):
        s1, s2, s3 = (Square() for _ in range(3))
        VGroup(s1, s2, s3).arrange(DOWN).center()
        self.play(
            AnimationGroup(
                s1.animate.shift(RIGHT).set(fill_opacity=1),
                s2.animate.shift(RIGHT).set(fill_opacity=1),
                s3.animate.shift(RIGHT).set(fill_opacity=1),
                lag_ratio=1
            )
        )

should essentially behave like

class AG(Scene):
    def construct(self):
        s1, s2, s3 = (Square() for _ in range(3))
        VGroup(s1, s2, s3).arrange(DOWN).center()
        self.play(s1.animate.shift(RIGHT).set(fill_opacity=1))
        self.play(s2.animate.shift(RIGHT).set(fill_opacity=1))
        self.play(s3.animate.shift(RIGHT).set(fill_opacity=1))

(In particular: the mobjects shouldn't all be added at the beginning of the animation, but only when it is "their turn" in the animation timeline.)

However, after just checking my example again, it doesn't seem like this PR is changing this behavior. I'm leaning towards simply closing this PR for now -- the animations in composition.py (or actually the general logic behind animations) deserves a refactor at some point in the future anyways.

[jsonvillanueva]

I'm leaning towards simply closing this PR for now -- the animations in composition.py (or actually the general logic behind animations) deserves a refactor at some point in the future anyways.

It would definitely be nice to provide time ranges for AnimationGroup with start/end times individually... rather than having to sort out the math with lag_ratio/length. Definitely an enhancement needed here.

But since this PR isn't changing the behavior, whether you wish to close this PR and reopen in the future, or start a new PR altogether, I'd be interested in setting start/end times directly.