Skip to content

Fix a bug of ignoring the closing path segment when automatically determining whether to snap - #32018

Merged
scottshambaugh merged 2 commits into
matplotlib:mainfrom
ayshih:snap_path_closure
Jul 24, 2026
Merged

Fix a bug of ignoring the closing path segment when automatically determining whether to snap#32018
scottshambaugh merged 2 commits into
matplotlib:mainfrom
ayshih:snap_path_closure

Conversation

@ayshih

@ayshih ayshih commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

PR summary

The current code for automatic determination of whether a path should be snapped – by checking whether all line segments are either horizontal or vertical – neglects to check the closing line segment for a closed path. That is, a closed path will be automatically determined to be snappable even when the closing line segment is not horizontal or vertical as long as all other line segments are either horizontal or vertical. This PR fixes that oversight.

A not-uncommon place for this bug to have an effect in the wild is when plotting contours, and one contour region is just a triangular section at the corner of the plot. If the diagonal of that triangle happens to be the closing line segment, then that contour region will snap, while its adjacent contour region will typically not snap. A handful of figure tests show this effect.

Here is a minimal example illustrating the bug:

import matplotlib.pyplot as plt
from matplotlib.patches import Polygon

# diagonal is not the closing segment, so will not be snapped
triangle1 = Polygon([[0, 0], [1, 0], [0, 1]])

# diagonal is the closing segment, so will be snapped due to the bug
triangle2 = Polygon([[0, 1], [1, 1], [1, 0]])

fig, ax = plt.subplots()

ax.add_patch(triangle1)
ax.add_patch(triangle2)

plt.show()

Before this PR

The gap between the two triangles is not a consistent width, showing that the two triangles are not snapping the same. The gap will change/flicker if the plot window is resized due to this difference in snapping.
before

After this PR

The gap between the two triangles is a consistent width, showing that both triangles are snapping the same.
after

AI Disclosure

No AI was used

PR checklist

@ayshih
ayshih force-pushed the snap_path_closure branch 3 times, most recently from 8e7924c to 5a00b40 Compare July 8, 2026 13:53
@ayshih ayshih changed the title Fix a bug that ignored the closing path segment when automatically determining whether to snap Fix a bug of ignoring the closing path segment when automatically determining whether to snap Jul 8, 2026
@ayshih
ayshih marked this pull request as ready for review July 8, 2026 14:18
@ayshih
ayshih force-pushed the snap_path_closure branch 2 times, most recently from 4848ceb to e5d637f Compare July 8, 2026 15:49

@QuLogic QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you run oxipng on the new images? If I've read GitHub's annotations correctly, some of them have grown quite a bit.

Comment thread src/path_converters.h Outdated
@ayshih
ayshih force-pushed the snap_path_closure branch 2 times, most recently from 23c0be7 to acfeb50 Compare July 8, 2026 19:28
@ayshih

ayshih commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Can you run oxipng on the new images? If I've read GitHub's annotations correctly, some of them have grown quite a bit.

Ah, good catch! Differences are now much more reasonable after using oxipng.

Comment thread src/path_converters.h Outdated
// If this contains only straight horizontal or vertical lines, it should be
// snapped to the nearest pixels
double x0 = 0, y0 = 0, x1 = 0, y1 = 0;
double xi = 0, yi = 0, x0 = 0, y0 = 0, x1 = 0, y1 = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Naming get's quite confusing here. Typically, x0 would be a constant and xi is used as a running variable $x_i$, but here it's the other way round x0 and x1 are the start end end points of segments and are iterated though, whereas xi is constant (i standing for initial).
The x0, x1 naming was defensible because you need to values, but combining this with xi is getting weird. I suggest to use xs, ys ("start") or px, py instead of xi, yi.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. I didn't rename x0 and x1 to minimize code changes, so I had to come up with another name: as you note, I used "i" for initial, but it didn't occur to me that it could instead be read as a running index. I'll change it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Variables now named x_start and y_start

@ayshih
ayshih force-pushed the snap_path_closure branch from 1f061d9 to 5395e83 Compare July 24, 2026 14:49
@scottshambaugh
scottshambaugh merged commit ee84151 into matplotlib:main Jul 24, 2026
42 checks passed
@QuLogic QuLogic added this to the v3.12.0 milestone Jul 24, 2026
@ayshih ayshih mentioned this pull request Jul 25, 2026
@ayshih
ayshih deleted the snap_path_closure branch July 25, 2026 03:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants