Skip to content

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

Merged

Conversation

irvanalhaq9
Copy link
Contributor

@irvanalhaq9 irvanalhaq9 commented Apr 16, 2025

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

  • The PR title is descriptive enough for the changelog, and the PR is labeled correctly
  • If applicable: newly added non-private functions and classes have a docstring including a short summary and a PARAMETERS section
  • If applicable: newly added functions and classes are tested

Copy link
Contributor

@chopan050 chopan050 left a 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 = points

we 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...

@github-project-automation github-project-automation bot moved this from 🆕 New to 👀 In review in Dev Board May 22, 2025
Copy link
Contributor

@chopan050 chopan050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@github-project-automation github-project-automation bot moved this from 👀 In review to 👍 To be merged in Dev Board May 22, 2025
@chopan050 chopan050 enabled auto-merge (squash) May 22, 2025 15:27
@irvanalhaq9
Copy link
Contributor Author

Thank you for the detailed explanation and suggestion — I really appreciate it!

@chopan050 chopan050 merged commit f6cdb54 into ManimCommunity:main May 22, 2025
21 checks passed
@github-project-automation github-project-automation bot moved this from 👍 To be merged to ✅ Done in Dev Board May 22, 2025
@irvanalhaq9 irvanalhaq9 deleted the fix-add-points-corner-connection branch May 22, 2025 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

PointWithTrace path updater no longer displays in 0.19.0
2 participants