Fix a bug of ignoring the closing path segment when automatically determining whether to snap - #32018
Conversation
8e7924c to
5a00b40
Compare
4848ceb to
e5d637f
Compare
QuLogic
left a comment
There was a problem hiding this comment.
Can you run oxipng on the new images? If I've read GitHub's annotations correctly, some of them have grown quite a bit.
23c0be7 to
acfeb50
Compare
Ah, good catch! Differences are now much more reasonable after using oxipng. |
acfeb50 to
1f061d9
Compare
| // 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; |
There was a problem hiding this comment.
Naming get's quite confusing here. Typically, x0 would be a constant and xi is used as a running variable
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Variables now named x_start and y_start
1f061d9 to
5395e83
Compare
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:
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.

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

AI Disclosure
No AI was used
PR checklist