Repository navigation
Conversation
There was a problem hiding this comment.
Did a superficial deslopping pass of some of the documentation and comments.
More generally, I think it would be useful to consciously choose some language for the terms used in a camera/render context. I don't like "logical/semantic" "frame/view". I would suggest that we adopt camera frame (or just frame when the context is clear) to describe the rectangle which controls the camera view (the position, angle, and size of the view), and I would suggest that the rectangle which defines the output image is called the viewport - in graphics engine circles, this is the usual nomenclature for the "screen rectangle". Another choice might be canvas but that doesn't work quite as well for 3D.
So for example, you might say "When both the width and height of the camera frame are provided, you must ensure that the pixel dimensions of the viewport have the same aspect ratio of the frame, otherwise (bad things happen)."
Happy to discuss more! I would also be happy to give the new camera guide a more thorough rewrite in a later PR once things settle down a bit.
nikolajmunk
left a comment
There was a problem hiding this comment.
Everything reads much more nicely now! I nevertheless went through and remarked on a few LLM-isms.
As a general note, I'm not personally convinced that "semantic (camera) view" and "owned" are useful terms to the reader.
5623b1f to
d0e8b25
Compare
77a3071 to
9b7a29a
Compare
There was a problem hiding this comment.
When I try to run the tests, I get an error:
> pytest .
Traceback (most recent call last):
File "<frozen runpy>", line 198, in _run_module_as_main
File "<frozen runpy>", line 88, in _run_code
File "D:\Documents\manim\.venv\Scripts\pytest.exe\__main__.py", line 10, in <module>
sys.exit(console_main())
~~~~~~~~~~~~^^
File "D:\Documents\manim\.venv\Lib\site-packages\_pytest\config\__init__.py", line 223, in console_main
code = main()
File "D:\Documents\manim\.venv\Lib\site-packages\_pytest\config\__init__.py", line 199, in main
ret: ExitCode | int = config.hook.pytest_cmdline_main(config=config)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_hooks.py", line 512, in __call__
return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_manager.py", line 120, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_callers.py", line 167, in _multicall
raise exception
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_callers.py", line 121, in _multicall
res = hook_impl.function(*args)
File "D:\Documents\manim\.venv\Lib\site-packages\xdist\plugin.py", line 223, in pytest_cmdline_main
auto_num_cpus = config.hook.pytest_xdist_auto_num_workers(config=config)
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_hooks.py", line 512, in __call__
return self._hookexec(self.name, self._hookimpls.copy(), kwargs, firstresult)
~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_manager.py", line 120, in _hookexec
return self._inner_hookexec(hook_name, methods, kwargs, firstresult)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_callers.py", line 167, in _multicall
raise exception
File "D:\Documents\manim\.venv\Lib\site-packages\pluggy\_callers.py", line 121, in _multicall
res = hook_impl.function(*args)
File "D:\Documents\manim\.venv\Lib\site-packages\xdist\plugin.py", line 23, in pytest_xdist_auto_num_workers
count = psutil.cpu_count(logical=use_logical) or psutil.cpu_count()
^^^^^^^^^^^^^^^^
AttributeError: module 'psutil' has no attribute 'cpu_count'
Hmm, nothing here changes the way our tests are setup; this is unchanged from main. Maybe try |
Retire unused nested targets, release retained buffers, and guard closed operations. Reject unused renderer keywords and build dispatch tables once per draw.
Expose Scene.get_image() and Scene.show() through Manager without stepping animations or emitting movie frames. Preserve active raster targets and capture OpenGL meshes on the context-owning thread.
Compute default frame bounds directly without a semantic cache, preserve custom frame hooks, and share perspective scaling between axes. Add mutation, boundary, and projection parity tests.
Co-authored-by: nikolajmunk <28557236+nikolajmunk@users.noreply.github.com>
Address review feedback on camera terminology, backend controls, custom camera selection, snapshot behavior, and inset views. Preserve the earlier camera test cleanup.
CairoRenderer and OpenGLRenderer are public and referenced from the guides and docstrings, but their modules were missing from the reference, so those links could not resolve.
Remove tests that repeat others or only check implementation details: a nine-way closed-target matrix on a private class, weakref checks of private buffers, scratch-target reuse, nested target sizing and subtree bookkeeping read from private attributes, a mocked check of draw arguments, the re-derived projection formula, trivial frame and constructor echoes, show() delegating to get_image(), and a slow CLI copy of the in-process square-geometry test. Trim the square-geometry and closed-renderer matrices to cases that differ.
The context was scaled to the pixel size and then given a full matrix, which replaces the scale. Pointed out in review.
update_frame took **kwargs only to read excluded_mobjects from them (as main's camera did); a keyword-only parameter says so, and other keywords are rejected instead of ignored.
The two loaders of Cairo background images (the camera's background and a vectorized mobject's background image) repeated the same open, convert, resize code, and each target kept its own file cache: every camera view's target read the files again. One function now loads an image at a size, and the renderer's targets share one cache of files, by path and size.
ThreeDCamera stored its rotation matrix and refreshed it only when a frame was prepared, so projections between frames (project_point after set_phi, or get_mobjects_to_display, which reads get_rotation_matrix) used the matrix of the previous frame. The matrix is now computed from the angle trackers when read; _drawing() still computes the view once per frame, so drawing costs the same. rotation_matrix is a read-only property, reset_rotation_matrix does nothing, and the _prepare_for_render hook that only refreshed the matrix is gone. Found in review.
Every camera has an animatable frame and auto_zoom() now, so these classes add nothing: MovingCamera is a Camera, and MovingCameraScene a Scene with the default camera. Both are deprecated with manim.utils.deprecation and log a warning when created. ZoomedScene, the init template and the examples use Scene and Camera, so they do not warn. Suggested in review.
Title "Working with cameras"; wording. The camera movement examples are rendered (manim directives) so readers see the result. Scenes that choose a camera take camera_class as a keyword with a default, so callers and subclasses can still choose another one. The OpenGL thread note and a redundant renderer hint are gone. In the deep dive, the camera-view step is split in two, and the frame array is described as row-major. ImageMobjectFromCamera says why it is not an AbstractImageMobject: it holds no pixels of its own.
| new_height = abs(top - bottom) | ||
| target = self.frame.animate if animate else self.frame | ||
| if new_width / self.frame.width > new_height / self.frame.height: | ||
| return target.set_x(x).set_y(y).set(width=new_width + margin) |
There was a problem hiding this comment.
Shouldn't it be new_width + 2 * margin?
I would expect the margin to be on both sides.
There was a problem hiding this comment.
Confusingly, the meaning of margin on main also is the left and right margin added. Should be fixed in a follow-up.
The frame was a ScreenRectangle subclass that computed its boundary and center with a shortcut, for the many projections of a 3D frame. Each drawn frame now computes its view once, so the shortcut no longer makes a measurable difference (3D and text benchmarks within noise), and the frame is an ordinary rectangle again. Raised in review.
auto_zoom() set the frame to the mobjects' bounding box; for a point (or mobjects without width and height) and no margin, the frame got no size, and every later auto_zoom divided by its width and height (also on main). It now raises instead, and compares the aspect ratios without dividing. Bounds are read with get_left()/get_right()/get_top()/get_bottom(), the width and height without abs(), and the docstring says that margin is added to the total width or height, as it always was. Found in review.
Mobject.get_image created a renderer and closed it in a finally clause. CairoRenderer is now a context manager (it closes on exit), and get_image uses it. The renderer also reads config attributes instead of keys and conversions, chooses its camera in one expression, and keeps the file writer's type. Suggested in review.
The package's __getattr__ imported every camera class to pick one from a dict; it imports the module that has the name and takes it from there. The camera reads config.frame_width, config.background_color and config.background_opacity as attributes, defaults last. Suggested in review.
| from manim.typing import MatrixMN, Point3D | ||
|
|
||
| from .. import config | ||
| from ..utils import opengl | ||
| from ... import config | ||
| from ...utils import opengl |
There was a problem hiding this comment.
| from manim.typing import MatrixMN, Point3D | |
| from .. import config | |
| from ..utils import opengl | |
| from ... import config | |
| from ...utils import opengl | |
| from manim import config | |
| from manim.typing import MatrixMN, Point3D | |
| from manim.utils import opengl |
There was a problem hiding this comment.
The rest of the codebase is predominantly relative imports, switching this should be done more consistently across the codebase (if we wanted to)
The examples of moving a scene's camera declare construct() -> None and the types of their helper, and begin_ambient_camera_rotation() and add_fixed_orientation_mobjects() declare their return and keyword types. Suggested in review.
behackl
left a comment
There was a problem hiding this comment.
Thanks for all the big and small comments; these reviews have improved the first part of this stack considerably again.
auto_zoom() refused a zoom only when the larger of the mobjects' width and height plus the margin was not positive, but it then sets whichever side needs the larger frame for the frame's aspect ratio. A 1.5 x 1 rectangle with margin=-1.2 passed the check on its width and gave the 16:9 frame a height of -0.2, flipping the picture. The check now uses the side that is set. Raised in review.
Since the background image cache moved from each target to the renderer, nothing cleared it: close() promises to release the image buffers, but a closed renderer kept every decoded background file (about 33 MB for a 4K image) for as long as the renderer object lived, for example in a scene kept after rendering or in Jupyter. Targets cleared their cache in close() before. close() now clears the renderer's cache. Raised in review.
CairoRenderer is a context manager, so the temporary renderer that draws Scene.get_image() closes at the end of a with block instead of in a finally clause. Same behavior; flagged by CodeQL.
The cameras guide's TwoCameraViews example was plain code; it is now a manim directive, rendered in the docs like the other camera examples (about a second at low quality). Mobject.show had no docstring, so the API reference left it out and the guide's :meth: reference to it was no link. Raised in review.
behackl
left a comment
There was a problem hiding this comment.
Fixed the bugs uncovered in the latest review round + implemented some changes that I've had postponed to follow-ups before.
Overview: What does this pull request change?
This PR makes the camera/renderer ownership boundary explicit: Cairo cameras describe semantic views, while
CairoRendererowns raster targets, PyCairo drawing, composition, static-image reuse, readback, and cleanup.It also packages concrete implementations under
manim.renderer.cairoandmanim.renderer.opengl, gives the default CairoCameraan animatable frame andauto_zoom(), and preserves ThreeD, ZoomedScene, nested-camera, image, point-cloud, background, and static-cache behavior.This change assumes the renderer-independent writer boundary introduced by #4976. Renderer-owned play loops, clocks, cache decisions, and writer scheduling remain unchanged for the later Manager execution refactor.
Motivation and Explanation: Why and how do your changes improve the library?
Cairo cameras previously combined semantic view state with pixel buffers, PyCairo contexts, drawing, composition, and readback. That made camera movement a special case and made render-resource ownership difficult to reason about.
After this change:
Camera,MovingCamera,MultiCamera, andThreeDCameraown semantic frame, background, ordering, projection, shading, tracker, and nested-view state;CairoRendererand private helpers own all Cairo resources and raster work;For ordinary Cairo scenes, camera movement no longer requires a specialized scene class:
MovingCameraandMovingCameraSceneare deprecated: useCameraandScene.ZoomedSceneis now a plainScenesubclass.Breaking changes and migration
Supported top-level imports remain unchanged:
Implementation-module imports are intentionally a clean break; no duplicate compatibility modules are retained.
Module paths
manim.camera.cameramanim.renderer.cairo.cameramanim.camera.moving_cameramanim.renderer.cairo.cameramanim.camera.multi_cameramanim.renderer.cairo.cameramanim.camera.three_d_cameramanim.renderer.cairo.cameramanim.renderer.cairo_renderermanim.renderer.cairo.rendererormanim.renderer.cairomanim.renderer.opengl_renderermanim.renderer.opengl.renderer,manim.renderer.opengl.camera, ormanim.renderer.openglmanim.renderer.opengl_renderer_windowmanim.renderer.opengl.windowmanim.renderer.shadermanim.renderer.opengl.shadermanim.renderer.shader_wrappermanim.renderer.opengl.shader_wrappermanim.renderer.vectorized_mobject_renderingmanim.renderer.opengl.vectorized_mobject_renderingOpenGL shader resources move from
manim/renderer/shaderstomanim/renderer/opengl/shaders. Plugins loading those resources directly must update their package-relative paths.Camera construction and raster access
Cairo camera constructors now accept semantic view settings only. The following raster-oriented arguments are removed:
pixel_width,pixel_height, andframe_rate;image_mode,n_channels, andpixel_array_dtype;backgroundpixel arrays andcairo_line_width_multiple; andMovingCamera.fixed_dimension.Use
config.pixel_width,config.pixel_height, andconfig.frame_ratefor render execution settings. Cairo output is standardized on renderer-owneduint8RGBA targets. Usebackground_image,background_color, andbackground_opacityfor camera backgrounds. Configure logical view geometry withframe=,frame_width=,frame_height=, or the publiccamera.framemobject. Unknown camera keywords now raiseTypeErrorinstead of being retained silently.Camera-owned raster APIs are also removed, including
camera.pixel_array,capture_mobjects(),get_image(), Cairo-context caches, anddisplay_*/apply_*drawing helpers. Direct raster users should render throughCairoRenderer:For one-off mobject images,
mobject.get_image(camera=camera)manages that temporary renderer automatically.Custom camera subclasses should implement semantic hooks such as object ordering, point projection, shading, and movement controls. Backend drawing extensions belong in a renderer integration rather than a camera subclass.
Removed camera implementations
MappingCamera,OldMultiCamera, andSplitScreenCameraare removed.apply_function()rather than throughMappingCamera.MultiCamerawith positionedImageMobjectFromCameradisplay mobjects.BackgroundColoredVMobjectDisplayer, previously exported frommanim.camera.cameraand the top-levelmanimnamespace, is removed without replacement.CairoRenderercomposites VMobjects with background images itself; color them withVMobject.color_using_background_image().ImageMobjectFromCameraremains available, but it is now a geometry-bearingMobjectrather than anAbstractImageMobject. It stores nopixel_arrayand has noget_pixel_array()API;CairoRendererrenders the source camera into a secondary target and composites it into the display geometry. Existing positioning, display-frame, and resampling usage remains supported.For ThreeD camera positioning, use
camera.frameorcamera.frame_center; the private_frame_centermobject is removed.ThreeDCamera.rotation_matrixis read-only and computed from the current angles when read, so projection queries between frames no longer need a draw first. Change the angle trackers, or overridegenerate_rotation_matrix().Camera.auto_zoom()raisesValueErrorfor mobjects with neither width nor height unlessmarginis positive. Before, the frame collapsed to zero size.CairoRenderer.update_frame()takesexcluded_mobjectsas a keyword-only argument.Links to added or changed documentation pages
Further Information and Comments
This PR does not move scheduling or resource lifecycle into
Manager, redesign OpenGL context lifetime, or unify Cairo and OpenGL cameras. Tests etc. pass locally.AI assistance was used during implementation and review. The resulting design, code, documentation, and tests were reviewed by the author.