Fix assertions and improve error messages when adding submobjects (#3756)

* Optimized AnimationGroup computation of start-end times with lag ratio

* Added extra comment for init_run_time

* Added full path to imports in composition.py

* Optimized AnimationGroup.interpolate

* Fixed final bugs

* Removed accidental print

* Final fix to AnimationGroup.interpolate

* Fixed animations being skipped unintentionally

* Fix and improve Mobject assertions when adding submobjects

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update examples in Mobject.add() and OpenGLMobject.add() docstrings

* overriden -> overridden

* Joined string in OpenGLMobject error message

* Address requested changes

* OpenGLVMObjects -> OpenGLVMobjects

* Use tuplify in VGroup.__setitem__()

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Author: chopan050

manim/mobject/mobject.py

@@ -116,6 +116,63 @@ def __init__(
         self.generate_points()
         self.init_colors()
 
+    def _assert_valid_submobjects(self, submobjects: Iterable[Mobject]) -> Self:
+        """Check that all submobjects are actually instances of
+        :class:`Mobject`, and that none of them is ``self`` (a
+        :class:`Mobject` cannot contain itself).
+
+        This is an auxiliary function called when adding Mobjects to the
+        :attr:`submobjects` list.
+
+        This function is intended to be overridden by subclasses such as
+        :class:`VMobject`, which should assert that only other VMobjects
+        may be added into it.
+
+        Parameters
+        ----------
+        submobjects
+            The list containing values to validate.
+
+        Returns
+        -------
+        :class:`Mobject`
+            The Mobject itself.
+
+        Raises
+        ------
+        TypeError
+            If any of the values in `submobjects` is not a :class:`Mobject`.
+        ValueError
+            If there was an attempt to add a :class:`Mobject` as its own
+            submobject.
+        """
+        return self._assert_valid_submobjects_internal(submobjects, Mobject)
+
+    def _assert_valid_submobjects_internal(
+        self, submobjects: list[Mobject], mob_class: type[Mobject]
+    ) -> Self:
+        for i, submob in enumerate(submobjects):
+            if not isinstance(submob, mob_class):
+                error_message = (
+                    f"Only values of type {mob_class.__name__} can be added "
+                    f"as submobjects of {type(self).__name__}, but the value "
+                    f"{submob} (at index {i}) is of type "
+                    f"{type(submob).__name__}."
+                )
+                # Intended for subclasses such as VMobject, which
+                # cannot have regular Mobjects as submobjects
+                if isinstance(submob, Mobject):
+                    error_message += (
+                        " You can try adding this value into a Group instead."
+                    )
+                raise TypeError(error_message)
+            if submob is self:
+                raise ValueError(
+                    f"Cannot add {type(self).__name__} as a submobject of "
+                    f"itself (at index {i})."
+                )
+        return self
+
     @classmethod
     def animation_override_for(
         cls,
@@ -414,12 +471,19 @@ def add(self, *mobjects: Mobject) -> Self:
             >>> len(outer.submobjects)
             1
 
+        Only Mobjects can be added::
+
+            >>> outer.add(3)
+            Traceback (most recent call last):
+            ...
+            TypeError: Only values of type Mobject can be added as submobjects of Mobject, but the value 3 (at index 0) is of type int.
+
         Adding an object to itself raises an error::
 
             >>> outer.add(outer)
             Traceback (most recent call last):
             ...
-            ValueError: Mobject cannot contain self
+            ValueError: Cannot add Mobject as a submobject of itself (at index 0).
 
         A given mobject cannot be added as a submobject
         twice to some parent::
@@ -433,12 +497,7 @@ def add(self, *mobjects: Mobject) -> Self:
             [child]
 
         """
-        for m in mobjects:
-            if not isinstance(m, Mobject):
-                raise TypeError("All submobjects must be of type Mobject")
-            if m is self:
-                raise ValueError("Mobject cannot contain self")
-
+        self._assert_valid_submobjects(mobjects)
         unique_mobjects = remove_list_redundancies(mobjects)
         if len(mobjects) != len(unique_mobjects):
             logger.warning(
@@ -464,10 +523,7 @@ def insert(self, index: int, mobject: Mobject) -> None:
         mobject
             The mobject to be inserted.
         """
-        if not isinstance(mobject, Mobject):
-            raise TypeError("All submobjects must be of type Mobject")
-        if mobject is self:
-            raise ValueError("Mobject cannot contain self")
+        self._assert_valid_submobjects([mobject])
         self.submobjects.insert(index, mobject)
 
     def __add__(self, mobject: Mobject):
@@ -520,13 +576,7 @@ def add_to_back(self, *mobjects: Mobject) -> Self:
         :meth:`add`
 
         """
-        if self in mobjects:
-            raise ValueError("A mobject shouldn't contain itself")
-
-        for mobject in mobjects:
-            if not isinstance(mobject, Mobject):
-                raise TypeError("All submobjects must be of type Mobject")
-
+        self._assert_valid_submobjects(mobjects)
         self.remove(*mobjects)
         # dict.fromkeys() removes duplicates while maintaining order
         self.submobjects = list(dict.fromkeys(mobjects)) + self.submobjects

manim/mobject/opengl/opengl_mobject.py

@@ -165,6 +165,64 @@ def __init__(
 
         self.should_render = should_render
 
+    def _assert_valid_submobjects(self, submobjects: Iterable[OpenGLMobject]) -> Self:
+        """Check that all submobjects are actually instances of
+        :class:`OpenGLMobject`, and that none of them is
+        ``self`` (an :class:`OpenGLMobject` cannot contain itself).
+
+        This is an auxiliary function called when adding OpenGLMobjects to the
+        :attr:`submobjects` list.
+
+        This function is intended to be overridden by subclasses such as
+        :class:`OpenGLVMobject`, which should assert that only other
+        OpenGLVMobjects may be added into it.
+
+        Parameters
+        ----------
+        submobjects
+            The list containing values to validate.
+
+        Returns
+        -------
+        :class:`OpenGLMobject`
+            The OpenGLMobject itself.
+
+        Raises
+        ------
+        TypeError
+            If any of the values in `submobjects` is not an
+            :class:`OpenGLMobject`.
+        ValueError
+            If there was an attempt to add an :class:`OpenGLMobject` as its own
+            submobject.
+        """
+        return self._assert_valid_submobjects_internal(submobjects, OpenGLMobject)
+
+    def _assert_valid_submobjects_internal(
+        self, submobjects: list[OpenGLMobject], mob_class: type[OpenGLMobject]
+    ) -> Self:
+        for i, submob in enumerate(submobjects):
+            if not isinstance(submob, mob_class):
+                error_message = (
+                    f"Only values of type {mob_class.__name__} can be added "
+                    f"as submobjects of {type(self).__name__}, but the value "
+                    f"{submob} (at index {i}) is of type "
+                    f"{type(submob).__name__}."
+                )
+                # Intended for subclasses such as OpenGLVMobject, which
+                # cannot have regular OpenGLMobjects as submobjects
+                if isinstance(submob, OpenGLMobject):
+                    error_message += (
+                        " You can try adding this value into a Group instead."
+                    )
+                raise TypeError(error_message)
+            if submob is self:
+                raise ValueError(
+                    f"Cannot add {type(self).__name__} as a submobject of "
+                    f"itself (at index {i})."
+                )
+        return self
+
     @classmethod
     def __init_subclass__(cls, **kwargs):
         super().__init_subclass__(**kwargs)
@@ -734,28 +792,33 @@ def add(
             >>> len(outer.submobjects)
             1
 
+        Only OpenGLMobjects can be added::
+
+            >>> outer.add(3)
+            Traceback (most recent call last):
+            ...
+            TypeError: Only values of type OpenGLMobject can be added as submobjects of OpenGLMobject, but the value 3 (at index 0) is of type int.
+
         Adding an object to itself raises an error::
 
             >>> outer.add(outer)
             Traceback (most recent call last):
             ...
-            ValueError: OpenGLMobject cannot contain self
+            ValueError: Cannot add OpenGLMobject as a submobject of itself (at index 0).
 
         """
         if update_parent:
             assert len(mobjects) == 1, "Can't set multiple parents."
             mobjects[0].parent = self
 
-        if self in mobjects:
-            raise ValueError("OpenGLMobject cannot contain self")
+        self._assert_valid_submobjects(mobjects)
+
         if any(mobjects.count(elem) > 1 for elem in mobjects):
             logger.warning(
                 "Attempted adding some Mobject as a child more than once, "
                 "this is not possible. Repetitions are ignored.",
             )
         for mobject in mobjects:
-            if not isinstance(mobject, OpenGLMobject):
-                raise TypeError("All submobjects must be of type OpenGLMobject")
             if mobject not in self.submobjects:
                 self.submobjects.append(mobject)
             if self not in mobject.parents:
@@ -784,11 +847,7 @@ def insert(self, index: int, mobject: OpenGLMobject, update_parent: bool = False
         if update_parent:
             mobject.parent = self
 
-        if mobject is self:
-            raise ValueError("OpenGLMobject cannot contain self")
-
-        if not isinstance(mobject, OpenGLMobject):
-            raise TypeError("All submobjects must be of type OpenGLMobject")
+        self._assert_valid_submobjects([mobject])
 
         if mobject not in self.submobjects:
             self.submobjects.insert(index, mobject)
@@ -880,10 +939,12 @@ def add_to_back(self, *mobjects: OpenGLMobject) -> OpenGLMobject:
         :meth:`add`
 
         """
+        self._assert_valid_submobjects(mobjects)
         self.submobjects = list_update(mobjects, self.submobjects)
         return self
 
     def replace_submobject(self, index, new_submob):
+        self._assert_valid_submobjects([new_submob])
         old_submob = self.submobjects[index]
         if self in old_submob.parents:
             old_submob.parents.remove(self)
@@ -2734,8 +2795,6 @@ def throw_error_if_no_points(self):
 
 class OpenGLGroup(OpenGLMobject):
     def __init__(self, *mobjects, **kwargs):
-        if not all(isinstance(m, OpenGLMobject) for m in mobjects):
-            raise Exception("All submobjects must be of type OpenGLMobject")
         super().__init__(**kwargs)
         self.add(*mobjects)
 

manim/mobject/opengl/opengl_vectorized_mobject.py

@@ -25,7 +25,7 @@
 )
 from manim.utils.color import BLACK, WHITE, ManimColor, ParsableManimColor
 from manim.utils.config_ops import _Data
-from manim.utils.iterables import listify, make_even, resize_with_interpolation
+from manim.utils.iterables import make_even, resize_with_interpolation, tuplify
 from manim.utils.space_ops import (
     angle_between_vectors,
     cross2d,
@@ -161,6 +161,9 @@ def __init__(
         if stroke_color is not None:
             self.stroke_color = ManimColor.parse(stroke_color)
 
+    def _assert_valid_submobjects(self, submobjects: Iterable[OpenGLVMobject]) -> Self:
+        return self._assert_valid_submobjects_internal(submobjects, OpenGLVMobject)
+
     def get_group_class(self):
         return OpenGLVGroup
 
@@ -266,7 +269,7 @@ def set_stroke(
 
         if width is not None:
             for mob in self.get_family(recurse):
-                mob.stroke_width = np.array([[width] for width in listify(width)])
+                mob.stroke_width = np.array([[width] for width in tuplify(width)])
 
         if background is not None:
             for mob in self.get_family(recurse):
@@ -1659,8 +1662,6 @@ def construct(self):
     """
 
     def __init__(self, *vmobjects, **kwargs):
-        if not all(isinstance(m, OpenGLVMobject) for m in vmobjects):
-            raise Exception("All submobjects must be of type OpenGLVMobject")
         super().__init__(**kwargs)
         self.add(*vmobjects)
 
@@ -1726,8 +1727,6 @@ def construct(self):
                         (gr-circle_red).animate.shift(RIGHT)
                     )
         """
-        if not all(isinstance(m, OpenGLVMobject) for m in vmobjects):
-            raise TypeError("All submobjects must be of type OpenGLVMobject")
         return super().add(*vmobjects)
 
     def __add__(self, vmobject):
@@ -1773,8 +1772,7 @@ def __setitem__(self, key: int, value: OpenGLVMobject | Sequence[OpenGLVMobject]
 
             >>> config.renderer = original_renderer
         """
-        if not all(isinstance(m, OpenGLVMobject) for m in value):
-            raise TypeError("All submobjects must be of type OpenGLVMobject")
+        self._assert_valid_submobjects(tuplify(value))
         self.submobjects[key] = value
 
 

manim/mobject/types/vectorized_mobject.py

@@ -172,6 +172,9 @@ def __init__(
         if stroke_color is not None:
             self.stroke_color = ManimColor.parse(stroke_color)
 
+    def _assert_valid_submobjects(self, submobjects: Iterable[VMobject]) -> Self:
+        return self._assert_valid_submobjects_internal(submobjects, VMobject)
+
     # OpenGL compatibility
     @property
     def n_points_per_curve(self) -> int:
@@ -2016,14 +2019,6 @@ def construct(self):
                         (gr-circle_red).animate.shift(RIGHT)
                     )
         """
-        for m in vmobjects:
-            if not isinstance(m, (VMobject, OpenGLVMobject)):
-                raise TypeError(
-                    f"All submobjects of {self.__class__.__name__} must be of type VMobject. "
-                    f"Got {repr(m)} ({type(m).__name__}) instead. "
-                    "You can try using `Group` instead."
-                )
-
         return super().add(*vmobjects)
 
     def __add__(self, vmobject: VMobject) -> Self:
@@ -2061,8 +2056,7 @@ def __setitem__(self, key: int, value: VMobject | Sequence[VMobject]) -> None:
             >>> new_obj = VMobject()
             >>> vgroup[0] = new_obj
         """
-        if not all(isinstance(m, (VMobject, OpenGLVMobject)) for m in value):
-            raise TypeError("All submobjects must be of type VMobject")
+        self._assert_valid_submobjects(tuplify(value))
         self.submobjects[key] = value
 
 
@@ -2390,8 +2384,7 @@ def add_key_value_pair(self, key: Hashable, value: VMobject) -> None:
             self.add_key_value_pair('s', square_obj)
 
         """
-        if not isinstance(value, (VMobject, OpenGLVMobject)):
-            raise TypeError("All submobjects must be of type VMobject")
+        self._assert_valid_submobjects([value])
         mob = value
         if self.show_keys:
             # This import is here and not at the top to avoid circular import

tests/module/mobject/mobject/test_mobject.py

@@ -26,15 +26,26 @@ def test_mobject_add():
 
     # check that Mobject.add() returns the Mobject (for chained calls)
     assert obj.add(Mobject()) is obj
+    assert len(obj.submobjects) == 13
+
     obj = Mobject()
 
     # a Mobject cannot contain itself
-    with pytest.raises(ValueError):
-        obj.add(obj)
+    with pytest.raises(ValueError) as add_self_info:
+        obj.add(Mobject(), obj, Mobject())
+    assert str(add_self_info.value) == (
+        "Cannot add Mobject as a submobject of itself (at index 1)."
+    )
+    assert len(obj.submobjects) == 0
 
     # can only add Mobjects
-    with pytest.raises(TypeError):
-        obj.add("foo")
+    with pytest.raises(TypeError) as add_str_info:
+        obj.add(Mobject(), Mobject(), "foo")
+    assert str(add_str_info.value) == (
+        "Only values of type Mobject can be added as submobjects of Mobject, "
+        "but the value foo (at index 2) is of type str."
+    )
+    assert len(obj.submobjects) == 0
 
 
 def test_mobject_remove():