Skip to content
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

Jump to bottom

[overlay] fix cluster by adapting tolerance #1093

Merged
merged 1 commit into from
Nov 23, 2022
Merged

[overlay] fix cluster by adapting tolerance #1093

merged 1 commit into from
Nov 23, 2022

Conversation

barendgehrels
Copy link
Collaborator

Fixes #1081

barendgehrels
barendgehrels commented Nov 16, 2022
View reviewed changes
doc/release_notes.qbk
@@ -26,6 +26,7 @@
[*Solved issues]

* [@https://github.com/boostorg/geometry/issues/1076 1076] Union: in rare cases it might miss one polygon
* [@https://github.com/boostorg/geometry/issues/1081 1081] Union: due to precision it might miss interior rings
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vissarion if this will not make it in 1.81 then I will update the PR (later)

vissarion
vissarion approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Thanks!

So threshold 1000 was too large and now 3 works for all the old cases plus the issue case. Is it possible to make this threshold adaptive in the future? Does it depend on the input?

@barendgehrels
Copy link
Collaborator Author

Thanks!

So threshold 1000 was too large and now 3 works for all the old cases plus the issue case. Is it possible to make this threshold adaptive in the future? Does it depend on the input?

That's right.

Yes, it might be possible to make it adaptive. I will study that later. For that I should create a new issue. It is convenient to use the input of the reported #1081 issue.

@barendgehrels barendgehrels mentioned this pull request Nov 16, 2022
Open
@barendgehrels
Copy link
Collaborator Author

Thanks!
So threshold 1000 was too large and now 3 works for all the old cases plus the issue case. Is it possible to make this threshold adaptive in the future? Does it depend on the input?

That's right.

Yes, it might be possible to make it adaptive. I will study that later. For that I should create a new issue. It is convenient to use the input of the reported #1081 issue.

Made it right away #1094

Mitsuhiko-Matsukawa added a commit to Mitsuhiko-Matsukawa/geometry that referenced this pull request Nov 17, 2022
@Mitsuhiko-Matsukawa
[overlay] fix cluster by adapting tolerance
bbb6f6d 
boostorg#1093
@barendgehrels barendgehrels force-pushed the issue-1081-fix-threshold branch from 0415568 to a67a7ae Compare November 23, 2022 12:56
@barendgehrels
Copy link
Collaborator Author

Thanks!

So threshold 1000 was too large and now 3 works for all the old cases plus the issue case. Is it possible to make this threshold adaptive in the future? Does it depend on the input?

It's now 1 because the @Mitsuhiko-Matsukawa reported another case failing with 3. This causes one regression in Buffer. But it seems to me that 1 makes more sense as a value than 3 so I decided to mark that case as a failure, and go for 1. The new testcase is added as well.

We should later look at that cluster-issue again, possibly using an adaptive approach.

@barendgehrels barendgehrels merged commit a908a21 into boostorg:develop Nov 23, 2022
@barendgehrels barendgehrels deleted the issue-1081-fix-threshold branch November 23, 2022 13:20
@vissarion vissarion added this to the 1.81 milestone Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vissarion vissarion vissarion approved these changes

@awulkiew awulkiew Awaiting requested review from awulkiew

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.81
Development

Successfully merging this pull request may close these issues.

bg::union_(polygon,ring) returns wrong result. Inner loop disappears.
2 participants
@barendgehrels @vissarion