-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Fix add_points_as_corners not connecting single point to existing path
#4219
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 add_points_as_corners not connecting single point to existing path
#4219
Conversation
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.
Hi and thanks for the changes!
I'm the author of PR #3765, which optimized many VMobject methods including this one. Originally, .add_points_as_corners() consisted of a for loop repeatedly calling .add_line_to() and destroying/recreating self.points, so I optimized it to do it only once. Unfortunately, I mistranslated some of the logic, causing the issue you're fixing now.
The concept you present is fine. However, if you notice, this new line you add:
start_corners = np.vstack([self.points[-1], points[:-1]])is functionally the same as what's already in the previous if branch:
start_corners = np.empty((num_points, self.dim))
start_corners[0] = self.points[-1]
start_corners[1:] = points[:-1]I prefer using those 3 lines, since np.vstack is, for some reason, quite slow and np.empty is faster.
Therefore, that logic should be instead factored out of the if-else. Instead of having:
if self.has_new_path_started():
# Pop the last point from self.points and
# add it to start_corners
start_corners = np.empty((num_points, self.dim))
start_corners[0] = self.points[-1]
start_corners[1:] = points[:-1]
end_corners = points
self.points = self.points[:-1]
else:
start_corners = np.vstack([self.points[-1], points[:-1]])
end_corners = pointswe could have:
start_corners = np.empty((num_points, self.dim))
start_corners[0] = self.points[-1]
start_corners[1:] = points[:-1]
end_corners = points
if self.has_new_path_started():
# Remove the last point from the new path.
self.points = self.points[:-1]Also, since we're now always using the last point from self, we should bring back the call to .throw_error_if_no_points() at the beginning of this method:
self.throw_error_if_no_points()
points = np.asarray(points).reshape(-1, self.dim)
num_points = points.shape[0]
if num_points == 0:
return self
# the rest...
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!
|
Thank you for the detailed explanation and suggestion — I really appreciate it! |
Overview: What does this pull request change?
Fixes #4218.
Motivation and Explanation: Why and how do your changes improve the library?
add_points_as_corners()did not include the last point of the existing path, causing no line to be added when only one new point was provided.This fix explicitly adds the last point of the existing path as the starting corner of the new segment, ensuring the path is continuous and no segment is skipped.
Links to added or changed documentation pages
Docs related to the issue after this fix: https://manimce--4219.org.readthedocs.build/en/4219/examples.html#pointwithtrace
Reviewer Checklist