add type annotations and docstrings for opengl_renderer.py #4537
Conversation
|
CI tests failing only on mac due to font-related things seems really weird lol. I don't think this is related to this PR at all |
The Window import was moved to module-level in a recent type annotation commit, but this causes opengl_renderer_window.py to be imported at load time, triggering pyglet which fails on headless systems (RTD) when it tries to create a shadow window. Moving the import into TYPE_CHECKING preserves type hints for mypy while avoiding runtime display dependency.
- Cast np.linalg.inv() result to correct type - Convert quaternion list to ndarray before passing to rotation_matrix_transpose_from_quaternion - Cast get_center return value - Fix return type for pixel_coords_to_space_coords (ensure float dtype) - Add type: ignore for moderngl.create_context backend arg (incorrect stubs) - Add type: ignore for blend_func assignment (incorrect stubs)
- Import MatrixMN and Point3D at runtime (needed for typing.cast) - Cast np.linalg.inv() result to correct MatrixMN type - Convert quaternion list to ndarray before passing to rotation_matrix_transpose_from_quaternion - Cast get_center and pixel_coords_to_space_coords return values to Point3D - Ensure float dtype in np.array literals for type consistency
The Window class is imported inside TYPE_CHECKING for type hints, but needs to be imported at runtime when actually creating a window. This import is deferred to avoid triggering pyglet display initialization on headless systems (RTD build).
for more information, see https://pre-commit.ci
behackl
left a comment
behackl
left a comment
There was a problem hiding this comment.
Thank you very much for your hard work with this contribution! There was an issue with mypy when running it locally that I've added a minor fix to improve, and there was a problem with the Window import where I've restored the previous behavior to avoid having the library import fail on headless systems.
Otherwise, the documentation and typehint improvements here seem fine to me (and are much appreciated)!
One thing to note, however: in case you'd like to continue improving the OpenGL rendering pipeline, have a look at what we are doing on the experimental branch -- basically a future-proof rewrite of a good chunk of the rendering system to make it less entwined and easier to maintain in the future.
Again, very much appreciate your work here!
2 participants
Overview: What does this pull request change?
Motivation and Explanation: Why and how do your changes improve the library?
For #3375
Links to added or changed documentation pages
Further Information and Comments
The typing revealed a bug in the
get_texture_idmethod: it is passed a path (string), but throughout the function it is treated as if it were a PIL Image. No test triggers this call, and I did not find anything that could trigger it. To trigger it, there must be a mobject that has a shader inmobject.get_shader_wrapper_list()with a non-emptyshader.texture.texture_paths. To fix this, thecreate_texturemethod open the path into a PIL Image.Also, there are some uses of
typing.cast. This is because of the implicit contracts "using OpenGLRenderer => the camera is an OpenGLCamera" and "using OpenGLRenderer => all mobjects are OpenGLMobject or OpenGLVMobject”. I think the proper way to deal with this would be to introduce generics in the Scene class, such asScene[CameraType, MobjectType], and define protocol classes to bound these types. For example, for the camera:but the cast seems quite reasonable compared to this complexity.
Reviewer Checklist