-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
fix(segmentation overlay): update viewport ds list upon seg delete - OHIF-2425 #5729
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
-resolves segmentation missing from list (from when PACS) -safety check for missing representation (for when segmentation is deleted)
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Quick note that this only resolves for segmentations, RTStructs are still under investigation (is not able to be loaded a second time after the first delete). |
…ateViewportDSListUponSegDelete
…again in removeDisplaySetLayer)
|
Hi @jbocce, Latest commit for PR now correctly works for both segmentations and RTStructs. removeDisplaySetLayer handles segmentation representation so was not needed to be removed again. As the commands for delete segmentation and removeDisplaySetLayer are pretty close, would it make sense to incorporate the displaySetService.deleteDisplaySet(segmentationid) snippet to removeDisplaySetLayer (this should also fix nothing happening when a segmentation created within the ohif is removed as a layer from overlay menu unless that is desired behaviour)? |
extensions/cornerstone/src/services/SegmentationService/SegmentationService.ts
Outdated
Show resolved
Hide resolved
jbocce
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much for this. See my comments. Also as I noted in a private conversation with you please address removing a segmentation from a viewport as well. See the video...
ScreenHunter.Jan.29.10.34.mp4
…ateViewportDSListUponSegDelete
|
Added relevant code within removeSegmentationFromViewport as well; however, after pulling most recent commits (likely Jan. 27 ones) I'm running into a number of new issues with segmentations, will try to work through and apply suggestions noted in comments as well. An example of new issue is I'm sometimes finding segmentations previously removed added back in now (additionally, adding segmentations to one viewport will end up adding it to other viewports with images that share a FoR). |
…SListUponSegDelete
- add segmentationExists check to getSopClassHandlerModule - now firing SEGMENTATION_REMOVED and SEGMENTATION_REPRESENTATION_REMOVED events - centralized segmentation removal in a listener - when a segmentation is deleted (completely), remove it from all viewports it overlays - when a segmentation is removed from a viewport, remove it as overlay from the viewport
| eventTarget.removeEventListener( | ||
| csToolsEnums.Events.SEGMENTATION_REMOVED, | ||
| this._onSegmentationModifiedFromSource | ||
| this._onSegmentationRemovedFromSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes more sense than what was there before. Now we actually fire the SEGMENTATION_REMOVED event!
| eventTarget.removeEventListener( | ||
| csToolsEnums.Events.SEGMENTATION_REPRESENTATION_REMOVED, | ||
| this._onSegmentationRepresentationModifiedFromSource | ||
| this._onSegmentationRepresentationRemovedFromSource |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for this one.
| loadPromises[SOPInstanceUID] && | ||
| cachedRTStructsSEG.has(rtDisplaySet.displaySetInstanceUID) | ||
| cachedRTStructsSEG.has(rtDisplaySet.displaySetInstanceUID) && | ||
| _segmentationExists(rtDisplaySet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an RTSTRUCT contour segmentation was deleted completed and then reloaded it wouldn't. So this check forces the reload when it no longer exists.
|
|
||
| const displaySet = displaySetService.getDisplaySetByUID(segmentationId); | ||
| if (displaySet) { | ||
| commandsManager.runCommand('removeDisplaySetLayer', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is removeDisplaySetLayer? i can't find it in our codebase
Context
A potential solution for
#5728
(Also a test noted in #5498)
Overlay UI is missing segmentations when they are added then removed from the segmentation panel as it is still contained in the viewport's display set list.
Changes & Results
Segmentation displayset layer is removed as part of the segmentation delete routine. Additionally, for segmentations created within viewer and not from an image source, it is cleaned up from the overall display set list (so as not to clutter the UI).
Testing
User is now able to add segmentation via overlay UI, delete (whether by accident or on purpose), and still add it back to same viewport via overlay UI without refreshing page.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
System:
OS: Windows 11 10.0.26200
CPU: (16) x64 Intel(R) Core(TM) i7-10700 CPU @ 2.90GHz
Memory: 2.85 GB / 15.72 GB
Binaries:
Node: 22.16.0
Yarn: 1.22.22
npm: 10.9.2
Browsers:
Chrome: 144.0.7559.59
Edge: Chromium (143.0.3650.66)