Close paths only if they haven't been clipped

Otherwise, a random spurious line connecting the last clipped point with
the initial point is drawn unintentionally.

Fixes matplotlib#21300

Author: QuLogic

lib/matplotlib/tests/test_simplification.py

@@ -354,6 +354,47 @@ def test_closed_path_nan_removal(fig_test, fig_ref):
     remove_ticks_and_titles(fig_ref)
 
 
+@check_figures_equal()
+def test_closed_path_clipping(fig_test, fig_ref):
+    vertices = []
+    for roll in range(8):
+        offset = 0.1 * roll + 0.1
+
+        # A U-like pattern.
+        pattern = [
+            [-0.5, 1.5], [-0.5, -0.5], [1.5, -0.5], [1.5, 1.5],  # Outer square
+            # With a notch in the top.
+            [1 - offset / 2, 1.5], [1 - offset / 2, offset],
+            [offset / 2, offset], [offset / 2, 1.5],
+        ]
+
+        # Place the initial/final point anywhere in/out of the clipping area.
+        pattern = np.roll(pattern, roll, axis=0)
+        pattern = np.concatenate((pattern, pattern[:1, :]))
+
+        vertices.append(pattern)
+
+    # Multiple subpaths are used here to ensure they aren't broken by closed
+    # loop clipping.
+    codes = np.full(len(vertices[0]), Path.LINETO)
+    codes[0] = Path.MOVETO
+    codes[-1] = Path.CLOSEPOLY
+    codes = np.tile(codes, len(vertices))
+    vertices = np.concatenate(vertices)
+
+    fig_test.set_size_inches((5, 5))
+    path = Path(vertices, codes)
+    fig_test.add_artist(patches.PathPatch(path, facecolor='none'))
+
+    # For reference, we draw the same thing, but unclosed by using a line to
+    # the last point only.
+    fig_ref.set_size_inches((5, 5))
+    codes = codes.copy()
+    codes[codes == Path.CLOSEPOLY] = Path.LINETO
+    path = Path(vertices, codes)
+    fig_ref.add_artist(patches.PathPatch(path, facecolor='none'))
+
+
 @image_comparison(['hatch_simplify'], remove_text=True)
 def test_hatch():
     fig, ax = plt.subplots()

lib/matplotlib/tests/test_transforms.py

@@ -201,7 +201,7 @@ def test_clipping_of_log():
                                  clip=(0, 0, 100, 100),
                                  simplify=False)
     tpoints, tcodes = zip(*result)
-    assert_allclose(tcodes, path.codes)
+    assert_allclose(tcodes, path.codes[:-1])  # No longer closed.
 
 
 class NonAffineForTest(mtransforms.Transform):

src/path_converters.h

@@ -336,6 +336,7 @@ class PathClipper : public EmbeddedQueue<3>
     double m_initX;
     double m_initY;
     bool m_has_init;
+    bool m_was_clipped;
 
   public:
     PathClipper(VertexSource &source, bool do_clipping, double width, double height)
@@ -347,7 +348,8 @@ class PathClipper : public EmbeddedQueue<3>
           m_moveto(true),
           m_initX(nan("")),
           m_initY(nan("")),
-          m_has_init(false)
+          m_has_init(false),
+          m_was_clipped(false)
     {
         // empty
     }
@@ -361,7 +363,8 @@ class PathClipper : public EmbeddedQueue<3>
           m_moveto(true),
           m_initX(nan("")),
           m_initY(nan("")),
-          m_has_init(false)
+          m_has_init(false),
+          m_was_clipped(false)
     {
         m_cliprect.x1 -= 1.0;
         m_cliprect.y1 -= 1.0;
@@ -372,21 +375,29 @@ class PathClipper : public EmbeddedQueue<3>
     inline void rewind(unsigned path_id)
     {
         m_has_init = false;
+        m_was_clipped = false;
         m_moveto = true;
         m_source->rewind(path_id);
     }
 
-    int draw_clipped_line(double x0, double y0, double x1, double y1)
+    int draw_clipped_line(double x0, double y0, double x1, double y1,
+                          bool closed=false)
     {
         unsigned moved = agg::clip_line_segment(&x0, &y0, &x1, &y1, m_cliprect);
         // moved >= 4 - Fully clipped
         // moved & 1 != 0 - First point has been moved
         // moved & 2 != 0 - Second point has been moved
+        m_was_clipped = m_was_clipped || (moved != 0);
         if (moved < 4) {
             if (moved & 1 || m_moveto) {
                 queue_push(agg::path_cmd_move_to, x0, y0);
             }
             queue_push(agg::path_cmd_line_to, x1, y1);
+            if (closed && !m_was_clipped) {
+                // Close the path only if the end point hasn't moved.
+                queue_push(agg::path_cmd_end_poly | agg::path_flags_close,
+                           x1, y1);
+            }
 
             m_moveto = false;
             return 1;
@@ -417,12 +428,23 @@ class PathClipper : public EmbeddedQueue<3>
             switch (code) {
             case (agg::path_cmd_end_poly | agg::path_flags_close):
                 if (m_has_init) {
-                    draw_clipped_line(m_lastX, m_lastY, m_initX, m_initY);
+                    // Queue the line from last point to the initial point, and
+                    // if never clipped, add a close code.
+                    draw_clipped_line(m_lastX, m_lastY, m_initX, m_initY,
+                                      true);
+                } else {
+                    // An empty path that is immediately closed.
+                    queue_push(
+                        agg::path_cmd_end_poly | agg::path_flags_close,
+                        m_lastX, m_lastY);
                 }
-                queue_push(
-                    agg::path_cmd_end_poly | agg::path_flags_close,
-                    m_lastX, m_lastY);
-                goto exit_loop;
+                // If paths were not clipped, then the above code queued
+                // something, and we should exit the loop. Otherwise, continue
+                // to the next point, as there may be a new subpath.
+                if (queue_nonempty()) {
+                    goto exit_loop;
+                }
+                break;
 
             case agg::path_cmd_move_to:
 
@@ -444,6 +466,7 @@ class PathClipper : public EmbeddedQueue<3>
                 m_initY = m_lastY = *y;
                 m_has_init = true;
                 m_moveto = true;
+                m_was_clipped = false;
                 // if the last command was moveto exit the loop to emit the code
                 if (emit_moveto) {
                     goto exit_loop;