fix(contour): fix intersect and subtract operations when one segment is fully inside another

[GhadeerAlbattarni]

Context

OHIF_REF: fix/5648-contour-combine

This PR fixes an issue in ohif OHIF/Viewers#5648

When performing contour logical operations between two segments where one segment is fully contained inside the other — meaning their edges do not cross — both the intersect and subtract operations produced incorrect results:

Intersect

When intersecting two segments where one segment is fully inside the other, the operation returned an empty segment instead of returning the smaller contained segment.

Subtract

When subtracting two contour segments where one segment is fully inside the other, for example subtracting a small sphere from a large sphere, the operation returned the larger sphere unchanged instead of producing the larger sphere with an internal hole.

Changes & Results

Intersection logic (polylineIntersect.ts):

• B fully inside A → intersection is B (was returning empty segment)
• A fully inside B → intersection is A (was returning empty segment)

Subtract logic (polylineSubtract.ts):

• B fully inside A → cut a hole in A (the original bug)
• A fully inside B → remove A entirely (return nothing now)
• B inside an existing hole of A → no-op (subtracting from empty space)

Pipeline (logicalOperators.ts, addPolylinesToSegmentation.ts, polylineInfoTypes.ts):

• Added holePolylines field to PolylineInfoWorld and PolylineInfoCanvas
• Load child annotation (hole) polylines when reading a segment before any operation
• Carry holes through all canvas ↔ world space conversions
• Save resulting holes as proper child annotations with correct winding direction

Testing

1. Open a study with two overlapping contour segmentations (e.g., StudyInstanceUIDs=1.2.840.113619.2.290.3.3767434740.226.1600859119.501) and hydrate RTSTRUCT series
2. Clicks on Combine Contour
   Perform the following operations:
   a. Large box intersecting ANY of the other (non large box) segments produces the inner geometry
   b. Big sphere intersect the small sphere produces the small sphere.
   c. Big sphere subtract the small sphere produces the big sphere with a hole the size of the small sphere.

Or you can do manual testing by drawing segments and perform Combine Contour operations on them (Intersect and subtract)

Checklist

PR

• My Pull Request title is descriptive, accurate and follows the
  semantic-release format and guidelines.

Code

• My code has been well-documented (function documentation, inline comments,
  etc.)

Public Documentation Updates

• [] The documentation page has been updated as necessary for any public API
  additions or removals.

Tested Environment

• OS: macOS 10.15.4
• Node version: v22.13.0
• Browser: Chrome 83.0.4103.116

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[jbocce]

The comment is a little confusing to me. In this case target is not necessarily a hold inside source right? Would a better comment be // Target is fully contained inside source (no edge crossings)?

[GhadeerAlbattarni]

Yes, comment updated.

[jbocce]

Since addPolylinesToSegmentation is the main, exported function, let's move this one below it. It makes for a better diff in the PR too.

[wayfarer3130]

At a minimum, with the current performance, a progress bar is going to be required since I had assumed it had crashed on some examples I ran, but figured out if I left it a minute or two it eventually completed.
The performance generally need to be improved to make this workable.
There are also various bugs identified by running /review on claude
The explanation of how to use it in the UI would also really help - it was not obvious that it didn't remove the old annotation, so I really didn't know exactly what it was doing. I'd suggest have a remove annotation button.
Probably doing it only on the current view, or every slice would also be helpful.

[wayfarer3130]

I had it just disappear the new annotation that was created - I had a sample that had multiple holes/overlaps in various ways and it just went away after clicking merge - took over a minute to go away.

[wayfarer3130]

Tested with:

on merge - it never returned anything, although that might have returned an empty segment index 3, but it never listed it in the side panel.

[wayfarer3130]

Claude review:

Code Review — cornerstone3D PR #2739

Title: fix(contour): fix intersect and subtract operations when one segment is fully inside another
Author: GhadeerAlbattarni · main ← fix/contour-combine-empty-segment
Scope: +191 / −37 across 6 files in packages/tools/src/utilities/contourSegmentation/
Link: #2739

Overview

Fixes OHIF/Viewers#5648: intersect and subtract returned wrong results when one contour segment fully contained another (no edge crossings). The PR:

1. Adds a third classification (isTargetInsideSource) to checkIntersection, alongside the existing isContourHole (= source inside target).
2. Uses that classification in intersectPolylinesSets (return the inner polygon) and subtractPolylineSets (cut a hole / remove entirely / no-op).
3. Plumbs holePolylines through PolylineInfoWorld/PolylineInfoCanvas, so child (hole) annotations survive the world ↔ canvas ↔ world round trip.
4. Reconstructs holes as proper child annotations in addPolylinesToSegmentation.ts.

The diagnosis and overall direction look correct. A few concrete issues below.

Issues & Suggestions

1. Hole winding direction is not enforced (likely renders incorrectly)

In addPolylinesToSegmentation.ts the parent gets targetWindingDirection: ContourWindingDirection.Clockwise, but the child hole call omits targetWindingDirection:

updateContourPolyline(
  holeAnnotation,
  { points: holePolylineCanvas, closed: true },   // no targetWindingDirection
  viewport
);

