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

gvauvert · 2025-01-02T17:06:17Z

Overview: What does this pull request change?

Fixes #4087

When a Mobject contains an empty VGroup, its width or height can be wrong

Motivation and Explanation: Why and how do your changes improve the library?

Bug fix + tests

Links to added or changed documentation pages

No documentation page, bug fix only.

Further Information and Comments

add test
fix bug
fix another test which result depends on the bug

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

gvauvert · 2025-01-03T08:52:44Z

tests/module/mobject/mobject/test_mobject.py

@@ -136,21 +136,19 @@ def test_mobject_dimensions_nested_mobjects():


 def test_mobject_dimensions_mobjects_with_no_points_are_at_origin():


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

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():

gvauvert · 2025-01-03T08:57:28Z

manim/mobject/mobject.py

@@ -2149,10 +2153,11 @@ def get_nadir(self) -> Point3D:

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


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:

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

chopan050

Thanks for this fix! I left some suggestions:

chopan050 · 2025-07-25T15:47:24Z

manim/mobject/mobject.py

+    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."""


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.

"""

chopan050 · 2025-07-25T15:51:09Z

manim/mobject/mobject.py

@@ -2149,10 +2153,11 @@ def get_nadir(self) -> Point3D:

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


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

chopan050 · 2025-07-25T15:55:34Z

tests/module/mobject/mobject/test_mobject.py

@@ -136,21 +136,19 @@ def test_mobject_dimensions_nested_mobjects():


 def test_mobject_dimensions_mobjects_with_no_points_are_at_origin():


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():

gvauvert mentioned this pull request Jan 2, 2025

Wrong result of Mobject.length_over_dim() #4087

Open

gvauvert commented Jan 3, 2025

View reviewed changes

gvauvert added 5 commits January 6, 2025 09:19

Issue-4087 Add bug test

33ac00b

Issue-4087 Fix bug

e9443ae

Issue-4087 Fix comment and add type hint

1e59773

Issue-4087 Fix comment

7337501

Issue-4087 Improve previous test instead of adding a new one

8b5d831

gvauvert force-pushed the fix-PR-4087 branch from 6792aff to 8b5d831 Compare January 6, 2025 09:19

github-project-automation bot added this to Dev Board Jul 25, 2025

github-project-automation bot moved this to 🆕 New in Dev Board Jul 25, 2025

chopan050 requested changes Jul 25, 2025

View reviewed changes

github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board Jul 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

gvauvert commented Jan 2, 2025 •

edited by chopan050

Loading

Uh oh!

gvauvert Jan 3, 2025

Uh oh!

chopan050 Jul 25, 2025

Uh oh!

gvauvert Jan 3, 2025

Uh oh!

chopan050 Jul 25, 2025 •

edited

Loading

Uh oh!

chopan050 left a comment

Uh oh!

chopan050 Jul 25, 2025

Uh oh!

chopan050 Jul 25, 2025 •

edited

Loading

Uh oh!

chopan050 Jul 25, 2025

Uh oh!

Uh oh!

		@@ -136,21 +136,19 @@ def test_mobject_dimensions_nested_mobjects():


		def test_mobject_dimensions_mobjects_with_no_points_are_at_origin():

		@@ -2149,10 +2153,11 @@ def get_nadir(self) -> Point3D:

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

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

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

		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 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.
+        """

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

gvauvert commented Jan 2, 2025 • edited by chopan050 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview: What does this pull request change?

Motivation and Explanation: Why and how do your changes improve the library?

Links to added or changed documentation pages

Further Information and Comments

Reviewer Checklist

Uh oh!

gvauvert Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

chopan050 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

gvauvert Jan 3, 2025

Choose a reason for hiding this comment

Uh oh!

chopan050 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chopan050 left a comment

Choose a reason for hiding this comment

Uh oh!

chopan050 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

chopan050 Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chopan050 Jul 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvauvert commented Jan 2, 2025 •

edited by chopan050

Loading

chopan050 Jul 25, 2025 •

edited

Loading

chopan050 Jul 25, 2025 •

edited

Loading