Fix labelmap edit with contour issues - #2838
Open
arul-trenser wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change updates contour editing to use live segmentation state, enforce locked and hidden segment rules, filter annotations before conversion, clean temporary representations, and limit fallback activation to cases with no remaining active segmentation. ChangesContour segmentation editing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LabelmapEditWithContour
participant SegmentationState
participant LabelmapBaseTool
User->>LabelmapEditWithContour: start contour annotation
LabelmapEditWithContour->>SegmentationState: resolve active segmentation
SegmentationState-->>LabelmapEditWithContour: active segmentation state
LabelmapEditWithContour->>SegmentationState: create temporary contour representation
User->>LabelmapEditWithContour: complete contour
LabelmapEditWithContour->>LabelmapBaseTool: convert eligible annotations
LabelmapBaseTool->>SegmentationState: update labelmap
LabelmapEditWithContour->>SegmentationState: remove temporary contour state
Possibly related PRs
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 |
![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 (1)
packages/tools/src/tools/segmentation/LabelmapEditWithContour.ts (1)
400-407: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFilter by the specific completed annotation, not by tool name.
annotationFiltermatches every annotation created byLabelMapEditWithContourTool, not only the one that just completed. This relies onremoveContourscleanup elsewhere inviewportContoursToLabelmapto keep at most one such annotation pending at a time. Filtering directly byannotationUIDremoves that dependency and matches the stated objective ("only the newly created ROI should be converted to a labelmap") without relying on other code paths to stay precise.♻️ Proposed refactor
BrushTool.viewportContoursToLabelmap(viewport, { annotationFilter: (annotations) => annotations.filter( - (a) => - (a as ContourSegmentationAnnotation).metadata?.originalToolName === - LabelMapEditWithContourTool.toolName + (a) => a.annotationUID === annotationUID ), });🤖 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/tools/segmentation/LabelmapEditWithContour.ts` around lines 400 - 407, Update the annotationFilter in the BrushTool.viewportContoursToLabelmap call to retain only the completed annotation by matching each annotation’s annotationUID against the specific newly completed annotation’s UID, rather than filtering by LabelMapEditWithContourTool.toolName. Preserve conversion of only that newly created ROI and remove the dependency on removeContours cleanup.
🤖 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/stateManagement/segmentation/SegmentationStateManager.ts`:
- Around line 590-599: Update the fallback-activation guard in the state update
logic to inspect the draft state’s remaining representations directly rather
than calling the stale-state getter getActiveSegmentation. When
activeRepresentationRemoved is true, determine whether any remaining
representation in state.viewportSegRepresentations[viewportId] is active, and
activate the first one only when none are active.
In `@packages/tools/src/tools/segmentation/LabelmapBaseTool.ts`:
- Around line 743-760: Update the per-voxel segment handling in the
LabelmapBaseTool conversion loop to verify that currentSegmentIndex resolves to
an existing entry in the segmentation’s segments collection before calling
isSegmentIndexLocked. Treat orphaned indices as unlocked and visible so they are
skipped safely without throwing, while preserving the existing lock and
visibility checks for valid segments.
- Around line 605-636: Update viewportContoursToLabelmap so annotations are
first restricted to entries with a non-empty data.contour.polyline, then pass
that filtered set through options.annotationFilter when provided. Preserve the
existing default filtering behavior and ensure viewAnnotations cannot contain
annotations that later violate the unconditional contour/polyline access.
- Around line 705-736: Re-validate the fallback activeIndex in the segment
usability logic around isSegmentIndexLocked and getSegmentIndexVisibility before
writing the annotation. After redirecting from a locked or hidden segment,
re-check activeIndex and remove or skip the annotation when it is also locked or
hidden; preserve the existing behavior for usable fallback segments.
In `@packages/tools/src/tools/segmentation/LabelmapEditWithContour.ts`:
- Around line 119-142: Guard the active-segmentation flow before dereferencing
it: after segmentation.getActiveSegmentation(viewportId), return null when
activeSeg is undefined, and after
getActiveSegmentIndex(activeSeg.segmentationId), return null when activeSegIndex
is undefined. Only then call isSegmentIndexLocked, getSegmentIndexVisibility,
and checkContourSegmentation, preserving the existing locked/hidden behavior.
---
Nitpick comments:
In `@packages/tools/src/tools/segmentation/LabelmapEditWithContour.ts`:
- Around line 400-407: Update the annotationFilter in the
BrushTool.viewportContoursToLabelmap call to retain only the completed
annotation by matching each annotation’s annotationUID against the specific
newly completed annotation’s UID, rather than filtering by
LabelMapEditWithContourTool.toolName. Preserve conversion of only that newly
created ROI and remove the dependency on removeContours cleanup.
🪄 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: ced514e3-7a9e-493a-a0cd-106105569158
📒 Files selected for processing (3)
packages/tools/src/stateManagement/segmentation/SegmentationStateManager.tspackages/tools/src/tools/segmentation/LabelmapBaseTool.tspackages/tools/src/tools/segmentation/LabelmapEditWithContour.ts
6 tasks
arul-trenser
force-pushed
the
fix-labelmap-edit-with-contour-issues
branch
from
August 5, 2026 07:09
1f7c71f to
0312299
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@sedghi Could you please take a look at this PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Contour editing tool had few bugs:
The PR is incorporated by FlyWheel.io.
Fixes: #2837
Fixes: #2836
Fixes: #2833
Changes & Results
SegmentationStateManager.ts: Don't force-activate another representation if viewport already has an active one.LabelmapBaseTool.ts:viewportContoursToLabelmapnow takesannotationFilter, so only needed annotation is converted.doneEditMemo()so undo/redo works for labelmap edit.LabelmapEditWithContour.ts:createMemo()now does nothing, since labelmap memo already handles undo/redo.annotationCompletednow converts only the just-drawn contour, usingoriginalToolNamematchTesting
#2833
Before:
2833-before.mp4
After:
2833-after.mp4
#2836
Before:
Scenario 1:
2836-MPR1-before.mp4
Scenario 2:
2836-MPR2-before.mp4
After:
Scenario 1:
2836-MPR1-after.mp4
Scenario 2:
2836-MPR2-after.mp4
#2837
Before:
2837-before.mp4
After:
2837-after.mp4
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes