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. |
📝 WalkthroughWalkthroughReplaces all manual polyline merge/subtract/intersect/XOR algorithms with a ChangesClipper2 Boolean Operations Overhaul
Sequence Diagram(s)sequenceDiagram
rect rgba(200, 220, 255, 0.5)
Note over contourSegmentationCompleted,applyStroke: Cursor gesture (add or remove stroke)
end
participant contourSegmentationCompleted
participant addContourStroke
participant removeContourStroke
participant applyStroke
participant collectSamePlaneSegmentTargets
participant applyBoolean
participant removeAnnotationCompletely
participant createNewAnnotationFromPolyline
contourSegmentationCompleted->>contourSegmentationCompleted: read contourHoleProcessingEnabled
alt contourHoleProcessingEnabled
contourSegmentationCompleted->>removeContourStroke: viewport, sourceAnnotation
removeContourStroke->>applyStroke: viewport, sourceAnnotation, BooleanOp.Difference
else
contourSegmentationCompleted->>addContourStroke: viewport, sourceAnnotation
addContourStroke->>applyStroke: viewport, sourceAnnotation, BooleanOp.Union
end
applyStroke->>applyStroke: convert stroke to canvas space
applyStroke->>collectSamePlaneSegmentTargets: viewport, sourceAnnotation
collectSamePlaneSegmentTargets-->>applyStroke: target annotations[]
applyStroke->>applyBoolean: subjects[targets+holes], clips[stroke], op
applyBoolean-->>applyStroke: PolygonWithHoles[]
applyStroke->>removeAnnotationCompletely: source + all replaced targets + holes
loop each result polygon
applyStroke->>createNewAnnotationFromPolyline: outer ring (Clockwise)
loop each hole
applyStroke->>createNewAnnotationFromPolyline: hole ring (CounterClockwise)
end
end
applyStroke->>contourSegmentationCompleted: ANNOTATION_CUT_MERGE_PROCESS_COMPLETED event
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/tools/src/utilities/contourSegmentation/SEMANTICS.md (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing language specifier on code fence.
The code fence lacks a language identifier. While this doesn't affect correctness, adding
typescriptortsafter the opening backticks improves syntax highlighting and tooling support.📝 Suggested fix
-``` +```typescript type Polygon = { outer: Point2[]; // simple closed polyline, CW, ≥ 3 vertices holes?: Point2[][]; // each simple closed, CCW, fully inside outer, // pairwise non-overlapping };</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@packages/tools/src/utilities/contourSegmentation/SEMANTICS.mdaround lines
22 - 28, The code fence in the SEMANTICS.md file containing the Polygon type
definition lacks a language identifier. Add the language specifiertypescript
immediately after the opening triple backticks () at the beginning of
the code block that defines the Polygon type with outer and holes properties to
enable proper syntax highlighting and tooling support.</details> <!-- cr-comment:v1:bd1369eb6fa2d34baf3520c5 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>packages/tools/src/eventListeners/annotations/contourSegmentation/contourSegmentationCompleted.ts (1)</summary><blockquote> `19-57`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Simplified listener delegates correctly to boolean operations.** The refactoring successfully removes inline intersection detection and delegates to `removeContourStroke`/`addContourStroke`, which map to Difference and Union operations per the context snippets. The documentation clearly explains the polarity model and references the formal specification. One observation: the stroke application calls (`removeContourStroke`/`addContourStroke`) have no explicit error handling. If those functions throw during boolean operations, this listener will propagate the exception, and `ANNOTATION_CUT_MERGE_PROCESS_COMPLETED` won't fire. This may be acceptable if the caller is expected to handle errors, but consider whether defensive error handling would improve resilience. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/src/eventListeners/annotations/contourSegmentation/contourSegmentationCompleted.ts` around lines 19 - 57, The calls to removeContourStroke and addContourStroke in the contourSegmentationCompletedListener function lack error handling, which means any exceptions thrown during boolean operations will propagate and prevent the ANNOTATION_CUT_MERGE_PROCESS_COMPLETED event from being triggered. Add a try-catch block around the conditional logic that calls removeContourStroke or addContourStroke to catch any potential errors, log them appropriately, and determine whether the triggerEvent call should still execute (e.g., ensure resilience by still notifying listeners even if the operation partially fails, or handle the error more explicitly depending on your application's requirements). ``` </details> <!-- cr-comment:v1:ab07bc4166b976181e06b0f1 --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In@packages/tools/src/utilities/contourSegmentation/applyContourStroke.ts:
- Around line 123-140: The triggerAnnotationModified(parent, element) call is
firing before the holes are processed and linked to the parent annotation. Move
the triggerAnnotationModified(parent, element) statement from its current
position (before the polygon.holes?.forEach loop) to after the loop completes,
so that listeners observe the parent with all its child holes already attached
via addChildAnnotation(parent, hole). This ensures the annotation modified event
is emitted only after the parent structure is fully updated.- Around line 63-68: When the guard condition fails and the function returns
early from removeContourStroke because DEFAULT_CONTOUR_SEG_TOOL_NAME is not
registered, the source annotation from the failed subtract operation remains in
state, leaving invalid additive geometry. Before the early return statement in
this guard block, remove or discard the source annotation that was created for
the Difference path to ensure proper cleanup and prevent the incomplete cut
annotation from persisting in the state.In
@packages/tools/src/utilities/contourSegmentation/polylineSubtract.ts:
- Around line 47-56: The toInfo function is only converting the parent contour
polyline and ignoring the child holes from the annotation. Modify the toInfo
function to also extract and convert any child holes from the
annotation.data.contour, convert each hole's polyline to canvas space using
convertContourPolylineToCanvasSpace (similar to how the parent polyline is
converted), and populate the holePolylines array field in the returned
PolylineInfoCanvas object so that annotations with holes are properly
represented when passed to subtractPolylineSets.In
@packages/tools/src/utilities/contourSegmentation/polylineUnify.ts:
- Around line 48-56: The toInfo function inside unifyAnnotationPolylines is not
preserving child holes from the annotation when building the PolylineInfoCanvas
object. To fix this, extract the holes array from annotation.data.contour (in
addition to the main polyline), convert each hole to canvas space using
convertContourPolylineToCanvasSpace with the viewport parameter, and include the
converted holes in the returned PolylineInfoCanvas object so that
BooleanOp.Union can preserve them during the union operation.In
@packages/tools/src/utilities/contourSegmentation/sharedOperations.ts:
- Around line 101-109: The hasIntersection return value on line 107 does not
account for the case where the target polyline is fully contained inside the
source polyline. Update the hasIntersection assignment to also include the
isTargetInsideSource condition so that it returns true when there is a
containment overlap, not just when there are line segment intersections or
contour holes. This ensures callers checking hasIntersection will properly
detect all types of overlaps between the source and target polylines.
Nitpick comments:
In
@packages/tools/src/eventListeners/annotations/contourSegmentation/contourSegmentationCompleted.ts:
- Around line 19-57: The calls to removeContourStroke and addContourStroke in
the contourSegmentationCompletedListener function lack error handling, which
means any exceptions thrown during boolean operations will propagate and prevent
the ANNOTATION_CUT_MERGE_PROCESS_COMPLETED event from being triggered. Add a
try-catch block around the conditional logic that calls removeContourStroke or
addContourStroke to catch any potential errors, log them appropriately, and
determine whether the triggerEvent call should still execute (e.g., ensure
resilience by still notifying listeners even if the operation partially fails,
or handle the error more explicitly depending on your application's
requirements).In
@packages/tools/src/utilities/contourSegmentation/SEMANTICS.md:
- Around line 22-28: The code fence in the SEMANTICS.md file containing the
Polygon type definition lacks a language identifier. Add the language specifier
typescriptimmediately after the opening triple backticks () at the
beginning of the code block that defines the Polygon type with outer and holes
properties to enable proper syntax highlighting and tooling support.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro Plus **Run ID**: `373be3c4-a334-4788-93ff-7219b3a4d1a1` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9cc703acb396d12b37fdc8af4727218574494caa and c8bf188ccbe7ac253d7c6ea543fa874204c11cb5. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `bun.lock` is excluded by `!**/*.lock` * `yarn.lock` is excluded by `!**/yarn.lock`, `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (17)</summary> * `packages/tools/package.json` * `packages/tools/src/eventListeners/annotations/contourSegmentation/contourSegmentationCompleted.ts` * `packages/tools/src/utilities/contourSegmentation/SEMANTICS.md` * `packages/tools/src/utilities/contourSegmentation/addPolylinesToSegmentation.ts` * `packages/tools/src/utilities/contourSegmentation/applyContourStroke.ts` * `packages/tools/src/utilities/contourSegmentation/clipperBooleanOps.spec.ts` * `packages/tools/src/utilities/contourSegmentation/clipperBooleanOps.ts` * `packages/tools/src/utilities/contourSegmentation/logicalOperators.ts` * `packages/tools/src/utilities/contourSegmentation/mergeMultipleAnnotations.ts` * `packages/tools/src/utilities/contourSegmentation/polylineInfoTypes.ts` * `packages/tools/src/utilities/contourSegmentation/polylineIntersect.ts` * `packages/tools/src/utilities/contourSegmentation/polylineSetOps.spec.ts` * `packages/tools/src/utilities/contourSegmentation/polylineSetOps.ts` * `packages/tools/src/utilities/contourSegmentation/polylineSubtract.ts` * `packages/tools/src/utilities/contourSegmentation/polylineUnify.ts` * `packages/tools/src/utilities/contourSegmentation/polylineXor.ts` * `packages/tools/src/utilities/contourSegmentation/sharedOperations.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
# Conflicts: # pnpm-lock.yaml # pnpm-workspace.yaml # utils/ExampleRunner/example-runner-cli.js
5 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
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores