Skip to content

Conversation

@henrikmidtiby
Copy link
Contributor

@henrikmidtiby henrikmidtiby commented Aug 10, 2025

Overview: What does this pull request change?

This PR add type annotations to mobject.py.

Issue #3375.

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

Comment on lines 652 to 656
# error: Argument "tip" to "add_tip" of "TipableVMobject" has incompatible type "VMobject"; expected "ArrowTip | None" [arg-type]
self.add_tip(tip=old_tips[0])
if has_start_tip:
# error: Argument "tip" to "add_tip" of "TipableVMobject" has incompatible type "VMobject"; expected "ArrowTip | None" [arg-type]
self.add_tip(tip=old_tips[1], at_start=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These issues can be handled by typing.cast, but I don't think that is the best way to deal with them.

Comment on lines 2294 to 2296
def __getitem__(self, key: int) -> VMobject:
return self.submobjects[key]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is used to define the return type when indexing into a VGroup. If the super class was used, this would return a Mobject, but in a VGroup it should be a VMobject.

temp = self.get_basis_vectors()
i_hat = temp.submobjects[0]
j_hat = temp.submobjects[1]
Copy link
Contributor Author

@henrikmidtiby henrikmidtiby Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the trick for defining the type of elements in a VGroup to be VMobject's does not work here. Which is the reason for this workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, but OK...

temp = array.get_entries()
x_coord = temp.submobjects[0]
y_coord = temp.submobjects[1]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently the trick for defining the type of elements in a VGroup to be VMobject's does not work here. Which is the reason for this workaround.

reference_colors: Sequence[ParsableManimColor],
length_of_output: int,
) -> list[ManimColor] | ManimColor:
) -> list[ManimColor]:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change as in PR #4380

"""
if length_of_output == 0:
return ManimColor(reference_colors[0])
return [ManimColor(reference_colors[0])]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change as in PR #4380.

Comment on lines +96 to +100
animation_overrides: dict[
type[Animation],
FunctionOverride,
] = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By specifying the type of animation_overrides here, a lot of type errors like "type[Mobject]" has no attribute "animation_overrides" are removed.

@henrikmidtiby
Copy link
Contributor Author

henrikmidtiby commented Aug 11, 2025

One of the main contributions in this PR is that when accessing elements in a VGroup, the type of these elements are now recognized as VMobjects.

This is achieved by adding/modifying these methods including type annotations to the VMobject class.

def __iter__(self) -> Iterator[VMobject]:
    return iter(self.split())
def split(self) -> list[VMobject]:
    result: list[VMobject] = [self] if len(self.points) > 0 else []
    return result + self.submobjects

And these to the VGroup class.

def __getitem__(self, key: int) -> VMobject:
    return self.submobjects[key]

@henrikmidtiby henrikmidtiby changed the title Typing mobject.py Add type annotations to mobject.py Aug 13, 2025
@henrikmidtiby henrikmidtiby mentioned this pull request Aug 13, 2025
22 tasks
@henrikmidtiby henrikmidtiby marked this pull request as ready for review August 21, 2025 21:31
@henrikmidtiby henrikmidtiby added the typehints For adding/discussing typehints label Dec 1, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the huge contribution!

I left some suggestions:

Comment on lines -211 to +215
line[start:end].set_color(color)
if end == start:
continue
for single_line in line[start:end]:
single_line.set_color(color)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking both the original and the new code with MyPy locally, it shows no difference (in both cases, there is an error because Mobject.set_color() does not accept None as an argument), so I don't really understand the reason for this change...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, on my computer I get the following error with the original code when I run the pytest testsuite.

        for line, color_range in zip(self.code_lines, color_ranges):
            for start, end, color in color_range:
>               line[start:end].set_color(color)
E               AttributeError: 'list' object has no attribute 'set_color'

Comment on lines 3456 to 3457
# error: "Callable[..., Animation]" has no attribute "_override_animate" [attr-defined]
temp_method._override_animate = animation_method
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typing method so that it has a dynamic attribute _override_animate is difficult.

I propose the following alternative, maybe for another PR: create an _AnimationBuilder class dictionary _AnimationBuilder.animate_override_map: dict[int, types.MethodType] which maps method IDs to their .animate overrides. Thus, this line could be replaced with

_AnimationBuilder.animate_override_map[id(method)] = animation_method

Inside _AnimationBuilder.__getattr__(), instead of doing

has_overriden_animation = hasattr(method, "_override_animate")

we could do

method_override = self.animate_override_map.get(id(method), None)

and then use it inside the internal function update_target().

In the meantime, let's just type ignore this one:

Suggested change
# error: "Callable[..., Animation]" has no attribute "_override_animate" [attr-defined]
temp_method._override_animate = animation_method
method._override_animate = animation_method # type: ignore[attr-defined]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets take this in a separate PR.

temp = self.get_basis_vectors()
i_hat = temp.submobjects[0]
j_hat = temp.submobjects[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's strange, but OK...

Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the huge contribution!

I left some suggestions. In particular, a raise ValueError(...) inside Mobject.arrange_in_grid() is causing an error while building the docs, so that one requires more attention.

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Dec 4, 2025
margin: float,
only_mobjects_in_frame: bool,
animate: Literal[False],
) -> Mobject: ...

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
margin: float,
only_mobjects_in_frame: bool,
animate: Literal[True],
) -> _AnimationBuilder: ...

Check notice

Code scanning / CodeQL

Statement has no effect

This statement has no effect.
@henrikmidtiby
Copy link
Contributor Author

@chopan050 Thanks for all the comments, even the minor nitpicks! I feel they help me improving the PR.
I will get back to resolving the remaining suggestions in the coming days.

@henrikmidtiby
Copy link
Contributor Author

The raise ValueError is now handled.
Then I discovered a new issue, with the following example

from manim import *

# Scene derived from the example given in the documentation
class TableExamples(Scene):
    def construct(self):
        t2 = MathTable(
            [["+", 0, 5, 10],
            [0, 0, 5, 10],
            [2, 2, 7, 12],
            [4, 4, 9, 14]],
            include_outer_lines=True)

        for line in t2.get_horizontal_lines()[:3]:
            line.set_color(BLUE)

        t2.get_vertical_lines().set_color(BLUE)
        t2.get_vertical_lines()[0].set_color(BLUE)

        # AttributeError: 'list' object has no attribute 'set_color'
        t2.get_vertical_lines()[:3].set_color(BLUE)

I have attempted to solve the issue through implementing a __getitem__ method in the VGroup class. But apparently it does not work... Here is the used implementation.

    def __getitem__(self, item: int) -> Self:
        return self.submobjects[item]

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

Labels

typehints For adding/discussing typehints

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

2 participants