-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implemented decorator :func:.manimation
for building a scene from a construct method
#1901
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must say that I disagree with that design (although It's pythonically interesting), and for mainly two reasons :
-
I dislike the design. Here,
SquareToCircle
is declared as a function, but intended to be used as it was a class, and called as such. This makes it unclear for the end user about what they are supposed to do and what is thisSquareToCircle
object. I find better to use arender
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. -
I strongly think that the config options you're quoting should not be set from a python file for clarity and separation sake, but rather use the
config
that have been made specifically for that, since these config options are more project-wide than scene-wide (i.e a user will likely not want a different resolution for two scenes of a single video). Quoting @leotrs (the man behind config system) here : I propose that we leave inside constants.py only those variables X such that the answer to the following question is "no": Would the user expect X to change from project to project? If the user has two entirely different projects for which they are using manim, and they would expect X to have the exact same value in both projects, then X should go in constants.py, and everything else should go in the config dict
In fact, I don't really approve the whole idea of having function to define a scene (I know, that's what I did for tests but .. meh that's tests there wasn't much choice). The problem is that the function will necessarily have parameter scene
containing the scene reference, which is … is exactly what python uses for class method, self
. So, the function would be a class method without being declared as such, which I find being a bad design.
But still, I don't want to be rude or whatever, that's really a great and interesting idea, with which I unfortunately disagree.
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.
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
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
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. :-) |
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
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 |
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 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 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 |
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 |
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:
|
Just to state this explicitly:
|
Personally, I'm a fan of the decorator syntax. It removes a layer of bloat from 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. |
This Matplotlib like approach is close to the Builder design pattern, except it A fix would be to use the context manager-like approach, which is also great in my eyes. |
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. |
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 |
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. |
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 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? |
.manimation
for building a scene from a construct method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is time that we finally merge this! Because it's just a great addition.
Im thinking if that would be possible to integrate in to the tests ? and maybe specify renderers in the decorator at some point? That would simplify the process a little bit i suppose!
But anyway great addition and i think config options and all that stuff should just be added after some triage of the community.
And in no way i feel this is bad design. It actually seems pretty intuitive that it returns an object, at least i never questioned that it does that.
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 :
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 PS : I don't want to make my opinion blocking on this topic |
* feat: added dvipsnames color palette * added svgnames color palette * fixed docstrings * Commit changes requested by JasonGrace2282 * Double backticks in xcolor --------- Co-authored-by: Francisco Manríquez <[email protected]> Co-authored-by: Francisco Manríquez Novoa <[email protected]> Co-authored-by: Aarush Deshpande <[email protected]>
* Add type hints to cli * Import click.Parameter instead of non existing cloup.Parameter * Stop ignoring manim.cli errors in mypy.ini * Fix mypy errors * Remove unused TypeVar C * click.Command.command -> cloup.Group.command * Address Jason's requested changes * exit() -> sys.exit()
* Add a time property to scene * fix mistaken search/replace * Add typehint
* Add typings to __main__.py * Remove `Unused parameter.` from docstrings
…LI) set to 0 to end after first animation (ManimCommunity#4013) * make checks involving from/upto_animation_number more explicit * add test for flag -n 0,0 for only rendering first animation * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Aarush Deshpande <[email protected]>
Bumps [tornado](https://github.com/tornadoweb/tornado) from 6.4.1 to 6.4.2. - [Changelog](https://github.com/tornadoweb/tornado/blob/v6.4.2/docs/releases.rst) - [Commits](tornadoweb/tornado@v6.4.1...v6.4.2) --- updated-dependencies: - dependency-name: tornado dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
updates: - [github.com/astral-sh/ruff-pre-commit: v0.7.0 → v0.8.0](astral-sh/ruff-pre-commit@v0.7.0...v0.8.0) - [github.com/pre-commit/mirrors-mypy: v1.12.1 → v1.13.0](pre-commit/mirrors-mypy@v1.12.1...v1.13.0) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Aarush Deshpande <[email protected]>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Aarush Deshpande <[email protected]>
for more information, see https://pre-commit.ci
|
||
from manim.constants import * | ||
from manim.mobject.opengl.opengl_compatibility import ConvertToOpenGL | ||
from manim.mobject.types.vectorized_mobject import VMobject | ||
from manim.utils.color import * | ||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.arc
definition
import
'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.arc
definition
import
|
||
from manim.constants import * | ||
from manim.mobject.opengl.opengl_compatibility import ConvertToOpenGL | ||
from manim.mobject.types.vectorized_mobject import VMobject | ||
from manim.utils.color import * | ||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.arc
definition
import
'VMobject' may not be defined if module
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.arc
definition
import
from collections.abc import Iterable | ||
from typing import Any | ||
|
||
import manim.mobject.geometry.tips as tips |
Check notice
Code scanning / CodeQL
Cyclic import Note
manim.mobject.geometry.tips
else: | ||
self.radius = math.inf | ||
self.radius = np.inf |
Check warning
Code scanning / CodeQL
Overwriting attribute in super-class or sub-class Warning
Arc
from manim.typing import InternalPoint3D, Point2D, Point3D, Vector3D | ||
from manim.utils.color import ParsableManimColor | ||
|
||
from ..matrix import Matrix # Avoid circular import |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.matrix
manim.mobject.geometry.line
definition
import
'Matrix' may not be defined if module
manim.mobject.matrix
manim.mobject.geometry.line
definition
import
'Matrix' may not be defined if module
manim.mobject.matrix
manim.mobject.geometry.line
definition
import
'Matrix' may not be defined if module
manim.mobject.matrix
manim.mobject.geometry.line
definition
import
'Matrix' may not be defined if module
manim.mobject.matrix
manim.mobject.geometry.line
definition
import
@@ -2360,7 +2686,7 @@ | |||
part.match_style(vmobject) | |||
self.add(part) | |||
|
|||
def point_from_proportion(self, alpha: float) -> np.ndarray: | |||
def point_from_proportion(self, alpha: float) -> Point3D: |
Check notice
Code scanning / CodeQL
Explicit returns mixed with implicit (fall through) returns Note
from manim.mobject.opengl.opengl_mobject import OpenGLPoint | ||
|
||
from .. import config, logger | ||
from ..animation.animation import Animation, Wait, prepare_animation | ||
from .._config import tempconfig |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.camera.camera
manim.scene.scene
definition
import
'Camera' may not be defined if module
manim.camera.camera
manim.scene.scene
definition
import
'Camera' may not be defined if module
manim.camera.camera
manim.scene.scene
definition
import
'Camera' may not be defined if module
manim.camera.camera
manim.scene.scene
definition
import
'Camera' may not be defined if module
manim.camera.camera
manim.scene.scene
definition
import
'Camera' may not be defined if module
manim.camera.camera
manim.scene.scene
definition
import
|
||
|
||
@overload | ||
def interpolate(start: float, end: float, alpha: float) -> float: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
|
||
|
||
@overload | ||
def interpolate(start: Point3D, end: Point3D, alpha: float) -> Point3D: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
cls, | ||
color: Sequence[ParsableManimColor], | ||
alpha: float = ..., | ||
) -> list[Self]: ... |
Check notice
Code scanning / CodeQL
Statement has no effect Note
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.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