-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix :meth:VMobject.pointwise_become_partial failing when vmobject is self
#4193
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
Fix :meth:VMobject.pointwise_become_partial failing when vmobject is self
#4193
Conversation
Line failing with buff and path_arc (issue #4132)Line failing with buff and path_arc
29759b5 to
efb532d
Compare
- make copy of vmobject.points if necessary
… pointwise_become_partial()
efb532d to
27741fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Indeed, when I wrote PR #3760, I missed the case where vmobject was the same Mobject as self (which wasn't an issue before my PR, because .get_cubic_bezier_tuples() returned a copy of the points). Thanks for catching this!
However, I think that your first approach was better:
Initially (in my first commit), I addressed this issue by modifying pointwise_become_partial so that it creates a copy of vmobject.points if vmobject is self, avoiding in-place modification of vmobject.points when self.points is set to np.empty(...). Making a copy is essential because self.points and vmobject.points are used together after self.points is set to np.empty(...).
because of two reasons:
-
Mainly, it is a more general fix which addresses the root cause of the problem: I missed to account for when
vmobjectisself, which seems to be a pretty common case (to make a Mobject become a part of itself). Many other methods usepointwise_become_partial()and we have to account for the case where the template VMobject may also be the transformed VMobject. -
The first approach involves copying only the NumPy array of points (and only in the case where
vmobjectis actuallyself), whereas the current approach involves copying the wholeVMobject(including points, colors and others), which is more expensive. AlthoughLine._account_for_buff()is a method which is called once and thus it doesn't really matter, it could matter if we implemented this in other methods which involve for loops.
So, could you please use the first approach for this PR? Thanks in advance!
EDIT: Plus, to prevent things like this from breaking again, can you please add some tests as well? At least one for the general case where vmobject is self, and maybe a check that points are calculated correctly for Line when passing both buff and path_arc.
|
Hi @chopan050, I've restored the fix where Added 2 tests:
Let me know if anything else needs adjusting! |
chopan050
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the changes!
Line failing with buff and path_arcVMobject.pointwise_become_partial failing when vmobject is self
EDIT by chopan050 - Original title: "Fix
Linefailing withbuffandpath_arc"Overview: What does this pull request change?
Fixes #4132
Motivation and Explanation: Why and how do your changes improve the library?
Issue Explanation
When
buffis given,Lineinternally callspointwise_become_partial, passingselfas the argument forvmobject:In Manim v0.19.0, the
pointwise_become_partialcode was modified in commit374eeeba(from PR #3760). This change introduced the issue.Initially (in my first commit), I addressed this issue by modifying
pointwise_become_partialso that it creates a copy ofvmobject.pointsifvmobjectisself, avoiding in-place modification ofvmobject.pointswhenself.pointsis set tonp.empty(...). Making a copy is essential becauseself.pointsandvmobject.pointsare used together afterself.pointsis set tonp.empty(...). See the related source code.However, I realized that a simpler solution exists which avoids modifying
pointwise_become_partial. Instead, the issue can be resolved by adjusting howLineuses it: When calling it, instead of passingvmobject=self, we should passvmobject=self.copy(). I’ve implemented this in my most recent commit.Reviewer Checklist