fix(segmentation): restrict overlay segmentation menu to same frame of reference as viewport background display set #5900
Conversation
…f reference as viewport - Add FrameOfReferenceUID to SEG and RTSTRUCT displaySet in SOP Class Handlers so the FOR is available for filtering - Sync optimisticOverlayDisplaySets when background display set changes so the overlay menu reflects the correct state after a background switch - Add FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align.
…segmentation-menu-FOR
✅ Deploy Preview for ohif-dev canceled.
|
| return labels; | ||
| } | ||
|
|
||
| getActiveSegmentationLocator(label: string) { |
There was a problem hiding this comment.
I am not crazy about the word Locator in this method name. Maybe ask AI for a better name?
There was a problem hiding this comment.
I changed the name here :)
| page, | ||
| viewportPageObject, | ||
| }) => { | ||
| const viewportId = await viewportPageObject.getIdOfNth(0); |
There was a problem hiding this comment.
I am not crazy about this getIdOfNth method. Could we not simply use await viewportPageObject.getNth(0) and then get the overlay object for it?
| return this.viewportPageObjectFactory(viewport); | ||
| } | ||
|
|
||
| async getIdOfNth(index: number): Promise<string> { |
There was a problem hiding this comment.
See my comment in the spec about possibly removing this metody. In general Playwright strongly suggests to NOT assert and/or get attributes, text or data of any kind... https://playwright.dev/docs/api/class-locator#locator-get-attribute
| await page.waitForTimeout(2000); | ||
|
|
||
| const segmentationLabels = await dataOverlay.getDropdownOptionLabels(); | ||
| expect(segmentationLabels.sort()).toEqual([...segmentationLabelsFOR1].sort()); |
There was a problem hiding this comment.
Are we sorting these because at present there is a bug whereby the labels might be in different order?
| await this.page.getByText('SELECT A SEGMENTATION').click(); | ||
| } | ||
|
|
||
| async getDropdownOptionLabels(): Promise<string[]> { |
There was a problem hiding this comment.
There are two concerns I have about this method:
- It goes against Playwright's principle to not get data and assert it. So instead this should be replaced by a utility that asserts the labels
- It assumes that the drop down menu is open. Instead whether we keep this method or replace it with a utility, this function/method itself should open and close the menu to get and/or assert its data.
|
|
||
| await dataOverlay.toggle(viewportId); | ||
| await dataOverlay.openAddSegmentationDropdown(viewportId); | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
No need to wait, especially if we replace getDropdownOptionLabels to be a utility that does an expect which will do the waiting for us.
| await dataOverlay.toggle(viewportId); | ||
| await dataOverlay.changeBackgroundDisplaySet('CT Std (FoR 2)-CT', viewportId); | ||
| await dataOverlay.openAddSegmentationDropdown(viewportId); | ||
| await page.waitForTimeout(2000); |
There was a problem hiding this comment.
See comment above about waiting.
| await dataOverlay.addSegmentation(segmentationFOR1Label, FOR1ViewportIdA); | ||
| await expect(dataOverlay.getOverlaySegmentationRow(segmentationFOR1Label)).toBeVisible(); | ||
| await dataOverlay.closeMenu(FOR1ViewportIdA); | ||
| await page.waitForTimeout(2000); |
| } | ||
|
|
||
| async closeMenu(viewportId: string = 'default') { | ||
| await this.toggle(viewportId); |
There was a problem hiding this comment.
I think it is also best to wait for the menu to no longer be visible before returning. Again these are the principles of GUI testing.
jbocce
left a comment
There was a problem hiding this comment.
Thank you for this. See my comments.
When a second overlay (e.g. RTSTRUCT) was added after an existing one (e.g. SEG), the viewport re-render triggered addOverlayRepresentationForDisplaySet for all existing overlays in a fire-and-forget forEach. Because SEG (LABELMAP) goes through multiple async microtask hops (handleViewportConversion chain) before reaching Cornerstone3D state, while RTSTRUCT (CONTOUR) skips that path and reaches Cornerstone3D synchronously, RTSTRUCT was always inserted into CST first. When CST re-inserted SEG afterwards it moved it to the end, reversing the display order to [RTSTRUCT, SEG] instead of the user-defined [SEG, RTSTRUCT]. Fix by processing overlay additions sequentially: addOverlayRepresentationForDisplaySet now returns its Promise, and both setVolumesForViewport and _setStackViewport await each overlay in order before starting the next. This ensures CST receives the representations in the same order they appear in the viewport's display sets.
| @@ -70,6 +70,14 @@ const segmentationRepresentationModifiedCallback = async ( | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
@wayfarer3130 I think these too make sense, but I just wanted another pair of eyes on these changes.
| return; | ||
| } | ||
|
|
||
| const targetFrameOfReferenceUID = viewport.getFrameOfReferenceUID(); |
There was a problem hiding this comment.
This line should be above the if condition up above so you get the FOR once.
| const sourceFrameOfReferenceUID = | ||
| getEnabledElementByViewportId(sourceViewportId)?.viewport?.getFrameOfReferenceUID(); | ||
|
|
||
| if (!sharedDisplaySetExists && targetFrameOfReferenceUID !== sourceFrameOfReferenceUID) { |
There was a problem hiding this comment.
I think I would add a comment inside
// Display sets can be overlayed if they apply to the same base display set, eg for instance frame references in a seg to a display set,
// OR they can be overlayed if they share a common FOR (eg different image display and segmentation applies to multiple FORs.)
There was a problem hiding this comment.
Thank you @wayfarer3130 for your review. I noticed that at the top of this method, it comments what this is doing and it reflects your comment here. So I didn't add it inside again, however if you think I should, I would do that.
There was a problem hiding this comment.
Yes there is a comment at the top of the method, so I think it is fine as is.
| } | ||
|
|
||
| return viewport.setStack(imageIds, initialImageIndexToUse).then(() => { | ||
| return viewport.setStack(imageIds, initialImageIndexToUse).then(async () => { |
There was a problem hiding this comment.
This is already an async function, would prefer:
await viewport.setStack(....)
and then just leave the internal processing to handle afterwards rather than mixing async functions and .then usage.
I know it was previously related, but lets make it consistent now.
| @@ -1122,11 +1122,11 @@ class CornerstoneViewportService extends PubSubService implements IViewportServi | |||
| await viewport.setVolumes(volumeInputArray); | |||
|
|
|||
| if (overlayProcessingResults?.length) { | |||
There was a problem hiding this comment.
Please add the overlay processing results to a single method since it looks like it gets repeated.
| ); | ||
| // store the segmentation presentation id in the viewport info | ||
| this.storePresentation({ viewportId: viewport.id }); | ||
| return segmentationRepresentationPromise; |
There was a problem hiding this comment.
Please update the return type for this method to be Promise or void or something that indicates it returns a promise.
| */ | ||
| referencedDisplaySetInstanceUID?: string; | ||
|
|
||
| FrameOfReferenceUID?: string; |
There was a problem hiding this comment.
Since this is a computed field at least some of the time, not a display field, I'd prefer to try to move towards lowerCamelCase.
In fact I'd like to do that for SeriesDate/SeriesTime etc as well, and might work with that for OHIF in the next release.
There was a problem hiding this comment.
Also, you need a comment that this is the FOR that applies for every frame within this display set, and that it will be undefined otherwise.
|
Thank you so much @wayfarer3130 and @jbocce for reviewing this PR. I made the requested changes |
| StudyInstanceUID, | ||
| SOPClassHandlerId, | ||
| SOPClassUID, | ||
| frameOfReferenceUID: null, |
There was a problem hiding this comment.
why are we using frameOfReferenceUID instead of FrameOfReferenceUID which is what is in dcmjs and matches the rest of SeriesDate,SeriesTime, etc. Anything dicom should follow same notation
| if ( | ||
| displaySet.FrameOfReferenceUID && | ||
| displaySet.FrameOfReferenceUID !== backgroundDisplaySet.FrameOfReferenceUID | ||
| displaySet.frameOfReferenceUID && |
There was a problem hiding this comment.
seems like this is breaking change and is not documented
…f reference as viewport background display set (#5900) - Add FrameOfReferenceUID to SEG and RTSTRUCT displaySet in SOP Class Handlers so the FOR is available for filtering - Sync optimisticOverlayDisplaySets when background display set changes so the overlay menu reflects the correct state after a background switch - Add FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align. - fix segmentation overlay order reversal on viewport re-render
…f reference as viewport background display set (#5900) - Add FrameOfReferenceUID to SEG and RTSTRUCT displaySet in SOP Class Handlers so the FOR is available for filtering - Sync optimisticOverlayDisplaySets when background display set changes so the overlay menu reflects the correct state after a background switch - Add FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align. - fix segmentation overlay order reversal on viewport re-render
* fix: Use newer ONNX version and load without errors * Only changes to enable SAM again * fix(seg hydration): auto-hydrate RT struct on second load with disableConfirmationPrompts (#5875) * chore(version): Update package versions to 3.13.0-beta.34 [skip ci] * fix(Threshold tool): Threshold tool no longer becomes deselected when the Dynamic option is selected (#5884) fix(Threshold tool): Added 'ThresholdCircularBrushDynamic' to the toolNames array so the evaluator correctly recognizes it as an active state for the Threshold button when Dynamic mode is selected. * chore(version): Update package versions to 3.13.0-beta.35 [skip ci] * fix: Modalities in study list should select starts with as primary (#5886) * chore(version): Update package versions to 3.13.0-beta.36 [skip ci] * fix(security): Bump tar version to address CVE-2026-31802. (#5893) * chore(version): Update package versions to 3.13.0-beta.37 [skip ci] * fix(segmentation): Display "No description S:{series number} {modality}" for segmentations with no label. (#5874) * Bump CS3D dependency to get the fallbackLabel field additions. * chore(version): Update package versions to 3.13.0-beta.38 [skip ci] * fix(window level): The window level value is not displayed by default on all the viewports when selecting common/custom layout and TMTV. (#5865) * fix(window level): Set up listener for viewport availability such that the initial window level can be read and displayed. * PR feedback. * PR feedback. --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> * chore(version): Update package versions to 3.13.0-beta.39 [skip ci] * fix(security): Bump flattened version to address CVE-2026-32141. (#5897) * chore(version): Update package versions to 3.13.0-beta.40 [skip ci] * fix(sr-hydration): enable hydration and arrow navigation for 3D SR measurements (#5887) Joe is away, so approving based on the code having the requested change, and otherwise looking good/passing tests. * fix(sr-hydration): enable hydration and arrows navigation for 3D SR measurements * test: add automated test for SR measurement navigation with arrows after hydration * add cross-study warning in the 3D branch * test: address reviewer feedback for the test * fix: support 3D and 2D annotations for SR hydration * test: improve navigation to first image --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> * chore(version): Update package versions to 3.13.0-beta.41 [skip ci] * feat: Add combined build (#5895) * Add combined build * Link script location update * Security and validation fixes * Allow specifying target path in PR description * fix: Version match * Fix build detection issue * fix: Playwright deploy * Separate out the branch merge guard * Update docs and link info * test: Update the layout change to wait for network idle * Move audit late so the rest of the build can be worked on * Add text with network check to ensure we see this change is updated * Attempt to fix the mpr loading on ohif-downstream * PR review comments * Update docs * Update to CS3D 4.20.0 * PR comments * Add log on ohif-integration builds * Update build test * Removed unused space to kickoff build * chore(version): Update package versions to 3.13.0-beta.42 [skip ci] * fix(SR): Added support for spline and live wire SR items. (#5870) * fix(SR): Added support for spline and live wire SR items. * Apply suggestion from @greptile-apps[bot] Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> * Add a script to checkout a worktree for test builds * fix: Allow download for testing sr validator * Remove script that wasn't intended to be included * Bump CS3D version. * PR comments - simplify code and use single codepath for download * Allow both download and save buttons for SEG and RTSTRUCT --------- Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: Bill Wallace <wayfarer3130@gmail.com> * chore(version): Update package versions to 3.13.0-beta.43 [skip ci] * chore(tests): contour segment interactions e2e tests - rename and togglevisibility (#5891) * chore(version): Update package versions to 3.13.0-beta.44 [skip ci] * chore(refactor): use public appConfig getter instead of private _appConfig field (#5923) * chore(version): Update package versions to 3.13.0-beta.45 [skip ci] * refactor(tests): update viewport page object usage to async and update all effected tests (#5927) * chore(version): Update package versions to 3.13.0-beta.46 [skip ci] * fix: prevent black viewport when navigating series with client-created segmentation (#5919) * chore(version): Update package versions to 3.13.0-beta.47 [skip ci] * fix(measurement): Restore viewport interactivity when deleting in-progress Spline or Livewire measurement (#5905) * chore(version): Update package versions to 3.13.0-beta.48 [skip ci] * fix(segmentation): restrict overlay segmentation menu to same frame of reference as viewport background display set (#5900) - Add FrameOfReferenceUID to SEG and RTSTRUCT displaySet in SOP Class Handlers so the FOR is available for filtering - Sync optimisticOverlayDisplaySets when background display set changes so the overlay menu reflects the correct state after a background switch - Add FOR matching guard to the hydrate segmentation synchronizer to prevent the hydration synchronizer from blindly mirroring segmentations from a source viewport to a target viewport if their primary Frames of Reference do not align. - fix segmentation overlay order reversal on viewport re-render * chore(version): Update package versions to 3.13.0-beta.49 [skip ci] * fix(security): update dependencies to fix security vulnerabilities (#5936) * chore(version): Update package versions to 3.13.0-beta.50 [skip ci] * fix(security): Update yarn.lock that was missed in PR #5936. (#5940) * chore(version): Update package versions to 3.13.0-beta.51 [skip ci] * feat(component): Adds SmartScrollbar to ui-next - OHIF-2558 (#5924) Co-authored-by: Joe Boccanfuso <joe.boccanfuso@radicalimaging.com> * fix(defaultRouteInit): pass sorted display sets to hanging protocol for deterministic viewport order (#5933) fix: pass sorted display sets to hanging protocol for deterministic viewport order The `applyHangingProtocol` function already sorts display sets by modality priority and series number into `sortedDisplaySets`, but the unsorted `displaySets` array was being passed to `hangingProtocolService.run()`. This caused non-deterministic viewport ordering across page loads because `displaySetService.getActiveDisplaySets()` returns display sets in creation order, which depends on asynchronous network responses. Made-with: Cursor * chore(version): Update package versions to 3.13.0-beta.52 [skip ci] * revert: rename DisplaySet.frameOfReferenceUID back to FrameOfReferenceUID (#5943) * chore(version): Update package versions to 3.13.0-beta.53 [skip ci] * fix(cornerstone): read FrameOfReferenceUID from display set in viewport service (#5950) * chore(version): Update package versions to 3.13.0-beta.54 [skip ci] * fix: ignore auth in git (#5955) * chore(version): Update package versions to 3.13.0-beta.55 [skip ci] * ONNX latest version * chore(version): Update package versions to 3.13.0-beta.56 [skip ci] * bun lock * fix high sev mathjs issue * Revert onnx changes * Update to recent CS3D version * Undo unneeded change * Add null check * Undo unneeded change --------- Co-authored-by: Ghadeer Albattarni <165973963+GhadeerAlbattarni@users.noreply.github.com> Co-authored-by: ohif-bot <danny.ri.brown+ohif-bot@gmail.com> Co-authored-by: Joe Boccanfuso <109477394+jbocce@users.noreply.github.com> Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Co-authored-by: diattamo <mmddiatta@gmail.com> Co-authored-by: Pedro Köhler <pedrokohlerbh@gmail.com> Co-authored-by: Dan Rukas <dan.rukas@gmail.com> Co-authored-by: Joe Boccanfuso <joe.boccanfuso@radicalimaging.com> Co-authored-by: Alireza <ar.sedghi@gmail.com>
Context
Fixes #5890
This PR fixes a bug where the segmentation overlay menu allowed users to select segmentations that did not share the same Frame of Reference (FOR) as the underlying viewport display set.
Also, in segmentation mode, the hydrate segmentation synchronizer added segmentations to all viewports regardless of their Frame of Reference, causing segmentations to appear immediately in viewports with a different FOR.
Root Cause
SEGandRTSTRUCTdisplay sets had noFrameOfReferenceUIDso the FOR filter ingetEnhancedDisplaySetssilently passed every SEG/RT and mark them asisOverlayable: truesince the the check never gets true, the check always (undefined && ...)inextensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/utils.tsThe hydrate segmentation synchronizer did not check whether the target viewport matched the source viewport's FOR. So the segmentations were added to all viewports with different FOR.
Changes & Results
FrameOfReferenceUIDto theDisplaySetand ensuredSEGandRTSTRUCTSop class handlers set it (from instance and/or referenced display set).In
getEnhancedDisplaySets, overlay display sets (SEG/RTSTRUCT) whoseFrameOfReferenceUIDdoes not match the viewport background’s are marked non-overlayable, so they no longer appear as selectable in the overlay segmentation menu.After
FOR-fix.mov
Testing
Open a study with multiple frames of reference in a segmentation mode
(e.g., StudyInstanceUIDs=1.3.6.1.4.1.12201.1091.126683095609223531686845324113579088978)
Open the overlay menu on a viewport
Click on add segmentation
Verify only segmentations matching that viewport’s background FOR should be listed.
Change the viewport background display set
Verify the segmentation display set will reset
Select a segmentation
Open another Viewport overlay menu
Verify that the selected segmentation (from previous viewport) was add only to the matching viewport display set FOR
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR fixes a bug where the segmentation overlay menu allowed selecting segmentations with a different Frame of Reference (FOR) than the viewport's background display set, and where the hydrate segmentation synchronizer propagated segmentations to viewports with mismatched FORs.
FrameOfReferenceUIDpropagation: Both the SEG and RTSTRUCT SOP class handlers now populateFrameOfReferenceUIDon their display sets — from the instance metadata directly and from the referenced display set as a fallback. This enables the existing FOR check ingetEnhancedDisplaySets(utils.ts) to properly filter non-matching overlays.useEffectinViewportDataOverlayMenuresets the optimistic overlay display set list when the background display set changes, keeping the UI in sync. However,overlayDisplaySetsis missing from the effect's dependency array.createHydrateSegmentationSynchronizernow compares source and target viewport FORs before propagating segmentation representations, preventing cross-FOR hydration.FrameOfReferenceUIDadded as an optional property to the coreDisplaySettype.Confidence Score: 4/5
extensions/cornerstone/src/components/ViewportDataOverlaySettingMenu/ViewportDataOverlayMenu.tsx— theuseEffectdependency array is incomplete.Important Files Changed
overlayDisplaySetsdependency is missing from the effect's dependency array, which could cause stale state.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[SEG/RTSTRUCT Instance Loaded] --> B[SOP Class Handler] B --> C[Set FrameOfReferenceUID on DisplaySet] C --> D{Referenced DisplaySet Available?} D -->|Yes| E[Copy FOR from Referenced DisplaySet] D -->|No| F[Subscribe for DISPLAY_SETS_ADDED event] F --> G[On event: Copy FOR from added DisplaySet] E --> H[getEnhancedDisplaySets] G --> H H --> I{displaySet.FrameOfReferenceUID matches background?} I -->|Yes or undefined| J[isOverlayable: true] I -->|No| K[isOverlayable: false] J --> L[Shown in Overlay Menu] K --> M[Hidden from Overlay Menu] N[Segmentation Hydration Sync] --> O{Shared DisplaySet exists?} O -->|Yes| P[Add segmentation to target viewport] O -->|No| Q{Source FOR == Target FOR?} Q -->|Yes| P Q -->|No| R[Skip - different Frame of Reference]Last reviewed commit: 9e5ae01