-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 incorrect assignment in Mobject.put_start_and_end_on #4008
base: main
Are you sure you want to change the base?
Conversation
e1482dd
to
fe0c9f6
Compare
Rebased code to resolve conflict with #4027 |
PR ManimCommunity#3718 changed the behavior of `Mobject.put_start_and_end_on` when `start == end`, however the assigment to `self.points` was incorrect. The existing code assigned the Point3D directly to `self.points`, that then becomes an array with shape `(3,)`, while instead it should really have shape `(1, 3)`. PR ManimCommunity#4027 added a comment that this should be fixed, but did not fix the issue. The comment is now superfluous. This commit fixes the incorrect code and associated test.
Explicitly mentioning the author and reviewer of #4027. @chopan050 @JasonGrace2282 what do you think of this fix? |
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.
what do you think of this fix?
IMO, it seems like the best course of action, given that this is a Mobject
method.
My only gripe with this is that, if you try to call this method on a closed VMobject
such as Circle
or Square
, the VMobject
ends up having a single point in points
, which I consider hacky and potentially undesired. Cairo VMobject
s should have 4N points and OpenGLVMobject
s should have 3N points... except in the case where there's a single lingering point indicating the start of a potentially new path, which, IMO, is handled weirdly.
I've been raising some discussions in Discord where I propose rewriting the points
of VMobject
(and Mobject
in general), but it will have to wait until the experimental branch is finished, since this is a huge change.
Meanwhile, IMO, it makes sense to override VMobject.put_start_and_end_on()
such that it becomes a straight line between start
and end
which is properly defined by 4 points (3 in OpenGL), but I'd like to ask the rest of the developers about it:
class VMobject(Mobject):
# ...
def put_start_and_end_on(self, start: Point3DLike, end: Point3DLike) -> self:
curr_start, curr_end = self.get_start_and_end()
if np.all(curr_end - curr_start == 0):
self.clear_points()
self.start_new_path(start).add_line_to(end)
return self
return super().put_start_and_end_on(start, end)
Overview: What does this pull request change?
PR #3718 changed the behavior of
Mobject.put_start_and_end_on
whenstart == end
, however the assigment toself.points
was incorrect. The existing code assigned the Point3D directly toself.points
, that then becomes an array with shape(3,)
, while instead it should really have shape(1, 3)
.This commit fixes the incorrect code and associated test.
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