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.

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

fix: revise traversal_switch_detector for isolated areas #1350

Closed
wants to merge 1 commit into from

Conversation

barendgehrels
Copy link
Collaborator

@barendgehrels barendgehrels commented Dec 4, 2024

Not ready for review

Fixes: issue 1349

This is quite a revisal. Issue #1349 was caused by a logic error, and not by a precision error. It was easily fixed by removing the (in hindsight weird) condition (added by me) to reverse the meaning of isolated.

But that caused the original issue again. I made an inverse case of that one as well, and that failed anyway (also with its earlier "fix") for difference.

I decided to go for another way to detect isolation, which is now clearer - but even then not that easy. Below some cases for reference.

Below some pictures of intersections (blue, green: input, turquoise: both (= intersection), dotted red/green: resulting intersection.

Case 869 (only ii turns)
image

Case 128 (with a separate polygon connected as "multiple" with only ii turns (turn 6,9,18), it should not be isolated)

image

Case 141 (idem but it should be isolated and the "multiple" region inside (turn 13,14) is a separate polygon)

image

Case case_recursive_boxes_81 which looks the same, but here it are two outer polygons with a part (at turn 26, 27, 42)

image

@tinko92
Copy link
Collaborator

tinko92 commented Dec 6, 2024

I think, this creates a regression. Consider

#include <iostream>
#include <boost/geometry.hpp>
#include <boost/geometry/geometries/geometries.hpp>

namespace bg = boost::geometry;
using point = bg::model::point<int, 2, bg::cs::cartesian>;
using polygon = bg::model::polygon<point>;
using multipoly = bg::model::multi_polygon<polygon>;

int main() {
    auto p1 = bg::from_wkt<multipoly>("MULTIPOLYGON(((2 10,2 8,0 8,0 10,2 10)),((10 8,10 2,8 2,8 0,0 0,0 4,2 4,2 8,4 8,4 10,6 10,6 8,6 6,8 6,8 8,10 8),(8 2,8 4,4 4,4 2,8 2)))");
    auto p2 = bg::from_wkt<multipoly>("MULTIPOLYGON(((2 6,2 4,2 2,2 0,0 0,0 6,2 6)),((2 10,2 8,0 8,0 10,2 10)),((6 8,6 6,2 6,2 8,6 8)),((8 4,8 2,6 2,6 4,8 4)),((10 8,10 6,8 6,8 8,10 8)),((8 10,8 8,6 8,6 10,8 10)))");
    multipoly p3;
    bg::sym_difference(p1, p2, p3);
    std::cout << bg::wkt(p3) << "\n";
    return 0;
}

Output on develop: MULTIPOLYGON(((10 6,10 2,8 2,8 0,2 0,2 4,0 4,0 6,10 6),(6 2,6 4,4 4,4 2,6 2)),((4 8,4 10,6 10,8 10,8 8,4 8)))
Output on this branch: MULTIPOLYGON(((10 6,10 2,8 2,8 0,2 0,2 4,0 4,0 6,10 6)),((4 8,4 10,6 10,8 10,8 8,4 8)))

@barendgehrels
Copy link
Collaborator Author

I think, this creates a regression. Consider

👍 I'll check

@barendgehrels
Copy link
Collaborator Author

I think, this creates a regression. Consider

👍 I'll check

Indeed it fails now. I have to revise it.

@barendgehrels barendgehrels marked this pull request as draft December 7, 2024 23:37
@barendgehrels
Copy link
Collaborator Author

The region information for #869 is incorrect - there is a fix that fixes this all but leads to yet another situation which goes wrong.
I have to revise the code, probably getting rid of the isolation concept.

@barendgehrels
Copy link
Collaborator Author

@tinko92 we (@vissarion and I) added you as a collaborator, now I can add you as a reviewer, great.

For the rest this PR is still in draft and might be closed by a replacement later this month.

@barendgehrels
Copy link
Collaborator Author

Replaced by #1356

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants