-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[EXPERIMENTAL] Remove ThreeDScene and rewrite Camera internals
#4059
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
[EXPERIMENTAL] Remove ThreeDScene and rewrite Camera internals
#4059
Conversation
ThreeDScene and rewrite Camera internalsThreeDScene and rewrite Camera internals
JasonGrace2282
left a comment
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.
LGTM! Just one small change/discussion
manim/renderer/renderer.py
Outdated
| self.capabilities = [ | ||
| (OpenGLVMobject, self.render_vmobject), | ||
| (ImageMobject, self.render_image), | ||
| (OpenGLMobject, lambda mob: None), |
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'm not a big fan of this in experimental, I would prefer a hard error (XYZ cannot be rendered)
Maybe a marker class is neccessary? Something like
class InvisibleMobject:
pass
class Camera(OpenGLMobject, InvisibleMobject):
...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.
While using lambda mob: None was admittedly a bad idea, and the InvisibleMobject marker class is interesting (see below), it would be great if we could render the submobjects of any OpenGLMobject such as a Group.
I was thinking of something like:
def __init__(self):
self.capabilities = [
(OpenGLVMobject, self.render_vmobject),
(ImageMobject, self.render_image),
]
def render(self, state: SceneState) -> None:
self.pre_render(state.camera)
for mob in state.mobjects:
self.render_mobject(mob)
self.post_render()
def render_mobject(self, mob: OpenGLMobject) -> None:
for MobClass, render_func in self.capabilities:
if isinstance(mob, MobClass):
render_func(mob)
break
else:
if not isinstance(mob, InvisibleMobject):
logger.warn(
f"{type(mob).__name__} cannot be rendered by {type(self).__name__}. "
"Attempting to render its submobjects..."
)
for submob in mob.submobjects:
self.render_mobject(submob)The idea is that, if the OpenGLMobject is neither an OpenGLVMobject nor an ImageMobject, then it is not directly rendered. Instead, this will attempt to render any of its submobjects, in case the renderer is capable of rendering them. This skips the Camera and attempts to render the contents of a Group.
Plus, because everything in SceneState.mobjects is an OpenGLMobject, we wouldn't really be raising any error. Instead, maybe we could only log a warning if the OpenGLMobject is not an InvisibleMobject such as a Group or a Camera.
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.
My concern is that it wouldn't raise any error/message for something we don't support (yet).
Maybe the following changes would work:
logger.debuginstead oflogger.warn(so users don't get bombarded when usingGroup)if not mob.submobjects: logger.warn(f"Cannot render {mob}")
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 made an implementation, and I would like you to take a look, please.
I'm still using logger.warning, but I implemented an ._unsupported_mob_warnings attribute to store warning messages and avoid spamming the console on every frame.
I marked Group as InvisibleMobject, so no warnings would be raised for it unless any of its submobjects is not renderable and is not an InvisibleMobject itself. Same with Camera. Any other OpenGLMobject will have a warning raised.
IDK about raising a hard error, though. If we want to go down that route, maybe we should enforce that Mobjects can't have submobjects unless they're explicitly Groups, but that might be controversial and should be done in another PR.
JasonGrace2282
left a comment
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.
LGTM. Out of curiosity, have you tried running the tests to see how many pass (with -n 0 to prevent crashing the computer)
| ) | ||
| # This prevents spamming the console. | ||
| if warning_message not in self._unsupported_mob_warnings: | ||
| logger.warning(warning_message) |
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.
Not specific to this PR/holding it back, but I wonder if we should switch to using the official method of warnings in python (warnings.warn) 🤔
Co-authored-by: Aarush Deshpande <[email protected]>
Co-authored-by: Aarush Deshpande <[email protected]>
for more information, see https://pre-commit.ci
332 failed, 226 passed, 29 skipped, 15 warnings, 3 errors None of them seem related to the changes in this PR. I made sure to remove all references to Many of them fail with the following error: manim/renderer/opengl_renderer.py:442: in get_pixels
raw = self.output_fbo.read(components=4, dtype="f1", clamp=True) # RGBA, floats
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <moderngl.Framebuffer object at 0x7d462446e0f0>, viewport = (0, 0, 1920, 1080), components = 4, attachment = 0, alignment = 1, dtype = 'f1', clamp = True
def read(self, viewport=None, components=3, attachment=0, alignment=1, dtype="f1", clamp=False):
if viewport is None:
viewport = (0, 0, self.width, self.height)
if len(viewport) == 2:
viewport = (0, 0, *viewport)
res, mem = mgl.writable_bytes(mgl.expected_size(viewport[2], viewport[3], 1, components, alignment, dtype))
> self.mglo.read_into(mem, viewport, components, attachment, alignment, clamp, dtype, 0)
E AttributeError: 'InvalidObject' object has no attribute 'read_into'so we have to figure out why |
ThreeDSceneand moved many of its methods intoCamera. For example,ThreeDScene.begin_ambient_camera_rotation()becomesCamera.begin_ambient_rotation()andThreeDScene.begin_3dillusion_camera_rotation()becomesCamera.begin_precession().ThreeDScenewere changed toScene.Camerawas rewritten to work directly with._theta,._phiand._gammaangles rather than using an.orientationquaternion which is constantly converted to a SciPyRotationand then to a matrix, Euler angles or another quaternion.._thetaangle forCamerais-TAU/4instead of0, which is more representative of its initial orientation. When increasing._phia bit, theCamerais positioned over the negative Y axis, which corresponds totheta=-TAU/4, nottheta=0.Cameranow directly uses.focal_distanceinstead of.focal_dist_to_height. This was needed in order to correctly implement zooming.Cameracan be properly zoomed in and out. Previously, it couldn't. Scaling theCameraused to only cause a difference in the focal distance.Reviewer Checklist