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

[JasonGrace2282]

Left some questions/typing stuff, but overall the changes look good to me!

Would also be great to have some tests.

[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]

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

[JasonGrace2282]

I found some review comments from a review I must not have finished lol.

From a quick look at the PR description again, my main concern with this is that users have done, e.g., mob.submobjects.reverse(), and that's still fully possible with the current implementation. 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?

[JasonGrace2282]

On another note, it's interesting how often the words extended and recurse are used in such scenarios, do they mean the same thing?
Maybe in a future PR, it might make sense to unify these to be more consistent?

[chopan050]

In this case extended has a different meaning.

If it had the same meaning as get_family()'s recurse, then it would mean something like "if recurse=False return a list with only this Mobject, otherwise keep recursing through this Mobject's parents, its parents' parents, etc... and return a list containing all of them".

However, this method always returns the ancestors of this Mobject.
extended here means "if extended=False only return the ancestors of this Mobject. Otherwise, get the entire family, and then also return the ancestors of all the family members". It's more in the sense of "extended family", in this case meaning your spouse, parents-in-law, children-in-law, etc.

[chopan050]

In any case, I just copy-pasted this from ManimGL TBH, and it's currently not being used anywhere 🤷

[JasonGrace2282]

Ah that makes sense - maybe it's a good idea to document this somewhere and/or add tests?

[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.