Compare with the established createPolylineHole helper in sharedOperations.ts, which deliberately flips the winding direction (Clockwise parent → CounterClockwise hole). The PR's holes will inherit whatever winding the canvas points happen to have — for the bug's "small sphere subtracted from big sphere" path the subtrahend is typically clockwise (same as the parent), so it will not render as a hole. Pass targetWindingDirection: ContourWindingDirection.CounterClockwise for the hole, mirroring createPolylineHole.

2. Existing holes in the minuend are ignored when polygons clip

In polylineSubtract.ts when edges cross, the code calls math.polyline.subtractPolylines(currentPolyline.polyline, polylineB.polyline) — passing only the outer ring, not currentPolyline.holePolylines. Existing holes are then re-filtered by "still fully inside the new piece," but if polylineB partially overlaps a hole the hole is silently dropped (it isn't fully inside any output piece) even though the part of the hole outside polylineB should remain. This is an edge case that the PR description doesn't claim to fix, but it's worth a comment so the next reader knows it's a limitation rather than an oversight.

3. isInsideExistingHole only checks "fully inside" a single hole

const isInsideExistingHole = existingHoles.some((hole) =>
  containsPoints(hole, polylineB.polyline)
);

If polylineB straddles a hole boundary (partly in the hole, partly in solid parent), this returns false and the code adds polylineB as a new hole, producing overlapping holes inside the same parent. Probably fine in practice (caller has already established containment via isTargetInsideSource), but worth a comment that this branch assumes "wholly in solid or wholly in a single hole."

4. Intersection ignores holes in the containing polygon

In polylineIntersect.ts:

• isContourHole branch returns { ...polyA } (polyA fully inside polyB) — correct, and spread carries holePolylines. Good.
• isTargetInsideSource branch returns { ...polyB } (polyB fully inside polyA) — but if polyA has a hole that polyB overlaps, the true intersection is polyB minus that hole. Currently the holes from polyA are dropped entirely. Same severity as Docs/exports #2: a real but narrow corner case, worth a // TODO.

5. checkIntersection flags are not mutually exclusive in name only

In sharedOperations.ts: isContourHole and isTargetInsideSource are derived independently then isTargetInsideSource adds !isContourHole. Works, but a one-line doc on the return shape would help future readers — particularly noting that "source/target" here means "first arg / second arg" (most call sites pass polyA/polyB where the source is the minuend in subtract, but the parameter names in the function are reversed-feeling). Even better, return a single discriminated union like relationship: 'edges-cross' | 'target-inside-source' | 'source-inside-target' | 'disjoint' — easier to reason about in callers than three booleans.

6. Helper extraction silently adds cachedStats: {} to the annotation shape

The extracted createContourSegmentationAnnotation helper in addPolylinesToSegmentation.ts sets cachedStats: {}, which the inlined annotation it replaced did not. Adding it is almost certainly an improvement (the type expects it), but it's an unannounced behavioral delta from the previous output. Worth a one-line note in the PR body.

7. No automated test coverage

The PR description lists only manual testing steps. Geometric set ops are exactly the kind of code where a regression months from now is silent and hard to spot. At minimum, add unit tests for checkIntersection covering the four cases (disjoint, edges cross, source inside target, target inside source) and for intersectPolylinesSets/subtractPolylineSets covering the containment scenarios called out in the PR description.

Smaller / Stylistic

• The pattern ...(holePolylines?.length ? { holePolylines: ... } : {}) is repeated four times across logicalOperators.ts and polylineSubtract.ts. A tiny withHoles(obj, holes) helper would centralize it. Optional.
• containsPoints(cleaned, hole) in polylineSubtract.ts is O(n·m) per pair and runs in a triple loop; fine for typical RT contours but worth keeping in mind if perf complaints surface.
• The inline comments ("Subtract polylines" / "It's already in a hole" / "Subtrahend is fully inside the minuend — cut a hole") are useful — keep them.

Risk

• Correctness of the targeted fix: the AABB-then-containment classification is correct and the new branches are placed in the right order (identical → edges-cross → target-inside-source → source-inside-target → disjoint).
• Rendering risk: issue chore: Reorganize build scripts for clarity #1 (hole winding) is the one to verify on screen before merging — if the screenshots in the PR description show a real hole in the donut after subtract, then updateContourPolyline is probably normalizing winding internally and the omission is benign; but explicitly setting CounterClockwise removes any doubt.
• Performance: negligible. Holes are usually 0–few per contour.
• Backwards compatibility: checkIntersection gains a new field; existing callers destructure only the fields they use, so adding a field is safe.

Recommendation

Approve after addressing #1 (hole winding) and adding at least one unit test for checkIntersection. The other points are non-blocking but worth a follow-up issue.

[wayfarer3130]

I suspect bug #1 above it the cause of disappearing segments, but it is so slow I can't really be sure.

Suggest trying this with clipper2-ts - I asked claude to try that, will report on how it works.

[wayfarer3130]

I would also look as part of this at fixing the general add/remove sections of existing segments - I could get very odd behaviour as to whether it xored, merged, subtracted or did something else when I drew another overlapping item - I hadn't known exactly what to expect on the previous operation, and it took so long that I thought it was getting mixed up with the operation after I hit the merge.