diff --git a/manim/mobject/mobject.py b/manim/mobject/mobject.py index 5d52899c7a..33a8e5b818 100644 --- a/manim/mobject/mobject.py +++ b/manim/mobject/mobject.py @@ -15,6 +15,7 @@ import types import warnings from collections.abc import Callable, Iterable, Iterator, MutableSet, Sequence +from contextlib import suppress from functools import partialmethod, reduce from pathlib import Path from typing import TYPE_CHECKING, Any, cast @@ -36,8 +37,12 @@ interpolate_color, ) from ..utils.exceptions import MultiAnimationOverrideException -from ..utils.iterables import list_update, remove_list_redundancies +from ..utils.iterables import ( + list_difference_update, + remove_list_redundancies, +) from ..utils.paths import straight_path +from ..utils.simple_functions import clip from ..utils.space_ops import angle_between_vectors, normalize, rotation_matrix if TYPE_CHECKING: @@ -550,8 +555,37 @@ def add(self, *mobjects: Mobject) -> Self: >>> parent.submobjects [child] + """ + return self._insert_submobjects(len(self.submobjects), mobjects) + + def insert(self, index: int, mobject: Mobject) -> Self: + """Inserts a mobject at a specific position into ``self.submobjects``. + + If ``mobject`` is already a submobject of ``self``, it will be moved to the new + position. + + Parameters + ---------- + index + The index at which + mobject + The mobject to be inserted. + """ + return self._insert_submobjects(index, (mobject,)) + + def _insert_submobjects(self, index: int, mobjects: Sequence[Mobject]) -> Self: + """Common backing implementation for :meth:`add`, :meth:`add_to_back`, and + :meth:`insert`. + + Inserts ``mobjects`` into :attr:`submobjects` such that they end up starting + at ``index`` (following the semantics of :meth:`list.insert`). Mobjects that + are already present are moved to the new position instead of being duplicated. """ self._assert_valid_submobjects(mobjects) + + if not mobjects: + return self + unique_mobjects = remove_list_redundancies(mobjects) if len(mobjects) != len(unique_mobjects): logger.warning( @@ -559,26 +593,47 @@ def add(self, *mobjects: Mobject) -> Self: "this is not possible. Repetitions are ignored.", ) - self.submobjects = list_update(self.submobjects, unique_mobjects) - return self + if not self.submobjects: + self.submobjects = unique_mobjects + return self - def insert(self, index: int, mobject: Mobject) -> Self: - """Inserts a mobject at a specific position into self.submobjects + n = len(self.submobjects) + + # Normalize the index to be in the range [0, n] following the semantics of list.insert + norm_index = clip(index if index >= 0 else n + index, 0, n) + + # Shortcut for the common case of adding a single mobject + if len(unique_mobjects) == 1: + mobject = unique_mobjects[0] + # If the mobject is already present at or next to the provided index, we + # don't need to do anything + if any( + self.submobjects[j] is mobject + for j in (norm_index - 1, norm_index) + if 0 <= j < n + ): + return self - Effectively just calls ``self.submobjects.insert(index, mobject)``, - where ``self.submobjects`` is a list. + try: + old_index = self.submobjects.index(mobject) + except ValueError: # mobject isn't already present + self.submobjects.insert(norm_index, mobject) + return self - Highly adapted from ``Mobject.add``. + # Compensate for list shifting after popping + new_index = norm_index if norm_index < old_index else norm_index - 1 + self.submobjects.pop(old_index) + self.submobjects.insert(new_index, mobject) + return self - Parameters - ---------- - index - The index at which - mobject - The mobject to be inserted. - """ - self._assert_valid_submobjects([mobject]) - self.submobjects.insert(index, mobject) + head = list_difference_update( + it.islice(self.submobjects, norm_index), unique_mobjects + ) + tail = list_difference_update( + it.islice(self.submobjects, norm_index, None), unique_mobjects + ) + + self.submobjects = [*head, *unique_mobjects, *tail] return self def __add__(self, mobject: Mobject) -> Self: @@ -631,11 +686,7 @@ def add_to_back(self, *mobjects: Mobject) -> Self: :meth:`add` """ - self._assert_valid_submobjects(mobjects) - self.remove(*mobjects) - # dict.fromkeys() removes duplicates while maintaining order - self.submobjects = list(dict.fromkeys(mobjects)) + self.submobjects - return self + return self._insert_submobjects(0, mobjects) def remove(self, *mobjects: Mobject) -> Self: """Remove :attr:`submobjects`. @@ -659,9 +710,14 @@ def remove(self, *mobjects: Mobject) -> Self: :meth:`add` """ - for mobject in mobjects: - if mobject in self.submobjects: - self.submobjects.remove(mobject) + if not self.submobjects: + return self + + if len(mobjects) == 1: + with suppress(ValueError): + self.submobjects.remove(mobjects[0]) + return self + self.submobjects = list_difference_update(self.submobjects, mobjects) return self def __sub__(self, other: Mobject) -> Self: @@ -1122,8 +1178,7 @@ def remove_updater(self, update_function: _Updater) -> Self: :meth:`get_updaters` """ - while update_function in self.updaters: - self.updaters.remove(update_function) + self.updaters = list_difference_update(self.updaters, [update_function]) return self def clear_updaters(self, recursive: bool = True) -> Self: @@ -2410,7 +2465,7 @@ def get_pieces(self, n_pieces: float) -> Group: return Group( *( template.copy().pointwise_become_partial(self, a1, a2) - for a1, a2 in zip(alphas[:-1], alphas[1:], strict=True) + for a1, a2 in it.pairwise(alphas) ) ) @@ -2629,7 +2684,7 @@ def construct(self): x = VGroup(s1, s2, s3, s4).set_x(0).arrange(buff=1.0) self.add(x) """ - for m1, m2 in zip(self.submobjects[:-1], self.submobjects[1:], strict=True): + for m1, m2 in it.pairwise(self.submobjects): m2.next_to(m1, direction, buff, **kwargs) if center: self.center() diff --git a/manim/mobject/opengl/opengl_mobject.py b/manim/mobject/opengl/opengl_mobject.py index a99b6aff2a..cec0f040cd 100644 --- a/manim/mobject/opengl/opengl_mobject.py +++ b/manim/mobject/opengl/opengl_mobject.py @@ -8,6 +8,7 @@ import types import warnings from collections.abc import Callable, Iterable, Iterator, Sequence +from contextlib import suppress from functools import partialmethod, wraps from math import ceil from typing import ( @@ -54,6 +55,7 @@ # from ..utils.iterables import batch_by_property from manim.utils.iterables import ( batch_by_property, + list_difference_update, list_update, listify, make_even, @@ -895,28 +897,42 @@ def add(self, *mobjects: OpenGLMobject, update_parent: bool = False) -> Self: self._assert_valid_submobjects(mobjects) - if any(mobjects.count(elem) > 1 for elem in mobjects): + # dict.fromkeys() removes duplicates while maintaining order + unique_mobjects = dict.fromkeys(mobjects) + if len(mobjects) != len(unique_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 len(unique_mobjects) == 1: + mobject = unique_mobjects.popitem()[0] if mobject not in self.submobjects: self.submobjects.append(mobject) - if self not in mobject.parents: - mobject.parents.append(self) - self.assemble_family() + if self not in mobject.parents: + mobject.parents.append(self) + self.assemble_family() + return self + + # Remove already-present mobjects + for mob in self.submobjects: + if mob in unique_mobjects: + unique_mobjects.pop(mob) + + if unique_mobjects: + self.submobjects.extend(unique_mobjects) + for mobject in unique_mobjects: + if self not in mobject.parents: + mobject.parents.append(self) + self.assemble_family() + return self def insert( self, index: int, mobject: OpenGLMobject, update_parent: bool = False ) -> Self: - """Inserts a mobject at a specific position into self.submobjects - - Effectively just calls ``self.submobjects.insert(index, mobject)``, - where ``self.submobjects`` is a list. - - Highly adapted from ``OpenGLMobject.add``. + """Inserts a mobject at a specific position into ``self.submobjects``. If the + mobject is already a submobject of ``self``, its position does not change. Parameters ---------- @@ -932,13 +948,12 @@ def insert( self._assert_valid_submobjects([mobject]) - if mobject not in self.submobjects: - self.submobjects.insert(index, mobject) - if self not in mobject.parents: mobject.parents.append(self) + if mobject not in self.submobjects: + self.submobjects.insert(index, mobject) + self.assemble_family() - self.assemble_family() return self def remove(self, *mobjects: OpenGLMobject, update_parent: bool = False) -> Self: @@ -967,12 +982,22 @@ def remove(self, *mobjects: OpenGLMobject, update_parent: bool = False) -> Self: assert len(mobjects) == 1, "Can't remove multiple parents." mobjects[0].parent = None - for mobject in mobjects: - if mobject in self.submobjects: - self.submobjects.remove(mobject) - if self in mobject.parents: - mobject.parents.remove(self) - self.assemble_family() + if not self.submobjects: + return self + + number_of_submobjects = len(self.submobjects) + if len(mobjects) == 1: + with suppress(ValueError): + self.submobjects.remove(mobjects[0]) + else: + self._submobjects = list_difference_update(self._submobjects, mobjects) + + if len(self.submobjects) != number_of_submobjects: + for mobject in mobjects: + with suppress(ValueError): + mobject.parents.remove(self) + + self.assemble_family() return self def add_to_back(self, *mobjects: OpenGLMobject) -> Self: @@ -1025,11 +1050,33 @@ def add_to_back(self, *mobjects: OpenGLMobject) -> Self: return self def replace_submobject(self, index: int, new_submob: OpenGLMobject) -> Self: + """Replaces the submobject at the given index with a new submobject. + If ``self.submobjects`` already contains the new submobject, it is also removed + from its previous position. + + Parameters + ---------- + index + The index of the submobject to replace. + new_submob + The new submobject to insert at the given index. + """ self._assert_valid_submobjects([new_submob]) old_submob = self.submobjects[index] - if self in old_submob.parents: - old_submob.parents.remove(self) + if old_submob == new_submob: + return self + + existing_index = None + with suppress(ValueError): + existing_index = self.submobjects.index(new_submob) self.submobjects[index] = new_submob + if existing_index is not None: + self.submobjects.pop(existing_index) + + with suppress(ValueError): + old_submob.parents.remove(self) + if self not in new_submob.parents: + new_submob.parents.append(self) self.assemble_family() return self @@ -1058,7 +1105,7 @@ def construct(self): x = OpenGLVGroup(s1, s2, s3, s4).set_x(0).arrange(buff=1.0) self.add(x) """ - for m1, m2 in zip(self.submobjects[:-1], self.submobjects[1:], strict=True): + for m1, m2 in it.pairwise(self.submobjects): m2.next_to(m1, direction, **kwargs) if center: self.center() @@ -1573,10 +1620,12 @@ def add_updater( return self def remove_updater(self, update_function: _Updater) -> Self: - for updater_list in [self.time_based_updaters, self.non_time_updaters]: - updater_list = cast("list[_Updater]", updater_list) - while update_function in updater_list: - updater_list.remove(update_function) + self.time_based_updaters = list_difference_update( + self.time_based_updaters, [update_function] + ) + self.non_time_updaters = list_difference_update( + self.non_time_updaters, [update_function] + ) self.refresh_has_updater_status() return self @@ -2489,7 +2538,7 @@ def get_pieces(self, n_pieces: int) -> OpenGLMobject: return OpenGLGroup( *( template.copy().pointwise_become_partial(self, a1, a2) - for a1, a2 in zip(alphas[:-1], alphas[1:], strict=True) + for a1, a2 in it.pairwise(alphas) ) ) diff --git a/manim/mobject/opengl/opengl_vectorized_mobject.py b/manim/mobject/opengl/opengl_vectorized_mobject.py index ea1c289d35..cc1756d198 100644 --- a/manim/mobject/opengl/opengl_vectorized_mobject.py +++ b/manim/mobject/opengl/opengl_vectorized_mobject.py @@ -588,7 +588,7 @@ def subdivide_sharp_curves( new_points.extend( [ partial_bezier_points(tup, a1, a2) - for a1, a2 in zip(alphas[:-1], alphas[1:], strict=True) + for a1, a2 in it.pairwise(alphas) ], ) else: @@ -776,9 +776,7 @@ def get_subpaths_from_points(self, points): # ) split_indices = [0, *split_indices, len(points)] return [ - points[i1:i2] - for i1, i2 in zip(split_indices[:-1], split_indices[1:], strict=True) - if (i2 - i1) >= nppc + points[i1:i2] for i1, i2 in it.pairwise(split_indices) if (i2 - i1) >= nppc ] def get_subpaths(self): diff --git a/manim/mobject/types/vectorized_mobject.py b/manim/mobject/types/vectorized_mobject.py index 8d05268539..a7bb5ac4f4 100644 --- a/manim/mobject/types/vectorized_mobject.py +++ b/manim/mobject/types/vectorized_mobject.py @@ -1372,9 +1372,7 @@ def _gen_subpaths_from_points( filtered = filter(filter_func, range(nppcc, len(points), nppcc)) split_indices = [0] + list(filtered) + [len(points)] return ( - points[i1:i2] - for i1, i2 in zip(split_indices[:-1], split_indices[1:], strict=True) - if (i2 - i1) >= nppcc + points[i1:i2] for i1, i2 in it.pairwise(split_indices) if (i2 - i1) >= nppcc ) def get_subpaths_from_points(self, points: CubicBezierPath) -> list[CubicSpline]: diff --git a/manim/utils/iterables.py b/manim/utils/iterables.py index cdd41e59d0..66708353a5 100644 --- a/manim/utils/iterables.py +++ b/manim/utils/iterables.py @@ -28,7 +28,7 @@ Reversible, Sequence, ) -from typing import TYPE_CHECKING, TypeVar, overload +from typing import TYPE_CHECKING, TypeVar, cast, overload import numpy as np @@ -133,7 +133,7 @@ def concatenate_lists(*list_of_lists: Iterable[T]) -> list[T]: return [item for lst in list_of_lists for item in lst] -def list_difference_update(l1: Iterable[T], l2: Iterable[T]) -> list[T]: +def list_difference_update(l1: Iterable[T], l2: Iterable[U]) -> list[T]: """Returns a list containing all the elements of l1 not in l2. Examples @@ -147,7 +147,7 @@ def list_difference_update(l1: Iterable[T], l2: Iterable[T]) -> list[T]: return [e for e in l1 if e not in l2] -def list_update(l1: Iterable[T], l2: Iterable[T]) -> list[T]: +def list_update(l1: Iterable[T], l2: Iterable[U]) -> list[T | U]: """Used instead of ``set.update()`` to maintain order, making sure duplicates are removed from l1, not l2. Removes overlap of l1 and l2 and then concatenates l2 unchanged. @@ -160,7 +160,7 @@ def list_update(l1: Iterable[T], l2: Iterable[T]) -> list[T]: [1, 3, 2, 4, 4] """ l2 = list(l2) - return list_difference_update(l1, l2) + l2 + return list_difference_update(l1, l2) + cast(list[T | U], l2) @overload diff --git a/tests/module/mobject/mobject/test_mobject.py b/tests/module/mobject/mobject/test_mobject.py index 051cd30c4b..966db6c705 100644 --- a/tests/module/mobject/mobject/test_mobject.py +++ b/tests/module/mobject/mobject/test_mobject.py @@ -78,6 +78,86 @@ def test_mobject_remove(): assert obj.remove(Mobject()) is obj +def test_mobject_insert(): + obj = Mobject() + m1, m2, m3, m4 = [Mobject(name=f"m{i}") for i in range(1, 5)] + # Insert into empty list + obj.insert(0, m1) + assert obj.submobjects == [m1] + + # Inserting shifts existing mobjects to the right + obj.insert(0, m2) + assert obj.submobjects == [m2, m1] + + # Inserting with negative index inserts counting from the end like a list + obj.insert(-1, m3) + assert obj.submobjects == [m2, m3, m1] + + # Inserting with index greater than length appends + obj.insert(10, m4) + assert obj.submobjects == [m2, m3, m1, m4] + + # Inserting an existing submobject moves it to the new position + obj.insert(4, m1) + assert obj.submobjects == [m2, m3, m4, m1] + + # Inserting an existing submobject at or immediately next to its current position + # does not change the order + obj.insert(3, m1) + assert obj.submobjects == [m2, m3, m4, m1] + obj.insert(3, m4) + assert obj.submobjects == [m2, m3, m4, m1] + + # can only insert Mobjects + with pytest.raises(TypeError) as add_str_info: + obj.insert(1, "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 0) is of type str." + ) + + +def test_mobject_add_to_back(): + obj = Mobject() + m1, m2, m3, m4 = [Mobject(name=f"m{i}") for i in range(1, 5)] + + # Adding to empty list is the same as adding normally + obj.add_to_back(m1) + assert obj.submobjects == [m1] + + # Adding a new submobject adds it to the back + obj.add_to_back(m2) + assert obj.submobjects == [m2, m1] + + # Adding existing submobjects that are already at the back does not change the order + obj.add_to_back(m1) + assert obj.submobjects == [m1, m2] + obj.add_to_back(m1, m2) + assert obj.submobjects == [m1, m2] + + # Adding an existing submobject moves it to the back + obj.add_to_back(m2) + assert obj.submobjects == [m2, m1] + + # In case of duplicates, the last occurrence of a submobject is kept + obj.add_to_back(m3, m1, m3) + assert obj.submobjects == [m1, m3, m2] + + # The order of the non-prepended submobjects is preserved + obj.remove(*obj.submobjects) + obj.add(m1, m2, m3, m4) + obj.add_to_back(m2, m4) + assert obj.submobjects == [m2, m4, m1, m3] + + # can only add Mobjects + with pytest.raises(TypeError) as add_str_info: + obj.add_to_back("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 0) is of type str." + ) + + def test_mobject_dimensions_single_mobject(): # A Mobject with no points and no submobjects has no dimensions empty = Mobject() diff --git a/tests/opengl/test_family_opengl.py b/tests/opengl/test_family_opengl.py index f16d0e4756..73addbe238 100644 --- a/tests/opengl/test_family_opengl.py +++ b/tests/opengl/test_family_opengl.py @@ -91,3 +91,105 @@ def test_shift_family(using_opengl_renderer): for m in family: np.testing.assert_allclose(positions_before[m] + RIGHT, positions_after[m]) + + +def test_opengl_mobject_family_updated_on_change(using_opengl_renderer): + """Test that the family of an OpenGLMobject is updated correctly when submobjects + are added or removed, and that the family is not updated if no changes are made. + """ + # This is based on the assumption that obj.family is replaced with a new list when submobjects + # are added or removed. If this implementation detail changes, this test may need to be updated. + obj = OpenGLMobject() + family = obj.get_family() + assert family == [obj] + submobs = [OpenGLMobject() for _ in range(10)] + + # Add new submobjects; family should be updated. + obj.add(*submobs) + assert family is not obj.get_family() + family = obj.get_family() + assert len(family) == 11 + for submob in submobs: + assert submob in family + + # Remove a submobject; family should be updated. + obj.remove(submobs[0]) + family = obj.get_family() + assert len(family) == 10 + assert submobs[0] not in family + + # Remove a submobject that is not in the family; family should not be updated. + obj.remove(OpenGLMobject()) + assert family is obj.get_family() + + # Add a submobject that is already in the family; family should not be updated. + obj.add(submobs[1]) + assert family is obj.get_family() + + # Add a mix of new and existing submobjects; family should be updated. + obj.add(OpenGLMobject(), submobs[2]) + assert family is not obj.get_family() + family = obj.get_family() + assert len(family) == 11 + + # Remove a mix of existing and non-existing submobjects; family should be updated. + obj.remove(submobs[3], OpenGLMobject()) + assert family is not obj.get_family() + + +def test_opengl_mobject_add_updates_parents(using_opengl_renderer): + """Test that the parents of an OpenGLMobject are updated correctly when they are added to + another OpenGLMobject. + """ + parent = OpenGLMobject(name="parent") + child, child2, child3 = [OpenGLMobject(name=f"child_{i}") for i in range(3)] + + # Initially, the child has no parent. + assert child.parents == [] + + # Add the child to the parent; the child's parent should be updated. + parent.add(child) + assert child.parents == [parent] + + # Add the child to another parent; the child's parent should be updated. + new_parent = OpenGLMobject() + new_parent.add(child) + for p in parent, new_parent: + assert p in child.parents + + # Remove the child from the new parent; the child's parent should be updated. + new_parent.remove(child) + assert new_parent not in child.parents + + # Add a child multiple times to the same parent; the child's parent should not + # be duplicated. + parent.remove(*parent.submobjects) + parent.add(child) + parent.add(child) + assert child.parents == [parent] + + # Remove a mix of existing and non-existing children; the existing child's parent should be + # updated correctly. + parent.remove(child2, child, child3) + for c in [child, child2, child3]: + assert parent not in c.parents + + +def test_replace_submobject_updates_parents(using_opengl_renderer): + """Test that replace_submobject() updates the parents of both the removed + and inserted submobjects correctly. + """ + parent = OpenGLMobject() + old_submob = OpenGLMobject() + new_submob = OpenGLMobject() + parent.add(old_submob) + + parent.replace_submobject(0, new_submob) + + assert parent not in old_submob.parents + assert new_submob.parents == [parent] + + # Inserting the same mobject again should not affect the parent list + parent.add(OpenGLMobject()) + parent.replace_submobject(1, new_submob) + assert new_submob.parents == [parent] diff --git a/tests/opengl/test_opengl_mobject.py b/tests/opengl/test_opengl_mobject.py index 947448f4cd..4396a4278c 100644 --- a/tests/opengl/test_opengl_mobject.py +++ b/tests/opengl/test_opengl_mobject.py @@ -106,3 +106,31 @@ def test_opengl_rotate_about_vertex_view(using_opengl_renderer): # The first vertex should remain in the same position (within numerical precision) rotated_vertices = triangle.get_vertices() np.testing.assert_allclose(rotated_vertices[0], first_vertex, atol=1e-6) + + +def test_replace_submobject(using_opengl_renderer): + """Test that replace_submobject() puts the new submobject in the correct + place and removes the old one. + """ + parent = OpenGLMobject() + old_submobs = [OpenGLMobject() for _ in range(3)] + parent.add(*old_submobs) + new_submob = OpenGLMobject() + + parent.replace_submobject(1, new_submob) + + assert parent.submobjects == [old_submobs[0], new_submob, old_submobs[2]] + assert old_submobs[1] not in parent.submobjects + + +def test_replace_submobject_with_existing_submobject(using_opengl_renderer): + """Test that replacing with a submobject that is already present moves it + to the new index instead of duplicating it. + """ + parent = OpenGLMobject() + submobs = [OpenGLMobject() for _ in range(3)] + parent.add(*submobs) + + parent.replace_submobject(0, submobs[2]) + + assert parent.submobjects == [submobs[2], submobs[1]]