Port ManimGL's Mobject family memoization

[chopan050]

Related issue: #373 Although I like the proposal described in the issue, ManimGL already implemented something different: family memoization. So it's easier to port that, at least for now.

Essentially, when we call Mobject.get_family(), instead of recomputing the whole family recursively and from scratch, we memoize it as in ManimGL. Every time a method like .add() or .remove() is called, which alters the submobjects and therefore the family, we delete the previous memo by setting it as None, to signal that it must be recalculated. The same must be done for all the parents of the Mobject, which this PR also ports from ManimGL.

This is an important step in calculating bounding boxes more efficiently.

Some points for discussion:

• I had to make Mobject.submobjects a property, because if you set it manually, the family must be altered. It works, but I don't like that submobjects.setter hides the deletion of the family behind the scenes. @JasonGrace2282 proposed making submobjects a read-only property, and only allowing it to be directly changed through a Mobject.set_submobjects() method:

    @property
    def submobjects(self) -> list[Mobject]:
        return self._submobjects

    def set_submobjects(self, new_submobjects: list[Mobject]) -> None:
        self._submobjects = new_submobjects
        self.note_changed_family()

I like the proposal, although it's a slightly breaking change and it might be done in a subsequent PR. Please share your opinions about this. I'd be glad to make that change if we agree on that.

• There were some methods, like Mobject.insert() or Mobject.shuffle(), which returned None instead of Self like other similar methods. I think they should also return Self, but it definitely should be another PR.

• There are some methods which we could optimize if we reaaaally wanted to, such as Mobject.add(), which instead of voiding the entire family for the current Mobject, it could append the submobjects' subfamilies to this family and filter redundancies, and only void the parents' families depending on the position of the Mobject. It would get more complex, however, and it doesn't seem to be really justified for now. Plus, I tried that before, and didn't manage to get it working because of the complexity. I decided to simply stick to the ManimGL implementation as-is, as much as possible.

Links to added or changed documentation pages

Further Information and Comments

Profiling with this test scene with a huge amount of nested Mobjects:

class FamilyScene(Scene):
    def construct(self):
        R = 0.2
        N = 16

        bottom_layer = [VGroup(DashedVMobject(Circle(R))).shift((-7.5 + i) * RIGHT + 4*DOWN) for i in range(N)]
        
        while N > 1:
            N //= 2
            top_layer = [VGroup() for _ in range(N)]
            for i, group in enumerate(top_layer):
                child_1, child_2 = bottom_layer[2*i : 2*i + 2]
                child_1_center, child_2_center = child_1[0].get_center(), child_2[0].get_center()
                node_center = 0.5*(child_1_center + child_2_center) + 2*UP
                node = DashedVMobject(Circle(R)).move_to(node_center)
                group.add(
                    node,
                    DashedLine(node_center, child_1_center),
                    child_1,
                    DashedLine(node_center, child_2_center),
                    child_2,
                )
            bottom_layer = top_layer

        tree = top_layer[0]
        self.add(tree)

        self.play(tree.animate.scale(0.5))
        self.play(Rotate(tree, TAU), run_time=2)
        self.play(tree.animate.shift(2*UR))
        self.play(tree.animate.shift(2*DL))
        self.wait()

Before	After

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

[chopan050]

I addressed the changes you requested. However, the docs aren't rendering because the example scene OpeningManim is crashing at line 16:

        self.play(
            Transform(title, transform_title),
            # ...
        )

For some reason the amount of points don't match, leading to this error:

  File "/home/chopan/manim/manim/utils/bezier.py", line 274, in interpolate
    return (1 - alpha) * start + alpha * end
           ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
ValueError: operands could not be broadcast together with shapes (84,3) (100,3) 

I'll be taking a look at this bug, any help is appreciated. Meanwhile I'll mark this PR as a draft.

[chopan050]

UPDATE (still in draft)

I managed to solve the error I mentioned. However:

1. There's another bug which is preventing the docs from being built: in the VariablesWithValueTracker scene there's a crash on line 12:

        self.play(Write(on_screen_var))

This is because the DecimalNumber in Variable is having its height set to 0 when finishing Write, which causes a ValueError on the font_size property:

  File "/home/chopan/manim/manim/mobject/text/numbers.py", line 149, in font_size
    raise ValueError("font_size must be greater than 0.")
ValueError: font_size must be greater than 0.

So this PR is kept as a draft. This issue with height and font_size seems common.

2. I'd prefer to have PR #3756 reviewed and merged before this one, because that other PR addresses a submobject assertion I initially did here. EDIT: it's merged now, thank you a lot!

[chopan050]

GOOD NEWS: it's now open for review again!

Regarding the bug I previously mentioned, the issue was in Mobject.__deepcopy__(): I had to call the Mobject.submobjects property to make sure that every submobject had the self in their parents.

I also prevented the deepcopying of Mobject._family, and had to manually skip that attribute when JSON encoding a Mobject with our own manim.utils.hashing._CustomEncoder.

[chopan050]

Some news:

• I hid the parents attribute with an underscore and instead made a read-only parents property
• I abstracted the logic of "append this Mobject to the submobjects' parents" into a Mobject._add_as_parent_of_submobs() method, to reduce and simplify the code a bit

[chopan050]

Hey, thanks for reviewing this! I applied the changes you requested.

I don't know how much of a breaking change/effort this would be, but maybe making the submobjects property return an immutable value is something to consider?

It's complicated, I tried returning a tuple instead of a list, but many other methods around the source code access the submobjects and expect it to be a list (not necessarily because they want to modify it directly: a lot of times they want to concatenate lists, and it's not possible to concatenate tuples to lists).

[chopan050]

It's complicated, I tried returning a tuple instead of a list, but many other methods around the source code access the submobjects and expect it to be a list (not necessarily because they want to modify it directly: a lot of times they want to concatenate lists, and it's not possible to concatenate tuples to lists).

Another solution is to make a wrapper like described here, but it seems overkill.

Anyways, there are 2 choices I can think of rn:

1. I could try and make the submobjects property return a tuple, and fix all the other places in the code which expect it to be a list, but that could potentially make this PR grow too much, a lot more than it already has.

2. Let's leave that for a subsequent PR, but that would allow users to break submobjects as you mentioned in the meantime.

What do you think, @JasonGrace2282?

[JasonGrace2282]

I think for now, option 2 is better. The experimental branch has fried my brains in regards to families, though, so I think I will take a look at this hopefully in the near future, just not today.