From 1948cf304f6ba1eeb27fd1016aa4cd95e4698ed4 Mon Sep 17 00:00:00 2001 From: Andreas Fabri Date: Tue, 15 Jun 2021 14:21:35 +0200 Subject: [PATCH 1/3] Fix (unintendedly) skipping the next point after erasure of a constraint point In 'it = set.erase(it)', it is already the next element, so the ++it of the loop makes the algorithm skip an element, which might have had to be removed if it also didn't have a corresponding triangulation vertex. --- .../Polyline_simplification_2/CMakeLists.txt | 3 +- .../Polyline_simplification_2/issue-5774.cpp | 78 +++++++++++++++++++ .../Polyline_constraint_hierarchy_2.h | 11 ++- 3 files changed, 86 insertions(+), 6 deletions(-) create mode 100644 Polyline_simplification_2/test/Polyline_simplification_2/issue-5774.cpp diff --git a/Polyline_simplification_2/test/Polyline_simplification_2/CMakeLists.txt b/Polyline_simplification_2/test/Polyline_simplification_2/CMakeLists.txt index 108065c6037..3f3bd6fcf89 100644 --- a/Polyline_simplification_2/test/Polyline_simplification_2/CMakeLists.txt +++ b/Polyline_simplification_2/test/Polyline_simplification_2/CMakeLists.txt @@ -36,8 +36,7 @@ endif() # Creating entries for all .cpp/.C files with "main" routine # ########################################################## +create_single_source_cgal_program( "issue-5774.cpp" ) create_single_source_cgal_program( "simplify_polygon_test.cpp" ) create_single_source_cgal_program( "simplify_polyline_with_duplicate_points.cpp" ) - - diff --git a/Polyline_simplification_2/test/Polyline_simplification_2/issue-5774.cpp b/Polyline_simplification_2/test/Polyline_simplification_2/issue-5774.cpp new file mode 100644 index 00000000000..aec19500739 --- /dev/null +++ b/Polyline_simplification_2/test/Polyline_simplification_2/issue-5774.cpp @@ -0,0 +1,78 @@ +#include +#include + +#include +#include +#include + +typedef CGAL::Exact_predicates_inexact_constructions_kernel K; +typedef CGAL::Point_2 Point; +typedef std::vector Polyline_2; +namespace PS = CGAL::Polyline_simplification_2; +typedef PS::Vertex_base_2 Vb; +typedef CGAL::Constrained_triangulation_face_base_2 Fb; +typedef CGAL::Triangulation_data_structure_2 TDS; +typedef CGAL::Constrained_Delaunay_triangulation_2 CDT; +typedef CGAL::Constrained_triangulation_plus_2 CT; +typedef PS::Stop_below_count_ratio_threshold Stop; +typedef PS::Squared_distance_cost Cost; + +void print_coords(Polyline_2 polyline) { + std::cout << std::endl; + std::cout << "Simplified coordinates:" << std::endl << std::endl; + + // Print out the simplified coordinates + unsigned int i = 0; + for (Point coord : polyline) { + std::cout << "[ "; + std::cout << coord.x(); + std::cout << ", "; + std::cout << coord.y(); + std::cout << " ]"; + if (i != polyline.size() - 1) std::cout << ","; + i++; + } + std::cout << std::endl << std::endl; +} + +void simplify_test() { + // Hard code a minimum working example where running PS::simplify results in + // self-intersections. There are no self-intersections when {27, 9} is + // omitted. + std::vector> coords = { + {64, 20}, {33, 27}, {27, 9}, {33, 18}, {44, 18}, {44, 8}, + {24, 0}, {0, 13}, {9, 49}, {84, 41}, {83, 29}, {64, 20}, + }; + // Simplification outputs: + // [ 64, 20 ],[ 27, 9 ],[ 44, 18 ],[ 24, 0 ], + // [ 9, 49 ],[ 83, 29 ],[ 64, 20 ],[ 64, 20 ] + + // Create polyline for simplifying later + Polyline_2 polyline; + + // Insert coordinates into polyline + for (std::vector coord : coords) { + Point pt(coord[0], coord[1]); + polyline.push_back(pt); + } + + // Insert polyline into ct and run simplify() + CT ct; + ct.insert_constraint(polyline.begin(), polyline.end()); + PS::simplify(ct, Cost(), Stop(0.2)); + Polyline_2 polyline_simplified; + + // Transfer simplified coordinates from ct to polyline for easy handling + auto cit = ct.constraints_begin(); + for (auto vit = ct.points_in_constraint_begin(*cit); + vit != ct.points_in_constraint_end(*cit); vit++) { + polyline_simplified.push_back(*vit); + } + + print_coords(polyline_simplified); + assert(polyline_simplified.size() == 4); +} + +int main() { + simplify_test(); +} diff --git a/Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h b/Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h index be275140aec..4b3ce2f9548 100644 --- a/Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h +++ b/Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h @@ -46,12 +46,13 @@ private: class Node { public: explicit Node(Vertex_handle vh, bool input = false) - : vertex_(vh), id(-1), input(input) + : vertex_(vh), point_(vertex_->point()), id(-1), input(input) {} - const Point& point() const { return vertex_->point(); } + const Point& point() const { return point_; } Vertex_handle vertex() const { return vertex_; } private: Vertex_handle vertex_; + Point point_; public: int id; bool input; @@ -70,7 +71,7 @@ public: > { public: - Point_it() : Vertex_it::iterator_adaptor_() {} + Point_it() : Point_it::iterator_adaptor_() {} Point_it(typename Vertex_list::all_iterator it) : Point_it::iterator_adaptor_(it) {} private: friend class boost::iterator_core_access; @@ -633,10 +634,12 @@ Polyline_constraint_hierarchy_2::remove_points_without_correspo { std::size_t n = 0; for(Point_it it = points_in_constraint_begin(cid); - it != points_in_constraint_end(cid); ++it) { + it != points_in_constraint_end(cid);) { if(cid.vl_ptr()->is_skipped(it.base())) { it = cid.vl_ptr()->erase(it.base()); ++n; + }else{ + ++it; } } return n; From 9aee0b6e6b49c9ce7c53382ff1e142426fd2fed6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mael=20Rouxel-Labb=C3=A9?= Date: Thu, 17 Jun 2021 10:17:34 +0200 Subject: [PATCH 2/3] Add a getter for the origin of the parabola + don't copy needlessly --- Apollonius_graph_2/include/CGAL/Parabola_2.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/Apollonius_graph_2/include/CGAL/Parabola_2.h b/Apollonius_graph_2/include/CGAL/Parabola_2.h index 4cc53f95020..ade19ec3946 100644 --- a/Apollonius_graph_2/include/CGAL/Parabola_2.h +++ b/Apollonius_graph_2/include/CGAL/Parabola_2.h @@ -267,16 +267,21 @@ public: } - inline Line_2 line() const + inline const Line_2& line() const { return l; } - inline Point_2 center() const + inline const Point_2& center() const { return c; } + inline const Point_2& origin() const + { + return o; + } + template< class Stream > void draw(Stream& W) const { From d2a25329e09271b77b81c3cce11f190549383a80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mael=20Rouxel-Labb=C3=A9?= Date: Thu, 17 Jun 2021 10:29:02 +0200 Subject: [PATCH 3/3] Use Parabola_2::origin() Only reason is to test the getter. --- Apollonius_graph_2/include/CGAL/Parabola_2.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Apollonius_graph_2/include/CGAL/Parabola_2.h b/Apollonius_graph_2/include/CGAL/Parabola_2.h index ade19ec3946..bfe3d8be27f 100644 --- a/Apollonius_graph_2/include/CGAL/Parabola_2.h +++ b/Apollonius_graph_2/include/CGAL/Parabola_2.h @@ -288,8 +288,8 @@ public: std::vector< Point_2 > p; std::vector< Point_2 > pleft, pright; - pleft.push_back(o); - pright.push_back(o); + pleft.push_back(origin()); + pright.push_back(origin()); const FT STEP(2); for (int i = 1; i <= 100; i++) { p = compute_points(i * i * STEP); @@ -316,7 +316,7 @@ public: W << Segment_2(pright[i], pright[i+1]); } - W << o; + W << origin(); } };