feat: add ClickSegmentTool for click-to-segment lesion segmentation#2780
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a one-click PET lesion segmentation feature: VOI intensity mapping utilities, adaptive grow-cut region probing, a lazy 3D flood-fill algorithm, labelmap commit and island-removal helpers, a ChangesOne-Click PET Lesion Segmentation
Estimated code review effort: 5 (Critical) | ~120 minutes Demo Helper Utilities
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ClickSegmentTool
participant probeAdaptiveRegion
participant runFloodFillSegmentation
participant IslandRemoval
participant Labelmap
User->>ClickSegmentTool: hover over pixel
ClickSegmentTool->>probeAdaptiveRegion: dry-run confinement probe
probeAdaptiveRegion-->>ClickSegmentTool: viability, band, context
ClickSegmentTool-->>User: cursor verdict (plus/blocked/pending)
User->>ClickSegmentTool: click to segment
ClickSegmentTool->>probeAdaptiveRegion: resolve intensity band
probeAdaptiveRegion-->>ClickSegmentTool: band, expandContext
ClickSegmentTool->>runFloodFillSegmentation: fillWithRange(seed, band)
runFloodFillSegmentation->>IslandRemoval: clean up islands
IslandRemoval-->>runFloodFillSegmentation: modified slices
runFloodFillSegmentation->>Labelmap: commit voxel masks
Labelmap-->>ClickSegmentTool: segmentation modified event
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Self-contained one-click segmentation tool: hover previews segmentability (plus/blocked/evaluating cursor), a single click derives a one-sided intensity threshold from the click and floods it in 3D, and expand/shrink step the result along its measured growth curve. Brings only OneClickSegment and its necessities on top of main: - New flood-fill engine (runFloodFillSegmentation, floodFillSliceLazy, commitSliceMasksToLabelmap, createEnsureSliceLoadedForVolume) and the config-less adaptive-region strategy (adaptiveRegionIntensityRange). - A standalone floodFillIslandRemoval + local neighborhoodStats so main's shared islandRemoval/GrowCutBaseTool/calculateNeighborhoodStats are left untouched (OneClickSegment adapts to main's async GrowCutBaseTool). - core: viewportVoiIntensityMapping + growCutLog; additive utility exports. - Example + unit tests; additive demo-helper support (async dropdown options, validateAndSortVolumeIds). Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
ede7b8b to
857c5f6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (4)
utils/demo/helpers/addDropdownToToolbar.ts (1)
170-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
applyDropdownOptionsin the error path instead of duplicating clear logic.The
.catch()handler (Lines 174-184) manually clears and repopulates the<select>instead of going throughapplyDropdownOptions, so it doesn't get placeholder preservation,data-placeholder/data-loadingbookkeeping, ormapreset consistency that the success path gets. It also leavescurrentMappointing to whatever it was before (likelyundefined), which is fine here but is incidental rather than explicit.♻️ Suggested consolidation
.catch((error) => { console.error('addDropdownToToolbar: failed to resolve options', error); - const elErrorOption = document.createElement('option'); - elErrorOption.value = ''; - elErrorOption.innerText = 'Failed to load'; - elErrorOption.selected = true; - while (elSelect.options.length > 0) { - elSelect.remove(0); - } - elSelect.append(elErrorOption); + currentMap = undefined; + applyDropdownOptions(elSelect, { + values: [''], + labels: ['Failed to load'], + } as optionTypeDefaultValue & optionTypeValues); })Also consider marking the error option
disabledso it can't be mistaken for a real selectable choice, though this is low-impact since it's the sole option present.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utils/demo/helpers/addDropdownToToolbar.ts` around lines 170 - 193, Reuse applyDropdownOptions in the addDropdownToToolbar error path instead of manually clearing and repopulating the select. In the .catch() block, build the failed-state options and pass them through applyDropdownOptions so placeholder handling, data-placeholder/data-loading cleanup, and currentMap reset stay consistent with the success path. If you keep the fallback “Failed to load” option, make it disabled as well so it can’t be selected as a real value.packages/tools/src/utilities/segmentation/floodFillSliceLazy.ts (1)
252-259: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
queue.lengthinstead of rescanning full slice masks.Each
setVisitedis paired with aqueue.push, including the seed, soqueue.lengthis already the exact filled count. This avoids scanning every allocated full-frame mask after sparse fills or hover dry-runs.♻️ Proposed simplification
- let voxelCount = 0; - for (const arr of sliceMasks.values()) { - for (let i = 0; i < arr.length; i++) { - if (arr[i] & FLOOD_SLICE_FLAG_VISITED) { - voxelCount++; - } - } - } + const voxelCount = queue.length;🤖 Prompt for AI Agents
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/segmentation/floodFillSliceLazy.ts` around lines 252 - 259, The voxel count in the flood-fill logic is being recomputed by rescanning all slice masks, even though each `setVisited` already corresponds to a `queue.push`. Update the counting logic in `floodFillSliceLazy` to use `queue.length` as the filled voxel count instead of iterating over `sliceMasks.values()`, and keep the seed inclusion consistent with the existing queue behavior. Make sure the final count returned from this path reflects the queue size directly so sparse fills and hover dry-runs avoid the extra full-frame scan.packages/tools/test/utilities/segmentation/floodFillSliceLazy.jest.js (1)
10-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for lazy slice loading.
The current cases do not exercise
ensureSliceLoaded, so the streaming-slice contract could regress without test coverage.🧪 Suggested test case
+ it('loads slices before reading from them', async () => { + const loaded = new Set(); + const result = await floodFill3dSliceLazy( + (_x, _y, z) => (loaded.has(z) ? 1 : undefined), + [20, 20, 1], + { + width: SIZE, + height: SIZE, + depth: 3, + equals, + yieldEvery: 0, + maxDeltaIJ: 0, + ensureSliceLoaded: async (z) => { + loaded.add(z); + }, + } + ); + + expect(result.truncated).toBe(false); + expect(result.voxelCount).toBe(3); + expect([...loaded].sort((a, b) => a - b)).toEqual([0, 1, 2]); + });🤖 Prompt for AI Agents
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/test/utilities/segmentation/floodFillSliceLazy.jest.js` around lines 10 - 83, Add a regression test around floodFill3dSliceLazy that specifically verifies lazy slice loading via ensureSliceLoaded. The current budget/shape gate cases only assert fill behavior and truncation, so add a scenario that starts with unloaded slices and confirms the algorithm requests them through the getter/loader path during traversal. Use the existing floodFill3dSliceLazy test setup and unique symbols like ensureSliceLoaded, getter, and floodFill3dSliceLazy to place the new assertion where the streaming-slice contract can be validated.packages/tools/examples/oneClickSegment/index.ts (1)
168-170: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid the
innerHTMLsink in the legend.The values are hard-coded today, but this still trips the unsafe-HTML rule. Build the SVG/text nodes with DOM APIs so future label changes cannot turn this into an injection sink.
🤖 Prompt for AI Agents
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/examples/oneClickSegment/index.ts` around lines 168 - 170, The legend rendering in the oneClickSegment example uses the unsafe innerHTML sink on item, which should be replaced with DOM construction even though the current values are hard-coded. Update the legend-building logic around the item assignment to create the SVG and any text content with DOM APIs, appending nodes directly instead of concatenating HTML, so future label changes cannot become an injection sink.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
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/tools/annotation/OneClickSegmentTool.ts`:
- Around line 1123-1165: The refill path in refillAtTolerance is not
transactional: it clears the previously accepted voxels from
lastClick.filledPoints before fillWithRange succeeds, so a later failure can
leave the labelmap empty. Update the OneClickSegmentTool refill flow to either
stage the new fill and commit it only after success, or restore the prior
segment state from lastClick.filledPoints in the catch path before rethrowing.
Make sure the rollback/commit logic is kept near the existing segmentIndex
clearing loop and the fillWithRange call, and that lastClick.filledPoints only
gets replaced once the refill has succeeded.
- Around line 917-933: The fill path in OneClickSegmentTool’s `fillWithRange`
can still run after the cached referenced volume has been evicted, so
`indexToWorld` and `computeVoxelBudget` may receive `undefined`. Add a guard
immediately after `cache.getVolume(referencedVolumeId)` and before building the
flood-fill arguments, and bail out or abort if the volume is missing. Keep the
check near the existing `runFloodFillSegmentation` call so all later
expand/refresh paths are protected.
- Around line 386-391: The shape gate in OneClickSegmentTool’s k-bounds check is
treating negative maxDeltaK values like a real limit, which causes all regions
to be rejected. Update the predicate in the function that returns the
bbox/voxelCount filter so it only applies the k-extent comparison when maxDeltaK
is non-negative, matching the behavior used by floodFill3dSliceLazy and
preserving -1 as unbounded.
In `@packages/tools/src/utilities/segmentation/commitSliceMasksToLabelmap.ts`:
- Around line 41-44: Validate the labelmap dimensions at the start of
commitSliceMasksToLabelmap before any voxel writes. Compare the caller-provided
width/height-derived frameSize against labelmapVolume.dimensions and ensure the
computed expectedLen matches the volume’s linear size; if they diverge,
throw/fail fast before entering the dense or fallback write paths. Keep the
check near the existing depth/frameSize/expectedLen setup so
commitSliceMasksToLabelmap and its voxelManager offset writes cannot proceed
with mismatched dimensions.
In `@packages/tools/src/utilities/segmentation/floodFillIslandRemoval.ts`:
- Around line 335-344: The external-island clearing logic in
floodFillIslandRemoval.ts is checking sourceVoxelManager instead of the current
preview state, so newly painted preview voxels over background are skipped.
Update the callback inside the island-removal pass to read the value from
previewVoxelManager before clearing, and only use sourceVoxelManager as a
secondary guard to avoid counting inherited/source voxels as cleared. Keep the
fix localized to the callback that iterates rle ranges and updates
clearedVoxels.
In
`@packages/tools/src/utilities/segmentation/growCut/intensityRange/adaptiveRegionIntensityRange.ts`:
- Line 3: The import in adaptiveRegionIntensityRange is using a non-existent
NumberVoxelManager export, which breaks compilation. Update the type import to
use the generic VoxelManager<T> from '`@cornerstonejs/core/utilities`' and adjust
the related type annotations in this module so the existing getAtIndex/getAtIJK
calls continue to type-check against VoxelManager. Use the
adaptiveRegionIntensityRange function and any helper types in this file to
locate and replace the incorrect NumberVoxelManager reference.
In `@packages/tools/src/utilities/segmentation/growCut/neighborhoodStats.ts`:
- Line 2: The neighborhoodStats module has the same broken NumberVoxelManager
import and will not compile independently. Update the import in
neighborhoodStats.ts to use the correct export/source that matches the rest of
the growCut utilities, and verify the local references in neighborhoodStats stay
consistent after the change. Use the neighborhoodStats module name and the
NumberVoxelManager symbol to locate and fix the import.
In
`@packages/tools/src/utilities/segmentation/growCut/runFloodFillSegmentation.ts`:
- Line 758: The onCommitted callback in runFloodFillSegmentation currently
reports the pre-cleanup floodedPoints, but island cleanup can change the final
painted voxel set afterward. Update the callback to emit the final
committed/painted point set produced after island removal and any promotion
logic, using the existing runFloodFillSegmentation flow so OneClickSegmentTool
stores the correct points for later shrink/expand cleanup.
- Around line 771-788: The cancellation path in runFloodFillSegmentation
currently promotes and returns a partial labelmap result, which should not
happen. In the runFloodFillSegmentation flow, check options.isCancelled()
immediately after the flood-fill work and before any call to
commitSliceMasksToLabelmapVolume or promotePreviewSegmentToFinal, then return
null without mutating labelmap. Keep the cancellation handling localized to the
flood-fill/commit logic so a user-triggered cancel never persists partial
segmentation.
In `@utils/demo/helpers/addDropdownToToolbar.ts`:
- Around line 34-40: Guard the dropdown option lookup in addDropdownToToolbar so
explicit values that are not present in map do not crash when selecting the
default. In addDropdownToToolbar, before using map.get(value) and setting
selected, verify the entry exists or skip/continue when it is undefined; apply
the same defensive check in the default-selection path that uses
defaultValue/defaultIndex. This keeps the toolbar setup safe even when
optionTypeValues diverges from map keys.
In `@utils/demo/helpers/validateAndSortVolumeIds.js`:
- Around line 55-59: The validation in validateAndSortVolumeIds is only checking
array shape, which lets invalid numeric IPP/IOP metadata reach projectOntoNormal
and be treated as valid. Tighten the checks in validateAndSortVolumeIds and the
related projection path so firstPlane.imagePositionPatient must contain exactly
3 finite numbers and firstIop must contain exactly 6 finite numbers before any
sorting/projection logic runs. Apply the same guard in the later validation
block referenced by the review so NaN-based results cannot pass through as
valid.
- Around line 41-44: The validateAndSortVolumeIds flow is passing the
caller-owned imageIds array directly into sortImageIdsAndGetSpacing, which can
mutate it for wadouri inputs. Update the validateAndSortVolumeIds logic to pass
a copy of imageIds into sortImageIdsAndGetSpacing while keeping sortedImageIds
assignment unchanged, so the caller’s original order is preserved.
In `@utils/ExampleRunner/example-info.json`:
- Line 368: The example description in the example-info registry does not match
the actual PET-only workflow. Update the description entry for the PET lesion
segmentation example so it refers to PET only, and keep the wording aligned with
the example’s single-series loading behavior in the same registry entry.
---
Nitpick comments:
In `@packages/tools/examples/oneClickSegment/index.ts`:
- Around line 168-170: The legend rendering in the oneClickSegment example uses
the unsafe innerHTML sink on item, which should be replaced with DOM
construction even though the current values are hard-coded. Update the
legend-building logic around the item assignment to create the SVG and any text
content with DOM APIs, appending nodes directly instead of concatenating HTML,
so future label changes cannot become an injection sink.
In `@packages/tools/src/utilities/segmentation/floodFillSliceLazy.ts`:
- Around line 252-259: The voxel count in the flood-fill logic is being
recomputed by rescanning all slice masks, even though each `setVisited` already
corresponds to a `queue.push`. Update the counting logic in `floodFillSliceLazy`
to use `queue.length` as the filled voxel count instead of iterating over
`sliceMasks.values()`, and keep the seed inclusion consistent with the existing
queue behavior. Make sure the final count returned from this path reflects the
queue size directly so sparse fills and hover dry-runs avoid the extra
full-frame scan.
In `@packages/tools/test/utilities/segmentation/floodFillSliceLazy.jest.js`:
- Around line 10-83: Add a regression test around floodFill3dSliceLazy that
specifically verifies lazy slice loading via ensureSliceLoaded. The current
budget/shape gate cases only assert fill behavior and truncation, so add a
scenario that starts with unloaded slices and confirms the algorithm requests
them through the getter/loader path during traversal. Use the existing
floodFill3dSliceLazy test setup and unique symbols like ensureSliceLoaded,
getter, and floodFill3dSliceLazy to place the new assertion where the
streaming-slice contract can be validated.
In `@utils/demo/helpers/addDropdownToToolbar.ts`:
- Around line 170-193: Reuse applyDropdownOptions in the addDropdownToToolbar
error path instead of manually clearing and repopulating the select. In the
.catch() block, build the failed-state options and pass them through
applyDropdownOptions so placeholder handling, data-placeholder/data-loading
cleanup, and currentMap reset stay consistent with the success path. If you keep
the fallback “Failed to load” option, make it disabled as well so it can’t be
selected as a real value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7370a71a-c646-4e16-a162-62ec5ab4e983
📒 Files selected for processing (23)
packages/core/src/utilities/index.tspackages/core/src/utilities/logger.tspackages/core/src/utilities/viewportVoiIntensityMapping.tspackages/tools/examples/oneClickSegment/index.tspackages/tools/src/index.tspackages/tools/src/tools/annotation/OneClickSegmentTool.tspackages/tools/src/tools/annotation/regionSegmentHoverCursors.tspackages/tools/src/tools/index.tspackages/tools/src/utilities/segmentation/commitSliceMasksToLabelmap.tspackages/tools/src/utilities/segmentation/createEnsureSliceLoadedForVolume.tspackages/tools/src/utilities/segmentation/floodFillIslandRemoval.tspackages/tools/src/utilities/segmentation/floodFillSliceLazy.tspackages/tools/src/utilities/segmentation/growCut/floodFillIntensityRangeTypes.tspackages/tools/src/utilities/segmentation/growCut/getViewportVoiMappingForVolume.tspackages/tools/src/utilities/segmentation/growCut/intensityRange/adaptiveRegionIntensityRange.tspackages/tools/src/utilities/segmentation/growCut/neighborhoodStats.tspackages/tools/src/utilities/segmentation/growCut/runFloodFillSegmentation.tspackages/tools/test/utilities/segmentation/adaptiveRegionIntensityRange.jest.jspackages/tools/test/utilities/segmentation/floodFillSliceLazy.jest.jsutils/ExampleRunner/example-info.jsonutils/demo/helpers/addDropdownToToolbar.tsutils/demo/helpers/index.jsutils/demo/helpers/validateAndSortVolumeIds.js
| const callback = (index, rle) => { | ||
| const [, jPrime, kPrime] = segmentSet.toIJK(index); | ||
| if (rle.value !== SegmentationEnum.ISLAND) { | ||
| for (let iPrime = rle.start; iPrime < rle.end; iPrime++) { | ||
| const clearPoint = toIJK([iPrime, jPrime, kPrime]); | ||
| const sourceVal = sourceVoxelManager.getAtIJKPoint(clearPoint); | ||
| if (sourceVal === this.previewSegmentIndex) { | ||
| previewVoxelManager.setAtIJKPoint(clearPoint, null); | ||
| clearedVoxels += 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the preview value before clearing external islands.
Line 340 reads from sourceVoxelManager, so newly added preview voxels over background are never cleared. Use the current preview value, and keep the source check only to avoid counting inherited/source voxels as cleared.
🐛 Proposed fix
const clearPoint = toIJK([iPrime, jPrime, kPrime]);
+ const previewVal = previewVoxelManager.getAtIJKPoint(clearPoint);
const sourceVal = sourceVoxelManager.getAtIJKPoint(clearPoint);
- if (sourceVal === this.previewSegmentIndex) {
+ if (
+ previewVal === this.previewSegmentIndex &&
+ sourceVal !== this.previewSegmentIndex
+ ) {
previewVoxelManager.setAtIJKPoint(clearPoint, null);
clearedVoxels += 1;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const callback = (index, rle) => { | |
| const [, jPrime, kPrime] = segmentSet.toIJK(index); | |
| if (rle.value !== SegmentationEnum.ISLAND) { | |
| for (let iPrime = rle.start; iPrime < rle.end; iPrime++) { | |
| const clearPoint = toIJK([iPrime, jPrime, kPrime]); | |
| const sourceVal = sourceVoxelManager.getAtIJKPoint(clearPoint); | |
| if (sourceVal === this.previewSegmentIndex) { | |
| previewVoxelManager.setAtIJKPoint(clearPoint, null); | |
| clearedVoxels += 1; | |
| } | |
| const callback = (index, rle) => { | |
| const [, jPrime, kPrime] = segmentSet.toIJK(index); | |
| if (rle.value !== SegmentationEnum.ISLAND) { | |
| for (let iPrime = rle.start; iPrime < rle.end; iPrime++) { | |
| const clearPoint = toIJK([iPrime, jPrime, kPrime]); | |
| const previewVal = previewVoxelManager.getAtIJKPoint(clearPoint); | |
| const sourceVal = sourceVoxelManager.getAtIJKPoint(clearPoint); | |
| if ( | |
| previewVal === this.previewSegmentIndex && | |
| sourceVal !== this.previewSegmentIndex | |
| ) { | |
| previewVoxelManager.setAtIJKPoint(clearPoint, null); | |
| clearedVoxels += 1; | |
| } |
🤖 Prompt for AI Agents
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/segmentation/floodFillIslandRemoval.ts` around
lines 335 - 344, The external-island clearing logic in floodFillIslandRemoval.ts
is checking sourceVoxelManager instead of the current preview state, so newly
painted preview voxels over background are skipped. Update the callback inside
the island-removal pass to read the value from previewVoxelManager before
clearing, and only use sourceVoxelManager as a secondary guard to avoid counting
inherited/source voxels as cleared. Keep the fix localized to the callback that
iterates rle ranges and updates clearedVoxels.
A click and each expand/shrink step now push one LabelmapMemo onto the segmentation history stack: runFloodFillSegmentation gained a historyVoxelManager option so the flood commit, preview promotion and island removal all write through the memo's RLE history layer, and expand/shrink records its clear+refill as a single undoable step. The example gets Undo/Redo buttons plus Ctrl/Cmd+Z, Shift+Z and Ctrl+Y. Also folds in review feedback on these code paths: guard against an evicted referenced volume before filling, restore the cleared voxels in place when a refill fails (no more empty labelmap on a blocked expand), never commit a cancelled fill, report the post-island-removal point set to onCommitted, validate labelmap vs mask dimensions before writing linear offsets, honor negative maxDeltaK as unbounded in the shape gate, and use VoxelManager<number> instead of the non-exported NumberVoxelManager core alias.
- floodFillSliceLazy: derive voxelCount from the BFS queue length instead of rescanning every allocated slice mask; add a regression test for lazy slice loading via ensureSliceLoaded - adaptiveRegionIntensityRange / neighborhoodStats: import the exported VoxelManager<number> type instead of the non-existent NumberVoxelManager core export (fixes the CI type error) - floodFillIslandRemoval: document why external island clearing reads the write-through source and clears with null (the suggested preview-value check would break the history-layer wiring) - addDropdownToToolbar: guard map lookups against missing entries and reuse applyDropdownOptions in the async error path - validateAndSortVolumeIds: pass a copy so the sorter cannot mutate the caller's imageIds; require finite IPP/IOP values before projection - example-info: the oneClickSegment example is PET-only
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
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/segmentation/growCut/runFloodFillSegmentation.ts`:
- Around line 788-816: `committedPoints` is only refreshed after island removal
in the `usePreview` path, so when preview is disabled it still reports the
pre-cleanup `floodedPoints`. Update `runFloodFillSegmentation` so the final
points passed to `onCommitted` always reflect the post-cleanup painted voxels,
regardless of `usePreview`, by recomputing or assigning `committedPoints` after
`removeInternalIslands()` / `removeExternalIslands()` in the main flood-fill
flow. Keep the fix in the `runFloodFillSegmentation` callback path that
`OneClickSegmentTool` consumes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c54a247-f44d-419d-a717-9656b5c5b3b7
📒 Files selected for processing (13)
packages/tools/examples/oneClickSegment/index.tspackages/tools/src/tools/annotation/OneClickSegmentTool.tspackages/tools/src/utilities/segmentation/commitSliceMasksToLabelmap.tspackages/tools/src/utilities/segmentation/floodFillIslandRemoval.tspackages/tools/src/utilities/segmentation/floodFillSliceLazy.tspackages/tools/src/utilities/segmentation/growCut/intensityRange/adaptiveRegionIntensityRange.tspackages/tools/src/utilities/segmentation/growCut/neighborhoodStats.tspackages/tools/src/utilities/segmentation/growCut/runFloodFillSegmentation.tspackages/tools/test/utilities/segmentation/commitSliceMasksToLabelmap.jest.jspackages/tools/test/utilities/segmentation/floodFillSliceLazy.jest.jsutils/ExampleRunner/example-info.jsonutils/demo/helpers/addDropdownToToolbar.tsutils/demo/helpers/validateAndSortVolumeIds.js
🚧 Files skipped from review as they are similar to previous changes (10)
- utils/ExampleRunner/example-info.json
- packages/tools/src/utilities/segmentation/growCut/neighborhoodStats.ts
- packages/tools/src/utilities/segmentation/floodFillIslandRemoval.ts
- packages/tools/src/utilities/segmentation/commitSliceMasksToLabelmap.ts
- utils/demo/helpers/validateAndSortVolumeIds.js
- packages/tools/src/utilities/segmentation/floodFillSliceLazy.ts
- packages/tools/src/utilities/segmentation/growCut/intensityRange/adaptiveRegionIntensityRange.ts
- packages/tools/examples/oneClickSegment/index.ts
- utils/demo/helpers/addDropdownToToolbar.ts
- packages/tools/src/tools/annotation/OneClickSegmentTool.ts
…t mode The previous fix only refreshed committedPoints in the preview path. Track the exact points removeInternalIslands paints (a value-scan of the modified slices would over-collect same-segment voxels from earlier operations) and concat them into the reported set in both preview and direct-paint modes; the preview promotion now reuses that same set, replacing the collectPreviewPointsOnSlices slice scan.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/test/utilities/segmentation/floodFillIslandRemoval.jest.js (1)
55-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest-created DOM element is never removed.
createViewportappends an element todocument.bodybut nothing removes it, even inafterEach. Harmless for a single test but will accumulate stray nodes if more tests are added to this suite later.♻️ Suggested cleanup
afterEach(() => { cache.purgeCache(); renderingEngine?.destroy(); + viewport?.element?.remove(); });Also applies to: 107-125
🤖 Prompt for AI Agents
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/test/utilities/segmentation/floodFillIslandRemoval.jest.js` around lines 55 - 58, The test setup in createViewport leaves behind the DOM element it appends to document.body, so the suite should explicitly clean it up in afterEach alongside cache.purgeCache() and renderingEngine?.destroy(). Update the test helper(s) used in floodFillIslandRemoval.jest.js to retain a reference to the created element and remove it from the DOM during teardown, including the other affected createViewport usages in the suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tools/test/utilities/segmentation/floodFillIslandRemoval.jest.js`:
- Around line 55-58: The test setup in createViewport leaves behind the DOM
element it appends to document.body, so the suite should explicitly clean it up
in afterEach alongside cache.purgeCache() and renderingEngine?.destroy(). Update
the test helper(s) used in floodFillIslandRemoval.jest.js to retain a reference
to the created element and remove it from the DOM during teardown, including the
other affected createViewport usages in the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ade3d66-edc7-4b00-acac-600a5ea5452a
📒 Files selected for processing (3)
packages/tools/src/utilities/segmentation/floodFillIslandRemoval.tspackages/tools/src/utilities/segmentation/growCut/runFloodFillSegmentation.tspackages/tools/test/utilities/segmentation/floodFillIslandRemoval.jest.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tools/src/utilities/segmentation/floodFillIslandRemoval.ts
- packages/tools/src/utilities/segmentation/growCut/runFloodFillSegmentation.ts
sedghi
changed the title
sedghi
changed the title
sedghi
changed the title
The 'one-click' name overpromised — the tool only segments coherent, contrast-distinct, lesion-scale regions, not arbitrary clicks. Rename the class, toolName, example, and docs to ClickSegmentTool / clickSegment. Shared growcut utilities (runOneClickGrowCut) are unrelated and untouched.
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
I've looked over it generally and am happy with it, but did find a few things from clod:
Code correctness & risks
growCutLog.setLevel('info') in core logger.ts is a global side effect at import time. [logger.ts]. This forces info-level logging for the growCut logger regardless of the host app's configuration, and runFloodFillSegmentation emits many log.info calls per click/expand plus ungated console.time/console.timeEnd/console.warn/console.info. In a library this is real console/perf noise in production. Recommend defaulting to warn (or leaving level to the consumer) and gating the console.time* timing labels behind the existing ENABLE_VERBOSE_FLOOD_FILL_LOGS flag.
floodFillIslandRemoval.ts (519 lines) is a fork of the existing islandRemoval.ts (383 lines). Same SegmentationEnum, same IslandRemoval class and algorithm, with added features (getInternalFilledPoints, usingPreviewLayer, no-preview warning, verbose logging). Two near-identical island-removal implementations in the same directory is a maintenance hazard — divergent bug fixes are almost guaranteed. Strongly prefer extending the existing class (or adding the new hooks to it) over forking. The base tool still imports the old one (../../utilities/segmentation/islandRemoval), so both will live on indefinitely.
Oblique-view path throws from preMouseDownCallback. super.preMouseDownCallback throws 'Oblique view is not supported yet' and this override doesn't wrap it, producing a rejected promise on click. Hover already blocks oblique via _isOrthogonalView → BLOCKED_CURSOR, so in practice the click is gated, but a click without a fresh probe (e.g. probe stale/expired) could still reach this throw. Consider catching it and calling notifySegmentationError for a clean UX.
validateAndSortVolumeIds is added and exported but unused by the new example, which sorts with csUtils.sortImageIdsAndGetSpacing directly. Either wire it into the example (it's the more robust validator) or drop it from this PR — as-is it's dead weight with no exercising caller.
Performance
Hover is heavier than it looks. Each throttled hover flush runs probeAdaptiveRegion (a ~193×193 window sweep + bucket-queue flood) once for the pointer plus up to 4 more times in neighborsAgree — ~5 full window analyses synchronously on the main thread per flush, before the async 3D dry-run. On large/fine-spaced volumes this is a lot of redundant per-hover compute. Consider caching the center probe result and/or sampling neighbors more cheaply (the neighbor votes don't need the full growth-curve sweep).
…ver cost - Leave growCutLog level to the consumer (no import-time setLevel) and gate the flood-fill console.time/timeEnd diagnostics behind the existing ENABLE_VERBOSE_FLOOD_FILL_LOGS flag; bare console.warn calls now go through the logger. - Rework floodFillIslandRemoval as a subclass of the existing IslandRemoval instead of a fork: the base algorithm lives in one place, with two small hooks (protected fill options and an onInternalPointFilled callback) so the subclass only carries the delta-safe external removal, painted-point tracking, and verbose logging. - Catch the unsupported-view throw from the base preMouseDownCallback so an oblique click surfaces a segmentation error instead of a rejected promise. - Wire validateAndSortVolumeIds into the clickSegment example so the helper has an exercising caller and the series is validated as a clean volume. - Record the click's 4 in-plane neighbor join levels during the adaptive probe's threshold sweep and read the hover consensus votes off that, so each hover flush runs one window analysis instead of five.
…t/one-click-segment
|
@wayfarer3130 Thanks for the careful pass — all five points were valid and are addressed in the latest push (also merged latest main, no conflicts):
All segmentation utility jest suites pass against the refactor (including the internal-fill tracking test now running through the subclass + base hook path). |
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
Thanks for the fixes.
2 participants
Context
Segmenting a PET lesion today means picking a strategy and tuning parameters
(disk radius, mean/percent thresholds, deltas) before you get a result you can
trust. This PR adds
ClickSegmentTool— a configuration-free,click-to-segment lesion tool built for PET-style hot lesions but
modality-agnostic. There is nothing to configure: no disk radii, no
strategies, no deltas.
The tool deliberately does not promise to segment anything you click: it
only accepts clicks on a coherent, contrast-distinct, lesion-scale region, and
the hover cursor tells you up front whether a click will land.
Changes & Results
New tool —
ClickSegmentTool(exported publicly from@cornerstonejs/tools):+cursor means a click here producesa meaningful segment; a blocked cursor means it will not (flat area, noise
speck, or an unbounded region), and a pending cursor means the region is
still being evaluated. Clicks on blocked spots are no-ops. Verdicts attach to
regions, not pixels, so the cursor does not flicker as you move within one
structure.
least as intense as the clicked structure and connected to it — so the
hottest core of a lesion is always included and there are no interior holes.
expand()/shrink()step deterministically along the growth curvemeasured at click time: each step clears the previous result and refills at
the next/previous stable threshold, so the segment visibly grows or shrinks
every step rather than silently no-op'ing.
one coherent, self-contained entity (compactness against its 3D bounding box,
plus a through-slice self-containment check). Large solid lesions pass;
sprawling webs (chained bones, noise) are rejected. There is no size or
extent cap.
LabelmapMemoon the segmentation history stack, so it steps throughclick operations the same way it does through brush strokes.
Supporting utilities (under
packages/tools/src/utilities/segmentation/):runFloodFillSegmentation— orchestrates the fill: dynamic band resolution,lazy 3D flood, commit, external/internal island removal, and optional
history recording via a
historyVoxelManageroption.floodFillSliceLazy— 3D BFS flood fill with per-slice visit masks andon-demand slice loading, with budget and shape-gate hooks.
adaptiveRegionIntensityRange— the config-free adaptive strategy (VOI-mappeddisplay bytes, seed snapping, one-sided band, quiet-run tolerance selection).
commitSliceMasksToLabelmap,createEnsureSliceLoadedForVolume,floodFillIslandRemoval,neighborhoodStats,getViewportVoiMappingForVolume.viewportVoiIntensityMappingutilities to map between display VOI andraw scalar intensity.
Demo & helpers:
clickSegmentexample (PET-only single viewport, hover-cursor legend,Shrink/Expand/Clear, Undo/Redo).
validateAndSortVolumeIdshelper (spatial slice ordering/validation) andaddDropdownToToolbarsupport for async (Promise) option sources.Testing
yarn run example clickSegmentand open the example.+cursor appears over lesions, a blockedcursor over background/flat areas.
each step; Undo / Redo (or Ctrl/Cmd+Z, Shift+Z, Ctrl+Y) step through
each operation.
Unit tests (Jest) added and passing:
adaptiveRegionIntensityRange,floodFillSliceLazy,floodFillIslandRemoval,commitSliceMasksToLabelmap.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit