Fixed ExPolygon::overlaps(), which was not commutative.

Wrote unit tests for Clipper polyline clipping operations.
Rewrote ExPolygon unit tests from Perl to C++.

src/libslic3r/ExPolygon.cpp

@@ -154,14 +154,18 @@ bool ExPolygon::overlaps(const ExPolygon &other) const
     svg.draw_outline(*this);
     svg.draw_outline(other, "blue");
     #endif
+
     Polylines pl_out = intersection_pl(to_polylines(other), *this);
+
     #if 0
     svg.draw(pl_out, "red");
     #endif
-    if (! pl_out.empty())
-        return true; 
-    //FIXME ExPolygon::overlaps() shall be commutative, it is not!
-    return this->contains(other.contour.points.front());
+
+    // See unit test SCENARIO("Clipper diff with polyline", "[Clipper]")
+    // for in which case the intersection_pl produces any intersection.
+    return ! pl_out.empty() ||
+           // If *this is completely inside other, then pl_out is empty, but the expolygons overlap. Test for that situation.
+           other.contains(this->contour.points.front());
 }
 
 void ExPolygon::simplify_p(double tolerance, Polygons* polygons) const

src/libslic3r/ExPolygon.hpp

@@ -60,6 +60,10 @@ public:
     // Does this expolygon overlap another expolygon?
     // Either the ExPolygons intersect, or one is fully inside the other,
     // and it is not inside a hole of the other expolygon.
+    // The test may not be commutative if the two expolygons touch by a boundary only,
+    // see unit test SCENARIO("Clipper diff with polyline", "[Clipper]").
+    // Namely expolygons touching at a vertical boundary are considered overlapping, while expolygons touching
+    // at a horizontal boundary are NOT considered overlapping.
     bool overlaps(const ExPolygon &other) const;
 
     void simplify_p(double tolerance, Polygons* polygons) const;

src/libslic3r/SLA/SupportPointGenerator.hpp

