fix: set turns in closed clusters as non traversable #1248

barendgehrels · 2024-02-25T10:36:34Z

No description provided.

barendgehrels · 2024-02-25T10:37:53Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+                // If the cluster is completely closed, mark it as not traversable.
+                for (auto index : cluster.second.turn_indices)
+                {
+                    m_turns[index].is_turn_traversable = false;


This is the main fix. Especially for linestrings, with flat end, there were many turns, completely closed, but still marked as traversable. This could cause the buffer fail (no or partial output)

barendgehrels · 2024-02-25T10:39:14Z

include/boost/geometry/algorithms/detail/overlay/cluster_info.hpp

-    {}
+    //! Number of spikes, where a segment goes to the cluster point
+    //! and leaves immediately in the opposite direction.
+    std::size_t spike_count{0};


This (and related code) is also necessary, to have a way out from a cluster which is closed, but where still is a way in and out. This is also related to floating point issues - the part between the spike legs might be indetectable

barendgehrels · 2024-02-25T10:40:58Z

include/boost/geometry/algorithms/detail/overlay/discard_duplicate_turns.hpp

+                            // The turns are not part of the same cluster,
+                            // or one is clustered and the other is not.
+                            // This is not corresponding.
+                            continue;


This was earlier not possible. But now that clusters are detected before, the start turn and its corresponding turn should be located on the same cluster.

We might be able to delete the corresponding_turn functions because of this.
Will check that, but if possible, it will be a different PR

It cannot be deleted, but it might be simplified

barendgehrels · 2024-02-25T10:42:43Z

include/boost/geometry/algorithms/detail/overlay/traversal.hpp

+        return to_start && to_start_index ? 5
+            : to_start ? 4
+            : next_in_same_cluster ? 2
+            : 3


The values just define the ordering. There is now 1 extra, lowest ordering: spike out

Is there a comment that explain this orderings?

✅ the comment was a bit above, but I didn't update it. Thanks.
Comment is moved to function description, and a short comment here.

barendgehrels · 2024-02-25T10:43:28Z

test/algorithms/buffer/buffer_variable_width.cpp

+    test_buffer("case3", "MULTILINESTRING((-18.00000000002045652536253328435122966766357421875 10.4999999997948929575386500800959765911102294921875,-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875),(-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875,18.00000000002045652536253328435122966766357421875 10.4999999997948947338954894803464412689208984375),(-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875,11.99999999996175148453403380699455738067626953125 -10.49999999983214848953139153309166431427001953125))",
+             {6.0, 4.5, 1.5}, 429.831);
+    test_buffer("case4", "MULTILINESTRING((-18.00000000002045652536253328435122966766357421875 10.4999999997948929575386500800959765911102294921875,-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875),(-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875,18.00000000002045652536253328435122966766357421875 10.4999999997948947338954894803464412689208984375),(-2.99999999998012700785920969792641699314117431640625 10.4999999997948929575386500800959765911102294921875,17.99999999995061017443731543608009815216064453125 -4.5000000001943778471513724070973694324493408203125))",
+             {6.0, 4.5, 1.5}, 423.195);


Cases 3 and 4 were failing before the fixes

awulkiew · 2024-02-25T12:02:05Z

include/boost/geometry/algorithms/detail/overlay/enrich_intersection_points.hpp

-        = detail::overlay::handle_colocations
+    bool has_colocations = false;
+
+    if (BOOST_GEOMETRY_CONDITION (! is_buffer))


BOOST_GEOMETRY_CONSTEXPR could be used instead.

Sure, I'll change it

As soon as your PR is merged ;-)

Ok, but in case you wasn't aware, this macro is already in develop:
https://github.com/boostorg/geometry/blob/develop/include/boost/geometry/util/constexpr.hpp

👍 right! Will update on Wednesday

✅ this one is updated, it is a new block and has an else branch

awulkiew · 2024-02-25T12:03:12Z

include/boost/geometry/algorithms/detail/overlay/handle_colocations.hpp

