From a06d8e942bb022b894e5772b2b39cb92397d94d5 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Fri, 17 Jan 2020 19:11:38 -0500 Subject: [PATCH 1/7] TST: a 0 length segment does not intersect with all other segments --- lib/matplotlib/tests/test_path.py | 43 +++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 8d94ca040a1d..41e943a1d47b 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -352,3 +352,46 @@ def test_full_arc(offset): maxs = np.max(path.vertices, axis=0) np.testing.assert_allclose(mins, -1) np.testing.assert_allclose(maxs, 1) + + +def test_disjoint_zero_length_segment(): + this_path = Path( + np.array([ + [824.85064295, 2056.26489203], + [861.69033931, 2041.00539016], + [868.57864109, 2057.63522175], + [831.73894473, 2072.89472361], + [824.85064295, 2056.26489203]]), + np.array([1, 2, 2, 2, 79], dtype=Path.code_type)) + + outline_path = Path( + np.array([ + [859.91051028, 2165.38461538], + [859.06772495, 2149.30331334], + [859.06772495, 2181.46591743], + [859.91051028, 2165.38461538], + [859.91051028, 2165.38461538]]), + np.array([1, 2, 2, 2, 2], + dtype=Path.code_type)) + + assert not outline_path.intersects_path(this_path) + assert not this_path.intersects_path(outline_path) + + +def test_intersect_zero_length_segment(): + this_path = Path( + np.array([ + [0, 0], + [1, 1], + ])) + + outline_path = Path( + np.array([ + [1, 0], + [.5, .5], + [.5, .5], + [0, 1], + ])) + + assert outline_path.intersects_path(this_path) + assert this_path.intersects_path(outline_path) From f8181c96961fe422a2dc0c8ee69cf16e48bb6e1b Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Fri, 17 Jan 2020 19:13:10 -0500 Subject: [PATCH 2/7] FIX: when iterating through segments skip 0 length segments One method to fix #15842 In this case we check in `path_intersect_path` that as we iterate through the segments, if we hit a 0 length segment we grab the next vertex before checking if the segments from the two paths intersect. --- src/_path.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/_path.h b/src/_path.h index 4a38aed5a621..80e4ba40c24a 100644 --- a/src/_path.h +++ b/src/_path.h @@ -887,9 +887,18 @@ bool path_intersects_path(PathIterator1 &p1, PathIterator2 &p2) c1.vertex(&x11, &y11); while (c1.vertex(&x12, &y12) != agg::path_cmd_stop) { + // if the segment in path 1 is 0 length, skip to next vertex + if ((x11 == x12) && (y11 == y12)) { + continue; + } c2.rewind(0); c2.vertex(&x21, &y21); + while (c2.vertex(&x22, &y22) != agg::path_cmd_stop) { + // if the segment in path 2 is 0 length, skip to next vertex + if ((x21 == x22) && (y21 == y22)) { + continue; + } if (segments_intersect(x11, y11, x12, y12, x21, y21, x22, y22)) { return true; } From b3476a8003fc566460216a5bc5a75d73bc709cc0 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Fri, 17 Jan 2020 19:19:10 -0500 Subject: [PATCH 3/7] FIX: 0 length segment intersect One method to fix #15842 In this case we check that if either segment is 0 length, it intersects with no other segments, not all other segments. --- src/_path.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/_path.h b/src/_path.h index 80e4ba40c24a..430c35c79391 100644 --- a/src/_path.h +++ b/src/_path.h @@ -834,6 +834,13 @@ inline bool segments_intersect(const double &x1, // it looks the atol value matters here bacause of round-off errors const double rtol = 1e-10; const double atol = 1e-13; + + // if either segment is 0 length, they do not intersect + + if ((x1 == x2 && y1 == y2) || (x3 == x3 && y3 == y4)) { + return false; + } + // determinant double den = ((y4 - y3) * (x2 - x1)) - ((x4 - x3) * (y2 - y1)); From 9729ac26465b54ff0e594bca5af0b6d062ff1f39 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Fri, 17 Jan 2020 19:24:38 -0500 Subject: [PATCH 4/7] MNT: make local variable const To make clear we won't be changing them. --- src/_path.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/_path.h b/src/_path.h index 430c35c79391..4e6a2501e071 100644 --- a/src/_path.h +++ b/src/_path.h @@ -857,19 +857,19 @@ inline bool segments_intersect(const double &x1, (fmin(x3, x4) <= fmin(x1, x2) && fmin(x1, x2) <= fmax(x3, x4)); } } - + return false; } - double n1 = ((x4 - x3) * (y1 - y3)) - ((y4 - y3) * (x1 - x3)); - double n2 = ((x2 - x1) * (y1 - y3)) - ((y2 - y1) * (x1 - x3)); + const double n1 = ((x4 - x3) * (y1 - y3)) - ((y4 - y3) * (x1 - x3)); + const double n2 = ((x2 - x1) * (y1 - y3)) - ((y2 - y1) * (x1 - x3)); - double u1 = n1 / den; - double u2 = n2 / den; + const double u1 = n1 / den; + const double u2 = n2 / den; - return ((u1 > 0.0 || isclose(u1, 0.0, rtol, atol)) && - (u1 < 1.0 || isclose(u1, 1.0, rtol, atol)) && - (u2 > 0.0 || isclose(u2, 0.0, rtol, atol)) && + return ((u1 > 0.0 || isclose(u1, 0.0, rtol, atol)) && + (u1 < 1.0 || isclose(u1, 1.0, rtol, atol)) && + (u2 > 0.0 || isclose(u2, 0.0, rtol, atol)) && (u2 < 1.0 || isclose(u2, 1.0, rtol, atol))); } From d5ae2015ebbd2a261bb4613df9911658159a6341 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Fri, 17 Jan 2020 19:32:01 -0500 Subject: [PATCH 5/7] MNT: alternate method of identifying 0 length segments Use length square being close to 0 rather than strict. I have a worry that because we are using "close" for identifying the determinant is 0, if we use strict equality for the length, then we may pass through the length check, but erroneously go through the den == 0 path. --- src/_path.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/_path.h b/src/_path.h index 4e6a2501e071..f1d16af7177e 100644 --- a/src/_path.h +++ b/src/_path.h @@ -836,8 +836,12 @@ inline bool segments_intersect(const double &x1, const double atol = 1e-13; // if either segment is 0 length, they do not intersect + // length-squared of each segment + const double lensq_A = (x1 - x2) * (x1 - x2) + (y1 - y2) * (y1 - y2); + const double lenqs_B = (x3 - x4) * (x3 - x4) + (y3 - y4) * (y3 - y4); - if ((x1 == x2 && y1 == y2) || (x3 == x3 && y3 == y4)) { + // one of the segments is 0 length + if (isclose(lensq_A, 0, rtol, atol) || isclose(lenqs_B, 0, rtol, atol)) { return false; } From 41fce6b88afd60d287154fea4809de842fca24c9 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Sat, 18 Jan 2020 23:01:49 -0500 Subject: [PATCH 6/7] TST: use pytest.mark.parametrize instead of a for loop Makes it a bit easier to identify failing cases. --- lib/matplotlib/tests/test_path.py | 107 +++++++++++++++--------------- 1 file changed, 52 insertions(+), 55 deletions(-) diff --git a/lib/matplotlib/tests/test_path.py b/lib/matplotlib/tests/test_path.py index 41e943a1d47b..4435b2337bef 100644 --- a/lib/matplotlib/tests/test_path.py +++ b/lib/matplotlib/tests/test_path.py @@ -265,81 +265,78 @@ def test_path_deepcopy(): copy.deepcopy(path2) -def test_path_intersect_path(): +@pytest.mark.parametrize('phi', np.concatenate([ + np.array([0, 15, 30, 45, 60, 75, 90, 105, 120, 135]) + delta + for delta in [-1, 0, 1]])) +def test_path_intersect_path(phi): # test for the range of intersection angles - base_angles = np.array([0, 15, 30, 45, 60, 75, 90, 105, 120, 135]) - angles = np.concatenate([base_angles, base_angles + 1, base_angles - 1]) eps_array = [1e-5, 1e-8, 1e-10, 1e-12] - for phi in angles: + transform = transforms.Affine2D().rotate(np.deg2rad(phi)) - transform = transforms.Affine2D().rotate(np.deg2rad(phi)) + # a and b intersect at angle phi + a = Path([(-2, 0), (2, 0)]) + b = transform.transform_path(a) + assert a.intersects_path(b) and b.intersects_path(a) - # a and b intersect at angle phi - a = Path([(-2, 0), (2, 0)]) - b = transform.transform_path(a) - assert a.intersects_path(b) and b.intersects_path(a) + # a and b touch at angle phi at (0, 0) + a = Path([(0, 0), (2, 0)]) + b = transform.transform_path(a) + assert a.intersects_path(b) and b.intersects_path(a) - # a and b touch at angle phi at (0, 0) - a = Path([(0, 0), (2, 0)]) - b = transform.transform_path(a) - assert a.intersects_path(b) and b.intersects_path(a) + # a and b are orthogonal and intersect at (0, 3) + a = transform.transform_path(Path([(0, 1), (0, 3)])) + b = transform.transform_path(Path([(1, 3), (0, 3)])) + assert a.intersects_path(b) and b.intersects_path(a) - # a and b are orthogonal and intersect at (0, 3) - a = transform.transform_path(Path([(0, 1), (0, 3)])) - b = transform.transform_path(Path([(1, 3), (0, 3)])) - assert a.intersects_path(b) and b.intersects_path(a) + # a and b are collinear and intersect at (0, 3) + a = transform.transform_path(Path([(0, 1), (0, 3)])) + b = transform.transform_path(Path([(0, 5), (0, 3)])) + assert a.intersects_path(b) and b.intersects_path(a) - # a and b are collinear and intersect at (0, 3) - a = transform.transform_path(Path([(0, 1), (0, 3)])) - b = transform.transform_path(Path([(0, 5), (0, 3)])) - assert a.intersects_path(b) and b.intersects_path(a) + # self-intersect + assert a.intersects_path(a) - # self-intersect - assert a.intersects_path(a) + # a contains b + a = transform.transform_path(Path([(0, 0), (5, 5)])) + b = transform.transform_path(Path([(1, 1), (3, 3)])) + assert a.intersects_path(b) and b.intersects_path(a) - # a contains b - a = transform.transform_path(Path([(0, 0), (5, 5)])) - b = transform.transform_path(Path([(1, 1), (3, 3)])) - assert a.intersects_path(b) and b.intersects_path(a) + # a and b are collinear but do not intersect + a = transform.transform_path(Path([(0, 1), (0, 5)])) + b = transform.transform_path(Path([(3, 0), (3, 3)])) + assert not a.intersects_path(b) and not b.intersects_path(a) + + # a and b are on the same line but do not intersect + a = transform.transform_path(Path([(0, 1), (0, 5)])) + b = transform.transform_path(Path([(0, 6), (0, 7)])) + assert not a.intersects_path(b) and not b.intersects_path(a) - # a and b are collinear but do not intersect + # Note: 1e-13 is the absolute tolerance error used for + # `isclose` function from src/_path.h + + # a and b are parallel but do not touch + for eps in eps_array: a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(3, 0), (3, 3)])) + b = transform.transform_path(Path([(0 + eps, 1), (0 + eps, 5)])) assert not a.intersects_path(b) and not b.intersects_path(a) - # a and b are on the same line but do not intersect + # a and b are on the same line but do not intersect (really close) + for eps in eps_array: a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(0, 6), (0, 7)])) + b = transform.transform_path(Path([(0, 5 + eps), (0, 7)])) assert not a.intersects_path(b) and not b.intersects_path(a) - # Note: 1e-13 is the absolute tolerance error used for - # `isclose` function from src/_path.h - - # a and b are parallel but do not touch - for eps in eps_array: - a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(0 + eps, 1), (0 + eps, 5)])) - assert not a.intersects_path(b) and not b.intersects_path(a) - - # a and b are on the same line but do not intersect (really close) - for eps in eps_array: - a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(0, 5 + eps), (0, 7)])) - assert not a.intersects_path(b) and not b.intersects_path(a) - - # a and b are on the same line and intersect (really close) - for eps in eps_array: - a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(0, 5 - eps), (0, 7)])) - assert a.intersects_path(b) and b.intersects_path(a) - - # b is the same as a but with an extra point + # a and b are on the same line and intersect (really close) + for eps in eps_array: a = transform.transform_path(Path([(0, 1), (0, 5)])) - b = transform.transform_path(Path([(0, 1), (0, 2), (0, 5)])) + b = transform.transform_path(Path([(0, 5 - eps), (0, 7)])) assert a.intersects_path(b) and b.intersects_path(a) - return + # b is the same as a but with an extra point + a = transform.transform_path(Path([(0, 1), (0, 5)])) + b = transform.transform_path(Path([(0, 1), (0, 2), (0, 5)])) + assert a.intersects_path(b) and b.intersects_path(a) @pytest.mark.parametrize('offset', range(-720, 361, 45)) From ffaf3027d91577f4611289704ed565df175fa1e5 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Tue, 21 Jan 2020 13:07:27 -0800 Subject: [PATCH 7/7] API/MNT: re-arrange 0 length segment detection - move the "close to 0" computation from segments_intersect to path_intersects_path so we only do it once. - change the signature of the c function isclose to back the atol and rtol into the function (to make sure the two places we use it stay in sync) --- src/_path.h | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/src/_path.h b/src/_path.h index f1d16af7177e..edba0db199d3 100644 --- a/src/_path.h +++ b/src/_path.h @@ -814,8 +814,13 @@ int count_bboxes_overlapping_bbox(agg::rect_d &a, BBoxArray &bboxes) } -inline bool isclose(double a, double b, double rtol, double atol) +inline bool isclose(double a, double b) { + // relative and absolute tolerance values are chosen empirically + // it looks the atol value matters here bacause of round-off errors + const double rtol = 1e-10; + const double atol = 1e-13; + // as per python's math.isclose return fabs(a-b) <= fmax(rtol * fmax(fabs(a), fabs(b)), atol); } @@ -830,25 +835,10 @@ inline bool segments_intersect(const double &x1, const double &x4, const double &y4) { - // relative and absolute tolerance values are chosen empirically - // it looks the atol value matters here bacause of round-off errors - const double rtol = 1e-10; - const double atol = 1e-13; - - // if either segment is 0 length, they do not intersect - // length-squared of each segment - const double lensq_A = (x1 - x2) * (x1 - x2) + (y1 - y2) * (y1 - y2); - const double lenqs_B = (x3 - x4) * (x3 - x4) + (y3 - y4) * (y3 - y4); - - // one of the segments is 0 length - if (isclose(lensq_A, 0, rtol, atol) || isclose(lenqs_B, 0, rtol, atol)) { - return false; - } - // determinant double den = ((y4 - y3) * (x2 - x1)) - ((x4 - x3) * (y2 - y1)); - if (isclose(den, 0.0, rtol, atol)) { // collinear segments + if (isclose(den, 0.0)) { // collinear segments if (x1 == x2 && x2 == x3) { // segments have infinite slope (vertical lines) // and lie on the same line return (fmin(y1, y2) <= fmin(y3, y4) && fmin(y3, y4) <= fmax(y1, y2)) || @@ -856,7 +846,7 @@ inline bool segments_intersect(const double &x1, } else { double intercept = (y1*x2 - y2*x1)*(x4 - x3) - (y3*x4 - y4*x3)*(x1 - x2); - if (isclose(intercept, 0.0, rtol, atol)) { // segments lie on the same line + if (isclose(intercept, 0.0)) { // segments lie on the same line return (fmin(x1, x2) <= fmin(x3, x4) && fmin(x3, x4) <= fmax(x1, x2)) || (fmin(x3, x4) <= fmin(x1, x2) && fmin(x1, x2) <= fmax(x3, x4)); } @@ -871,10 +861,10 @@ inline bool segments_intersect(const double &x1, const double u1 = n1 / den; const double u2 = n2 / den; - return ((u1 > 0.0 || isclose(u1, 0.0, rtol, atol)) && - (u1 < 1.0 || isclose(u1, 1.0, rtol, atol)) && - (u2 > 0.0 || isclose(u2, 0.0, rtol, atol)) && - (u2 < 1.0 || isclose(u2, 1.0, rtol, atol))); + return ((u1 > 0.0 || isclose(u1, 0.0)) && + (u1 < 1.0 || isclose(u1, 1.0)) && + (u2 > 0.0 || isclose(u2, 0.0)) && + (u2 < 1.0 || isclose(u2, 1.0))); } template @@ -898,16 +888,16 @@ bool path_intersects_path(PathIterator1 &p1, PathIterator2 &p2) c1.vertex(&x11, &y11); while (c1.vertex(&x12, &y12) != agg::path_cmd_stop) { - // if the segment in path 1 is 0 length, skip to next vertex - if ((x11 == x12) && (y11 == y12)) { + // if the segment in path 1 is (almost) 0 length, skip to next vertex + if ((isclose((x11 - x12) * (x11 - x12) + (y11 - y12) * (y11 - y12), 0))){ continue; } c2.rewind(0); c2.vertex(&x21, &y21); while (c2.vertex(&x22, &y22) != agg::path_cmd_stop) { - // if the segment in path 2 is 0 length, skip to next vertex - if ((x21 == x22) && (y21 == y22)) { + // if the segment in path 2 is (almost) 0 length, skip to next vertex + if ((isclose((x21 - x22) * (x21 - x22) + (y21 - y22) * (y21 - y22), 0))){ continue; } if (segments_intersect(x11, y11, x12, y12, x21, y21, x22, y22)) {