Skip to content

Fix/buffer use cluster for within piece #1404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix/buffer use cluster for within piece #1404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,8 +466,8 @@ struct buffered_piece_collection
turn_in_piece_visitor
<
geometry::cs_tag_t<point_type>,
turn_vector_type, piece_vector_type, DistanceStrategy, Strategy
> visitor(m_turns, m_pieces, m_distance_strategy, m_strategy);
turn_vector_type, cluster_type, piece_vector_type, DistanceStrategy, Strategy
> visitor(m_turns, m_clusters, m_pieces, m_distance_strategy, m_strategy);

geometry::partition
<
Expand Down
50 changes: 48 additions & 2 deletions include/boost/geometry/algorithms/detail/buffer/turn_in_piece_visitor.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ template
<
typename CsTag,
typename Turns,
typename Clusters,
typename Pieces,
typename DistanceStrategy,
typename UmbrellaStrategy
Expand All @@ -46,6 +47,7 @@ template
class turn_in_piece_visitor
{
Turns& m_turns; // because partition is currently operating on const input only
Clusters const& m_clusters;
Pieces const& m_pieces; // to check for piece-type
DistanceStrategy const& m_distance_strategy; // to check if point is on original or one_sided
UmbrellaStrategy const& m_umbrella_strategy;
Expand Down Expand Up @@ -77,6 +79,44 @@ class turn_in_piece_visitor
return false;
}

// Returns true if the turn is part of a cluster, and one of the other turns in the same
// cluster is involved in the same piece as the operations in the turn.
template <typename Turn, typename Piece>
inline bool skip_by_same_cluster(Turn const& turn, Piece const& piece) const
{
if (turn.cluster_id <= 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does a negative cluster_id means and why we are not searching for it? It is the case that m_clusters does not contain negative values so this condition speeds up the search?

{
return false;
}

auto it = m_clusters.find(turn.cluster_id);
if (it == m_clusters.end())
{
return false;
}

for (auto const& index : it->second.turn_indices)
{
if (index == turn.turn_index)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This produces a comparison of integers of different signedness (long int and size_t) warning in algorithms_buffer_linestring_aimes.test and other tests.

{
continue;
}
auto const& other_turn = m_turns[index];
auto const& seg_id0 = other_turn.operations[0].seg_id;
auto const& seg_id1 = other_turn.operations[1].seg_id;

if (seg_id0.piece_index == piece.index
|| seg_id1.piece_index == piece.index)
{
// One of the other turns in the same cluster is an intersection
// with the same piece.
// Therefore, the turn under inspection cannot be within that piece.
return true;
}
}
return false;
}

template <typename NumericType>
inline bool is_one_sided(NumericType const& left, NumericType const& right) const
{
Expand All @@ -96,10 +136,11 @@ class turn_in_piece_visitor

public:

inline turn_in_piece_visitor(Turns& turns, Pieces const& pieces,
inline turn_in_piece_visitor(Turns& turns, Clusters const& clusters, Pieces const& pieces,
DistanceStrategy const& distance_strategy,
UmbrellaStrategy const& umbrella_strategy)
: m_turns(turns)
, m_clusters(clusters)
, m_pieces(pieces)
, m_distance_strategy(distance_strategy)
, m_umbrella_strategy(umbrella_strategy)
Expand Down Expand Up @@ -140,7 +181,12 @@ class turn_in_piece_visitor

if (piece.type == geometry::strategy::buffer::buffered_empty_side)
{
return false;
return true;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not make any difference - but it should have been true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that this case was not covered by a unit test? Could we add one?

}

if (skip_by_same_cluster(turn, piece))
{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the fix

return true;
}

if (piece.type == geometry::strategy::buffer::buffered_point)
Expand Down
25 changes: 23 additions & 2 deletions test/algorithms/buffer/buffer_cases.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,10 @@ static std::string const rt_w16
static std::string const rt_w17
= "MULTIPOLYGON(((3 1,4 2,4 1,3 1)),((5 3,6 4,6 3,5 3)),((5 0,5 1,6 1,6 0,5 0)),((8 5,9 6,9 5,8 5)),((8 5,7 4,7 5,8 5)))";

// Error in turn_in_piece, see readme
// Needs checking turns of same cluster in turn_in_piece
static std::string const rt_w18
= "MULTIPOLYGON(((4 4,4 5,5 4,4 4)),((5 6,6 7,6 6,5 6)),((5 1,4 1,4 2,5 3,5 1)),((7 6,7 7,8 7,7 6)),((0 6,1 6,1 5,1 4,0 4,0 6)),((1 8,2 7,1 7,1 8)),((1 8,2 9,2 8,1 8)),((1 6,1 7,2 6,1 6)),((7 3,7 2,6 2,7 3)),((7 3,7 4,8 4,8 3,7 3)),((3 2,3 1,2 1,3 2)),((3 2,2 2,2 3,3 3,3 2)),((5 8,5 7,4 7,4 8,5 8)),((5 8,6 9,6 8,5 8)))";


// Contains a cc turn (1) located wrongly.
// Reported at 1/1, but should be 0/1 - or it should have a segment id belonging to previous segment.
// Fixed by removing specific arriving handling.
Expand All @@ -102,6 +101,7 @@ static std::string const rt_w21
= "MULTIPOLYGON(((2 8,2 9,3 8,2 8)),((4 6,4 7,5 6,4 6)),((4 2,5 2,4 1,4 2)))";

// Needs a spike. Also, it misses a small triangle, so area is not completely correct
// Also, needs checking turns of same cluster in turn_in_piece
static std::string const rt_w22
= "MULTIPOLYGON(((6 3,6 4,7 4,7 3,6 3)),((6 1,6 2,7 2,6 1)),((2 8,2 9,3 9,2 8)),((4 7,5 8,5 7,4 7)),((2 1,2 0,1 0,2 1)),((2 1,2 2,3 2,3 1,2 1)),((3 3,2 2,2 3,3 3)),((3 3,3 4,4 3,3 3)),((0 5,1 4,1 3,0 3,0 5)),((8 6,9 6,9 5,8 5,8 6)),((8 6,7 6,6 6,7 7,8 6)),((1 3,1 2,0 2,1 3)),((4 3,5 3,4 2,4 3)),((4 2,5 1,4 1,4 2)))";

Expand Down Expand Up @@ -157,4 +157,25 @@ static std::string const rt_w34
static std::string const rt_w35
= "MULTIPOLYGON(((6 6,6 7,7 7,7 6,6 6)),((5 4,5 5,6 5,6 4,5 4)),((4 0,4 1,5 0,4 0)),((0 0,1 1,1 0,0 0)),((7 0,7 1,8 1,8 0,7 0)),((0 2,0 3,1 3,1 2,0 2)),((3 3,4 2,3 2,3 3)),((3 3,3 4,4 4,4 3,3 3)))";

// Needs checking turns of same cluster in turn_in_piece
// Also it has an interesting shape and several overlapping collinear borders in its rings.
static std::string const rt_w36
= "MULTIPOLYGON(((5 0,6 1,6 0,5 0)),((2 0,2 1,3 0,2 0)),((1 4,1 5,2 4,1 4)),((2 7,3 8,3 7,2 7)),((1 8,1 9,2 9,1 8)),((3 4,3 5,4 5,3 4)),((7.5 4.5,8 5,8 4,7 4,7 5,7.5 4.5)))";

// Needs checking turns of same cluster in turn_in_piece
static std::string const rt_w37
= "MULTIPOLYGON(((5 7,6 8,6 7,5 7)),((1 7,1 8,2 8,1 7)),((1 5,2 6,2 5,1 5)),((1 5,2 4,1 4,1 5)))";

// Needs checking turns of same cluster in turn_in_piece
static std::string const rt_w38
= "MULTIPOLYGON(((8 6,8 7,9 7,9 6,8 6)),((1 3,1 4,2 4,1 3)),((2 3,3 4,3 3,2 3)),((8 3,8 4,9 4,9 3,8 3)),((1 8,1 9,2 8,1 8)),((1 0,1 1,2 0,1 0)),((5 1,5 2,6 1,5 1)),((5 5,5 6,6 6,6 5,5 5)))";

// Needs checking turns of same cluster in turn_in_piece
static std::string const rt_w39
= "MULTIPOLYGON(((1 1,1 2,2 1,1 1)),((3 6,4 7,4 6,3 6)),((1 6,2 7,2 6,1 6)),((1 6,2 5,1 5,1 6)),((1 7,1 8,2 8,1 7)),((7 8,8 7,8 6,7 6,7 8)))";

// Needs checking turns of same cluster in turn_in_piece
static std::string const rt_w40
= "MULTIPOLYGON(((6 8,6 9,7 8,6 8)),((7 6,8 6,8 5,7 4,7 6)),((8 8,8 9,9 9,8 8)),((1 6,2 5,1 5,1 6)),((1 6,2 7,2 6,1 6)),((2 5,2 6,3 6,2 5)),((1 5,2 4,1 4,1 5)))";

#endif
15 changes: 7 additions & 8 deletions test/algorithms/buffer/buffer_multi_polygon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,10 +625,7 @@ void test_all()
TEST_BUFFER(rt_w15, join_miter, end_flat, 80.1348, 1.0);
TEST_BUFFER(rt_w16, join_miter, end_flat, 31.6495, 1.0);
TEST_BUFFER(rt_w17, join_miter, end_flat, 33.74264, 1.0);

#if defined(BOOST_GEOMETRY_TEST_FAILURES)
TEST_BUFFER(rt_w18, join_miter, end_flat, 82.4779, 1.0);
#endif
TEST_BUFFER(rt_w18, join_miter, end_flat, 83.4779, 1.0);

#if defined(BOOST_GEOMETRY_TEST_FAILURES) || defined(BOOST_GEOMETRY_CONCEPT_FIX_ARRIVAL)
// See comments at issue issue_1262
Expand All @@ -637,10 +634,7 @@ void test_all()

TEST_BUFFER(rt_w20, join_miter, end_flat, 63.0269, 1.0);
TEST_BUFFER(rt_w21, join_miter, end_flat, 26.3137, 1.0);

#if defined(BOOST_GEOMETRY_TEST_FAILURES)
TEST_BUFFER(rt_w22, join_miter, end_flat, 86.0416, 1.0);
#endif
TEST_BUFFER(rt_w22, join_miter, end_flat, 86.1274, 1.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a change in area here. Did the expected result changed?


TEST_BUFFER(rt_w23, join_miter, end_flat, 59.5711, 1.0);

Expand Down Expand Up @@ -672,6 +666,11 @@ void test_all()
TEST_BUFFER_VALIDITY_FALSE_NEGATIVE(rt_w35, join_round32, end_flat, 51.63174, 1.0);

TEST_BUFFER(rt_w35, join_miter, end_flat, 57.6569, 1.0);
TEST_BUFFER(rt_w36, join_miter, end_flat, 60.1274, 1.0);
TEST_BUFFER(rt_w37, join_miter, end_flat, 30.6569, 1.0);
TEST_BUFFER(rt_w38, join_miter, end_flat, 68.2279, 1.0);
TEST_BUFFER(rt_w39, join_miter, end_flat, 46.2279, 1.0);
TEST_BUFFER(rt_w40, join_miter, end_flat, 49.0490, 1.0);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These new 5 all fail in develop


test_one<multi_polygon_type, polygon_type>("nores_mt_1", nores_mt_1, join_round32, end_flat, 13.4113, 1.0);
test_one<multi_polygon_type, polygon_type>("nores_mt_2", nores_mt_2, join_round32, end_flat, 17.5265, 1.0);
Expand Down