Skip to content

Conversation

@arnaud-ma
Copy link
Contributor

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_id method: 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 in mobject.get_shader_wrapper_list() with a non-empty shader.texture.texture_paths. To fix this, the create_texture method 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 as Scene[CameraType, MobjectType], and define protocol classes to bound these types. For example, for the camera:

class Camera(typing.Protocol): 
    minimal methods/attributes needed for a camera ...

_CameraType = typing.TypeVar("_CameraType", bound=Camera)

class Scene(typing.Generic[_CameraType]: 
     def __init__(self, ..., camera_class: type[_CameraType]):
         ...

but the cast seems quite reasonable compared to this complexity.

Reviewer Checklist

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

@arnaud-ma
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant