Fix PathNanRemover for closed loops

QuLogic · QuLogic · commit 483c01c7bf14 · 2021-10-22T23:53:55.000-04:00
If we ever break a path due to a NaN, then it can never be closed, as
Agg will try to close it back to the last MOVETO. This would not be the
initial point of the loop, but the first point after the break. So we
need to emulate a CLOSEPOLY by adding a LINETO the original initial
point in the path.
diff --git a/lib/matplotlib/tests/test_simplification.py b/lib/matplotlib/tests/test_simplification.py
@@ -6,7 +6,8 @@
 
 import pytest
 
-from matplotlib.testing.decorators import image_comparison
+from matplotlib.testing.decorators import (
+    check_figures_equal, image_comparison, remove_ticks_and_titles)
 import matplotlib.pyplot as plt
 
 from matplotlib import patches, transforms
@@ -230,7 +231,7 @@ def test_sine_plus_noise():
     assert simplified.vertices.size == 25240
 
 
-@image_comparison(['simplify_curve'], remove_text=True)
+@image_comparison(['simplify_curve'], remove_text=True, tol=0.017)
 def test_simplify_curve():
     pp1 = patches.PathPatch(
         Path([(0, 0), (1, 0), (1, 1), (np.nan, 1), (0, 0), (2, 0), (2, 2),
@@ -245,6 +246,114 @@ def test_simplify_curve():
     ax.set_ylim((0, 2))
 
 
+@check_figures_equal()
+def test_closed_path_nan_removal(fig_test, fig_ref):
+    ax_test = fig_test.subplots(2, 2).flatten()
+    ax_ref = fig_ref.subplots(2, 2).flatten()
+
+    # NaN on the first point also removes the last point, because it's closed.
+    path = Path(
+        [[-3, np.nan], [3, -3], [3, 3], [-3, 3], [-3, -3]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])
+    ax_test[0].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-3, np.nan], [3, -3], [3, 3], [-3, 3], [-3, np.nan]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.LINETO])
+    ax_ref[0].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN on second-last point should not re-close.
+    path = Path(
+        [[-2, -2], [2, -2], [2, 2], [-2, np.nan], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])
+    ax_test[0].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-2, -2], [2, -2], [2, 2], [-2, np.nan], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.LINETO])
+    ax_ref[0].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # Test multiple loops in a single path (with same paths as above).
+    path = Path(
+        [[-3, np.nan], [3, -3], [3, 3], [-3, 3], [-3, -3],
+         [-2, -2], [2, -2], [2, 2], [-2, np.nan], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY,
+         Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.CLOSEPOLY])
+    ax_test[1].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-3, np.nan], [3, -3], [3, 3], [-3, 3], [-3, np.nan],
+         [-2, -2], [2, -2], [2, 2], [-2, np.nan], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.LINETO,
+         Path.MOVETO, Path.LINETO, Path.LINETO, Path.LINETO, Path.LINETO])
+    ax_ref[1].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN in first point of CURVE3 should not re-close, and hide entire curve.
+    path = Path(
+        [[-1, -1], [1, -1], [1, np.nan], [0, 1], [-1, 1], [-1, -1]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE3, Path.CURVE3, Path.LINETO,
+         Path.CLOSEPOLY])
+    ax_test[2].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-1, -1], [1, -1], [1, np.nan], [0, 1], [-1, 1], [-1, -1]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE3, Path.CURVE3, Path.LINETO,
+         Path.CLOSEPOLY])
+    ax_ref[2].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN in second point of CURVE3 should not re-close, and hide entire curve
+    # plus next line segment.
+    path = Path(
+        [[-3, -3], [3, -3], [3, 0], [0, np.nan], [-3, 3], [-3, -3]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE3, Path.CURVE3, Path.LINETO,
+         Path.LINETO])
+    ax_test[2].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-3, -3], [3, -3], [3, 0], [0, np.nan], [-3, 3], [-3, -3]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE3, Path.CURVE3, Path.LINETO,
+         Path.LINETO])
+    ax_ref[2].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN in first point of CURVE4 should not re-close, and hide entire curve.
+    path = Path(
+        [[-1, -1], [1, -1], [1, np.nan], [0, 0], [0, 1], [-1, 1], [-1, -1]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.CLOSEPOLY])
+    ax_test[3].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-1, -1], [1, -1], [1, np.nan], [0, 0], [0, 1], [-1, 1], [-1, -1]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.CLOSEPOLY])
+    ax_ref[3].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN in second point of CURVE4 should not re-close, and hide entire curve.
+    path = Path(
+        [[-2, -2], [2, -2], [2, 0], [0, np.nan], [0, 2], [-2, 2], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.LINETO])
+    ax_test[3].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-2, -2], [2, -2], [2, 0], [0, np.nan], [0, 2], [-2, 2], [-2, -2]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.LINETO])
+    ax_ref[3].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # NaN in third point of CURVE4 should not re-close, and hide entire curve
+    # plus next line segment.
+    path = Path(
+        [[-3, -3], [3, -3], [3, 0], [0, 0], [0, np.nan], [-3, 3], [-3, -3]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.LINETO])
+    ax_test[3].add_patch(patches.PathPatch(path, facecolor='none'))
+    path = Path(
+        [[-3, -3], [3, -3], [3, 0], [0, 0], [0, np.nan], [-3, 3], [-3, -3]],
+        [Path.MOVETO, Path.LINETO, Path.CURVE4, Path.CURVE4, Path.CURVE4,
+         Path.LINETO, Path.LINETO])
+    ax_ref[3].add_patch(patches.PathPatch(path, facecolor='none'))
+
+    # Keep everything clean.
+    for ax in [*ax_test.flat, *ax_ref.flat]:
+        ax.set(xlim=(-3.5, 3.5), ylim=(-3.5, 3.5))
+    remove_ticks_and_titles(fig_test)
+    remove_ticks_and_titles(fig_ref)
+
+
 @image_comparison(['hatch_simplify'], remove_text=True)
 def test_hatch():
     fig, ax = plt.subplots()
diff --git a/src/path_converters.h b/src/path_converters.h
@@ -165,6 +165,9 @@ class PathNanRemover : protected EmbeddedQueue<4>
     bool m_has_codes;
     bool valid_segment_exists;
     bool m_last_segment_valid;
+    bool m_was_broken;
+    double m_initX;
+    double m_initY;
 
   public:
     /* has_codes should be true if the path contains bezier curve segments, or
@@ -173,7 +176,8 @@ class PathNanRemover : protected EmbeddedQueue<4>
      */
     PathNanRemover(VertexSource &source, bool remove_nans, bool has_codes)
         : m_source(&source), m_remove_nans(remove_nans), m_has_codes(has_codes),
-          m_last_segment_valid(false)
+          m_last_segment_valid(false), m_was_broken(false),
+          m_initX(nan("")), m_initY(nan(""))
     {
         // ignore all close/end_poly commands until after the first valid
         // (nan-free) command is encountered
@@ -208,14 +212,41 @@ class PathNanRemover : protected EmbeddedQueue<4>
                    are found along the way, the queue is emptied, and
                    the next curve segment is handled. */
                 code = m_source->vertex(x, y);
-                /* The vertices attached to STOP and CLOSEPOLY left are never
-                   used, so we leave them as-is even if NaN. However, CLOSEPOLY
-                   only makes sense if a valid MOVETO command has already been
-                   emitted. */
-                if (code == agg::path_cmd_stop ||
-                    (code == (agg::path_cmd_end_poly | agg::path_flags_close) &&
-                     valid_segment_exists)) {
+                /* The vertices attached to STOP and CLOSEPOLY are never used,
+                 * so we leave them as-is even if NaN. */
+                if (code == agg::path_cmd_stop) {
                     return code;
+                } else if (code == (agg::path_cmd_end_poly |
+                                    agg::path_flags_close) &&
+                           valid_segment_exists) {
+                    /* However, CLOSEPOLY only makes sense if a valid MOVETO
+                     * command has already been emitted. But if a NaN was
+                     * removed in the path, then we cannot close it as it is no
+                     * longer a loop. We must emulate that by inserting a
+                     * LINETO instead. */
+                    if (m_was_broken) {
+                        if (m_last_segment_valid && (
+                                std::isfinite(m_initX) &&
+                                std::isfinite(m_initY))) {
+                            /* Join to start if both ends are valid. */
+                            queue_push(agg::path_cmd_line_to, m_initX, m_initY);
+                            break;
+                        } else {
+                            /* Skip the close, in case there are additional
+                             * subpaths. */
+                            continue;
+                        }
+                        m_was_broken = false;
+                        break;
+                    } else {
+                        return code;
+                    }
+                } else if (code == agg::path_cmd_move_to) {
+                    /* Save the initial point in order to produce the last
+                     * segment closing a loop, *if* we broke the loop. */
+                    m_initX = *x;
+                    m_initY = *y;
+                    m_was_broken = false;
                 }
 
                 if (needs_move_to) {
@@ -240,6 +271,7 @@ class PathNanRemover : protected EmbeddedQueue<4>
                     break;
                 }
 
+                m_was_broken = true;
                 queue_clear();
 
                 /* If the last point is finite, we use that for the
