fix: clipper2 intersection definition#2742
Conversation
…t intersection The intersectPolylinesSets function returned empty results when one polyline was entirely contained inside the other (no edge crossings). - When A is fully inside B (isContourHole), the intersection is A - When B is fully inside A, the intersection is B
…lly enclosed in minuend Subtracting a shape that was fully enclosed by another (e.g. small sphere inside big sphere) had no effect and returned the original shape unchanged. The subtract operation now correctly handles all spatial relationships and preserves holes through chained operations. Changes: - polylineInfoTypes: add optional holePolylines field to both world and canvas polyline info types - polylineSubtract: handle B-inside-A by cutting a hole, A-inside-B by removing entirely, and skip adding a hole when B is already inside an existing hole (no-op). Propagate existing holes through edge-crossing subtractions - logicalOperators: load hole polylines from child annotations when reading a segment, propagate them through canvas/world space conversions, and carry them back to world space in the operation result - addPolylinesToSegmentation: save hole polylines as proper child annotations with correct winding direction
Added an isTargetInsideSource flag to checkIntersection so that both containment directions (source inside target, target inside source) are detected in one place, rather than scattered containsPoints calls across each caller.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
@dan-rukas and @sedghi I was looking at Ghadeer's changes for union/intersection and running into some usability and performance issues with them, and I realized that the area isn't sufficiently well defined to really know what all we are doing, so with clod, I wrote a semantic behaviour document around the contours: As well, I had it change to use clipper2-ts instead of a custom implementation to try to address some of the performance concerns. I think the semantics changes are mostly reasonable - it basically says that only the source/basis contour gets affected, and only when there is an overlap to start with. I'm not completely comfortable with the definition on that yet, since I think that a union with a contour that is entirely within a hole SHOULD get included. I'm going to update that. There is also some question about the user interface in OHIF - I think the union/intersect/difference should be three buttons with a cancel operation, and the whole dialog should have a help button explaining that it applies to the CURRENT view only, and there should be a progress dialog/indication that something has been applied. Then, the current self-intersect behaviour is very confusion - I have updated the definitions for that to be derived from the union and intersection definitions, and added specific add/remove behaviour that is independent of holes/location of start. |
@dan-rukas, @sedghi and @wayfarer3130 Thanks for this info @wayfarer3130. It sounds to me then that we should discuss this further, but it very much sounds like for 3.13 we should stay the course as far as contour logical operations are concerned and revisit this in the next release. |
3 participants
Context
Intersections with holes are not working consistently. This PR builds on top of changes from:
#2739
to see if we can figure out a reasonable solution for this.
Changes & Results
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment