From db3f6968889c557a49ef2b9d493d4b5379ab6ccc Mon Sep 17 00:00:00 2001 From: Vojtech Bubnik Date: Tue, 15 Nov 2022 15:32:16 +0100 Subject: [PATCH] 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 | 12 +- src/libslic3r/ExPolygon.hpp | 4 + src/libslic3r/SLA/SupportPointGenerator.hpp | 3 +- tests/fff_print/test_clipper.cpp | 121 +++++++++++++++++--- tests/libslic3r/CMakeLists.txt | 1 + tests/libslic3r/test_expolygon.cpp | 67 +++++++++++ xs/t/04_expolygon.t | 69 ----------- 7 files changed, 187 insertions(+), 90 deletions(-) create mode 100644 tests/libslic3r/test_expolygon.cpp delete mode 100644 xs/t/04_expolygon.t diff --git a/src/libslic3r/ExPolygon.cpp b/src/libslic3r/ExPolygon.cpp index 61c204044..771603548 100644 --- a/src/libslic3r/ExPolygon.cpp +++ b/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 diff --git a/src/libslic3r/ExPolygon.hpp b/src/libslic3r/ExPolygon.hpp index 58f0d33e9..55c3e60ef 100644 --- a/src/libslic3r/ExPolygon.hpp +++ b/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; diff --git a/src/libslic3r/SLA/SupportPointGenerator.hpp b/src/libslic3r/SLA/SupportPointGenerator.hpp index 5cb7b7069..a9e094386 100644 --- a/src/libslic3r/SLA/SupportPointGenerator.hpp +++ b/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.; diff --git a/tests/fff_print/test_clipper.cpp b/tests/fff_print/test_clipper.cpp index dc49ce0f1..ea923b48e 100644 --- a/tests/fff_print/test_clipper.cpp +++ b/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. +// tests for ExPolygon::overlaps(const ExPolygon &other) +SCENARIO("Clipper intersection with polyline", "[Clipper]") +{ + 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); + }; + + 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 } }, + { } + }); + }; + + 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; - - subject.emplace_back(-2000, -1000, 10); - subject.emplace_back(-2000, 1000, 10); - subject.emplace_back( 2000, 1000, 10); - subject.emplace_back( 2000, -1000, 10); - - 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); + 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); -} \ No newline at end of file +} + diff --git a/tests/libslic3r/CMakeLists.txt b/tests/libslic3r/CMakeLists.txt index ed7d8a92b..1cd27c6c9 100644 --- a/tests/libslic3r/CMakeLists.txt +++ b/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 diff --git a/tests/libslic3r/test_expolygon.cpp b/tests/libslic3r/test_expolygon.cpp new file mode 100644 index 000000000..63d763e2f --- /dev/null +++ b/tests/libslic3r/test_expolygon.cpp @@ -0,0 +1,67 @@ +#include + +#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().norm() < SCALED_EPSILON; +} + +static bool polygons_close_permuted(const Polygon &poly1, const Polygon &poly2, const std::vector &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})); + } + } +} diff --git a/xs/t/04_expolygon.t b/xs/t/04_expolygon.t deleted file mode 100644 index 9132c44b9..000000000 --- a/xs/t/04_expolygon.t +++ /dev/null @@ -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__