[traverse] reverse meaning of isolation in difference #887

barendgehrels · 2021-07-08T10:54:18Z

This fixes #869 , the recently found problem in difference and interconnected interiors, and should fix similar cases too (though they were not yet in the test suite).

The root cause was that isolated interior rings were detected but in case of difference, it's meaning should be reversed.

barendgehrels · 2021-07-08T10:55:07Z

Note that I get an unrelated error

clang-linux.compile.c++.without-pch detail/bin/algorithms_visit.test/clang-linux-6.0.0/debug/visit.o
In file included from detail/visit.cpp:18:
../boost/geometry/geometries/adapted/std_any.hpp:42:16: error: no template named 'any_cast' in namespace 'std'; did you mean simply 'any_cast'?
        return std::any_cast<T>(any_ptr);
               ^~~~~~~~~~~~~
               any_cast
../boost/any.hpp:249:17: note: 'any_cast' declared here
    ValueType * any_cast(any * operand) BOOST_NOEXCEPT

but maybe it's fixed in the meantime

barendgehrels · 2021-07-08T10:55:50Z

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

+                    op.enriched.isolated
+                            = reverseMeaningInOp
+                            ? prop.isolated == isolation_no
+                            : prop.isolated == isolation_yes;


this is the fix

The first version did have prop.isolated == isolation_no but that is the default, we should not revert all of them into yes.

But it now does not look symmetrical, theoretically there should be cases where reversal should be necessary - but we don't have them currently in our test set.

Can you reverse one of the test cases? To make sure the other option also gets triggered by test suite?

We test all our difference operations in two ways, so both a - b and b - a . If that is what you mean.

Or you probably mean (what I call) inverse them, so make a large polygon around a polygon and make the polygon itself an interior of that. Yes, I could do that, I will try to construct a case where this should happen. I now know where I’m looking for. I will do that, but not today.

Thanks.

barendgehrels · 2021-07-08T10:57:56Z

Also I get, unrelated:

In file included from ../boost/geometry/strategies/convex_hull/cartesian.hpp:16:
../boost/geometry/strategy/cartesian/side_robust.hpp:53:20: error: non-constant-expression cannot be narrowed from type 'typename coordinate_type<point_xy<int, cartesian> >::type' (aka 'int') to 'double' in initializer list [-Wc++11-narrowing]
        vec2d pa { get<0>(p1), get<1>(p1) };

awulkiew · 2021-07-08T12:08:46Z

@barendgehrels I don't know what's heppening with the first one because the code is behind #ifndef BOOST_NO_CXX17_HDR_ANY. Maybe there is something wrong with clang-6 and we'd need a workaround. Are you setting -std?

Regarding the second one. This was already fixed: #878

barendgehrels · 2021-07-08T14:44:05Z

@barendgehrels I don't know what's heppening with the first one because the code is behind #ifndef BOOST_NO_CXX17_HDR_ANY. Maybe there is something wrong with clang-6 and we'd need a workaround. Are you setting -std?

Regarding the second one. This was already fixed: #878

