Implement metaclass approach in geometry module to make mobjects compatible with cairo and opengl rendering

[behackl]

Changelog / Overview

This PR is a proof of concept and introduces a metaclass MetaVMobject which allows compatible mobjects to use the same class for both OpenGL rendering and cairo rendering.

Motivation

Extensive code duplication is dangerous for the hygiene of the code base. If it works in a broader setting as well, this approach offers at least a temporary solution while we are working on full support for OpenGL while maintaining support for cairo.

The solution is not ideal, as it introduces its own problems:

• it only works for classes whose implementation is more or less the same for both cairo and OpenGL rendering,
• intellisense will most likely be broken to some degree: currently, the IDE cannot know that on this branch Polygon inherits from VMobject (or OpenGLVMobject),
• the information that Polygon inherits from either VMobject or OpenGLVMobject will have to be documented manually,
• and probably some further problems I haven't thought of so far.

Nevertheless, I think the advantages of a temporary solution following this idea (a maintainable code base for the time while we sort-of support two renderers at the same time; the fact that the implementation is relatively straightforward) outweigh these disadvantages.

Explanation for Changes

The metaclass overrides __call__, which means that, for example, custom code is run when Polygon(*args, **kwargs) is called. In particular, instead of simply creating an instance of the Polygon class as currently implemented, this code first checks the value of config.renderer and constructs a new class inheriting from the Polygon class in geometry.py, but also inheriting from either VMobject (in case the renderer is cairo), or OpenGLVMobject (in case the renderer is OpenGL). The resulting class is also called Polygon, and for users there shouldn't be any noticeable differences.

In particular, this means that Mobject and OpenGLMobject wouldn't have to be merged at all (which turned out to be rather delicate and introduced hard to trace down bugs), but we can simply inherit from the correct class dynamically instead.

Testing Status

I just tested this locally: the snippet

class RendererTest1(Scene):
    def construct(self):
        poly = Polygon([-3, 0, 0], [-1, 1, 0], [0, 3, 0], [1, 1, 0], [3, 0, 0], [1, -1, 0], [0, -3, 0], [-1, -1, 0])
        self.play(Create(poly))
        self.wait()

(where the modified Polygon class is used directly), as well as the snippet

class RendererTest2(Scene):
    def construct(self):
        poly = RegularPolygon(7)
        self.play(Create(poly))
        self.wait()

(where a class inheriting from Polygon is used; no changes to the implementation of RegularPolygon had to be made) render sucessfully for me with --renderer=opengl as well as --renderer=cairo.

Overall Progress in geometry.py

(work in progress; for now this is a list of all classes in the current geometry.py)

TipableVMobject(metaclass=MetaVMobject)   [compatible]
    Arc(TipableVMobject)  [heavily diverged]
        ArcBetweenPoints(Arc)  [diverged but fixable]
            CurvedArrow(ArcBetweenPoints) [compatible]
                CurvedDoubleArrow(CurvedArrow)  [compatible]

        Circle(Arc)  [diverged but fixable]
            Dot(Circle) [compatible]
                SmallDot(Dot) [compatible]
                AnnotationDot(Dot) [has no OpenGL version -- compatible]
                LabeledDot(Dot) [has no OpenGL version (on this PR); depends on Text rendering]

            Ellipse(Circle)  [compatible]
            Annulus(Circle)  [compatible]

        AnnularSector(Arc)  [compatible]
            Sector(AnnularSector) [compatible]

    Line(TipableVMobject) [compatible, minor differences in Line.pointify]
        DashedLine(Line) [compatible]
        TangentLine(Line) [compatible]
        Arrow(Line) [heavily diverged]
            Vector(Arrow) [compatible]
            DoubleArrow(Arrow) [diverged (but if Arrow works, so does DoubleArrow)]

Elbow(metaclass=MetaVMobject) [compatible]

CubicBezier(metaclass=MetaVMobject) [diverged]

