Simplify the animation logic within Scene

[eulertour]

Major changes include:

• Removed the background argument from CairoRenderer.update_frame(). It was only used once, to return a static image which was later passed back to the CairoRenderer and composed with an image that moved as the animation progressed. I replaced its functionality with CairoRenderer.save_static_mobject_data(), which saves the image to the renderer instead.

• Added Scene.get_moving_and_static_mobjects() to simplify the logic prior to beginning the animation loop in which static and moving mobjects are separated.

• Removed Scene.begin_animations(), Scene.finish_animations(), Scene.clean_up_animations(), Scene.progress_through_animations(), and Scene.update_animation_to_time(). They were each only used once and were not very complicated, so I moved their contents into Scene.play_internal().

• Removed these lines from the logic that was previously in Scene.finish_animations(). AFAICT hey don't do anything.

• Removed several instance variables from Scene. Scene.mobjects_from_last_animation was only used once, Scene.last_t was only used in an antipattern way to pass data to a function.

Depends on #148

[leotrs]

Do you think this can wait until after the release? Given that we don't have enough tests for all the CLI flags, I'm a bit hesitant to touch the rendering logic right before a release.

[eulertour]

Sure, but we can still get the review done until then.

Depends on #148

[eulertour]

I feel like the dep bot is never working 😕

[huguesdevimeux]

Just a quick question about these removed lines.
Suppose that animation n is skipped, but not animation n+1, and animation n+1 uses some stuff that were supposedly updated by animation n, in any way. Will this work without these lines? The mobject that was supposed to be updated in animation n wouldn't have been.

[eulertour]

Manim handles animation skipping by setting the time progression used in the animation loop to one which holds a single value, the length of the animations being played.

manim/manim/scene/scene.py

Lines 562 to 566 in 3b6519a

	if file_writer_config["skip_animations"] and not override_skip_animations:
	times = [run_time]
	else:
	step = 1 / self.renderer.camera.frame_rate
	times = np.arange(0, run_time, step)

so the relevant mobjects will have already been updated by the time they reach this point.

[behackl]

Apart from a missing docstring, this looks good to me, thank you for your effort with this cleanup!

I don't necessarily agree with you overriding the "2 reviewers"-rule and merging this without any approvals. To be fair, you asked several times and I could have indicated that I planned looking at this. At the same time I don't think it would have halted any further progress if this PR would have been open for a little longer.

[behackl]

New methods added should have at least a basic docstring (oneline summary); and in the ideal case a list of parameters.

[behackl]

These argument lists are not correctly formatted at all. (But this is fixed in #576, AFAIR.)

[behackl]

git bisect says that merging this introduced a problem with Mobject.become: The example

class FadeInAndOut(Scene):
    def construct(self):
        square = Square(color=BLUE).shift(2 * UP)
        annotation = Text("Fade In", height=0.8)
        self.add(annotation)
        self.play(FadeIn(square))

        annotation.become(Text("Fade Out", height=0.8))
        self.add(annotation)
        self.play(FadeOut(square))

from our documentation renders as

Same thing if the call to become is replaced by playing a Transform animation; the old text is simply kept on screen.

[eulertour]

Where in our documentation is this example?

[behackl]

Where in our documentation is this example?

It's not yet included, it will be added with #576.