This seems like the wrong approach as currently we leave it to the user to control if they want snapping or not and changes it to never snap.

Okay, so now I've changed the code to set the snap mode toSNAP_FALSE only ifgc.snap_mode == SNAP_AUTO (which isNone in Python, which occurs when the user either ignores this value or explicitly sets it toNone).
Based on the current codebase,SNAP_AUTO leads to two different snapping methods for the same set of vertices, based on the diagonality of the lines formed by those vertices (i.e., vertex order).

This is still a behavior change (hence the large number of failing tests). Changing the meaning of "auto" to mean "do not snap" also does not seem like a correct change to me.

I'm not saying this is the final solution, but it is a possible solution, as it solves#30549.
Tests are definitely required.

We are not changing any meaning.
First call is to a function inpath_converter.h from_backend_agg.h. Over there,SNAP_AUTO leads to the relevant function deciding whether or not to snapbased on the vertex order (i.e., the diagonality of the line formed by any two adjacent vertices). So, two sets of vertices consisting of thesame points, each in different order, would internally lead either tosnap=True orsnap=False, which makes no sense (because the points are exactly the same).

Regardless, in the event ofsnap, that function (inpath_converter.h) deals with the snapping; in the event ofdon't snap, the calling function in_backend_agg.h performs the snapping anyway by invoking the AGG's affine translation function (which produces a different result). Either way snapping takes place.

Just for the second argument in the call to thepath_converter.h function, convertingSNAP_AUTO to eitherSNAP_TRUE orSNAP_FALSE avoids the above mentioned problem. I went withSNAP_FALSE because another function-call a few lines below also sendsSNAP_FALSE (and notgc.snap_mode) as its second argument.

Regarding the failing tests, I agree about that.
Could that be because of the way the tests are implemented? (i.e., the expected images are as per the original code)

You are getting local failures on the main branch? We do have some flaky tests, but none of them are related to this sort of thing (mostly subprocess issues in CI machines).

Are you sure you either used editable mode or re-build/re-installed Matplotlib?

This change is effectively removing "auto" mode in this case which is at best a change in behavior and at worst a regression for cases where this is valuable (either way it is causing tests to fail).

I think you need to look at

On another bit of consideration, I think the problem is thatshould_snap above does not correctly considerclose code; the fix is to treat it withpath_cmd_line_to.

On another bit of consideration, I think the problem is thatshould_snap above does not correctly considerclose code; the fix is to treat it withpath_cmd_line_to.

path_cmd_line_to is where the problem occurs. If the line formed by the vertices is horizontal or vertical, it returns true; but if it's a diagonal, then bothdx > 0 anddy > 0, and so it returns false.

I don't know how to fix this part, because I don't know the reasoning behindnot snapping non-horizontal/non-vertical lines.

There was an error on my part (did not do a hard reset); the tests no longer fail fortest_axes on the original branch, but they still do fail for a few other tests.

It would be helpful to know how thebaseline (expected) images are generated in the first place. Could those images be incompatible with the new code if it were hypothetically to be changed?

Please readhttps://matplotlib.org/devdocs/devel/testing.html for how the images are made and can be updated.

I think this is the right fix here, but needs to be verified. This at least does not fail the existing tests:

diff --git a/src/path_converters.h b/src/path_converters.hindex 1482aeed95..4aab6ecca5 100644--- a/src/path_converters.h+++ b/src/path_converters.h@@ -560,7 +560,7 @@ class PathSnapper     {         // 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 x0 = 0, y0 = 0, x1 = 0, y1 = 0, xs = 0, ys = 0;         unsigned code;         switch (snap_mode) {@@ -570,12 +570,20 @@ class PathSnapper             }             code = path.vertex(&x0, &y0);-            if (code == agg::path_cmd_stop) {+            switch (code) {+            case agg::path_cmd_stop:                 return false;+            case agg::path_cmd_move_to:+                xs = x0;+                ys = y0;             }             while ((code = path.vertex(&x1, &y1)) != agg::path_cmd_stop) {                 switch (code) {+                case agg::path_cmd_move_to:+                    xs = x1;+                    ys = x1;+                    break;                 case agg::path_cmd_curve3:                 case agg::path_cmd_curve4:                     return false;@@ -583,9 +591,16 @@ class PathSnapper                     if (fabs(x0 - x1) >= 1e-4 && fabs(y0 - y1) >= 1e-4) {                         return false;                     }+                    break;+                case agg::path_cmd_end_poly:+                    if (fabs(xs - x0) >= 1e-4 && fabs(ys - y0) >= 1e-4) {+                        return false;+                    }                 }                 x0 = x1;                 y0 = y1;++             }             return true;

I think the problem is that we are checking if the path has any lines that are not purely vertical or horizontal, however we did not consider the close_poly code which is effectively "line to back to the last move to". With out this if the points are ordered such that we only get horizontal and vertical line-to we decide we should snap, but if the points are ordered such that the lineto goes across the diagonal then we decide not to clip. With this patch we also consider the line draw by the close_poly and should consistently decide to NOT snap with all orders of the triangle points.

Another way to verify that this is the correct explanation is to rotate the triangle by 15deg or so so that it has no horizontal or vertical lines.

Thank you very much,@jklymak . I've checked out that link before. It still does not explain how the baseline images are created in the first place.
Based onthis link, the baseline images are part of the original source package. My guess is that the baseline images are generated using the same code, and are considered the new baseline images (benchmarks) based on some collective decision/approval...

The above fix is better, I agree.test_collections.py succeeds. But it still fails intest_axes.py fortest_sticky_tolerance_contourf() andtest_preset_clip_paths() for the proposed fix (if you remove the fix, it passes).

I guess the corresponding baseline images need an update?

Also, I don't know if a similar fix is required for the Cairo backend as well, as even thebackend_cairo.py file has adraw_markers function.

What I had in mind was something like:

importmatplotlib.pyplotaspltfrommatplotlib.transformsimportAffine2Dimportmatplotlib.patchesasmpatchesimportmatplotlib.pathasmpathp=mpath.Path.unit_regular_polygon(3)pp=mpath.Path.make_compound_path(p.transformed(Affine2D().translate(1,1)),p.transformed(Affine2D().translate(1,10)),)fig,ax=plt.subplots(1,1)ax.add_artist(mpatches.PathPatch(pp))ax.set_xlim(0,2)ax.set_ylim(0,11)plt.show()

however, I was a bit confused and forgot that the Python side (effective) enums and the c++ side (effective) enums are not perfectly aligned and what we have from the Python side for the close poly is 79 which is exactly the| you added.

It appears you can make a pathological path that does not have the move_to between the polygons (it draws something, not 100% sure it should and if what it draws is right, but it does not fail to draw and in not obviously wrong). Probably should add the extra resetting ofxs andys on close just to be safe.

Some of these failures look like they are images that should be re-generated (they are pretty clearly due to triangles being snapped differently), but I do not understand all of the failures.

I think this is indeed a bug that needs to be fixed (the bug being the "should we snap" logic missed an edge case of N horizonal/vertical lines and then a diagonal line on the close poly), we just need to understand why each of the changed images is affected to be sure we are not putting new bugs in!