@@ -84,8 +84,7 @@ public:
         float                                   overhangs_area = 0.f;
         
         bool overlaps(const Structure &rhs) const { 
-            //FIXME ExPolygon::overlaps() shall be commutative, it is not!
-            return this->bbox.overlap(rhs.bbox) && (this->polygon->overlaps(*rhs.polygon) || rhs.polygon->overlaps(*this->polygon)); 
+            return this->bbox.overlap(rhs.bbox) && this->polygon->overlaps(*rhs.polygon);
         }
         float overlap_area(const Structure &rhs) const { 
             double out = 0.;

tests/fff_print/test_clipper.cpp

@@ -2,25 +2,115 @@
 
 #include "test_data.hpp"
 #include "clipper/clipper_z.hpp"
+#include "libslic3r/clipper.hpp"
 
 using namespace Slic3r;
 
-// Test case for an issue with duplicity vertices (same XY coordinates but differ in Z coordinates) in Clipper 6.2.9,
-// (related to https://sourceforge.net/p/polyclipping/bugs/160/) that was fixed in Clipper 6.4.2.
-SCENARIO("Clipper Z", "[ClipperZ]")
+// tests for ExPolygon::overlaps(const ExPolygon &other)
+SCENARIO("Clipper intersection with polyline", "[Clipper]")
 {
-    ClipperLib_Z::Path subject;
+    struct TestData { 
+        ClipperLib::Path subject;
+        ClipperLib::Path clip;
+        ClipperLib::Paths result;
+    };
+
+    auto run_test = [](const TestData &data) {
+        ClipperLib::Clipper clipper;
+        clipper.AddPath(data.subject, ClipperLib::ptSubject, false);
+        clipper.AddPath(data.clip, ClipperLib::ptClip, true);
+
+        ClipperLib::PolyTree polytree;
+        ClipperLib::Paths    paths;
+        clipper.Execute(ClipperLib::ctIntersection, polytree, ClipperLib::pftNonZero, ClipperLib::pftNonZero);
+        ClipperLib::PolyTreeToPaths(polytree, paths);
+
+        REQUIRE(paths == data.result);
+    };
 
-    subject.emplace_back(-2000, -1000, 10);
-    subject.emplace_back(-2000,  1000, 10);
-    subject.emplace_back( 2000,  1000, 10);
-    subject.emplace_back( 2000, -1000, 10);
+    WHEN("Open polyline completely inside stays inside") {
+        run_test({
+            { { 10, 0 }, { 20, 0 } },
+            { { -1000, -1000 }, { -1000,  1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { { { 20, 0 }, { 10, 0 } } }
+        });
+    };
+    WHEN("Closed polyline completely inside stays inside") {
+        run_test({
+            { { 10, 0 }, { 20, 0 }, { 20, 20 }, { 10, 20 }, { 10, 0 } },
+            { { -1000, -1000 }, { -1000,  1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { { { 10, 0 }, { 20, 0 }, { 20, 20 }, { 10, 20 }, { 10, 0 } } }
+        });
+    };
+    WHEN("Polyline which crosses right rectangle boundary is trimmed") {
+        run_test({
+            { { 10, 0 }, { 2000, 0 } },
+            { { -1000, -1000 }, { -1000,  1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { { { 1000, 0 }, { 10, 0 } } }
+        });
+    };
+    WHEN("Polyline which is outside clipping region is removed") {
+        run_test({
+            { { 1500, 0 }, { 2000, 0 } },
+            { { -1000, -1000 }, { -1000,  1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { }
+        });
+    };
 
-    ClipperLib_Z::Path clip;
-    clip.emplace_back(-1000, -2000, -5);
-    clip.emplace_back(-1000,  2000, -5);
-    clip.emplace_back( 1000,  2000, -5);
-    clip.emplace_back( 1000, -2000, -5);
+    WHEN("Polyline on left vertical boundary is kept") {
+        run_test({
+            { { -1000, -1000 }, { -1000, 1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { { { -1000, -1000 }, { -1000, 1000 } } }
+        });
+        run_test({
+            { { -1000, 1000 }, { -1000, -1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { { { -1000, 1000 }, { -1000, -1000 } } }
+        });
+    };
+    WHEN("Polyline on right vertical boundary is kept") {
+        run_test({
+            { { 1000, -1000 }, { 1000, 1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { { { 1000, -1000 }, { 1000, 1000 } } }
+        });
+        run_test({
+            { { 1000, 1000 }, { 1000, -1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000,  1000 }, { 1000, -1000 } },
+            { { { 1000, 1000 }, { 1000, -1000 } } }
+        });
+    };
+    WHEN("Polyline on bottom horizontal boundary is removed") {
+        run_test({
+            { { -1000, -1000 }, { 1000, -1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { }
+        });
+        run_test({
+            { { 1000, -1000 }, { -1000, -1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { }
+            });
+    };
+    WHEN("Polyline on top horizontal boundary is removed") {
+        run_test({
+            { { -1000, 1000 }, { 1000, 1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { }
+        });
+        run_test({
+            { { 1000, 1000 }, { -1000, 1000 } },
+            { { -1000, -1000 }, { -1000, 1000 }, { 1000, 1000 }, { 1000, -1000 } },
+            { }
+            });
+    };
+}
+
+SCENARIO("Clipper Z", "[ClipperZ]")
+{
+    ClipperLib_Z::Path subject { { -2000, -1000, 10 }, { -2000,  1000, 10 }, { 2000,  1000, 10 }, { 2000, -1000, 10 } };
+    ClipperLib_Z::Path clip{ { -1000, -2000, -5 }, { -1000,  2000, -5 }, { 1000,  2000, -5 }, { 1000, -2000, -5 } };
 
     ClipperLib_Z::Clipper clipper;
     clipper.ZFillFunction([](const ClipperLib_Z::IntPoint &e1bot, const ClipperLib_Z::IntPoint &e1top, const ClipperLib_Z::IntPoint &e2bot,
@@ -40,4 +130,5 @@ SCENARIO("Clipper Z", "[ClipperZ]")
     REQUIRE(paths.front().size() == 2);
     for (const ClipperLib_Z::IntPoint &pt : paths.front())
         REQUIRE(pt.z() == 1);
-}
+}
+

tests/libslic3r/CMakeLists.txt

@@ -12,6 +12,7 @@ add_executable(${_TEST_NAME}_tests
 	test_config.cpp
 	test_curve_fitting.cpp
 	test_elephant_foot_compensation.cpp
+	test_expolygon.cpp
 	test_geometry.cpp
 	test_placeholder_parser.cpp
 	test_polygon.cpp

tests/libslic3r/test_expolygon.cpp

@@ -0,0 +1,67 @@
+#include <catch2/catch.hpp>
+
+#include "libslic3r/Point.hpp"
+#include "libslic3r/Polygon.hpp"
+#include "libslic3r/ExPolygon.hpp"
+
+using namespace Slic3r;
+
+static inline bool points_close(const Point &p1, const Point &p2)
+{
+    return (p1 - p2).cast<double>().norm() < SCALED_EPSILON;
+}
+
+static bool polygons_close_permuted(const Polygon &poly1, const Polygon &poly2, const std::vector<int> &permutation2)
+{
+    if (poly1.size() != poly2.size() || poly1.size() != permutation2.size())
+        return false;
+    for (size_t i = 0; i < poly1.size(); ++ i)
+        if (poly1[i] != poly2[permutation2[i]])
+            return false;
+    return true;
+}
+
+SCENARIO("Basics", "[ExPolygon]") {
+    GIVEN("ccw_square") {
+        Polygon ccw_square{ { 100, 100 }, { 200, 100 }, { 200, 200 }, { 100, 200 } };
+        Polygon cw_hole_in_square{ { 140, 140 }, { 140, 160 }, { 160, 160 }, { 160, 140 } };
+        ExPolygon expolygon { ccw_square, cw_hole_in_square };
+        THEN("expolygon is valid") {
+            REQUIRE(expolygon.is_valid());
+        }
+        THEN("expolygon area") {
+            REQUIRE(expolygon.area() == Approx(100*100-20*20));
+        }
+        WHEN("Expolygon scaled") {
+            ExPolygon expolygon2 = expolygon;
+            expolygon2.scale(2.5);
+            REQUIRE(expolygon.contour.size() == expolygon2.contour.size());
+            REQUIRE(expolygon.holes.size() == 1);
+            REQUIRE(expolygon2.holes.size() == 1);
+            for (size_t i = 0; i < expolygon.contour.size(); ++ i)
+                REQUIRE(points_close(expolygon.contour[i] * 2.5, expolygon2.contour[i]));
+            for (size_t i = 0; i < expolygon.holes.front().size(); ++ i)
+                REQUIRE(points_close(expolygon.holes.front()[i] * 2.5, expolygon2.holes.front()[i]));
+        }
+        WHEN("Expolygon translated") {
+            ExPolygon expolygon2 = expolygon;
+            expolygon2.translate(10, -5);
+            REQUIRE(expolygon.contour.size() == expolygon2.contour.size());
+            REQUIRE(expolygon.holes.size() == 1);
+            REQUIRE(expolygon2.holes.size() == 1);
+            for (size_t i = 0; i < expolygon.contour.size(); ++ i)
+                REQUIRE(points_close(expolygon.contour[i] + Point(10, -5), expolygon2.contour[i]));
+            for (size_t i = 0; i < expolygon.holes.front().size(); ++ i)
+                REQUIRE(points_close(expolygon.holes.front()[i] + Point(10, -5), expolygon2.holes.front()[i]));
+        }
+        WHEN("Expolygon rotated around point") {
+            ExPolygon expolygon2 = expolygon;
+            expolygon2.rotate(M_PI / 2, Point(150, 150));
+            REQUIRE(expolygon.contour.size() == expolygon2.contour.size());
+            REQUIRE(expolygon.holes.size() == 1);
+            REQUIRE(expolygon2.holes.size() == 1);
+            REQUIRE(polygons_close_permuted(expolygon2.contour, expolygon.contour, { 1, 2, 3, 0}));
+            REQUIRE(polygons_close_permuted(expolygon2.holes.front(), expolygon.holes.front(), { 3, 0, 1, 2}));
+        }
+    }
+}

xs/t/04_expolygon.t

@@ -1,69 +0,0 @@
-#!/usr/bin/perl
-
-use strict;
-use warnings;
-
-use List::Util qw(first sum);
-use Slic3r::XS;
-use Test::More tests => 7;
-
-use constant PI => 4 * atan2(1, 1);
-
-my $square = [  # ccw
-    [100, 100],
-    [200, 100],
-    [200, 200],
-    [100, 200],
-];
-my $hole_in_square = [  # cw
-    [140, 140],
-    [140, 160],
-    [160, 160],
-    [160, 140],
-];
-
-my $expolygon = Slic3r::ExPolygon->new($square, $hole_in_square);
-
-ok $expolygon->is_valid, 'is_valid';
-
-is_deeply $expolygon->clone->pp, [$square, $hole_in_square], 'clone';
-
-is $expolygon->area, 100*100-20*20, 'area';
-
-{
-    my $expolygon2 = $expolygon->clone;
-    $expolygon2->scale(2.5);
-    is_deeply $expolygon2->pp, [
-        [map [ 2.5*$_->[0], 2.5*$_->[1] ], @$square],
-        [map [ 2.5*$_->[0], 2.5*$_->[1] ], @$hole_in_square]
-        ], 'scale';
-}
-
-{
-    my $expolygon2 = $expolygon->clone;
-    $expolygon2->translate(10, -5);
-    is_deeply $expolygon2->pp, [
-        [map [ $_->[0]+10, $_->[1]-5 ], @$square],
-        [map [ $_->[0]+10, $_->[1]-5 ], @$hole_in_square]
-        ], 'translate';
-}
-
-{
-    my $expolygon2 = $expolygon->clone;
-    $expolygon2->rotate(PI/2, Slic3r::Point->new(150,150));
-    is_deeply $expolygon2->pp, [
-        [ @$square[1,2,3,0] ],
-        [ @$hole_in_square[3,0,1,2] ]
-        ], 'rotate around Point';
-}
-
-{
-    my $expolygon2 = $expolygon->clone;
-    $expolygon2->rotate(PI/2, [150,150]);
-    is_deeply $expolygon2->pp, [
-        [ @$square[1,2,3,0] ],
-        [ @$hole_in_square[3,0,1,2] ]
-        ], 'rotate around pure-Perl Point';
-}
-
-__END__