manim icon indicating copy to clipboard operation
manim copied to clipboard

Implemented decorator :func:`.manimation` for building a scene from a construct method

Open behackl opened this issue 4 years ago • 22 comments

Overview: What does this pull request change?

As a follow-up from a recent Discord discussion, this PR is a proof of concept for a decorator that allows to remove one level of indentation when writing your animations. Whether or not we go ahead with this idea is subject to discussion.

Motivation and Explanation: Why and how do your changes improve the library?

This is a demo script which, when running python on it, renders the scene with the specified temporary config options.

from manim import *

@manimation  # equivalent to @manimation(scene_class=Scene)
def square_to_circle(scene: Scene):
    s = Square()
    c = Circle()
    scene.play(Create(s))
    scene.play(Transform(s, c))
    scene.play(FadeOut(*scene.mobjects))

with tempconfig({'frame_rate': 30}):
    square_to_circle.render()

Links to added or changed documentation pages

https://manimce--1901.org.readthedocs.build/en/1901/reference/manim.scene.scene.html#module-manim.scene.scene

Further Information and Comments

The current version is not compatible with the CLI (yet), but I think it would not be too hard to change this.

Note that the interface originally described in this description also included overriding the __call__ of a scene; this has since been removed from the proposal.

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

behackl avatar Aug 12 '21 22:08 behackl

I dislike the design.

That's fair. I am used to the current way of writing scenes as well, but people have been pretty vocal about it in our Discord, so I thought I'd draft a proposal for it.

I find better to use a render method instead of a direct instantiation, as we have currently, thing that (at first glance) should not be possible to use with this decorator approach.

That's also perfectly fair, sure, and on second thought I also pretty much agree – making the class callable doesn't really provide a lot of benefit. However, it is easily possible to adapt this to the decorator approach as well. The @manim_scene decorator turns a function into a scene object, meaning that after defining the function as above, SquareToCircle is an initialized Scene-object. Calling SquareToCircle.render() works.

config options you're quoting should not be set from a python file for clarity and separation sake

For large projects with multiple scenes I agree, users should use a config file to store their settings. But this syntax isn't intended for large projects, it's for quick-and-dirty scenes where you might want to quickly animate a concept and don't want to spend a whole lot of time setting the configuration up. When developing and reviewing, I write such scenes literally all the time, and I feel it would help my workflow if I can make these changes locally in an easy way.

If I had to decide between

config.frame_rate = 42
config.frame_width = ...,
config.frame_height = ...,
config.background_color = ...
scene = MyScene()
scene.render()

and

MyScene.render(frame_rate=42, frame_width=..., frame_height=..., background_color=...),

I think that the second version expresses what I am doing in a cleaner way. Besides, I don't want to circumvent the config system, not at all -- in the current version of this PR, a tempconfig with the passed options is created in which the scene is rendered, thus temporarily overwriting the preexisting config options with the settings that are explicitly passed to render.

I don't want to be rude or whatever

I don't think your response or your stance are rude at all. This is something that has to be discussed, and I'm happy you contributed your point of view. :-)

behackl avatar Aug 13 '21 15:08 behackl

For large projects with multiple scenes I agree, users should use a config file to store their settings. But this syntax isn't intended for large projects, it's for quick-and-dirty scenes where you might want to quickly animate a concept and don't want to spend a whole lot of time setting the configuration up. When developing and reviewing, I write such scenes literally all the time, and I feel it would help my workflow if I can make these changes locally in an easy way.

In fact, at the beginning, the idea was to set all the config content primarily (exclusively) from a .cfg file, and pretty much never from a python script, so, the two cases you're pointing out are ... not theoretically encouraged. However, there is theory and the real world, and as you said, it can be annoying to have to write a new .cfg. This is something to discuss.

a tempconfig with the passed options is created in which the scene is rendered, thus temporarily overwriting the preexisting config options with the settings that are explicitly passed to render.

Since all the stuff in config are thought to be project-dependent, this case shouldn't happen, no ? I mean, if you want to have a different frame rate, it's very likely that you are working on another project, so you should therefore work on another python file/another cwd and .cfg.

huguesdevimeux avatar Aug 13 '21 17:08 huguesdevimeux

As I've said, I don't see this as an addition for people that are actually working on a coherent project. I see it as syntax that allows to quickly test something. I definitely share the point of view that for projects, a dedicated .cfg should be the (single) source of truth for configuration options.

But let me put it differently: I see some arguments (convenience for quick testing and prototyping) in favor of allowing to pass configuration options to the render method of a scene, but not really any downsides. I even believe that allowing to pass rendering options makes it easier to use manim from other libraries (where you mainly want to work with the Python interface, and not worry about .cfg files or using the CLI).

As a thought experiment (and I admit, this does derail this discussion a bit), imagine if matplotlib could only be used like manim: your plot output settings have to be saved to a separate matplotlib.cfg file beforehand, and while it would be possible to hack something together from your script, the encouraged way of compiling would be using a matplotlib command line interface. Sure, it's not fair to compare the libraries in that way – but I can't keep myself from wondering whether a more library- and scripting-centered interface wouldn't be more Pythonic.

behackl avatar Aug 13 '21 17:08 behackl

But let me put it differently: I see some arguments (convenience for quick testing and prototyping) in favor of allowing to pass configuration options to the render method of a scene, but not really any downsides. I even believe that allowing to pass rendering options makes it easier to use manim from other libraries (where you mainly want to work with the Python interface, and not worry about .cfg files or using the CLI).

Well, it's just a question of code organization and practices. The main idea behind using config (and not putting everything in constant.py) was to establish a sort of separation between scene settigns. The idea was to improve code clarity by letting only pure-manim within a python file. For example, I would be against implementing some sort of control of caching in the public API ; I think that this is something related to the rendering process, that should be separated from the pure-manim stuff, so set though either config and CLI.

That being said, if one want to quickly hack something, one uses CLI flags, they provide largely enough configuration to span any quick need. The real problem is that we are heading toward a no-CLI manim, which wasn't the case when the config/constants separation was established. But my point still stands, I strongly think that this separation is necessary.

Anyway, that's not the debate, but this is nevertheless a very fair one that will be imposed to us soon.

Concerning this approach, I disapprove it (even with the config debate being aside) and I'm in favor of a Matplotlib like design (no indentation, scene = Scene() \ scene.play(Create(..)), but I would love to hear other's point of view.

huguesdevimeux avatar Aug 13 '21 18:08 huguesdevimeux

I would be also in favor of a matplotlib-like behavior, e.g.

scn = Scene()
d= Dot()
scn.add(d)
scn.render()