I'm just calling b2 --toolset=clang and my clang version is clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
(I've also clang version 5.0.2 (tags/RELEASE_502/final) but that gives me other errors, and more)

vissarion · 2021-07-08T15:53:40Z

@barendgehrels I don't know what's heppening with the first one because the code is behind #ifndef BOOST_NO_CXX17_HDR_ANY. Maybe there is something wrong with clang-6 and we'd need a workaround. Are you setting -std?
Regarding the second one. This was already fixed: #878

I'm just calling b2 --toolset=clang and my clang version is clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
(I've also clang version 5.0.2 (tags/RELEASE_502/final) but that gives me other errors, and more)

I am also getting something similar with gcc

gcc.compile.c++ ../../bin.v2/libs/geometry/test/algorithms/detail/algorithms_visit.test/gcc-5~c++14/debug/threading-multi/visibility-hidden/visit.o
In file included from test/algorithms/detail/visit.cpp:18:0:
../../boost/geometry/geometries/adapted/std_any.hpp:19:15: fatal error: any: No such file or directory
compilation terminated.

awulkiew · 2021-07-08T17:09:10Z

@barendgehrels @vissarion AFAIU C++17 headers support was added to Boost.Config in 1.76.
Which version of Boost.Config do you have?
When did you last update the whole Boost (all of the modules)?

kleunen · 2021-07-08T18:02:58Z

I have the following test case which was correct under the old version, but seems to be incorrect, after this patch:
https://wandbox.org/permlink/2zBpf9zFb1Uepu7n

barendgehrels · 2021-07-08T21:08:55Z

@barendgehrels @vissarion AFAIU C++17 headers support was added to Boost.Config in 1.76.
Which version of Boost.Config do you have?
When did you last update the whole Boost (all of the modules)?

@awulkiew ah, of course, sorry. Will update soon, it's long ago indeed.

barendgehrels · 2021-07-08T21:09:20Z

I have the following test case which was correct under the under the old version, but seems to be incorrect, after this patch:
https://wandbox.org/permlink/2zBpf9zFb1Uepu7n

Thanks! I will look at it soon

awulkiew · 2021-07-08T23:57:27Z

LGTM. Is this a bugfix? Would you like to release it with 1.77 if possible? Could we merge #886 before this PR in case you wanted to postpone this one to 1.78?

barendgehrels · 2021-07-09T06:17:42Z

LGTM. Is this a bugfix? Would you like to release it with 1.77 if possible? Could we merge #886 before this PR in case you wanted to postpone this one to 1.78?

Wouter found a problem in another case, I’ve still to check that. Therefore it’s better to postpone it to 1.78

barendgehrels · 2021-07-14T09:04:40Z

I have the following test case which was correct under the under the old version, but seems to be incorrect, after this patch:
https://wandbox.org/permlink/2zBpf9zFb1Uepu7n

I can reproduce it and I've a reduced version for it. I will create a separate issue for this, to add this testcase, and will try to fix it (large input... it might take a while).
It's a combination of complex input multipolygon - while the solved case is simple. So alternatively we can merge this already and (try to) fix it afterwards

kleunen · 2021-07-14T09:28:11Z

Wouter found a problem in another case, I’ve still to check that. Therefore it’s better to postpone it to 1.78

yes, there is no need to rush to a fix. In many cases actually the sym_difference works well. I sort the input polygons now by size and if I do a sym_difference in this order the output is fine. So I really think it is a corner case and you don't want to break functionality working well for other users.

barendgehrels · 2021-07-14T12:02:38Z

The regression in the two cases referred by #888 should be fixed now

barendgehrels · 2021-07-14T12:05:56Z

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

@@ -565,10 +591,83 @@ struct traversal_switch_detector
        }
    }

+#if defined(BOOST_GEOMETRY_DEBUG_TRAVERSAL_SWITCH_DETECTOR)
+    void debug_show_results()


this shows quite a neat list of results and I would like to keep it.

For example

BEGIN SWITCH DETECTOR (region_ids and isolation) REVERSE_2 ADD (s:0, m:0) TO REGION 1 ADD (s:1, m:0) TO REGION 2 ADD (s:1, m:1) TO REGION 3 END SWITCH DETECTOR REGION 1 multiple [turns 0 1 2] regions { 2 : via [ 0 1 2 ] } REGION 2 yes [turns 0 1 2 3] regions { 1 : via [ 0 1 2 ] } { 3 : via [ 3 ] } REGION 3 yes [turns 3] regions { 2 : via [ 3 ] } II 0 (50, 70) -> [0/1] (1 false) / (2 false) II 1 (70, 70) -> [0/1] (1 false) / (2 false) II 2 (70, 50) -> [0/1] (1 false) / (2 false) II 3 (50, 50) -> [1/1] (2 true) / (3 true)

