From 88ea9049ff56214c6fdd58124799426300c7d5e0 Mon Sep 17 00:00:00 2001 From: Barend Gehrels Date: Wed, 16 Aug 2023 14:58:02 +0200 Subject: [PATCH] [intersection] remove closing point correctly * the function (in namespace detail) is split and two parts are renamed on purpose, because functionality is changed * tests are added * intersection.cpp split into new file intersection_integer.cpp * testing point count now if specified --- .../overlay/append_no_dups_or_spikes.hpp | 74 ++++++----- .../detail/overlay/traversal_ring_creator.hpp | 4 +- test/algorithms/overlay/overlay_cases.hpp | 6 + .../set_operations/intersection/Jamfile | 1 + .../intersection/intersection.cpp | 42 ------ .../intersection/intersection_integer.cpp | 122 ++++++++++++++++++ .../intersection/test_intersection.hpp | 64 +++++---- 7 files changed, 209 insertions(+), 104 deletions(-) create mode 100644 test/algorithms/set_operations/intersection/intersection_integer.cpp diff --git a/include/boost/geometry/algorithms/detail/overlay/append_no_dups_or_spikes.hpp b/include/boost/geometry/algorithms/detail/overlay/append_no_dups_or_spikes.hpp index a21ee9832b..0af3c31c5a 100644 --- a/include/boost/geometry/algorithms/detail/overlay/append_no_dups_or_spikes.hpp +++ b/include/boost/geometry/algorithms/detail/overlay/append_no_dups_or_spikes.hpp @@ -168,56 +168,64 @@ inline void append_no_collinear(Range& range, Point const& point, } } -template -inline void clean_closing_dups_and_spikes(Range& range, - Strategy const& strategy, - RobustPolicy const& robust_policy) +// Should only be called internally, from traverse. +template +inline void remove_spikes_at_closure(Ring& ring, Strategy const& strategy, + RobustPolicy const& robust_policy) { - std::size_t const minsize - = core_detail::closure::minimum_ring_size - < - geometry::closure::value - >::value; - - if (boost::size(range) <= minsize) + // It assumes a closed ring (whatever the closure value) + constexpr std::size_t min_size + = core_detail::closure::minimum_ring_size + < + geometry::closed + >::value; + + if (boost::size(ring) < min_size) { + // Don't act on too small rings. return; } - static bool const closed = geometry::closure::value == geometry::closed; - -// TODO: the following algorithm could be rewritten to first look for spikes -// and then erase some number of points from the beginning of the Range - bool found = false; do { found = false; - auto first = boost::begin(range); - auto second = first + 1; - auto ultimate = boost::end(range) - 1; - if (BOOST_GEOMETRY_CONDITION(closed)) - { - ultimate--; - } + auto const first = boost::begin(ring); + auto const second = first + 1; + auto const penultimate = boost::end(ring) - 2; // Check if closing point is a spike (this is so if the second point is // considered as collinear w.r.t. the last segment) - if (point_is_collinear(*second, *ultimate, *first, + if (point_is_collinear(*second, *penultimate, *first, strategy.side(), // TODO: Pass strategy? robust_policy)) { - range::erase(range, first); - if (BOOST_GEOMETRY_CONDITION(closed)) - { - // Remove closing last point - range::resize(range, boost::size(range) - 1); - // Add new closing point - range::push_back(range, range::front(range)); - } + // Remove first point and last point + range::erase(ring, first); + range::resize(ring, boost::size(ring) - 1); + // Close the ring again + range::push_back(ring, range::front(ring)); + found = true; } - } while(found && boost::size(range) > minsize); + } while (found && boost::size(ring) >= min_size); +} + +template +inline void fix_closure(Ring& ring, Strategy const& strategy) +{ + if (BOOST_GEOMETRY_CONDITION(geometry::closure::value == geometry::open)) + { + if (! boost::empty(ring) + && detail::equals::equals_point_point(range::front(ring), range::back(ring), strategy)) + { + // Correct closure: traversal automatically closes rings. + // Depending on the geometric configuration, + // remove_spikes_at_closure can remove the closing point. + // But it does not always do that. Therefore it is corrected here explicitly. + range::resize(ring, boost::size(ring) - 1); + } + } } diff --git a/include/boost/geometry/algorithms/detail/overlay/traversal_ring_creator.hpp b/include/boost/geometry/algorithms/detail/overlay/traversal_ring_creator.hpp index fd3513ff2c..ae9510cff3 100644 --- a/include/boost/geometry/algorithms/detail/overlay/traversal_ring_creator.hpp +++ b/include/boost/geometry/algorithms/detail/overlay/traversal_ring_creator.hpp @@ -274,6 +274,9 @@ struct traversal_ring_creator if (traverse_error == traverse_error_none) { + remove_spikes_at_closure(ring, m_strategy, m_robust_policy); + fix_closure(ring, m_strategy); + std::size_t const min_num_points = core_detail::closure::minimum_ring_size < @@ -282,7 +285,6 @@ struct traversal_ring_creator if (geometry::num_points(ring) >= min_num_points) { - clean_closing_dups_and_spikes(ring, m_strategy, m_robust_policy); rings.push_back(ring); m_trav.finalize_visit_info(m_turn_info_map); diff --git a/test/algorithms/overlay/overlay_cases.hpp b/test/algorithms/overlay/overlay_cases.hpp index 2936a06dd4..d3124e4bca 100644 --- a/test/algorithms/overlay/overlay_cases.hpp +++ b/test/algorithms/overlay/overlay_cases.hpp @@ -1113,6 +1113,12 @@ static std::string issue_1108[2] = "POLYGON((22 1,22 0,14 0,18 -1.2696790939262529996,12 0,22 1))" }; +static std::string issue_1184[2] = +{ + "POLYGON((1169 177,2004 177,2004 1977,1262 1977,1169 177))", + "POLYGON((1179 371,1175 287,1179 287,1179 371))" +}; + static std::string ggl_list_20120229_volker[3] = { "POLYGON((1716 1554,2076 2250,2436 2352,2796 1248,3156 2484,3516 2688,3516 2688,3156 2484,2796 1248,2436 2352,2076 2250, 1716 1554))", diff --git a/test/algorithms/set_operations/intersection/Jamfile b/test/algorithms/set_operations/intersection/Jamfile index ec4a9684c2..c65ebc6732 100644 --- a/test/algorithms/set_operations/intersection/Jamfile +++ b/test/algorithms/set_operations/intersection/Jamfile @@ -29,4 +29,5 @@ test-suite boost-geometry-algorithms-intersection [ run intersection_pl_l.cpp : : : : algorithms_intersection_pl_l ] [ run intersection_pl_pl.cpp : : : : algorithms_intersection_pl_pl ] [ run intersection_tupled.cpp : : : : algorithms_intersection_tupled ] + [ run intersection_integer.cpp : : : : algorithms_intersection_integer ] ; diff --git a/test/algorithms/set_operations/intersection/intersection.cpp b/test/algorithms/set_operations/intersection/intersection.cpp index c37f1d5bff..4ce0127604 100644 --- a/test/algorithms/set_operations/intersection/intersection.cpp +++ b/test/algorithms/set_operations/intersection/intersection.cpp @@ -826,33 +826,6 @@ void test_rational() 1, 7, 5.47363293); } -template -void test_ticket_10868(std::string const& wkt_out) -{ - typedef bg::model::point point_type; - typedef bg::model::polygon - < - point_type, /*ClockWise*/false, /*Closed*/false - > polygon_type; - typedef bg::model::multi_polygon multipolygon_type; - - polygon_type polygon1; - bg::read_wkt(ticket_10868[0], polygon1); - polygon_type polygon2; - bg::read_wkt(ticket_10868[1], polygon2); - - multipolygon_type multipolygon_out; - bg::intersection(polygon1, polygon2, multipolygon_out); - std::stringstream stream; - stream << bg::wkt(multipolygon_out); - - BOOST_CHECK_EQUAL(stream.str(), wkt_out); - - test_one("ticket_10868", - ticket_10868[0], ticket_10868[1], - 1, 7, 20266195244586.0); -} - int test_main(int, char* []) { BoostGeometryWriteTestConfiguration(); @@ -869,21 +842,6 @@ int test_main(int, char* []) test_rational > >(); #endif -#if defined(BOOST_GEOMETRY_TEST_FAILURES) - // ticket #10868 still fails for 32-bit integers - test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); - -#if !defined(BOOST_NO_INT64_T) || defined(BOOST_HAS_MS_INT64) - test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); -#endif - - if (BOOST_GEOMETRY_CONDITION(sizeof(long) * CHAR_BIT >= 64)) - { - test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); - } - - test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); -#endif #endif #if defined(BOOST_GEOMETRY_TEST_FAILURES) diff --git a/test/algorithms/set_operations/intersection/intersection_integer.cpp b/test/algorithms/set_operations/intersection/intersection_integer.cpp new file mode 100644 index 0000000000..e5b8270fc6 --- /dev/null +++ b/test/algorithms/set_operations/intersection/intersection_integer.cpp @@ -0,0 +1,122 @@ +// Boost.Geometry +// Unit Test + +// Copyright (c) 2007-2023 Barend Gehrels, Amsterdam, the Netherlands. + +// This file was modified by Oracle on 2015-2022. +// Modifications copyright (c) 2015-2022, Oracle and/or its affiliates. +// Contributed and/or modified by Menelaos Karavelas, on behalf of Oracle +// Contributed and/or modified by Adam Wulkiewicz, on behalf of Oracle + +// Use, modification and distribution is subject to the Boost Software License, +// Version 1.0. (See accompanying file LICENSE_1_0.txt or copy at +// http://www.boost.org/LICENSE_1_0.txt) + +#include "test_intersection.hpp" +#include + +#include + +#define TEST_INTERSECTION(caseid, clips, points, area) \ + (test_one) \ + ( #caseid, caseid[0], caseid[1], clips, points, area, settings) + +namespace +{ + std::string rectangles[2] = + { + "POLYGON((1000 1000,2000 1000,2000 2000,1000 2000))", + "POLYGON((500 500,1500 500,1500 1500,500 1500))" + }; + + std::string start_within[2] = + { + "POLYGON((1000 1000,2000 1000,2000 2000,1000 2000))", + "POLYGON((1250 1250,1500 750,1750 1250,1750 1750))" + }; +} + + +template +void test_areal() +{ + static bool is_open = bg::closure::value == bg::open; + + ut_settings settings; + settings.test_point_count = true; + + TEST_INTERSECTION(rectangles, 1, is_open ? 4 : 5, 250000.0); + TEST_INTERSECTION(start_within, 1, is_open ? 5 : 6, 218750.0); + TEST_INTERSECTION(issue_1184, 1, is_open ? 3 : 4, 156.0); +} + +template +void test_all() +{ + using polygon = bg::model::polygon

; + using polygon_ccw = bg::model::polygon; + using polygon_open = bg::model::polygon; + using polygon_ccw_open = bg::model::polygon; + test_areal(); + test_areal(); + test_areal(); + test_areal(); +} + +template +void test_ticket_10868(std::string const& wkt_out) +{ + using point_type = bg::model::point; + using polygon_type = bg::model::polygon + < + point_type, /*ClockWise*/false, /*Closed*/false + >; + using multipolygon_type = bg::model::multi_polygon; + + polygon_type polygon1; + polygon_type polygon2; + bg::read_wkt(ticket_10868[0], polygon1); + bg::read_wkt(ticket_10868[1], polygon2); + + multipolygon_type multipolygon_out; + bg::intersection(polygon1, polygon2, multipolygon_out); + std::stringstream stream; + stream << bg::wkt(multipolygon_out); + + // BOOST_CHECK_EQUAL(stream.str(), wkt_out); + + test_one("ticket_10868", + ticket_10868[0], ticket_10868[1], + 1, 7, 20266195244586.0); +} + + + +int test_main(int, char* []) +{ + BoostGeometryWriteTestConfiguration(); + + test_all >(); + + +#if defined(BOOST_GEOMETRY_TEST_FAILURES) + // ticket #10868 still fails for 32-bit integers + test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); + + test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); +#endif + + #if defined(BOOST_GEOMETRY_TEST_FAILURES) + // Ticket #10868 was normally not run for other types + // It results in a different area (might be fine) + // and it reports self intersections. + if (BOOST_GEOMETRY_CONDITION(sizeof(long) * CHAR_BIT >= 64)) + { + test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); + } + + test_ticket_10868("MULTIPOLYGON(((33520458 6878575,33480192 14931538,31446819 18947953,30772384 19615678,30101303 19612322,30114725 16928001,33520458 6878575)))"); + #endif + + return 0; +} diff --git a/test/algorithms/set_operations/intersection/test_intersection.hpp b/test/algorithms/set_operations/intersection/test_intersection.hpp index 4baa6f8fbd..8c448a10c6 100644 --- a/test/algorithms/set_operations/intersection/test_intersection.hpp +++ b/test/algorithms/set_operations/intersection/test_intersection.hpp @@ -50,12 +50,15 @@ struct ut_settings : ut_base_settings { double percentage; - bool debug; + bool debug{false}; + bool test_point_count{false}; + + bool debug_wkt{false}; + bool debug_dsv{false}; explicit ut_settings(double p = 0.0001, bool tv = true) : ut_base_settings(tv) , percentage(p) - , debug(false) {} }; @@ -82,7 +85,7 @@ void check_result(IntersectionOutput const& intersection_output, { // here n should rather be of type std::size_t, but expected_point_count // is set to -1 in some test cases so type int was left for now - n += static_cast(bg::num_points(*it, true)); + n += static_cast(bg::num_points(*it, false)); } if (! expected_hole_count.empty()) @@ -95,10 +98,15 @@ void check_result(IntersectionOutput const& intersection_output, ? bg::length(*it) : bg::area(*it); - if (settings.debug) + if (settings.debug_wkt) { std::cout << std::setprecision(20) << bg::wkt(*it) << std::endl; } + if (settings.debug_dsv) + { + // Write as DSV (which, by default, does not add a closing point) + std::cout << std::setprecision(20) << bg::dsv(*it) << std::endl; + } } if (settings.test_validity()) @@ -112,21 +120,17 @@ void check_result(IntersectionOutput const& intersection_output, << " type: " << (type_for_assert_message())); } - boost::ignore_unused(n); - #if ! defined(BOOST_GEOMETRY_NO_BOOST_TEST) -#if defined(BOOST_GEOMETRY_USE_RESCALING) - // Without rescaling, point count might easily differ (which is no problem) - if (expected_point_count > 0) + // Only test if explicitly mentioned + if (settings.test_point_count) { - BOOST_CHECK_MESSAGE(bg::math::abs(n - expected_point_count) < 3, + BOOST_CHECK_MESSAGE(n == expected_point_count, "intersection: " << caseid << " #points expected: " << expected_point_count << " detected: " << n << " type: " << (type_for_assert_message()) ); } -#endif if (! expected_count.empty()) { @@ -166,9 +170,22 @@ typename bg::default_area_result::type test_intersection(std::string const& int expected_point_count = 0, expectation_limits const& expected_length_or_area = 0, ut_settings const& settings = ut_settings()) { + using coordinate_type = typename bg::coordinate_type::type; + constexpr bool is_ccw = + bg::point_order::value == bg::counterclockwise + || bg::point_order::value == bg::counterclockwise; + constexpr bool is_open = + bg::closure::value == bg::open + || bg::closure::value == bg::open; + if (settings.debug) { - std::cout << std::endl << "case " << caseid << std::endl; + + std::cout << std::endl << "case " << caseid + << " " << string_from_type::name() + << (is_ccw ? " ccw" : "") + << (is_open ? " open" : "") + << std::endl; } typedef typename setop_output_type::type result_type; @@ -179,13 +196,12 @@ typename bg::default_area_result::type test_intersection(std::string const& #if ! defined(BOOST_GEOMETRY_TEST_ONLY_ONE_TYPE) if (! settings.debug) { - // Check _inserter behaviour with stratey - typedef typename bg::strategy::intersection::services::default_strategy - < - typename bg::cs_tag::type - >::type strategy_type; + // Check inserter behaviour with stratey + using strategy_type + = typename bg::strategies::relate::services::default_strategy::type; result_type clip; - bg::detail::intersection::intersection_insert(g1, g2, std::back_inserter(clip), strategy_type()); + bg::detail::intersection::intersection_insert(g1, g2, + std::back_inserter(clip), strategy_type()); } #endif @@ -226,22 +242,14 @@ typename bg::default_area_result::type test_intersection(std::string const& #if defined(TEST_WITH_SVG) { bool const is_line = bg::geometry_id::type::value == 2; - typedef typename bg::coordinate_type::type coordinate_type; - - bool const ccw = - bg::point_order::value == bg::counterclockwise - || bg::point_order::value == bg::counterclockwise; - bool const open = - bg::closure::value == bg::open - || bg::closure::value == bg::open; std::ostringstream filename; filename << "intersection_" << caseid << "_" << string_from_type::name() << string_from_type::name() - << (ccw ? "_ccw" : "") - << (open ? "_open" : "") + << (is_ccw ? "_ccw" : "") + << (is_open ? "_open" : "") #if defined(BOOST_GEOMETRY_USE_RESCALING) << "_rescaled" #endif