That would have the following benefits in my opinion:

  • It is easier to write documentation, as less indention is needed
  • It is easier to write documentation as one does not have to give names for the scenes, e.g. CircleExample
  • When copy and pasting from the documentation examples, it would be great to get a building block that is ready to use, without further adjustment. (e.g. now one has to remove the CircleExample class name and getting the indention right.)
  • it feels more like other python modules like e.g. matplotlib, and I think ecspecially for jupyter notebooks, that would be a really great feature (see issue: https://github.com/ManimCommunity/manim/issues/1707)

kolibril13 avatar Aug 13 '21 20:08 kolibril13

Just to state this explicitly:

  • I don't want to deprecate our CLI, but I think the CLI should only set the config options passed to it (plus instantiating the requested class + call SelectedClass.render()). I'm not sure to which degree this is currently true.
  • I don't want to change the default way how we write scenes, at least not with this PR. This is exclusively about introducing an alternative way of writing scenes, and whether we should make this alternative available to users or not.
  • The matplotlib-way of doing things (import manim as mn; scene = mn.Scene(); scene.play(mn.Create(mn.Square())) etc.) feels a bit odd to me, because Manim's users need to access way more Manim classes than in matplotlib's case. I think it's a good idea to move more in that direction, but at the moment I don't see how this script-oriented paradigm combines well with keeping the current state of config and not allowing to pass / override parameters in a more convenient way directly from the script.
  • The discussion of whether we should work on making it easier to set config options (e.g., by passing them to the render method) can be entirely separate from this PR. It is loosely connected, but the decorator works just as well without being able to pass config options.

behackl avatar Aug 13 '21 21:08 behackl

Personally, I'm a fan of the decorator syntax. It removes a layer of bloat from having to write: def construct(self): and the extra layer of indentation. Also I'm a very big fan of being able to pass in config options easily in the file. When drafting many small scenes (either for a project, or for testing), having this kind of flexibility is incredibly useful, and circumvents having to write:

class MySc(Scene):
  config.background_color = WHITE
  config.frame_width = 20

which applies the config changes to every scene within the file. For unified projects and very large scenes, this isn't a big concern, but for ease of use and quick drafting/testing, attaching config to a specific scene makes a lot more sense than project/manim-wide configurations. If you wanted to have two scenes in a project with different settings (i.e a lightmode/darkmode scene), this would be very challenging with what we currently have.

As for the matplotlib-esque approach, it feels a little off to me. Manim's currently based on distinct scenes, so having them grouped/organized via a level of indentation makes sense to me. It might be useful if you're sharing lots of mobjects across several scenes? But I'm not sure how common of a use-case this is.

hydrobeam avatar Aug 13 '21 21:08 hydrobeam

I would be also in favor of a matplotlib-like behavior, e.g.

scn = Scene()
d= Dot()
scn.add(d)
scn.render()

This Matplotlib like approach is close to the Builder design pattern, except it build is called render. I'm afraid it won't work with manim, because the methods play, etc. are linked to a timeline, and therefore must be executed directly during their initial calls (i.e, you can't "store" their calls and call them after, when the scene is built). You can't remove the .render at the end because Manim needs an end-scene marker, to e.g. combine the partial movie files.

A fix would be to use the context manager-like approach, which is also great in my eyes.

huguesdevimeux avatar Aug 16 '21 09:08 huguesdevimeux

I like the possibility to be able to directly run the python file to render the scene without the need to use the command line. Most IDEs provide a simple way to run a python file. I know that they often propose to configure that shortcut to something else that would run the appropriate manim command, but I feel like it's a bit overkilling for quick tests and debugging.

leleogere avatar Aug 19 '21 08:08 leleogere

I'm not sure whether we have reached a conclusion about this. Just to make sure we are on the same page, the latest version of this proposal is an interface like

from manim import *

@manim_scene
def SquareToCircle(scene):
    s = Square()
    c = Circle()
    scene.play(Create(s))
    scene.play(Transform(s, c))
    scene.play(FadeOut(*scene.mobjects))

# SquareToCircle is *usually* just a function, but the manim_scene decorator
# turns it directly into a Scene object

SquareToCircle.render()

This PR is only about this interface; while personally I would like to be able to pass config options for rendering as kwargs to Scene.render, this can be discussed completely separately. :-)

behackl avatar Aug 30 '21 19:08 behackl

I'm closing this for now. Should the community be interested in such a feature at some point in the future, we can come back to it I guess.

behackl avatar Oct 02 '21 09:10 behackl

I'd like to revisit this. My central thought behind it is that I feel the library should be flexible enough to support the full range between CLI-based rendering (which, at this point, I am sort of satisfied with -- the CLI itself needs some improvements and cleanups, but generally speaking the concept behind it is solid and works well) and script-based rendering.

The pattern in which we write and render scenes from within a script via

class MySuperScene(Scene):
    def construct(self):
        # animation code

scene_instance = MySuperScene()
scene_instance.render()

feels bloated in contrast to the decorator-based approach from above where we have

@manimation
def my_super_scene(scene):
    # animation code

my_super_scene.render()

(Note: forget about the __call__ idea and passing additional config options to render that were discussed earlier in this PR, this is purely about the decorator that turns a specified function into an object of a class where the decorated method is used as construct.)

While yes, it might be unintuitive that the decorator returns an object rather than another function, I don't really feel that this is bad design -- the behavior just needs to be documented properly.

Any thoughts?

behackl avatar May 18 '22 09:05 behackl

Hello and thanks for the review request.

I was out of the loop for some time (this issue is more than one year old).

I'm still not really in favour of this syntax change, and for two reasons :

  • The main one is that I'm not sure if we can reach an equilibrium. I doubt we will be able in the future to equally maintain the two versions, and at some point either any dev will spend twice as much time or we will end up fostering one approach rather than the other.
  • I highly doubt about the forward compatibility of such change : it will likely bring down some good ideas, just because they won't be compatible with the two syntaxes.
  • Such change has to be documented somewhere, and if MC wants to have a good documentation, it's likely that we will have to include two syntaxes in every example.

To put all in one, I'm not especially against the new syntax - It's a not a really bad design from my POV. I'm just against oscillating between two dramatically different syntaxes, because it will create confusion, additional work if we want to make things thoroughly, and most importantly, it will make MC appears like without having a clear direction of development.

As to chose, I like better the decorator approach (I know, I changed my mind if you read above messages ^^), but I also think the construct approach is way more coherent given how manim is built, and, well, works well.

PS : I don't want to make my opinion blocking on this topic

huguesdevimeux avatar Nov 17 '22 22:11 huguesdevimeux

print("PARSING MODULES", module.__name__)
test = type("TestScene", (Scene,), {})
test.__module__ = module.__name__
setattr(module, "TestScene", test)

So there is a way to dynamically hook in to the list of scenes that are present in the cli tool image

The problem i currently and mainly have is that the module member are statically allocated which makes it pretty difficult to add new members from inside the decorator but in theory it is possible to do it.

MrDiver avatar Dec 26 '22 18:12 MrDiver

I agree with @huguesdevimeux last comment, this PR would introduce a huge branching in the manim ecosystem. Afterwards, there will be two ways to write manim animations, one with the construct syntax, one with the decorator syntax. This will affect all documentation, all shared code from community examples, and the worst: It won't be easy any longer to copy and paste example snippets from one context to another, because the code might have to be translated every time.

Actually, I like the decorator approach more, but by making these change we should be aware of the possible consequences.

Just to mention it again, my perfect manim syntax would look like this:

scn = Scene()
d= Dot()
scn.add(d)
scn.render() 

It would make it easier to recycle variables in Jupyter notebooks and reduce the pain of indentation. Also, this syntax would be more coherent with other visualization python packages like Matplotlib, Napari and Bokeh.

kolibril13 avatar Jan 03 '23 10:01 kolibril13

Afterwards, there will be two ways to write manim animations, one with the construct syntax, one with the decorator syntax.

The decorator is just shorthand notation for the construct method. Practically, all that changes is that one level of indentation is removed. Nothing else. I do not see how this creates a huge branching in our ecosystem.

This will affect all documentation, all shared code from community examples, and the worst: It won't be easy any longer to copy and paste example snippets from one context to another, because the code might have to be translated every time.

I don't intend to change any documentation examples to this "shortcut notation", for now. For the longest time, people interacting with our Discord bot have used the shortcut implemented there that you can just post your construct method, and so far I haven't really heard any complaints about interactions with code snippets posted there.

Just to mention it again, my perfect manim syntax would look like this:

scn = Scene()
d= Dot()
scn.add(d)
scn.render() 

Why would? You can already write scenes like this. Try it out with this snippet:

import manim as mn

scn = mn.Scene()
c = mn.Circle()
s = mn.Square()
scn.play(mn.Create(c))
scn.wait()
scn.play(mn.Transform(c, s))
scn.renderer.scene_finished(scn)

Apart from the last line and the fact that setting render config options is rather clunky, this works perfectly fine, and pretty much always has. This might still not be quite what you mean: whenever you play things here, the scene is already being rendered. (And obviously there are no integrated IPython display hooks, so this wouldn't show any output in a notebook.)

reduce the pain of indentation.

I don't think this is a good argument or motivation in the context of a Python library.


To summarize my 2 cents regarding this suggestion: I don't see this as a fundamentally different syntax compared to the usual construct-way of writing scenes. It is more on the level of "rendering a scene using the manim CLI" vs. "rendering a scene as a script file"; it allows for some convenience in some cases, but it is not intended as a drop-in replacement. If you want full customization options, you will want to use the default class syntax.

It is good to see some more discussion about this, but so far I can't follow the arguments that paint the picture of a world where this decorator is a major syntax change. I am obviously happy to keep talking about it though. :-)

behackl avatar Jan 03 '23 11:01 behackl

Practically, all that changes is that one level of indentation is removed. Nothing else. I do not see how this creates a huge branching in our ecosystem.

It's not only the level of indentation, here is an example:

class Example(Scene):
    def construct(self):
        s = Square()
        c = Circle()
        self.play(Create(s))
        self.play(Transform(s, c))
        self.play(FadeOut(*self.mobjects))

would become

@manimation  
def square_to_circle(scene: Scene):
    s = Square()
    c = Circle()
    scene.play(Create(s))
    scene.play(Transform(s, c))
    scene.play(FadeOut(*scene.mobjects))

so just in this example, I have to replace 4 times the call self to scene. This seems like a small change, but I think it isn't. In practice, I think this will be cumbersome to always be aware of this renaming. Similar to replacing print "hello" to print("hello") in the transition from python2 to python3.

Try it out with this snippet

Great! Many thanks for this nugget! I was not aware of this syntax, and this could be used in Jupyter like this:

import manim as mn
from IPython.display import Video

scn = mn.Scene()
c = mn.Circle()
s = mn.Square()
scn.play(mn.Create(c))
scn.wait()
scn.play(mn.Transform(c, s))
scn.renderer.scene_finished(scn)
Video("media/videos/1080p60/Scene.mp4")

reduce the pain of indentation.

I don't think this is a good argument or motivation in the context of a Python library.

I just think that indentation in manim is currently more complex than it must be.

kolibril13 avatar Jan 03 '23 12:01 kolibril13

so just in this example, I have to replace 4 times the call self to scene. This seems like a small change, but I think it isn't.

You can also rename the argument of construct to scene -- or the argument of square_to_circle to self. You don't have to change anything in the code block itself, if you don't want to.

behackl avatar Jan 03 '23 12:01 behackl

You can also rename the argument of construct to scene -- or the argument of square_to_circle to self. You don't have to change anything in the code block itself, if you don't want to.

Ohh, interesting, I did not think of that. For consistency, do we want to encourage the use of self over the use of scene?

kolibril13 avatar Jan 03 '23 12:01 kolibril13

You can also rename the argument of construct to scene -- or the argument of square_to_circle to self. You don't have to change anything in the code block itself, if you don't want to.

Ohh, interesting, I did not think of that. For consistency, do we want to encourage the use of self over the use of scene?

Sure, I wouldn't mind that. 👍

behackl avatar Jan 03 '23 12:01 behackl

nice, with this I am happy:

@manimation  
def square_to_circle(self: Scene):
    s = Square()
    c = Circle()
    self.play(Create(s))
    self.play(Transform(s, c))
    self.play(FadeOut(*self.mobjects))

kolibril13 avatar Jan 03 '23 12:01 kolibril13

For consistency, do we want to encourage the use of self over the use of scene?

Definitely agreed on that one, and i would also go as far to say that we should probably implement a warning if the first argument isn't named self so that the user is aware of this difference.

MrDiver avatar Jan 03 '23 13:01 MrDiver