-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Allow VGroup type subscripting #3606
base: main
Are you sure you want to change the base?
Allow VGroup type subscripting #3606
Conversation
This comment has been minimized.
This comment has been minimized.
What are you trying to achieve with this feature? |
I agree that would be the best solution, but I wanted to avoid (if possible) having to manually make every class a
Oh I didn't know that, thanks for letting me know :) |
Having class VGroup(VMobject, Generic[T], metaclass=ConvertToOpenGL):
def __init__(self, *vmobjects: T, **kwargs):
super().__init__(**kwargs)
self.add(*vmobjects) But it would imply
Not sure I understood this part, could you please give an example? |
It is not in fact the case, but maybe something like
Sure, ideally we would only change what class VGroup(VMobject, Generic[T], metaclass=ConvertToOpenGL):
def __init__(self, *vmobjects: T, **kwargs):
super().__init__(**kwargs)
self.add(*vmobjects)
class Subclass1(VGroup):
pass
class Subclass2(VGroup[T]):
pass
Subclass1[VMobject] # error
Subclass2[VMobject] # no error It was just me being lazy and not wanting to have to create a |
I see. While this might look cumbersome to add the type variable to every subclass, this is required for type checkers to understand the generic class. Adding a bare In my opinion, the type variable should only be added to
You can do something like: VMobjectT = TypeVar("VMobjectT", bound=VMobject)
class VGroup(VMobject, Generic[VMobjectT], metaclass=ConvertToOpenGL):
def __init__(self, *vmobjects: VMobjectT, **kwargs): ... That way, users that use different vmobjects in a vgroup won't be impacted: g = VGroup(DashedVMobject(), DashedVMobject())
reveal_type(g) # VGroup[DashedVMobject()]
g = VGroup(DashedVMobject(), Triangle())
reveal_type(g) # "VGroup[DashedVMobject | Triangle]" for pyright | "VGroup[VMobject]" for mypy We could also make use of PEP 696 in this case. It is still in draft but will most probably make it: from typing_extensions import TypeVar
VMobjectT = TypeVar("VMobjectT", bound=VMobject, default=VMobject) That way a plain As you can see, it is not perfect as mypy and pyright handle unions differently. But that could still work |
And I assume all subclasses would be subclassing class ManimBanner(VGroup[VMobject]):
... |
If the subclass behaves the same way |
.Mobject.__class_getitem__
to allow type subscripting
Seeing as PEP 696 was accepted I've gone ahead and made the changes, let me know what you think! |
9caa588
to
99c3241
Compare
this week I'll have time to review your typing related PRs. |
d4a45d2
to
1d895b2
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
) | ||
from manim.mobject.geometry.line import Line | ||
from manim.mobject.geometry.polygram import RoundedRectangle | ||
from manim.mobject.mobject import Mobject | ||
from manim.mobject.types.vectorized_mobject import VGroup | ||
from manim.utils.color import BLACK, RED, YELLOW, ManimColor, ParsableManimColor | ||
|
||
if TYPE_CHECKING: |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.shape_matchers
definition
import
'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
manim.mobject.geometry.shape_matchers
definition
import
@@ -70,7 +70,7 @@ | |||
from manim.constants import * | |||
from manim.mobject.geometry.arc import Dot | |||
from manim.mobject.svg.svg_mobject import SVGMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.text.text_mobject
definition
import
'VGroup' may not be defined if module
manim.mobject.types.vectorized_mobject
manim.mobject.text.text_mobject
definition
import
@@ -70,7 +70,7 @@ | |||
from manim.constants import * | |||
from manim.mobject.geometry.arc import Dot | |||
from manim.mobject.svg.svg_mobject import SVGMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.text.text_mobject
definition
import
'VMobject' may not be defined if module
manim.mobject.types.vectorized_mobject
manim.mobject.text.text_mobject
definition
import
@@ -70,7 +70,7 @@ | |||
from manim.constants import * | |||
from manim.mobject.geometry.arc import Dot | |||
from manim.mobject.svg.svg_mobject import SVGMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject | |||
from manim.mobject.types.vectorized_mobject import VGroup, VMobject, VMobjectT |
Check failure
Code scanning / CodeQL
Module-level cyclic import Error
manim.mobject.types.vectorized_mobject
manim.mobject.text.text_mobject
definition
import
'VMobjectT' may not be defined if module
Allows things like
VGroup[Rectangle]
[v1]
Adds a
__class_getitem__
method toMobject
This was not useful, as discussed later
[v2]
Make
VGroup
a genericGuideline
Since
VGroup
's are invariant, typeVGroup
's similar to lists. For example:Correct:
Incorrect:
Also renders correctly in the docs, ex LinearTransformationScene.get_ghost_vectors