Polygon(metaclass=MetaVMobject) [compatible]
    RegularPolygon(Polygon)  [compatible]
         Triangle(RegularPolygon) [compatible]
    Rectangle(Polygon) [compatible]
        Square(Rectangle) [compatible]
        RoundedRectangle(Rectangle) [compatible]

ArcPolygon(metaclass=MetaVMobject)  [has no OpenGL version]
ArcPolygonFromArcs(metaclass=MetaVMobject)  [has no OpenGL version]

ArrowTip(metaclass=MetaVMobject) [diverged / no support for different shapes in OpenGL]
    ArrowTriangleTip(ArrowTip, Triangle) [diverged]
        ArrowTriangleFilledTip(ArrowTriangleTip) [no OpenGL version]

    ArrowCircleTip(ArrowTip, Circle)  [no OpenGL version]
        ArrowCircleFilledTip(ArrowCircleTip)  [no OpenGL version]

    ArrowSquareTip(ArrowTip, Square)  [no OpenGL version]
        ArrowSquareFilledTip(ArrowSquareTip)  [no OpenGL version]

Cutout(metaclass=MetaVMobject)  [no OpenGL version]

Angle(metaclass=MetaVMobject)  [no OpenGL version]
    RightAngle(Angle)  [no OpenGL version]

Checklist

• I have read the Contributing Guidelines
• I have written a descriptive PR title (see top of PR template for examples)
• I have written a changelog entry for the PR or deem it unnecessary
• My new functions/classes either have a docstring or are private
• My new functions/classes have tests added and (optional) examples in the docs
• My new documentation builds, looks correctly formatted, and adds no additional build warnings

Reviewer Checklist

• The PR title is descriptive enough
• The PR is labeled correctly
• The changelog entry is completed if necessary
• Newly added functions/classes either have a docstring or are private
• Newly added functions/classes have tests added and (optional) examples in the docs
• Newly added documentation builds, looks correctly formatted, and adds no additional build warnings

[huguesdevimeux]

This is smart and pythonic, I like it.

The only issue I have is for autocompletion. I'm not sure that IDEs will like this.

[friedkeenan]

Customizing issubclass I think would be good, so stuff like issubclass(Dot, VMobject) would still work. This can be done by overriding __subclasscheck__ in the metaclass.

[behackl]

I've been playing around a bit more with this approach and got most of geometry working with minimal modifications (basically: replacing most Class.__init__ calls by super().__init__). It works very well for all classes where the implementation did not change too much between the cairo and the OpenGL version -- but for some classes like Arrow, which are implemented in a completely different way in opengl_geometry.py than they are in geometry.py, this causes problems.

We could pursue a strategy like this:

1. Introduce the metaclass approach to all user-facing classes whose OpenGL-implementation is (very) close to the cairo version, be happy that many classes are compatible with OpenGL + cairo simultaneously, and unhappy about classes that are not. Explicitly document (a list?) which classes are compatible, and which are not (there we will need to keep the OpenGLXYZ class around).
2. Refactor the classes who have diverged strongly to eventually also be able to do step 1.

The fact that some of the implementations are that different is also problematic: many improvements / features made to and for the community edition are not carried over to the OpenGL implementation. Simultaneously, the community edition has some extremely messy code (like the one for Arrow) where I am not sure whether it is worth to keep the implementation around, or whether we should just switch to the OpenGL implementation / throw the entire thing away and write it from scratch.

Before moving forwards, I'd like to hear opinions / feedback of the community @ManimCommunity/core -- if we commit to this metaclass approach, we'll have to pursue it together as well; I don't have the resources to implement this all on my own.

[markromanmiller]

I think I'm well-convinced that this is a good medium-term fix.

One thing I'd add is that for any classes that can't be merged, the error messages for using the wrong renderer should be swift and clear.

@behackl can you describe a little more what kind of support you would need?

[behackl]

One thing I'd add is that for any classes that can't be merged, the error messages for using the wrong renderer should be swift and clear.

Definitely, I fully agree.

@behackl can you describe a little more what kind of support you would need?

I'll push the latest version of my branch in a moment, the tests should look better then (at least a little). The most work then is going through the classes (for this PR I'd suggest we stick to geometry only), test whether they work with both renderers. If a class works out-of-the-box we are happy, if not the difference in implementation between the OpenGL and cairo version has to be investigated.

If it is just a minor difference (like some keyword parameters renamed, or a init_points missing), then the current (cairo) implementation can be slightly adjusted and the class should work -- and otherwise we should probably just throw it on the pile of classes that have diverged too much and cannot be merged in the first iteration.

[friedkeenan]

Ah the docs build is failing because the MovingAngle scene in examples.rst is trying to use point_from_proportion on an Angle mobject which no longer has points of its own. A more descriptive error will occur after #1302 is merged

[friedkeenan]

Though this brings up an issue with inheritance with this model. Will Ellipse still be able to inherit from Circle, and Circle from Arc? This is especially important for when users create their own custom mobjects; they shouldn't need to have such technical knowledge about the merging of cairo and opengl mobjects and metaclasses

[behackl]

Ah the docs build is failing because the MovingAngle scene in examples.rst is trying to use point_from_proportion on an Angle mobject which no longer has points of its own. A more descriptive error will occur after #1302 is merged

Indeed. I probably should have fixed Angle in a different way; I needed to get rid of the multiple inheritance though, it caused problems after shifting to super calls (as generate_points of both Arc and Elbow were called at some point). We could keep self.angle_mobject as is currently, but instead of adding it then as a submobject of the angle, we could just set the points of angle to be those of the angle_mobject. I've seen this in some other place in geometry.py as well.

Will Ellipse still be able to inherit from Circle, and Circle from Arc?

As far as I am concerned: yes. On this branch, Line (which inherits from TipableVMobject, which in turn inherits from VMobject) works for both cairo and OpenGL rendering; same for Polygon and RegularPolygon.

However, this assumes that the implementation of the higher-level Mobjects is more or less independent of the lower-level (VMobject, OpenGLVMobject) ones. Which isn't necessarily the case; for extreme discrepancies we'll have to merge/adapt the OpenGL and cairo implementations; in a first round we might be able to do so by introducing some if config.renderer == ...-type statements.

[behackl]

I have updated the PR description and added a list of all classes (including their inheritance) in geometry.py. Overall, the status of the geometry module looks pretty good; there are only two classes that are severely diverged (Arc, Arrow).

• "compatible" means that the implementation of the OpenGL version and the cairo version are basically the same, or just call (compatible) methods in Mobject or VMobject.
• "diverged" etc. means that the implementation of the OpenGL version is conceptually different than the cairo version. It doesn't mean, however, that it is certain that the class doesn't work in OpenGL -- for example, I'd expect that while Arrow is implemented pretty differently, it should not be too difficult to get it working with OpenGL. Help classifying these cases (works with OpenGL / broken under OpenGL but fixable / severely broken, separate OpenGL implementation should be kept around) would be appreciated.

I'll investigate Arc a bit more; will take a break now though. If anyone else wants to jump in, feel free to do so + enjoy. 😄

If we can get it running, then I'd confident that this PR could also be merged soon.

[behackl]

Since the config is (explicitly) the first thing we import, all the classes will be initialized after the config is set, which means we should be able to use the config on class initialization.

This avoids weird double inheritance for every class and solves the issubclass check; might even be able to keep Angle the way it was

I like the idea and it would be great if we could get that to work -- however, I'm having troubles getting it running. It somehow seems that __init__ is not called for instances of classes constructed via the metaclass. Here is a traceback I get when trying to self.play(Create(Circle())):

