-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Improve speed and robustness of Mobject and OpenGLMobject add/remove operations
#4957
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.
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
base: main
Are you sure you want to change the base?
Changes from 13 commits
c888d99
594e874
fd515a3
685d93d
c9cd0e4
70f4f05
04fcddd
fd327ab
20a632f
c192f20
d2c69b0
52c04b3
2e61447
b230410
5edc826
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,7 +37,11 @@ | |
| interpolate_color, | ||
| ) | ||
| from ..utils.exceptions import MultiAnimationOverrideException | ||
| from ..utils.iterables import list_update, remove_list_redundancies | ||
| from ..utils.iterables import ( | ||
| list_difference_update, | ||
| list_update, | ||
| remove_list_redundancies, | ||
| ) | ||
| from ..utils.paths import straight_path | ||
| from ..utils.space_ops import angle_between_vectors, normalize, rotation_matrix | ||
|
|
||
|
|
@@ -552,6 +557,17 @@ def add(self, *mobjects: Mobject) -> Self: | |
|
|
||
| """ | ||
| self._assert_valid_submobjects(mobjects) | ||
|
|
||
| if len(mobjects) == 1: | ||
| mobject = mobjects[0] | ||
| # If the mobject is already the last submobject, we don't need to do anything | ||
| if self.submobjects and mobject is self.submobjects[-1]: | ||
| return self | ||
| with suppress(ValueError): | ||
| self.submobjects.remove(mobject) | ||
| self.submobjects.append(mobject) | ||
| return self | ||
|
|
||
| unique_mobjects = remove_list_redundancies(mobjects) | ||
| if len(mobjects) != len(unique_mobjects): | ||
| logger.warning( | ||
|
|
@@ -563,12 +579,10 @@ def add(self, *mobjects: Mobject) -> Self: | |
| return self | ||
|
|
||
| def insert(self, index: int, mobject: Mobject) -> 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. | ||
| """Inserts a mobject at a specific position into ``self.submobjects``. | ||
|
|
||
| Highly adapted from ``Mobject.add``. | ||
| If ``mobject`` is already a submobject of ``self``, it will be moved to the new | ||
| position. | ||
|
|
||
| Parameters | ||
| ---------- | ||
|
|
@@ -578,7 +592,27 @@ def insert(self, index: int, mobject: Mobject) -> Self: | |
| The mobject to be inserted. | ||
| """ | ||
| self._assert_valid_submobjects([mobject]) | ||
| self.submobjects.insert(index, mobject) | ||
|
|
||
| # Normalize index to match list.insert | ||
| if index < 0: | ||
| index = max(0, len(self.submobjects) + index) | ||
| else: | ||
| index = min(index, len(self.submobjects)) | ||
|
|
||
| try: | ||
| old_index = self.submobjects.index(mobject) | ||
| except ValueError: # mobject isn't already present | ||
| self.submobjects.insert(index, mobject) | ||
| return self | ||
|
|
||
| if index in (old_index, old_index + 1): # Position will remain unchanged | ||
| return self | ||
|
Comment on lines
+617
to
+627
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be a bit simplified. with suppress(ValueError):
old_index = self.submobjects.index(mobject)
# Compensate for list shifting after popping
if old_index <= norm_index: # <= or <?
norm_index -= 1
self.submobjects.pop(old_index)
self.submobjects.insert(norm_index, mobject)
return self
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO the early return in line 620 makes it more clear what is happening, even if it means a bit of duplicated code. I'm inclined to leave it alone for now. |
||
|
|
||
| # Compensate for list shifting after popping | ||
| new_index = index if index < old_index else index - 1 | ||
| self.submobjects.pop(old_index) | ||
| self.submobjects.insert(new_index, mobject) | ||
|
|
||
| return self | ||
|
|
||
| def __add__(self, mobject: Mobject) -> Self: | ||
|
|
@@ -632,9 +666,17 @@ def add_to_back(self, *mobjects: Mobject) -> Self: | |
|
|
||
| """ | ||
| self._assert_valid_submobjects(mobjects) | ||
| self.remove(*mobjects) | ||
| # dict.fromkeys() removes duplicates while maintaining order | ||
| self.submobjects = list(dict.fromkeys(mobjects)) + self.submobjects | ||
| 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.", | ||
| ) | ||
| existing_mobs = self.submobjects | ||
| self.submobjects = list(unique_mobjects) | ||
| self.submobjects.extend(m for m in existing_mobs if m not in unique_mobjects) | ||
|
|
||
| return self | ||
|
|
||
| def remove(self, *mobjects: Mobject) -> Self: | ||
|
|
@@ -659,9 +701,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 +1169,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 +2456,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 +2675,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() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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): | ||
| if len(mobjects) == 1: | ||
| mobject = mobjects[0] | ||
| if mobject not in self.submobjects: | ||
| self.submobjects.append(mobject) | ||
| if self not in mobject.parents: | ||
| mobject.parents.append(self) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Arent the two following statements equivalent? if mobject not in self.submobjects:
if self not in mobject.parents:If yes, then the inner check is superfluous.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, if that's true, maybe only using the second term could be more efficient. (as mobjects typically have more submobjects than parents)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've had the same thought. They're not 100% equivalent - my assumption is that whoever wrote the code initially wanted to catch a case where a child was somehow added to a parent without adding the parent to its own parent-list (in which case I should maybe move the latter part out by one level to catch the reverse case?). I decided to leave it as it originally was and maybe think about parent handling in a future PR; IMO this needs an entirely different data structure. |
||
| self.assemble_family() | ||
| return self | ||
|
|
||
| # 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 mobject not in self.submobjects: | ||
| self.submobjects.append(mobject) | ||
| if self not in mobject.parents: | ||
| mobject.parents.append(self) | ||
| self.assemble_family() | ||
|
|
||
| # Remove already-present mobjects | ||
| for mob in self.submobjects: | ||
| if mob in unique_mobjects: | ||
| unique_mobjects.pop(mob) | ||
|
nikolajmunk marked this conversation as resolved.
|
||
|
|
||
| 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. | ||
|
nikolajmunk marked this conversation as resolved.
|
||
|
|
||
| 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) | ||
|
nikolajmunk marked this conversation as resolved.
|
||
| 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) | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.