fix(seg): prevent segmentations from spreading to all viewports before hydration confirmation in 3D four-up#5967
Conversation
…n confirmation in 3D four-up
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const singleBaseViewport = nextBaseVolumeIds.length === 1; | ||
|
|
||
| if ( | ||
| singleBaseViewport && | ||
| nextBaseVolumeIds.length && |
There was a problem hiding this comment.
singleBaseViewport declaration
singleBaseViewport is nextBaseVolumeIds.length === 1, so the subsequent nextBaseVolumeIds.length && check is always true when singleBaseViewport is true. The redundant guard can be dropped to clarify intent.
| const singleBaseViewport = nextBaseVolumeIds.length === 1; | |
| if ( | |
| singleBaseViewport && | |
| nextBaseVolumeIds.length && | |
| if ( | |
| singleBaseViewport && | |
| existingVolumeIds.length >= nextBaseVolumeIds.length && | |
| volumeIdPrefixesMatch(existingVolumeIds, nextBaseVolumeIds.length, nextBaseVolumeIds) && | |
| viewportMatchesDesiredVolumePresentation(viewport, viewportInfo) | |
| ) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
Line: 1161-1165
Comment:
**Redundant condition after `singleBaseViewport` declaration**
`singleBaseViewport` is `nextBaseVolumeIds.length === 1`, so the subsequent `nextBaseVolumeIds.length &&` check is always `true` when `singleBaseViewport` is `true`. The redundant guard can be dropped to clarify intent.
```suggestion
if (
singleBaseViewport &&
existingVolumeIds.length >= nextBaseVolumeIds.length &&
volumeIdPrefixesMatch(existingVolumeIds, nextBaseVolumeIds.length, nextBaseVolumeIds) &&
viewportMatchesDesiredVolumePresentation(viewport, viewportInfo)
) {
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
@wayfarer3130 any comment on this? nextBaseVolumeIds.length is only checked in this condition so it is a bit odd IMHO.
| const MIN_STACK_VIEWPORTS_TO_ENQUEUE_RESIZE = 12; | ||
| const MIN_VOLUME_VIEWPORTS_TO_ENQUEUE_RESIZE = 6; | ||
|
|
||
| function getOrderedVolumeActorReferencedIds(viewport: Types.IVolumeViewport): string[] { |
There was a problem hiding this comment.
The name of this function implies ordering yet I see no ordering. Thoughts @wayfarer3130?
| .filter(Boolean) as string[]; | ||
| } | ||
|
|
||
| function volumeIdPrefixesMatch(a: string[], bPrefixLen: number, b: string[]): boolean { |
There was a problem hiding this comment.
Let's give better names to a, bPrefixLen and b. Thanks.
| } | ||
|
|
||
| /** | ||
| * Only treat base volumes as identical (skip setVolumes) when the enabled viewport |
There was a problem hiding this comment.
I suspect this comment is better suited where this function is called???
| */ | ||
| function viewportMatchesDesiredVolumePresentation( | ||
| viewport: Types.IViewport, | ||
| viewportInfo: ViewportInfo |
There was a problem hiding this comment.
Maybe rename this desiredViewportInfo?
|
Please have a look at the failing Playwright tests. Thanks. |
| }; | ||
|
|
||
| if (Array.isArray(hangingProtocolUpdates)) { | ||
| for (const u of hangingProtocolUpdates) { |
There was a problem hiding this comment.
This is perhaps one of the greatest additions for our automated tests. Thank you for this.
I was thinking that perhaps a third function should also be added that combines the two we have here. Call it say waitForViewportRenderCycle. The code might be something like the following. In general this new utility would be the most used and the other two just in case someone wants to do something more specific for whatever reason. Thoughts?
type WaitForRenderCycleToCompleteOptions = {
/**
* Timeout for waiting until at least one viewport reaches `needsRender`.
*/
needsRenderTimeout?: number;
/**
* Timeout for waiting until all viewports are `rendered`
* (and optionally volume-loaded).
*/
renderedTimeout?: number;
/**
* If true (default), also waits for any volume actors referenced by the
* viewports to report loaded during the rendered phase.
*/
waitVolumeLoad?: boolean;
};
/**
* Waits for a full render cycle:
* 1) any viewport requests render (`needsRender`)
* 2) all viewports finish rendering (`rendered`)
*/
const waitForViewportRenderCycle = async (
page: Page,
options: WaitForRenderCycleToCompleteOptions = {}
) => {
const {
needsRenderTimeout = 5000,
renderedTimeout = 15000,
waitVolumeLoad = true,
} = options;
await waitForAnyViewportNeedsRender(page, { timeout: needsRenderTimeout });
await waitForViewportsRendered(page, { timeout: renderedTimeout, waitVolumeLoad });
};
There was a problem hiding this comment.
@jbocce This is a smart idea, I like it. I test it and it works great.
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Thanks for the changes. Please see my comments.
…ume viewports configureViewportForLayerAddition was defaulting to 'stack' via the isSameDisplaySet check when viewportOptions arrived without a viewportType (e.g. from the HP overlay early-return path). This caused ORTHOGRAPHIC MPR viewports to be re-initialised as stack viewports — stack viewports ignore the orientation field entirely and render images in their native DICOM acquisition plane (axial). Fix: check the live Cornerstone viewport type first (same pattern as the existing orientation recovery on line 65). If the viewport is already orthographic, keep viewportType as 'volume' and skip the isSameDisplaySet check.
…set layer configureViewportForLayerRemoval was unconditionally hardcoding viewportType as 'volume', which converted stack viewports to orthographic during layer removal. This was hidden by configureViewportForLayerAddition accidentally resetting the type back to 'stack' on re-addition, but once that function was fixed to respect the current viewport type, the hardcoded 'volume' in removal was exposed as a bug.
| await viewport.addVolumes(toAdd); | ||
| } | ||
| } else { | ||
| await viewport.setVolumes(baseVolumeInputs); |
There was a problem hiding this comment.
@jbocce Regarding the test (SEGDataOverlayForUnreferencedDisplaySetNoHydration.spec.ts) we were discussing this morning, this is what caused the change.
Before, setVolumes was called with volumeInputArray, which had both the background display set (Apparent Diffusion Coefficient) and the SEG which caused later the to create its images and displayed in the viewport as we saw.
await viewport.setVolumes(volumeInputArray);
Now setVolumes is called with baseVolumeInputs and this comes from filteredVolumeInputArray which filters out the SEG so base volume has only (Apparent Diffusion Coefficient)
I do think that what we currently have (after the change) is the correct behaviour, just to display the selected background and the SEGs masks on the top of it.
What do you think :) ?
There was a problem hiding this comment.
@GhadeerAlbattarni, yes I agree. Sorry for the delay to respond.
jbocce
left a comment
jbocce
left a comment
There was a problem hiding this comment.
Looks great to me. We are now headed for decreased testing time. @wayfarer3130 please have a look, I believe I tagged you on a few items.
| function getVolumeActorReferencedIds(viewport: Types.IVolumeViewport): string[] { | ||
| const actors = viewport.getActors?.() ?? []; | ||
| return actors | ||
| .filter(ae => ae.actor?.getClassName?.() === 'vtkVolume') |
There was a problem hiding this comment.
Could we use ac instead of ae? ae is typically used to mean application entity in the DICOM sense, so it took me a moment to realize it wasn't that.
|
|
||
| if (Array.isArray(hangingProtocolUpdates)) { | ||
| for (const hpUpdate of hangingProtocolUpdates) { | ||
| const entry = hpUpdate as { |
There was a problem hiding this comment.
Could we get the hpUpdates list type declared upstream and passed into this function properly declared? I'd rather add a type of the hanging protocol code that can get applied here and pass it around properly. I'm not sure if that is in grid viewport or hp updates. If you use an interface, it is easy enough to add fields later if we need.
| } | ||
|
|
||
| viewport.viewportOptions.viewportType = 'volume'; | ||
| const csViewport = cornerstoneViewportService.getCornerstoneViewport(viewportId); |
There was a problem hiding this comment.
Could you write this
csViewportOrthographic = ...?.type === 'orthographic'
It just makes the assignment to viewportType a bit more obvious.
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
Just a couple of quite minor changes for improved typing/naming in one spot.
3 participants
Acknowledgement
This PR is based on the implementation from PR #5907 by @wayfarer3130, so thanks to all his amazing work!
Also, thanks to @jbocce for all the efforts he made to improve the tests!
Context
Fixes #5953
When a segmentation (SEG) series was loaded into one of the MPR viewports in the 3D four-up layout, all viewports rendered the segmentation before user confirm hydration.
This PR updates the behavior so that:
Changes & Results
HangingProtocolService.ts: Return only the active viewport for overlay display sets (SEG/RTSTRUCT) — prevents all viewports being updated with the SEG before user confirmationCornerstoneViewportService.ts: SkipsetVolumes()when the same base volume is already loaded; addrender()aftersetPresentations()so segmentation is visible immediately; guardjumpToSliceso it doesn't reset MPR position when volume was unchangeduseSegmentationPresentationStore.ts: Filter overlay UIDs from the segmentation presentation ID to match the key used byupdateStoredSegmentationPresentationcommandsModule.ts: GuardloadSegmentationDisplaySetsForViewportagainst an emptyupdatedViewportslist andset viewports needsRender before hydration
hydrationUtils.ts: AddmergeVolumeSharingViewportsas a safety-net for non-HP layouts: finds all grid viewports that share the same referenced volume and includes them in the post-hydration updatehydrationUtils.test.ts: Unit tests for the newmergeVolumeSharingViewportsbehaviourwaitForViewportsRendered.ts: New test utility withwaitForViewportsRenderedandwaitForAnyViewportNeedsRendercheckForScreenshot.ts: Clean up intermediate screenshot artifacts (actual/diff/expected) between retry attempts; increase default retry delay from 500ms to 1250msTesting
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Greptile Summary
This PR prevents SEG/RTSTRUCT display sets from spreading to all viewports before hydration confirmation in MPR and 3D four-up layouts. The fix spans the HP service (early return for overlays),
CornerstoneViewportService(skipsetVolumeswhen volume unchanged, secondrender()after presentations), the segmentation presentation store (filter overlay UIDs from key), and a newmergeVolumeSharingViewportsutility that ensures all volume-sharing viewports are updated post-hydration. Tests replace fixedpage.waitForTimeoutcalls with event-driven render-cycle helpers.Confidence Score: 4/5
Safe to merge; one P2 test-reliability concern around waitForViewportRenderCycle used for layout changes.
All production-code changes are well-reasoned and covered by existing + new unit/E2E tests. The only finding is a P2 in the test utility usage that could cause intermittent CI failures on the MPR layout-change step but does not affect production behaviour.
tests/SEGHydrationFromMPR.spec.ts (waitForViewportRenderCycle on layout change), extensions/cornerstone/src/utils/hydrationUtils.ts (pre-existing HP-entry overwrite risk in mergeVolumeSharingViewports).
Important Files Changed
Prompt To Fix All With AI
Reviews (6): Last reviewed commit: "apply PR feedback 2" | Re-trigger Greptile