╭──────────────────────────────────────────────────────────────────────────────────────╮
│ File "/Users/behackl/code/manim/manim/cli/render/commands.py", line 131, in render   │
│    128                     scene_classes = scene_classes_from_file(file)             │
│    129                     SceneClass = scene_classes[0]                             │
│    130                     scene = SceneClass(renderer)                              │
│  ❱ 131                     status = scene.render()                                   │
│    132                     if status:                                                │
│    133                         continue                                              │
│    134                     else:                                                     │
│ File "/Users/behackl/code/manim/manim/scene/scene.py", line 201, in render           │
│     198         """                                                                  │
│     199         self.setup()                                                         │
│     200         try:                                                                 │
│  ❱  201             self.construct()                                                 │
│     202         except EndSceneEarlyException:                                       │
│     203             pass                                                             │
│     204         except RerunSceneException as e:                                     │
│ File "myex.py", line 5, in construct                                                 │
│     2                                                                                │
│     3 class Geom(Scene):                                                             │
│     4     def construct(self):                                                       │
│  ❱  5         a = Circle()                                                           │
│     6         self.play(Create(a))                                                   │
│     7         self.wait()                                                            │
│     8                                                                                │
│ File "/Users/behackl/code/manim/manim/mobject/geometry.py", line 480, in __init__    │
│     477         anchors_span_full_range=False,                                       │
│     478         **kwargs                                                             │
│     479     ):                                                                       │
│  ❱  480         super().__init__(                                                    │
│     481             radius=radius,                                                   │
│     482             start_angle=0,                                                   │
│     483             angle=TAU,                                                       │
│ File "/Users/behackl/code/manim/manim/mobject/geometry.py", line 294, in __init__    │
│     291         self.start_angle = start_angle                                       │
│     292         self.angle = angle                                                   │
│     293         self._failed_to_get_center = False                                   │
│  ❱  294         super().__init__(**kwargs)                                           │
│     295                                                                              │
│     296     def generate_points(self):                                               │
│     297         self.set_pre_positioned_points()                                     │
│ File "/Users/behackl/code/manim/manim/mobject/geometry.py", line 119, in __init__    │
│     116         self.tip_length = tip_length                                         │
│     117         self.normal_vector = normal_vector                                   │
│     118         self.tip_style = tip_style                                           │
│  ❱  119         super().__init__(**kwargs)                                           │
│     120                                                                              │
│     121     # Adding, Creating, Modifying tips                                       │
│     122                                                                              │
│ File "/Users/behackl/code/manim/manim/mobject/types/vectorized_mobject.py", line 135,│
│in __init__                                                                           │
│     132         self.shade_in_3d = shade_in_3d                                       │
│     133         self.tolerance_for_point_equality = tolerance_for_point_equality     │
│     134         self.n_points_per_cubic_curve = n_points_per_cubic_curve             │
│  ❱  135         Mobject.__init__(self, **kwargs)                                     │
│     136                                                                              │
│     137     def get_group_class(self):                                               │
│     138         return VGroup                                                        │
│ File "/Users/behackl/code/manim/manim/mobject/mobject.py", line 80, in __init__      │
│      77         self.updaters = []                                                   │
│      78         self.updating_suspended = False                                      │
│      79         self.reset_points()                                                  │
│  ❱   80         self.generate_points()                                               │
│      81         self.init_colors()                                                   │
│      82                                                                              │
│      83         # OpenGL data.                                                       │
│ File "/Users/behackl/code/manim/manim/mobject/geometry.py", line 298, in             │
│generate_points                                                                       │
│     295                                                                              │
│     296     def generate_points(self):                                               │
│     297         self.set_pre_positioned_points()                                     │
│  ❱  298         self.scale(self.radius, about_point=ORIGIN)                          │
│     299         self.shift(self.arc_center)                                          │
│     300                                                                              │
│     301     # Points are set a bit differently when rendering via OpenGL.            │
│ File "/Users/behackl/code/manim/manim/mobject/mobject.py", line 1054, in scale       │
│    1051                                                                              │
│    1052         """                                                                  │
│    1053         if config.renderer == "opengl":                                      │
│  ❱ 1054             self.apply_points_function(                                      │
│    1055                 lambda points: scale_factor * points,                        │
│    1056                 works_on_bounding_box=True,                                  │
│    1057                 **kwargs,                                                    │
│ File "/Users/behackl/code/manim/manim/mobject/mobject.py", line 1192, in             │
│apply_points_function                                                                 │
│    1189         if about_point is None and about_edge is not None:                   │
│    1190             about_point = self.get_bounding_box_point(about_edge)            │
│    1191                                                                              │
│  ❱ 1192         for mob in self.get_family():                                        │
│    1193             arrs = []                                                        │
│    1194             if len(self.data["points"]):                                     │
│    1195                 arrs.append(mob.data["points"])                              │
│ File "/Users/behackl/code/manim/manim/mobject/mobject.py", line 2003, in get_family  │
│    2000     def get_family(self, recurse=True):                                      │
│    2001         if config.renderer == "opengl":                                      │
│    2002             if recurse:                                                      │
│  ❱ 2003                 return self.family                                           │
│    2004             else:                                                            │
│    2005                 return [self]                                                │
│    2006         else:                                                                │
│ File "/Users/behackl/code/manim/manim/mobject/mobject.py", line 529, in __getattr__  │
│     526             return types.MethodType(setter, self)                            │
│     527                                                                              │
│     528         # Unhandled attribute, therefore error                               │
│  ❱  529         raise AttributeError(f"{type(self).__name__} object has no attribute │
│     530                                                                              │
│     531     @property                                                                │
│     532     def width(self):                                                         │
╰──────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: Circle object has no attribute 'family'

Any ideas?

[friedkeenan]

It looks like config["renderer"] == "opengl" but VMobject is being used. Could I see the code you're using for MetaVMobject?

[behackl]

It looks like config["renderer"] == "opengl" but VMobject is being used. Could I see the code you're using for MetaVMobject?

Sure, it's basically 1:1 what you suggested above:

class MetaVMobject(ABCMeta):
    def __new__(cls, name, bases, namespace):
        if len(bases) == 0:
            if config.renderer == "opengl":
                bases = (OpenGLVMobject,)
            else:
                bases = (VMobject,)

        print(cls, name, bases)
        return super().__new__(cls, name, bases, namespace)

(Ah, even my debug print made it in the paste. 😄)

[friedkeenan]

Hm, looking at it, it seems as if the config is getting filled with its default values and then later being filled in correctly. That doesn't seem uh, intentional

[friedkeenan]

Aha, it works when I have a manim.cfg file with

[CLI]
renderer=opengl

It seems the config at the start of importing only takes options from files, not the command line

[behackl]

I have added manual parsing of sys.argv for any passed --renderer options in __init__.py, which now also allows us to use the __new__ approach for the metaclass sketched above. I've selectively tested a few of the classes, it still all seemed to work.

I think this is ready for a new review round. :-)

[friedkeenan]

Second pass, staying away from Angle

[friedkeenan]

Pretty much just style suggestions, but I think even with the code as is, I'd be okay with it being merged

[behackl]

Pretty much just style suggestions, but I think even with the code as is, I'd be okay with it being merged

All of your suggestions looked good to me, I've applied them all. Thanks for reviewing this, I'm very happy we were able to move this forwards, and I really like the current implementation of the metaclass. (I've also checked the documentation: the current approach even renders classes for which the metaclass is set as inheriting from VMobject, which is great!)

Assuming the tests and documentation build pass cleanly, I'll leave this open for a little while longer in case other devs want to take a look -- but otherwise I'll merge this tomorrow at the very latest to avoid merge conflicts. I do think that this is a substantial step towards transitioning to OpenGL. :-)

[friedkeenan]

Should the corresponding classes in opengl_geometry.py be removed, or should that be handled in a later PR?

[behackl]

Should the corresponding classes in opengl_geometry.py be removed, or should that be handled in a later PR?

I'd keep them around for now and if things work well with the metaclass approach remove them in 1-2 releases.