Skip to content

Fix wrong value of MObject width/height with empty VGroup #4088

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+27 −24
Open

Fix wrong value of MObject width/height with empty VGroup #4088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
33ac00b
Issue-4087 Add bug test
gvauvert Jan 2, 2025
e9443ae
Issue-4087 Fix bug
gvauvert Jan 2, 2025
1e59773
Issue-4087 Fix comment and add type hint
gvauvert Jan 2, 2025
7337501
Issue-4087 Fix comment
gvauvert Jan 3, 2025
8b5d831
Issue-4087 Improve previous test instead of adding a new one
gvauvert Jan 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 13 additions & 8 deletions manim/mobject/mobject.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1979,13 +1979,13 @@ def restore(self) -> Self:
self.become(self.saved_state)
return self

def reduce_across_dimension(self, reduce_func: Callable, dim: int):
def reduce_across_dimension(self, reduce_func: Callable, dim: int) -> float | None:
"""Find the min or max value from a dimension across all points in this and submobjects."""
Comment on lines +1982 to 1983
Copy link
Contributor

Choose a reason for hiding this comment

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

As this method can now also return None, it would be nice to document this new behavior in the docstring. Something along the lines of:

Suggested change
def reduce_across_dimension(self, reduce_func: Callable, dim: int) -> float | None:
"""Find the min or max value from a dimension across all points in this and submobjects."""
def reduce_across_dimension(self, reduce_func: Callable, dim: int) -> float | None:
"""Find the min or max value from a dimension across all points in this Mobject and its
submobjects. This allows for using :meth:`~.length_over_dim` to calculate its length over
a dimension, i.e. its height, width or depth. If this Mobject is empty, return ``None``,
since this Mobject should not be taken into account when calculating lengths.
Parameters
----------
reduce_func
The reducer function to use in order to calculate a value over a dimension.
dim
The dimension to use. It should be 0, 1 or 2, representing the X, Y or Z coordinate,
respectively.
Returns
-------
float | None
The min or max value over the dimension specified by ``dim``, or ``None`` if this
Mobject is empty.
"""

assert dim >= 0
assert dim <= 2
if len(self.submobjects) == 0 and len(self.points) == 0:
# If we have no points and no submobjects, return 0 (e.g. center)
return 0
# If we have no points and no submobjects, return None
return None

# If we do not have points (but do have submobjects)
# use only the points from those.
Expand All @@ -1998,7 +1998,11 @@ def reduce_across_dimension(self, reduce_func: Callable, dim: int):
# smallest dimension they have and compare it to the return value.
for mobj in self.submobjects:
value = mobj.reduce_across_dimension(reduce_func, dim)
rv = value if rv is None else reduce_func([value, rv])
if rv is None:
rv = value
else:
if value is not None:
rv = reduce_func([value, rv])
return rv

def nonempty_submobjects(self) -> list[Self]:
Expand Down Expand Up @@ -2149,10 +2153,11 @@ def get_nadir(self) -> Point3D:

def length_over_dim(self, dim: int) -> float:
Copy link
Author

Choose a reason for hiding this comment

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

As this function is meant to be used only internally, it should be private

Suggested change
def length_over_dim(self, dim: int) -> float:
def __length_over_dim(self, dim: int) -> float:

or at least protected

Suggested change
def length_over_dim(self, dim: int) -> float:
def _length_over_dim(self, dim: int) -> float:

Copy link
Contributor

@chopan050 chopan050 Jul 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It makes sense. In that case, .reduce_across_dimension() is an even stronger case of a method which should be marked as internal.

Now, I would probably not mark them as internal in this PR, because there's already a PR which proposes marking some methods as "internal". It's worth checking it and proposing to include these methods in that list:
#3793

"""Measure the length of an :class:`~.Mobject` in a certain direction."""
return self.reduce_across_dimension(
max,
dim,
) - self.reduce_across_dimension(min, dim)
val_max = self.reduce_across_dimension(max, dim)
val_min = self.reduce_across_dimension(min, dim)
if val_max is None and val_min is None:
return 0
return val_max - val_min

def get_coord(self, dim: int, direction: Vector3D = ORIGIN):
"""Meant to generalize ``get_x``, ``get_y`` and ``get_z``"""
Expand Down
30 changes: 14 additions & 16 deletions tests/module/mobject/mobject/test_mobject.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import numpy as np
import pytest

from manim import DL, UR, Circle, Mobject, Rectangle, Square, VGroup
from manim import DL, DR, UL, UR, Circle, Mobject, Rectangle, Square, VGroup


def test_mobject_add():
Expand Down Expand Up @@ -136,21 +136,19 @@ def test_mobject_dimensions_nested_mobjects():


def test_mobject_dimensions_mobjects_with_no_points_are_at_origin():
Copy link
Author

Choose a reason for hiding this comment

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

As the previous test was testing an unexpected behavior, it should have been linked to an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Now, given that the unexpected behavior is fixed and empty Mobjects are no longer considered as "at the origin", it would be nice to change the name of this test in order to reflect that:

Suggested change
def test_mobject_dimensions_mobjects_with_no_points_are_at_origin():
def test_mobject_dimensions_mobjects_with_no_points():

rect = Rectangle(width=2, height=3)
rect.move_to([-4, -5, 0])
outer_group = VGroup(rect)

# This is as one would expect
assert outer_group.width == 2
assert outer_group.height == 3

# Adding a mobject with no points has a quirk of adding a "point"
# to [0, 0, 0] (the origin). This changes the size of the outer
# group because now the bottom left corner is at [-5, -6.5, 0]
# but the upper right corner is [0, 0, 0] instead of [-3, -3.5, 0]
outer_group.add(VGroup())
assert outer_group.width == 5
assert outer_group.height == 6.5
for direction in [DL, DR, UL, UR]:
rect = Rectangle(width=2, height=3)
rect.move_to(direction * 10)
outer_group = VGroup(rect)

# This is as one would expect
assert outer_group.width == 2
assert outer_group.height == 3

# Adding a mobject with no points does not change its size
outer_group.add(VGroup())
assert outer_group.width == 2
assert outer_group.height == 3


def test_mobject_dimensions_has_points_and_children():
Expand Down
Loading