BLOCKED BY VTK MERGE FIRST DO NOT MERGE feat(segmentation): labelmap edge projection over MIP for generic planar viewports#2784
Open
sedghi wants to merge 4 commits into
Open
BLOCKED BY VTK MERGE FIRST DO NOT MERGE feat(segmentation): labelmap edge projection over MIP for generic planar viewports#2784sedghi wants to merge 4 commits into
sedghi wants to merge 4 commits into
Conversation
…wports Adds the labelmapMIPGeneric example and makes LABELMAP_EDGE_PROJECTION_BLEND work on generic planar viewports: the labelmap mounts as an overlay display set that inherits the source display set's slab thickness, and segmentation overlays keep their labelmap transfer functions instead of having a VOI grayscale ramp reapplied over them. Editing performance: segmentation-data modifications no longer re-render projection-heavy viewports live. triggerSegmentationRenderForModified renders cheap viewports immediately and defers viewports that re-march a slab per fragment (legacy edge-projection blend or generic slab presentations) until the modification stream goes quiet (240ms trailing). The generic volume-slice render path applies the same deferral to volume-modified repaints of slab overlays and drops its full-texture invalidation so brush edits upload only the modified slices. Legacy independent-components path fixes: the combined actor is re-added under the render plan's representationUID (the plan tore it down otherwise), restore removes the injected CPU scalars and label-outline property state, the data-modified merge now touches only modifiedSlicesToUse with direct per-slice copies instead of a full-volume per-voxel loop, and the debounced listener is removed with removeEventListenerDebounced (the plain removal leaked the wrapper, leaving zombie merges after delete/re-add cycles). The generic slab label outline requires vtk.js with the slab-projected label outline (Kitware/vtk-js#3541).
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughAdds debounced re-rendering for projection-heavy segmentation overlays, updates planar volume rendering to batch overlay refreshes and preserve overlay VOI behavior, propagates slab-thickness metadata to labelmap overlays, refines labelmap scalar restoration and partial-slice updates, and adds a labelmap-over-MIP example. ChangesSegmentation overlay projection rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (2)
packages/tools/src/stateManagement/segmentation/SegmentationRenderingEngine.ts (1)
304-304: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMagic delay duplicated across two files.
DEFERRED_SEGMENTATION_RENDER_DELAY_MS(240ms) here duplicatesPROJECTING_OVERLAY_RENDER_DELAY_MSinVtkVolumeSliceRenderPath.ts. Extracting a single shared constant would avoid the two debounce windows drifting apart if one is tuned later.Also applies to: 304-304
🤖 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/stateManagement/segmentation/SegmentationRenderingEngine.ts` at line 304, The segmentation render delay is duplicated as a magic value, so the debounce windows can drift apart if one is changed later. Extract a single shared constant for the 240ms delay and use it in both `DEFERRED_SEGMENTATION_RENDER_DELAY_MS` and `PROJECTING_OVERLAY_RENDER_DELAY_MS`, updating `SegmentationRenderingEngine` and `VtkVolumeSliceRenderPath` to reference the same symbol.packages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts (1)
101-135: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDebounce keys off
rendering.dataPresentation, which isundefineduntil the firstupdateDataPresentationcall.
isProjectingOverlayreadsrendering.dataPresentation?.slabThickness, butdataPresentationis initialized toundefinedat line 97 and only populated by a laterupdateDataPresentationcall. AnyIMAGE_VOLUME_MODIFIEDevents that fire before the first presentation update will bypass the debounce and render immediately, even for what will become a projecting overlay. This is a narrow startup-ordering edge case with a safe fallback (an extra immediate render), not a correctness break.🤖 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/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts` around lines 101 - 135, The debounce decision in VtkVolumeSliceRenderPath is reading rendering.dataPresentation before it is guaranteed to be initialized, so early IMAGE_VOLUME_MODIFIED events can skip the intended delayed render path. Update the event handler to base isProjectingOverlay on a value that is available before the first updateDataPresentation call, or defer the debounce decision until the presentation state has been set. Use the existing subscribeToVolumeEvents callback, renderings’s dataPresentation state, and the deferredProjectionRenderTimer logic to keep the projecting-overlay behavior consistent during startup.
🤖 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/examples/labelmapMIPGeneric/index.ts`:
- Around line 73-100: The "Add Labelmap Representation" handler currently always
calls addLabelmapRepresentationToViewport, which can create duplicates for the
same viewportId2 and segmentationId on repeated clicks. Update the onClick logic
to first check segmentation.state.getSegmentationRepresentations(viewportId2)
for an existing Labelmap representation matching segmentationId, and only call
addLabelmapRepresentationToViewport when none is present, similar to the guard
already used in the delete handler.
---
Nitpick comments:
In
`@packages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.ts`:
- Around line 101-135: The debounce decision in VtkVolumeSliceRenderPath is
reading rendering.dataPresentation before it is guaranteed to be initialized, so
early IMAGE_VOLUME_MODIFIED events can skip the intended delayed render path.
Update the event handler to base isProjectingOverlay on a value that is
available before the first updateDataPresentation call, or defer the debounce
decision until the presentation state has been set. Use the existing
subscribeToVolumeEvents callback, renderings’s dataPresentation state, and the
deferredProjectionRenderTimer logic to keep the projecting-overlay behavior
consistent during startup.
In
`@packages/tools/src/stateManagement/segmentation/SegmentationRenderingEngine.ts`:
- Line 304: The segmentation render delay is duplicated as a magic value, so the
debounce windows can drift apart if one is changed later. Extract a single
shared constant for the 240ms delay and use it in both
`DEFERRED_SEGMENTATION_RENDER_DELAY_MS` and
`PROJECTING_OVERLAY_RENDER_DELAY_MS`, updating `SegmentationRenderingEngine` and
`VtkVolumeSliceRenderPath` to reference the same symbol.
🪄 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: b61f7c1c-c2a6-4f86-86f7-e100a7b77f6f
📒 Files selected for processing (7)
packages/core/src/RenderingEngine/GenericViewport/Planar/VtkVolumeSliceRenderPath.tspackages/core/src/RenderingEngine/GenericViewport/Planar/planarRuntimeTypes.tspackages/tools/examples/labelmapMIPGeneric/index.tspackages/tools/src/eventListeners/segmentation/segmentationDataModifiedEventListener.tspackages/tools/src/stateManagement/segmentation/SegmentationRenderingEngine.tspackages/tools/src/tools/displayTools/Labelmap/addVolumesAsIndependentComponents.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.ts
Comment on lines
+73
to
+100
| addButtonToToolbar({ | ||
| title: 'Add Labelmap Representation', | ||
| onClick: () => { | ||
| segmentation.addLabelmapRepresentationToViewport(viewportId2, [ | ||
| { | ||
| segmentationId, | ||
| config: { | ||
| blendMode: BlendModes.LABELMAP_EDGE_PROJECTION_BLEND, | ||
| }, | ||
| }, | ||
| ]); | ||
| }, | ||
| }); | ||
|
|
||
| addButtonToToolbar({ | ||
| title: 'Delete Labelmap Representation', | ||
| onClick: () => { | ||
| const representations = | ||
| segmentation.state.getSegmentationRepresentations(viewportId2); | ||
|
|
||
| if (representations && representations.length > 0) { | ||
| segmentation.removeSegmentationRepresentation(viewportId2, { | ||
| segmentationId, | ||
| type: SegmentationRepresentations.Labelmap, | ||
| }); | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard "Add Labelmap Representation" against re-adding an existing representation.
The delete handler checks getSegmentationRepresentations(viewportId2) before removing, but the add handler unconditionally calls addLabelmapRepresentationToViewport even if a representation already exists for viewportId2/segmentationId, risking duplicate representations or errors on repeated clicks.
🔧 Proposed fix
addButtonToToolbar({
title: 'Add Labelmap Representation',
onClick: () => {
+ const representations =
+ segmentation.state.getSegmentationRepresentations(viewportId2);
+ if (representations && representations.length > 0) {
+ return;
+ }
segmentation.addLabelmapRepresentationToViewport(viewportId2, [
{
segmentationId,
config: {
blendMode: BlendModes.LABELMAP_EDGE_PROJECTION_BLEND,
},
},
]);
},
});🤖 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/labelmapMIPGeneric/index.ts` around lines 73 - 100,
The "Add Labelmap Representation" handler currently always calls
addLabelmapRepresentationToViewport, which can create duplicates for the same
viewportId2 and segmentationId on repeated clicks. Update the onClick logic to
first check segmentation.state.getSegmentationRepresentations(viewportId2) for
an existing Labelmap representation matching segmentationId, and only call
addLabelmapRepresentationToViewport when none is present, similar to the guard
already used in the delete handler.
Member
Author
|
@salimkanoun also speed improvements |
Contributor
|
Amazing, thanks! |
The merge with main made Viewport.flip protected, so casting IViewport to Partial<IVolumeViewport> now fails TS2352 (protected-vs-public flip overlap). Use the same minimal structural cast already used for the planar-method probe below it. Claude-Session: https://claude.ai/code/session_01NJNAWVWWtguh3aA8w1R9mp
6 tasks
sedghi
changed the title
sedghi
changed the title
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
Labelmap edge projection over MIP only worked on legacy volume viewports; generic planar viewports had no working labelmap-over-MIP path at all. This PR adds that capability, with a new
labelmapMIPGenericexample.The generic-viewport slab label outline relies on the slab-projected label outline added to the vtk.js
ImageResliceMapperin Kitware/vtk-js#3541 (which also carries a fix that keeps mapper fragment blocks compatible with render-pass postambles - early returns skipped the OIT weight write and composited the outlines white - and a slab-loop exit that keeps huge slab thicknesses proportional to the in-volume march). Until that vtk.js change is released, this PR is blocked; running the example requires a linked vtk.js build containing that PR.The editing-performance and legacy-path fixes that do not depend on vtk.js were split out into #2797, which can merge independently.
Changes & Results
Generic planar viewports (new capability + example
labelmapMIPGeneric)slabThickness, so labels project across the same slab as the MIP instead of reflecting only the current slice.Editing performance in the generic render path
Measured on the example (PET volume, Apple Silicon): brush strokes cost ~1.3 ms of main thread per pointer move, the projection viewport refreshes ~240 ms after the stroke ends, and rotating the generic MIP viewport with a whole-volume slab (50000) plus outline overlay renders at ~30 ms/frame.
Testing
pnpm run example labelmapMIPGeneric(requires linked vtk.js with feat(ImageResliceMapper): add LABELMAP_EDGE_PROJECTION_BLEND Kitware/vtk-js#3541): click "Add Labelmap Representation" - the MIP viewport should show outlines in the segment colors (not white). Draw on the left viewport with the brush: drawing stays responsive, and the MIP outline updates shortly after the stroke ends. Rotate the MIP viewport with the mouse wheel. Delete and re-add the representation.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment