[union] fix missing interior ring and double traversed exterior ring by barendgehrels · Pull Request #1113 · boostorg/geometry

barendgehrels · 2023-02-15T15:06:44Z

fixes: #1109 and #1108
keeps fixed: #1081

vissarion

Thanks, I am OK with merging!

vissarion · 2023-02-17T13:41:44Z

+#else
+        using point_t = decltype(turns[*indices.begin()].point);
+        using coor_t = typename coordinate_type<point_t>::type;
+        coor_t centroid_0{};


I do not remember using this initialization in boost geometry. I am not against though, just to check.

I'd prefer to assign 0 explicitly because for user-defined numerical type this could result in default ctor call which could initialize variable to something different than 0.

I will update it. Thanks.

vissarion · 2023-02-17T13:42:34Z

+// The coordinates are averaged.
+// It is necessary to avoid artefacts and invalidities.
+template <typename Turns, typename Clusters>
+inline void colocate_clusters(Turns& turns, Clusters const& clusters)


Shouldn't the order be the other way around? Such that the output is after then input?

You are right will fix it.

vissarion · 2023-02-17T13:48:47Z

    test_one<multi_polygon_type, polygon_type>("rt_p11", rt_p11, join_miter, end_flat, 28.7426, 1.0);
    test_one<multi_polygon_type, polygon_type>("rt_p12", rt_p12, join_miter, end_flat, 22.5711, 1.0);
+
+    // Needs centroid of cluster turn points


this is related to flag BOOST_GEOMETRY_USE_FIRST_POINT_AT_CLUSTER

This define is now gone.
✔️

vissarion · 2023-02-17T13:52:03Z

-    // which is now return T(1) (earlier it was 1000)
-    double const multiplier = 1.0 / 1000.0;
+    // which is now return T(100) (earlier it was 1000)
+    double const multiplier = 1.0 / 10.0;


does it hold multiplier = T / 1000.0; ?

I don't understand. Maybe the comment needs to point to the source in get_clusters.hpp

My question is: "should multiplier be a function of T e.g. multiplier = T / 1000.0; instead of manually update the fraction every time you change the threshold?" Am I missing something in the context here?

Changed these lines a bit, avoiding the multiplier and enhancing the comment
✔️

My question is: "should multiplier be a function of T e.g. multiplier = T / 1000.0; instead of manually update the fraction every time you change the threshold?" Am I missing something in the context here?

Sorry, I noticed this answer only after my own reply.
But I changed it, without multiplier now. I hope the threshold won't change again. Is it fine like this?

awulkiew · 2023-02-18T19:28:57Z

+        for (const auto& index : indices)
+        {
+            centroid_0 += geometry::get<0>(turns[index].point);
+            centroid_1 += geometry::get<1>(turns[index].point);
+        }
+        centroid_0 /= indices.size();
+        centroid_1 /= indices.size();


AFAIU this is only correct for cartesian. Can this be a problem?

Another issue is that for integral coordinates this could result in overflow.

Good points.
Happening on the dateline would be a rare case... but, see below.
Integral coordinates: if they are clustered, they are (by definition) on the same location. So they don't need this, the first point is fine.
I'll update the implementation to cover these two situations (which means: for geographic or integer coordinates take the first).

So indeed it was a problem, reported as #1116 which was found on this branch.
Good catch!
My updated implementation solved that already, but I will create a testcase.
That will be done in a separate PR. If you approve, this can be merged now.

barendgehrels · 2023-02-22T12:30:39Z

+        for (++it; it != indices.end(); ++it)
+        {
+            turns[*it].point = first_point;
+        }


Because all implementation is here, I removed the define, better like this.
Thanks for the comments.

barendgehrels · 2023-02-22T12:31:24Z

    }

    remove_clusters(turns, clusters);
+    colocate_clusters(clusters, turns);


This now looks a bit weird (by the order change).
I will change it in a later PR but I didn't want to do it in this one, to keep the size smaller.

fixes: boostorg#1109 and boostorg#1108 keeps fixed: boostorg#1081

vissarion · 2023-03-06T08:49:19Z

@barendgehrels do you want to have this PR in 1.82 beta? Deadline 8/3

Side note: there is an error in buffer tests https://app.circleci.com/pipelines/github/boostorg/geometry/374/workflows/e4f969a0-4b45-4af2-8316-328437fde513/jobs/19175 probably not related to this PR.

barendgehrels added the bug label Feb 15, 2023

barendgehrels requested review from awulkiew and vissarion February 15, 2023 15:06

barendgehrels self-assigned this Feb 15, 2023

vissarion approved these changes Feb 17, 2023

View reviewed changes

awulkiew reviewed Feb 18, 2023

View reviewed changes

barendgehrels force-pushed the issue-1108-fix-interior-ring-for-union branch from 7f75e4f to 51fa43c Compare February 22, 2023 12:30

barendgehrels commented Feb 22, 2023

View reviewed changes

[union] fix missing interior ring and double traversed exterior ring

ec7f9c9

fixes: boostorg#1109 and boostorg#1108 keeps fixed: boostorg#1081

barendgehrels force-pushed the issue-1108-fix-interior-ring-for-union branch from 51fa43c to ec7f9c9 Compare February 22, 2023 12:34

awulkiew approved these changes Mar 7, 2023

View reviewed changes

vissarion merged commit dfcbb75 into boostorg:develop Mar 7, 2023

vissarion added this to the 1.82 milestone Apr 3, 2023

barendgehrels deleted the issue-1108-fix-interior-ring-for-union branch December 14, 2024 11:14

Uh oh!

Conversation

barendgehrels commented Feb 15, 2023

Uh oh!

vissarion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vissarion commented Mar 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants