fix: nearly identical input should give an intersection#1479
fix: nearly identical input should give an intersection#1479barendgehrels wants to merge 1 commit into
Conversation
barendgehrels
force-pushed
the
issue-1471
branch
from
854cd41 to
a6ef4d0
Compare
@vissarion thanks for the quick review. It's a bugfix, 1.92 is still open, I think we can make that. Then I'll also include it in the release notes. |
I agree, if Tinko is OK, I will merge it to main.
Thanks, I will then update the boost release notes. |
tinko92
left a comment
tinko92
left a comment
There was a problem hiding this comment.
Sorry that I couldn't review this yesterday, there are some lines that were non-obvious to me (I left comments). I am OK with merging this before the release since it fixes cases and does not break anything, so my comments are not blockers.
Most comments are about passing something (constant false or a computed value) for overlay_has_turns being ambiguous. Maybe some argument name that indicates "decide within for the no turns special case" with a default value would be more clear here. But again, I am OK with merging this since it is well-limited to a special case, does not seem to break anything and fixes a real problem.
| constexpr bool is_binary_operation | ||
| = OverlayType == overlay_union | ||
| || OverlayType == overlay_intersection | ||
| || OverlayType == overlay_difference; |
There was a problem hiding this comment.
I have not reviewed dissolve sufficiently to comment on how it should be treated here. This is not a blocker for me for merging since it is in extensions.
| if (bgeo::intersects(geometry1, geometry2) | ||
| && ! bgeo::touches(geometry1, geometry2)) | ||
| { | ||
| BOOST_CHECK_MESSAGE(! result_intersection.empty(), |
There was a problem hiding this comment.
Minor: Probably more consistent to pull all conditions into the macro since we only if on settings here while many checks have compound conditions.
| >::type strategy_type; | ||
|
|
||
| bg::detail::overlay::select_rings<OverlayType>(geometry1, geometry2, empty, selected, strategy_type()); | ||
| bg::detail::overlay::select_rings<OverlayType>(geometry1, geometry2, empty, selected, strategy_type(), false); |
There was a problem hiding this comment.
Why do we pass false here? I think the winded case has turns and the argument that would preserve the prior within_other = range_in_geometry > 0 behavior would be true since it's negated.
| std::map<ring_identifier, properties> selected; | ||
|
|
||
| detail::overlay::select_rings<overlay_dissolve>(input_ring, turn_info_per_ring, selected, strategy); | ||
| detail::overlay::select_rings<overlay_dissolve>(input_ring, turn_info_per_ring, selected, strategy, ! turns.empty()); |
There was a problem hiding this comment.
This value may be true or false but is never used if I understand correctly.
3 participants
Fixes: #1471
✅ AI assisted
Nearly identical polygons should intersect
Two almost-identical polygons (coordinates differing by ~1 epsilon) produced an empty intersection, a doubled union, and a wrong difference.
Root cause: such input yields zero turns, so select_rings decides containment from a single representative point — but that point is a ring vertex, which lands exactly on the other ring's boundary (range_in_geometry == 0), and an on-boundary point was treated as "not within".
Fix: in
select_rings, when the overlay produced no turns, treat an entirely-on-boundary ring as "within" for the binary set operations (union/intersection/difference).Related to commit 2780c9d (which changed this code, reversal would have fixed this as well - but would have given regression in other cases).
This PR also enhances the test (that initially passed for #1471): if two geometries intersect but don't touch, their intersection must be non-empty — independent of the overlay path, so it catches this class of bug that area-balance checks cannot.