Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port ManimGL's Mobject family memoization #3742

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

chopan050
Copy link
Contributor

@chopan050 chopan050 commented May 2, 2024

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
image image

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

manim/mobject/mobject.py Dismissed Show dismissed Hide dismissed
@behackl behackl added enhancement Additions and improvements in general performance labels May 2, 2024
@behackl behackl added this to the v0.19.0 milestone May 2, 2024
Copy link
Contributor

@JasonGrace2282 JasonGrace2282 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Would also be great to have some tests.

manim/mobject/mobject.py Outdated Show resolved Hide resolved
manim/mobject/mobject.py Outdated Show resolved Hide resolved
manim/mobject/mobject.py Outdated Show resolved Hide resolved
@chopan050
Copy link
Contributor Author

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 chopan050 marked this pull request as draft May 11, 2024 18:33
manim/mobject/mobject.py Fixed Show fixed Hide fixed
@chopan050
Copy link
Contributor Author

chopan050 commented May 21, 2024

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.

  1. I'd prefer to have PR Fix assertions and improve error messages when adding submobjects #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 chopan050 marked this pull request as ready for review May 30, 2024 13:51
@chopan050
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Additions and improvements in general performance
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

None yet

3 participants