RFC: rework output configuration options and their consumption - #4966
RFC: rework output configuration options and their consumption#4966behackl wants to merge 14 commits into
Conversation
behackl
added
refactor
|
I'm hoping to take a closer look at this over the weekend, but here are some immediate thoughts. I'm not very familiar with the details of the current config-render-scene pipeline, so perhaps these are all obvious or irrelevant!
Exciting stuff so far 👍 |
|
Great questions!
|
Cool, I thought that might be the case! In my mind, a "session" would be the execution and rendering of all scenes provided by the user, but that's just nomenclature stuff. I'm also thinking about this from a non-CLI perspective (let's say I'm building an editor for Manim which builds its own manager or whatever), but none of this seems to directly preclude doing that, so I'm happy there. Another off-the-cuff thought before I start looking at the code: Maybe I'm an extreme outlier and my workflow shouldn't weigh too heavily in these considerations, but I actually find it very useful to be able to invoke class CombinedScene(Scene):
def construct(self):
square = Square()
self.play(FadeIn(square))
self.play(FadeOut(square))
# this scene is very dense, so turn off caching
with tempconfig(dict(disable_caching=True)):
ReusableScene.construct(self)
self.play(FadeOut(*self.mobjects))Obviously I'll survive if I have to do something else! There's also this workaround which again isn't strictly necessary, but definitely a nice thing to have at your disposal. |
nikolajmunk
left a comment
nikolajmunk
left a comment
There was a problem hiding this comment.
I've only taken a cursory glance at the implementation itself so far, but here's a quick pass of documentation. Many of these were pretty LLM-y; in particular, I've noticed that LLMs love to write documentation that explains what was changed rather than what is now true about the code. For example it might write "this function accepts both string and integer input; integers are correctly cast to string and do not raise an error" rather than just "this function accepts string and int input". I've tried to make the wording clearer, more human-sounding and more useful to future users of Manim.
Hope these are of any use!
| scene captures the immutable session specification before renderer | ||
| initialization, while the renderer still owns its camera, clock, play count, | ||
| skip state, and file writer. The manager exposes the session and forwarding | ||
| views of renderer state. The scene and renderer therefore still have | ||
| substantial interplay that later refactors aim to remove. |
There was a problem hiding this comment.
Not sure what to do about this one, it's very hard to read so I don't understand the paragraph well enough to rephrase it 😅
| Finally, by default Manim outputs ``.mp4`` files. To request GIF output instead, | ||
| use ``--format=gif``. GIF and final-frame PNG names include the installed Manim |
There was a problem hiding this comment.
Is this paragraph necessary if we have the "Output formats" section above? Maybe this it could be moved there.
| def save_last_frame(self) -> bool: | ||
| """Whether to save the last frame of the scene as an image file (-s).""" | ||
| return self._d["save_last_frame"] | ||
| """Whether to use final-state-only PNG output (-s).""" |
There was a problem hiding this comment.
Personal opinion: final-state-only PNG output" sounds ugly and it would be cool to find a better name. Even just "final-state PNG output" feels better!
There was a problem hiding this comment.
I've consistently renamed this to "last frame", which is much clearer IMO. Good call!
| require rendering a video using the -l flag from a scene. Then we will test | ||
| (in this case, SquareToCircle), that lives in | ||
| ``test_scene_rendering/simple_scene.py``. Change directories to ``tests/``, | ||
| For instance, a test that checks low-quality rendering first requires rendering |
There was a problem hiding this comment.
IMO, "...a test that checks low-quality rendering will first require rendering..." would work better here just to avoid the possible interpretation that the low-quality test is the first test of multiple to come.
There was a problem hiding this comment.
Agreed -- I've pushed an easier to read reworded version!
| from presentation requests such as opening the completed artifact or displaying | ||
| a live preview. It also preserves whether dry-run execution was requested. A dry | ||
| run and an artifact-less render both have an effective output format of ``none``, | ||
| but the reason remains available without rereading mutable global configuration. |
There was a problem hiding this comment.
Maybe something like "but a dry run is treated as if it were a normal render session with a provided output format." instead?
It doesn't feel useful to the reader to know what the system doesn't do.
There was a problem hiding this comment.
Agreed -- attempted to make the intent behind dry_run a bit clearer.
Co-authored-by: nikolajmunk <28557236+nikolajmunk@users.noreply.github.com>
|
OK, as far as I can tell everything looks good on the code side. I have a few comments about config options:
As a side note, here's a possibly overengineered thing I'm wondering: would it perhaps make sense to encode the constraints of the config (e.g. |
behackl
left a comment
behackl
left a comment
There was a problem hiding this comment.
Agreed with all direct suggestions, and attempted to improve wording for all other regions where you left comments as well. Thanks for the detailed look at this!
| require rendering a video using the -l flag from a scene. Then we will test | ||
| (in this case, SquareToCircle), that lives in | ||
| ``test_scene_rendering/simple_scene.py``. Change directories to ``tests/``, | ||
| For instance, a test that checks low-quality rendering first requires rendering |
There was a problem hiding this comment.
Agreed -- I've pushed an easier to read reworded version!
| from presentation requests such as opening the completed artifact or displaying | ||
| a live preview. It also preserves whether dry-run execution was requested. A dry | ||
| run and an artifact-less render both have an effective output format of ``none``, | ||
| but the reason remains available without rereading mutable global configuration. |
There was a problem hiding this comment.
Agreed -- attempted to make the intent behind dry_run a bit clearer.
| scene captures the immutable session specification before renderer | ||
| initialization, while the renderer still owns its camera, clock, play count, | ||
| skip state, and file writer. The manager exposes the session and forwarding | ||
| views of renderer state. The scene and renderer therefore still have | ||
| substantial interplay that later refactors aim to remove. |
| Finally, by default Manim outputs ``.mp4`` files. To request GIF output instead, | ||
| use ``--format=gif``. GIF and final-frame PNG names include the installed Manim |
| def save_last_frame(self) -> bool: | ||
| """Whether to save the last frame of the scene as an image file (-s).""" | ||
| return self._d["save_last_frame"] | ||
| """Whether to use final-state-only PNG output (-s).""" |
There was a problem hiding this comment.
I've consistently renamed this to "last frame", which is much clearer IMO. Good call!
Thanks for the careful review, much appreciated!
Ah, this is a great question! I tend to agree that any "smartness" in the handling of the output format should be restricted to when the output format is set to There actually is an argument that could be made for the library always appending a 1-frame long animation at the end (which would resolve the issue reported many times that for a scene just containing a I think what I'd like to do is implement a combination of your two suggestions: only let manim try to be smart about the output format while the value is
Are you sure? This might just be explained incorrectly in the docs, the code just has two sequential
It's an interesting suggestion, and sort of prompts the question whether it should be allowed to specify multiple output formats at once. I tend to agree, but would rather want to implement this as a fancy new feature later, separately from this refactor.
Interesting. I am sort of satisfied with the restrictions being all spelled out in I'll push a commit to change the behavior overriding user intent with the output format when there are no animations, plus fix the wording regarding preview + open in file browser. Thanks again! |
Yep, that's a very nice intermediary step!
Oop you're right! Both work at the same time on my machine. I had interpreted
Agreed! I do like the idea of
I was envisioning |
|
Pushed changes as discussed and fixed one test that failed as a consequence. I have also added a couple more cheap tests to make sure the behaviour is exactly what we want for now. (Need a lot of tests for the upcoming file writer decoupling and renderer migrations.) |
2 participants
Summary
This RFC normalizes render output and presentation configuration into one immutable session specification, resolved once when a
Sceneis initialized.At present, output intent is spread across
format,write_to_movie,save_last_frame,save_pngs,save_as_gif, transparency,dry_run, and renderer-specific preview behavior. Those values can conflict, depend on CLI/config parsing order, and are reread or mutated while rendering. In particular, OpenGL and Cairo currently interpret-pdifferently, andSceneFileWriterdecides what to produce by repeatedly consulting mutable global configuration.The proposed model has:
formatvalue for the primary artifact;OutputSpecfor resolved artifact intent;PresentationSpecfor post-render and live presentation requests;RenderSessionSpeccombining both and preserving dry-run execution intent;live_preview;Scene, before renderer initialization; andOutputSpecforSceneFileWriter, with no fallback to global output configuration.This is intended as a foundation for the subsequent
Manager/timeline and audio-aware file-writer work, without attempting that larger decomposition here.User-facing output model
--formatselects one primary artifact:autononemp4,mov,webm,gifpng-s/--save_last_frame.png-sequencedry_runremains a separate execution request. It resolves output tononefor that session without mutating the configured format or other output settings. The immutable session retainsdry_run=True, allowing later execution coordination to distinguish a dry run from another artifact-less session without rereading global configuration.Presentation is independent of artifact selection:
-p/--previewalways opens the completed artifact after rendering, for either renderer.-l/--live-previewrequests renderer-provided live display. OpenGL advertises support; Cairo rejects the request with a clear error.format=autodoes not write a file. Passing a concrete video format displays and records simultaneously.--show_in_file_browserremains a post-render action on the completed artifact.Invalid combinations are rejected during session resolution rather than silently rewritten. This includes transparent MP4, section output with a non-video format, post-render preview without an artifact, live preview with
dry_run, and live preview with final-state-only PNG output.Implementation outline
OutputFormatand frozen, validatedOutputSpecvalues.PresentationSpecandRenderSessionSpecvalues and a singleresolve_render_session()entry point; the session retainsdry_runseparately from its effective output format.RendererCapabilities; Cairo declareslive_preview=Falseand OpenGL declareslive_preview=True.Sceneand pass it torenderer.init_scene(scene, session_spec).session_spec.outputexplicitly toSceneFileWriter.Managerexpose the captured session/output intent and perform post-render presentation from that snapshot.format,output_file, and related global output flags during finalization.SceneFileWriter.final_file_path; this may be a file or the image-sequence directory.-sand make--format=pnguse the same path.playcalls produces a still image rather than an empty movie.media/images/<module>/<Scene>/0000.png.Breaking changes and migration
This RFC deliberately removes overlapping and already-deprecated interfaces rather than carrying synthetic compatibility state into the new model.
Removed CLI and config options
--write_to_movie/[CLI] write_to_movie/config.write_to_movieformat=autoor a concrete video format to write output; useformat=noneto disable it.-g/--save_pngs,[CLI] save_pngs, andconfig.save_pngs--format=png-sequence/format=png-sequence.-i/--save_as_gif,[CLI] save_as_gif, andconfig.save_as_gif--format=gif/format=gif.[CLI] save_last_frameformat=png. The-sCLI convenience and programmaticconfig.save_last_framealias remain.--force_window,[CLI] force_window, andconfig.force_window-l/--live-preview. Supply a concrete format as well if output should also be recorded.-fshort form--show_in_file_browser.config.movie_file_extensionandconfig.resolve_movie_file_extension()OutputSpec.The format predicate helpers
is_mp4_format,is_gif_format,is_png_format,is_webm_format,is_mov_format, andwrite_to_movieare also removed frommanim.utils.file_ops. Callers should inspect the resolvedOutputSpecinstead.Changed behavior
--format=pngchanges meaning. It previously wrote every rendered frame. It now uses the established fast final-state-only behavior of-s. Use--format=png-sequencefor the old frame-sequence behavior.<Scene>/0000.png) rather than being emitted beside one another with the scene name as a filename prefix.-pchanges meaning. It no longer opens the live render window; it renders an artifact and opens that artifact afterward, matching Cairo. Use-l --renderer=openglfor live display.--renderer=openglwithout live preview now follows normalformat=autobehavior and writes MP4/MOV.--live-previewwithformat=autoremains display-only.format=auto,mov, orwebm.save_sectionsrequires a video format.previeworshow_in_file_browserwith resolvedformat=nonenow fails early.format=Noneor an empty format normalizes toauto. The stored configuration uses canonical format strings rather than “unset” as another output state.Scenehas been constructed no longer affect that scene. Callers usingtempconfigmust construct the scene inside the intended configuration context.renderer,transparent, andoutput_file;formatis now actually loaded from config files. Users accidentally relying on the old overwrite/ignore behavior may see different output.dry_runno longer mutates neighboring options. Code that inspectedformat,write_all, or still-output settings after settingdry_runwill now see the original request preserved.config.output_filewith the completed path.config.output_fileremains the requested name. Integrations that need the produced artifact must usescene.renderer.file_writer.final_file_path.config.previewno longer aliasesenable_gui.enable_guiis interpreted as a live-preview request during session resolution;previewexclusively means opening completed output.Renderer and file-writer extension APIs
Custom renderers and direct file-writer users need updates:
capabilities: RendererCapabilitiesdeclaration.renderer.init_scene(scene)becomesrenderer.init_scene(scene, session_spec).OpenGLRenderer.should_create_window()now receives the resolvedsession_spec.SceneFileWriter(renderer, scene_name, ...)now requires an explicitoutput_specargument. There is intentionally no global-config fallback.SceneFileWriterclasses injected through a renderer must accept the newoutput_speckeyword.open_media_file(file_writer)now requires explicitpreview=andshow_in_file_browser=keyword arguments and opensfinal_file_pathinstead of reconstructing paths from global config.Scene.session_spec,Manager.session_spec,Manager.output_spec, andSceneFileWriter.output_specare available to replace the old global reads.Out of scope / follow-up work
This RFC does not yet:
Manager;SceneFileWriterinto segment encoding, audio timeline, and artifact assembly services;custom_folders; orThose remain stacked follow-ups once the session intent and ownership boundary are agreed upon.
RFC questions
The main points on which feedback would be useful are:
formatvalues and thepng/png-sequencedistinction the right user model?-prenderer-independent and reserving-lfor live display the right split?Sceneinitialization the right point to freeze output and presentation intent?(And just to be explicit about this: I've been working a bunch with GPT-5.6/Sol to put this together; everything in here has been reviewed at least coarsely by me though; critical parts and the general design shape were hand-crafted.)