@@ -357,7 +356,7 @@ inline bool handle_colocations(Turns& turns, Clusters& clusters,
    {
        std::cout << kv.first << std::endl;
        for (auto const& toi : kv.second)
-        {
+    {


probably wrong indent

awulkiew · 2024-02-25T12:10:10Z

include/boost/geometry/algorithms/detail/overlay/enrich_intersection_points.hpp

+                Reverse1,
+                Reverse2,
+                OverlayType
+            >(clusters, turns, target_operation,


target_operation is known at compile-time. It could be passed as template parameter and then checked with if BOOST_GEOMETRY_CONSTEXPR inside gather_cluster_properties. But a different PR would probably be better for this.

Will do it in another PR indeed. Not today.

awulkiew · 2024-02-25T12:11:28Z

include/boost/geometry/algorithms/detail/overlay/handle_colocations.hpp

@@ -475,7 +489,8 @@ inline void gather_cluster_properties(Clusters& clusters, Turns& turns,
            turn_operation_type& op = turn.operations[ranked.operation_index];

            if (set_startable
-                    && for_operation == operation_union && cinfo.open_count == 0)
+                && for_operation == operation_union


Here is the condition for if constexpr. Or is it not always the case that for_operation is known at compile-time?

Yes, it is known. I'll update.

I didn't update this one, and the ones below.
Because for_operation is passed as a normal parameter.
This needs more refactoring and it is better to separate that.

awulkiew · 2024-02-25T12:14:34Z

include/boost/geometry/algorithms/detail/overlay/traversal.hpp

@@ -791,6 +794,24 @@ public :
            return false;
        }

+        if (is_union && cinfo.open_count == 0 && cinfo.spike_count > 0)


is_union is static const so I guess it can cause warnings.

indeed, I'll update.
Thanks for your quick review.

Similarly here: not updated. Here it was not passed, but it is used in many places like this in this source file. And they should all be updated.
I guess, because of the combination of conditions, there are no warnings (not sure though). In other places it was combined as well, in this source.
So also this should go to another PR.

awulkiew · 2024-02-25T12:21:13Z

Thanks for the PR and the detailed explanation of changes. :)

vissarion

Thanks, LGTM in general.

vissarion · 2024-02-28T09:29:15Z

include/boost/geometry/algorithms/detail/overlay/traversal.hpp

+        return to_start && to_start_index ? 5
+            : to_start ? 4
+            : next_in_same_cluster ? 2
+            : 3


Is there a comment that explain this orderings?

vissarion · 2024-02-28T09:33:44Z

test/algorithms/buffer/buffer_variable_width.cpp

+        <
+            polygon_t
+        >(mls,
+            std::back_inserter(result),


indentation issue here.

vissarion · 2024-02-28T09:40:41Z

test/algorithms/buffer/buffer_variable_width.cpp

+                                            DistanceStrategy const& distance,
+                                            OutputRange &output_range) const
+    {
+        const auto result =


Maybe auto const which seems that is more common as a style in BG?
Also why not auto const&?

Yes it is. But on my office it's not, so I sometimes don't switch at the right moment...
Thanks.
BTW, in this case it should not be & because it's an assignment from a function result. (Though, technically, it's in line with C++ specs)

vissarion · 2024-02-28T09:46:31Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+            >(m_clusters, m_turns, detail::overlay::operation_union,
+            offsetted_rings, offsetted_rings, m_strategy);
+
+        for (const auto& cluster : m_clusters)


auto const&?

vissarion · 2024-02-28T09:47:57Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+            if (cluster.second.open_count == 0 && cluster.second.spike_count == 0)
+            {
+                // If the cluster is completely closed, mark it as not traversable.
+                for (auto index : cluster.second.turn_indices)


what about auto const& here?

Indeed. Will fix later today. Same for your similar comments, thanks.

vissarion · 2024-02-28T09:48:42Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+        for (auto const& cluster : m_clusters)
+        {
+            bool is_traversable = false;
+            for (auto index : cluster.second.turn_indices)


similarly, what about using auto const& here?

vissarion · 2024-02-28T09:48:55Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+            }
+            if (is_traversable)
+            {
+                for (auto index : cluster.second.turn_indices)


barendgehrels added the bug label Feb 25, 2024

barendgehrels requested review from awulkiew and vissarion February 25, 2024 10:36

barendgehrels self-assigned this Feb 25, 2024

barendgehrels commented Feb 25, 2024

View reviewed changes

barendgehrels force-pushed the fix/buffer-linestring-turn-in-piece branch from 142d505 to a1e8572 Compare February 25, 2024 10:49

awulkiew reviewed Feb 25, 2024

View reviewed changes

awulkiew approved these changes Feb 25, 2024

View reviewed changes

vissarion approved these changes Feb 28, 2024

View reviewed changes

fix: set turns in closed clusters as non traversable

5dae1a0

barendgehrels force-pushed the fix/buffer-linestring-turn-in-piece branch from a1e8572 to 5dae1a0 Compare February 28, 2024 16:20

barendgehrels merged commit acaed28 into boostorg:develop Feb 28, 2024
23 checks passed

barendgehrels deleted the fix/buffer-linestring-turn-in-piece branch February 28, 2024 16:34

barendgehrels mentioned this pull request Mar 27, 2024

Buffer gives incorrect result for negative distance #1262

Closed

vissarion added this to the 1.85 milestone Apr 1, 2024

fix: set turns in closed clusters as non traversable #1248

fix: set turns in closed clusters as non traversable #1248

Conversation

barendgehrels commented Feb 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew commented Feb 25, 2024

vissarion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels Feb 28, 2024 •

edited

Loading

barendgehrels Feb 28, 2024 •

edited

Loading