barendgehrels requested review from awulkiew and vissarion July 8, 2021 10:54

barendgehrels commented Jul 8, 2021

View reviewed changes

barendgehrels mentioned this pull request Jul 8, 2021

Sym difference may generate polygon with disconnected interior #869

Closed

awulkiew approved these changes Jul 8, 2021

View reviewed changes

barendgehrels mentioned this pull request Jul 14, 2021

Subsequent errors in difference related to switch traversal #888

Open

barendgehrels force-pushed the fix/traverse branch from a7c26dc to 97f82d2 Compare July 14, 2021 09:56

barendgehrels commented Jul 14, 2021

View reviewed changes

barendgehrels added 3 commits July 21, 2021 11:38

[traverse] reverse meaning of isolation in difference

4597a16

[traverse] avoid using isolated for originally non-isolated regions

68ed6fe

[test] add testcase for issue boostorg#888 polygon 53

4b3f655

barendgehrels force-pushed the fix/traverse branch from 23a2aeb to 4b3f655 Compare July 21, 2021 10:34

barendgehrels merged commit 42edbd3 into boostorg:develop Jul 28, 2021

barendgehrels deleted the fix/traverse branch July 28, 2021 11:10

evgeniyfeder mentioned this pull request Feb 22, 2024

Difference returns wrong result if hole touch the boundary of subtrahend #1244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[traverse] reverse meaning of isolation in difference #887

[traverse] reverse meaning of isolation in difference #887

barendgehrels commented Jul 8, 2021 •

edited

Loading

barendgehrels commented Jul 8, 2021

barendgehrels Jul 8, 2021

barendgehrels Jul 14, 2021

kleunen Jul 14, 2021

barendgehrels Jul 14, 2021

barendgehrels commented Jul 8, 2021

awulkiew commented Jul 8, 2021

barendgehrels commented Jul 8, 2021 •

edited

Loading

vissarion commented Jul 8, 2021

awulkiew commented Jul 8, 2021

kleunen commented Jul 8, 2021 •

edited

Loading

barendgehrels commented Jul 8, 2021

barendgehrels commented Jul 8, 2021

awulkiew commented Jul 8, 2021

barendgehrels commented Jul 9, 2021

barendgehrels commented Jul 14, 2021

kleunen commented Jul 14, 2021 •

edited

Loading

barendgehrels commented Jul 14, 2021

barendgehrels Jul 14, 2021 •

edited

Loading

[traverse] reverse meaning of isolation in difference #887

[traverse] reverse meaning of isolation in difference #887

Conversation

barendgehrels commented Jul 8, 2021 • edited Loading

barendgehrels commented Jul 8, 2021

barendgehrels Jul 8, 2021

Choose a reason for hiding this comment

barendgehrels Jul 14, 2021

Choose a reason for hiding this comment

kleunen Jul 14, 2021

Choose a reason for hiding this comment

barendgehrels Jul 14, 2021

Choose a reason for hiding this comment

barendgehrels commented Jul 8, 2021

awulkiew commented Jul 8, 2021

barendgehrels commented Jul 8, 2021 • edited Loading

vissarion commented Jul 8, 2021

awulkiew commented Jul 8, 2021

kleunen commented Jul 8, 2021 • edited Loading

barendgehrels commented Jul 8, 2021

barendgehrels commented Jul 8, 2021

awulkiew commented Jul 8, 2021

barendgehrels commented Jul 9, 2021

barendgehrels commented Jul 14, 2021

kleunen commented Jul 14, 2021 • edited Loading

barendgehrels commented Jul 14, 2021

barendgehrels Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

barendgehrels commented Jul 8, 2021 •

edited

Loading

barendgehrels commented Jul 8, 2021 •

edited

Loading

kleunen commented Jul 8, 2021 •

edited

Loading

kleunen commented Jul 14, 2021 •

edited

Loading

barendgehrels Jul 14, 2021 •

edited

Loading