3b1b / manim

Animation engine for explanatory math videos
MIT License
67.99k stars 6.07k forks source link

Unpropagated state in Succession #370

Closed bhbr closed 5 years ago

bhbr commented 5 years ago
        circle = Circle()
        self.play(Succession(
            ApplyMethod(circle.move_to, 3*RIGHT),
            ApplyMethod(circle.move_to, 3*UP),
        ))

I expect this to move the circle first to the right, then diagonally up and left. However, it jumps back to the origin and moves up from there. This wouldn't be surprising if the implementation of Succession did not contain some state propagation mechanism (animation/composition.py:68-82) whose job is to initialize the second animation with the final state of the first. But to my eyes, something in that piece of code does not work correctly.

(The workaround to use successive play calls is not an option, as I am working on an animation class Draw that traces a pencil over a sequence of paths, while also moving the pen in between. So the succession of animations must be generated programatically.)

bhbr commented 5 years ago

ezgif-4-e70f0dead69f

Elteoremadebeethoven commented 5 years ago

You can try:

self.play(circle.move_to, 3*RIGHT,
             circle.move_to, 3*UP)
eulertour commented 5 years ago

Looking at the comment for Succession https://github.com/3b1b/manim/blob/465951ab88211e4a07e1a17b7ad370cb662f7d06/animation/composition.py#L35-L46 I think what you want to do is

circle = Circle()                                                       
self.play(Succession(                                                   
    ApplyMethod, circle.move_to, 3*RIGHT,                               
    ApplyMethod, circle.move_to, 3*UP,     
)) 

But this still strikes me as unexpected behavior, as I would expect this to move right, then move up from the right-shifted position.

3b1b commented 5 years ago

The problem here is that Succession should not be considered a complete class. Making it play well with Scene is somewhat tricky, enough so that it makes me think something should fundamentally change either the way Animations work, Scene works, or some combination.

The issue here is that an underlying assumption of Animation (and Transform in particular) is that initialization happens immediately before the animation takes place. For example, Transform creates a copy of the first argument which it uses for reference during the interpolation, which is why you see the behavior above.

One possible solution is to update Animation, and any relevant subclasses, to factor out the part of initialization that depends on this assumption, and to have Scene and Succession each be responsible for triggering that special initialization only at the moment immediately before the animation begins.

eulertour commented 5 years ago

But this still strikes me as unexpected behavior, as I would expect this to move right, then move up from the right-shifted position.

I was confusing move_to() with shift(), so it's actually working as expected. That said, the code for Succession does seem needlessly complicated for a comparatively simple effect. I'll have to look into simplifying it somehow.

bhbr commented 5 years ago

I found a way to make it work, writing a wrapper class ScheduledAnimation that initializes the wrapped animation just in time, in update(0). This required suppressing premature update calls upon initialization through the introduction of an 'update_on_init' flag.

I still need to test this solution properly. I'll keep you posted whether it does.

3b1b commented 5 years ago

This is now fixed.