manim icon indicating copy to clipboard operation
manim copied to clipboard

Fixing the behavior of `.become` to not modify target mobject via side effects fix color linking

Open MrDiver opened this issue 2 years ago • 2 comments

Fixes #3505

So originally .become transforms the target mobject and moves all the values in to the transformed mobject. Which makes them linked in many ways because they share the same references to the data.

This resulted in some weird behavior when changing one mobject causes another to change without any direct causation or intuition for that.

This further resulted in a test of ours being reliant on this behavior because it was not visible what the side-effect happened to change.

Originally the test named test_become resulted in this

Code
def test_become(scene):
    s = Rectangle(width=2, height=1, color=RED).shift(UP)
    d1, d2, d3 = (Dot() for _ in range(3))

    s1 = s.copy().become(d1, match_width=True).set_opacity(0.25).set_color(BLUE)
    s2 = (
        s.copy()
        .become(d2, match_height=True, match_center=True)
        .set_opacity(0.25)
        .set_color(GREEN)
    )
    s3 = s.copy().become(d3, stretch=True).set_opacity(0.25).set_color(YELLOW)

    scene.add(s, d1, d2, d3, s1, s2, s3)

grafik

Which seems correct on the first look, but if we take a closer look at what is actually displayed in that scene, we realize that d1,d2,d3 are also displayed, which should be dots.

Which then results in this behavior when we actually try to unlink the data by copying all colors from the mobjects instead of linking them

For example, by adding an if statement in interpolate_color inside VMobject

grafik

Now the question becomes why does it look so weird all of a sudden when we just copied the colors. Right so the problem here lies that now that the colors are unlinked the Dots retain their original color which is WHITE by default which makes it so bright all of a sudden.

But why where they modified in the first place. If we extend the .become method to copy the target mobject instead of transforming it we result in this scene.

grafik

Which suddenly makes a lot more sense, because we have 3 Dots overlayed onto each other in the middle with the color white. And then actual mobjects that should be displayed also have the correct color and became a Dot, but instead of transforming their target, they only transformed themselves.

Which then results in a lot more intuitive behavior and not a c-style return parameter behavior.

It will then also simplify the test to the following

Code
def test_become(scene):
    s = Rectangle(width=2, height=1, color=RED).shift(UP)
    d = Dot()

    s1 = s.copy().become(d, match_width=True).set_opacity(0.25).set_color(BLUE)
    s2 = (
        s.copy()
        .become(d, match_height=True, match_center=True)
        .set_opacity(0.25)
        .set_color(GREEN)
    )
    s3 = s.copy().become(d, stretch=True).set_opacity(0.25).set_color(YELLOW)

    scene.add(s, d, s1, s2, s3)

Because why did we need 3 Dots in the first place if we are just calling .become on another Mobject.

This will also enable a few more intuitive animations like the following

Code
class Test(Scene):
    def construct(self):
        target_square = Square()
        dot = Dot().shift(RIGHT*2+UP*2)
        triangle = Triangle().shift(RIGHT*2)
        letter = Tex("A").shift(RIGHT*2+DOWN*2)

        self.add(target_square, dot, triangle, letter)
        self.wait()
        self.play(target_square.animate.become(dot, stretch=True, match_center=True))
        self.wait()
        self.play(target_square.animate.become(triangle, stretch=True, match_center=True))
        self.wait()
        self.play(target_square.animate.become(letter, stretch=True, match_center=True))
        self.wait()

https://github.com/ManimCommunity/manim/assets/17803932/ef4eb59d-84fa-41a5-92c2-b9640c27d79a

Instead of the following happening in the current version.

https://github.com/ManimCommunity/manim/assets/17803932/f684ba6f-614f-4d0b-8cae-9aeacb483451

So I would propose the following changes with this PR

  • A behavior change of .become which was unintuitive also with the test case
  • A copying of colors in the interpolate_color function to avoid linking colors with animations
  • A change and review of the test that is currently used for checking .become and also additional testing on that part!

MrDiver avatar Dec 09 '23 12:12 MrDiver

Should we add the code as a doctest for become somewhere ?

MrDiver avatar Dec 12 '23 17:12 MrDiver

I've extended the docstring with some tests and examples.

There is a slight change in behavior with become now using a copy of the target instead of modifying the target itself. I'm fine with this, we might want to ask users in our Discord for their opinion too?

Otherwise this is good to go from my side.

behackl avatar Dec 18 '23 13:12 behackl