fix(state synchronization): SEG and RT viewports should keep their before hydration voi #3560

sedghi · 2023-07-26T16:35:34Z

Context

As described in the issue above, when having SEG or RT viewport, and the user clicks on the load/hydrate, the old
viewport VOI LUT is not preserved and is removed. The reason for this is that this action is an "update"
action, and we only storePresentation state on "set" actions (during disable element).

Changes & Results

refactored the storePresentation into a command so that we can call it from both set and update actions.
In addition the getPresentationIds should not increment the index at the end of the id if the action is an update (
if in the same viewportIndex and displaySetInstanceUid we are requesting a new presentation id)

Testing

https://deploy-preview-3560--ohif-dev.netlify.app/viewer?StudyInstanceUIDs=1.3.6.1.4.1.14519.5.2.1.1706.8374.643249677828306008300337414785
1x2
d&d SEG on the right
Invert the left viewport
Load the SEG (hydrate)
see that the left viewport is inverted (was not before the fix)

Checklist

PR

My Pull Request title is descriptive, accurate and follows the
semantic-release format and guidelines.

Code

My code has been well-documented (function documentation, inline comments,
etc.)

Public Documentation Updates

The documentation page has been updated as necessary for any public API
additions or removals.

Tested Environment

[] OS:
[] Node version:
[] Browser:

feat(presentation update): Consolidate storePresentation state updates This commit consolidates the storePresentation state updates for the OHIFCornerstoneRTViewport and OHIFCornerstoneSEGViewport components in the cornerston-dicom-rt and cornerstone-dicom-seg extensions, respectively. The storePresentation state is now updated at relevant points during the component lifecycle, ensuring that the state is stored correctly when necessary. No special consideration has been made for the rest of the changes in the codebase.

netlify · 2023-07-26T16:35:40Z

✅ Deploy Preview for ohif-platform-docs canceled.

Name	Link
🔨 Latest commit	`2751b6a`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-docs/deploys/64d2a5f8f62b980008136a46

netlify · 2023-07-26T16:35:44Z

✅ Deploy Preview for ohif-dev canceled.

Name	Link
🔨 Latest commit	`2751b6a`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-dev/deploys/64d2a5f87b936600080bdac9

codecov · 2023-07-26T16:39:49Z

Codecov Report

Merging #3560 (2751b6a) into master (156c1ba) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3560   +/-   ##
=======================================
  Coverage   42.81%   42.81%           
=======================================
  Files          80       80           
  Lines        1448     1448           
  Branches      340      340           
=======================================
  Hits          620      620           
  Misses        663      663           
  Partials      165      165

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4f037ae...2751b6a. Read the comment docs.

cypress · 2023-07-26T16:45:40Z

Passing run #3427 ↗︎

0	36	0	0	0

Details:

add review comments
Project: Viewers	Commit: `2751b6ad31`
Status: Passed	Duration: 03:06 💡
Started: Aug 8, 2023 8:37 PM	Ended: Aug 8, 2023 8:41 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

jbocce · 2023-08-08T16:07:13Z

I started testing these changes with regards to the steps outlined in #3410. And the way I read that issue, there are TWO problems. The first problem in outlined in steps 1-4 and the second problem in steps 5-8. The second problem appears to be resolved with these changes. However, the first problem appears to remain. Namely...

Open this study: https://deploy-preview-3560--ohif-dev.netlify.app/viewer?StudyInstanceUIDs=1.3.12.2.1107.5.2.32.35162.30000015050317233592200000046
Note the auto W/L=95/35
Drag the SEG into the viewport
Previous W/L has been undone to become W/L=689/315

Or is this expected?

jbocce · 2023-08-08T17:04:14Z

platform/docs/docs/configuration/configurationFiles.md

@@ -120,6 +121,7 @@ Here are a list of some options available:
 if auth headers are used, a preflight request is required.
 - `maxNumRequests`: The maximum number of requests to allow in parallel. It is an object with keys of `interaction`, `thumbnail`, and `prefetch`. You can specify a specific number for each type.
 - `showLoadingIndicator`: (default to true), if set to false, the loading indicator will not be shown when navigating between studies.
+- `supportsWildcard`: (default to false), if set to true, the datasource will support wildcard matching for patient name and patient id.


Does it make sense to further specify that this is for study level (QIDO) search?

sedghi · 2023-08-08T17:11:07Z

started testing these changes with regards to the steps outlined in #3410. And the way I read that issue, there are TWO problems. The first problem in outlined in steps 1-4 and the second problem in steps 5-8. The second problem appears to be resolved with these changes. However, the first problem appears to remain. Namely...

It is not a problem and it is by design. If you change the w/l it will keep it for SEG, if not it will reset it to volume which is intended

platform/core/src/services/ViewportGridService/getPresentationIds.ts

jbocce · 2023-08-08T18:01:02Z

extensions/cornerstone-dicom-seg/src/viewports/OHIFCornerstoneSEGViewport.tsx

@@ -160,6 +168,8 @@ function OHIFCornerstoneSEGViewport(props) {
      viewportIndex,
      segDisplaySet,
    }).then(isHydrated => {
+      storePresentationState();


Why do we store the presentation after/once the SEG is hydrated? I would think that would be too late with respect to fixing issue #3410, but maybe this is for something else. This comment applies to other similar changes in this PR.

I added comment, sorry it was not clear you are right

Sorry, but I am still a bit confused. The then callback is called AFTER the SEG is hydrated, correct? If so, then the first line of the comment you added seems a bit contradictory. But maybe I am missing something.

Thanks for your changes. However, considering the issue seemed fixed even before the most recent changes, I am not sure if the changes actually contribute to the fix. Just wanted to mention this.

jbocce · 2023-08-08T18:02:42Z

started testing these changes with regards to the steps outlined in #3410. And the way I read that issue, there are TWO problems. The first problem in outlined in steps 1-4 and the second problem in steps 5-8. The second problem appears to be resolved with these changes. However, the first problem appears to remain. Namely...

It is not a problem and it is by design. If you change the w/l it will keep it for SEG, if not it will reset it to volume which is intended

Ok. Thanks. I have verified that it keeps the changed w/l for the SEG.

jbocce · 2023-08-08T18:02:54Z

See my comments.

jbocce · 2023-08-08T18:10:33Z

Hmmm. Is this behaviour expected?

ScreenHunter.Aug.08.14.07.mp4

sedghi · 2023-08-08T18:23:41Z

Yes it is correct, since you chose the second DS but the SEG is on the third DS.

extensions/cornerstone-dicom-rt/src/utils/promptHydrateRT.ts

extensions/cornerstone-dicom-seg/src/utils/promptHydrateSEG.ts

jbocce

Approved.

wayfarer3130 · 2023-08-09T12:04:47Z

extensions/cornerstone/src/Viewport/OHIFCornerstoneViewport.tsx

@@ -256,7 +230,9 @@ const OHIFCornerstoneViewport = React.memo(props => {
    setImageScrollBarHeight();

    return () => {
-      storePresentation();
+      commandsManager.runCommand('storePresentation', {
+        viewportIndex,


Please use viewportId throughout - we are trying to remove references to viewportIndex generally.

that is another big PR that I will care later

wayfarer3130 · 2023-08-09T12:05:15Z

extensions/cornerstone/src/commandsModule.ts

@@ -616,6 +617,35 @@ function commandsModule({
      );
      toolGroup.setToolEnabled(ReferenceLinesTool.toolName);
    },
+    storePresentation: ({ viewportIndex }) => {


This is much cleaner to have in the commandsModule to allow re-use.

wayfarer3130

Generally good with the changes. Might change the viewportId, but will leave that up to you to decide.

sedghi added 3 commits July 24, 2023 16:07

docs - NCI funding and data sources

c107a3a

Merge branch 'master' of github.com:OHIF/Viewers into fix/temp-temp-temp

287e3cc

sedghi changed the title ~~fix/temp temp temp~~ fix(state syncronization): SEG and RT viewports should keep their old voi before hydratoin Jul 26, 2023

sedghi changed the title ~~fix(state syncronization): SEG and RT viewports should keep their old voi before hydratoin~~ fix(state synchronization): SEG and RT viewports should keep their old voi before hydratoin Jul 26, 2023

sedghi changed the title ~~fix(state synchronization): SEG and RT viewports should keep their old voi before hydratoin~~ fix(state synchronization): SEG and RT viewports should keep their old voi before hydration Jul 26, 2023

sedghi changed the title ~~fix(state synchronization): SEG and RT viewports should keep their old voi before hydration~~ fix(state synchronization): SEG and RT viewports should keep their before hydration voi Jul 26, 2023

sedghi requested a review from wayfarer3130 July 26, 2023 16:44

james-hanks mentioned this pull request Aug 2, 2023

[Bug] SEG/RTSTRUCT changes W/L after loading #3410

Closed

sedghi requested a review from jbocce August 8, 2023 12:22

jbocce reviewed Aug 8, 2023

View reviewed changes

platform/core/src/services/ViewportGridService/getPresentationIds.ts Outdated Show resolved Hide resolved

jbocce reviewed Aug 8, 2023

View reviewed changes

sedghi added 2 commits August 8, 2023 14:18

Merge branch 'master' of github.com:OHIF/Viewers into fix/temp-temp-temp

7862502

apply review comments

2360db5

sedghi added 3 commits August 8, 2023 15:28

apply review comments

16f1ca4

Merge branch 'master' of github.com:OHIF/Viewers into fix/temp-temp-temp

3960b76

apply review comments

06a2647

jbocce reviewed Aug 8, 2023

View reviewed changes

extensions/cornerstone-dicom-rt/src/utils/promptHydrateRT.ts Outdated Show resolved Hide resolved

jbocce reviewed Aug 8, 2023

View reviewed changes

extensions/cornerstone-dicom-seg/src/utils/promptHydrateSEG.ts Outdated Show resolved Hide resolved

jbocce approved these changes Aug 8, 2023

View reviewed changes

add review comments

2751b6a

wayfarer3130 reviewed Aug 9, 2023

View reviewed changes

wayfarer3130 approved these changes Aug 9, 2023

View reviewed changes

sedghi merged commit 8adf663 into master Aug 9, 2023

jbocce mentioned this pull request Aug 23, 2023

feat(grid): remove viewportIndex and only rely on viewportId #3591

Merged

sedghi mentioned this pull request Sep 26, 2023

release candidate 3p7 RC1 #3680

Closed

This was referenced Oct 10, 2023

candidate 3p7 rc 2 #3712

Closed

feat(v3p7): segmentation mode and cloud server configuration #3717

Closed

feat(v3p7): segmentation mode and cloud server configuration #3718

Closed

feat(v3p7): segmentation mode and cloud server configuration #3720

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(state synchronization): SEG and RT viewports should keep their before hydration voi #3560

fix(state synchronization): SEG and RT viewports should keep their before hydration voi #3560

sedghi commented Jul 26, 2023 •

edited

Loading

netlify bot commented Jul 26, 2023 •

edited

Loading

netlify bot commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

cypress bot commented Jul 26, 2023 •

edited

Loading

jbocce commented Aug 8, 2023 •

edited

Loading

jbocce Aug 8, 2023

sedghi commented Aug 8, 2023

jbocce Aug 8, 2023

sedghi Aug 8, 2023

jbocce Aug 8, 2023

jbocce Aug 9, 2023

jbocce commented Aug 8, 2023

jbocce commented Aug 8, 2023

jbocce commented Aug 8, 2023

sedghi commented Aug 8, 2023

jbocce left a comment

wayfarer3130 Aug 9, 2023

sedghi Aug 9, 2023

wayfarer3130 Aug 9, 2023

wayfarer3130 left a comment

fix(state synchronization): SEG and RT viewports should keep their before hydration voi #3560

fix(state synchronization): SEG and RT viewports should keep their before hydration voi #3560

Conversation

sedghi commented Jul 26, 2023 • edited Loading

Context

Changes & Results

Testing

Checklist

PR

Code

Public Documentation Updates

Tested Environment

netlify bot commented Jul 26, 2023 • edited Loading

✅ Deploy Preview for ohif-platform-docs canceled.

netlify bot commented Jul 26, 2023 • edited Loading

✅ Deploy Preview for ohif-dev canceled.

codecov bot commented Jul 26, 2023 • edited Loading

Codecov Report

cypress bot commented Jul 26, 2023 • edited Loading

Passing run #3427 ↗︎

jbocce commented Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

sedghi commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbocce commented Aug 8, 2023

jbocce commented Aug 8, 2023

jbocce commented Aug 8, 2023

sedghi commented Aug 8, 2023

jbocce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wayfarer3130 left a comment

Choose a reason for hiding this comment

sedghi commented Jul 26, 2023 •

edited

Loading

netlify bot commented Jul 26, 2023 •

edited

Loading

netlify bot commented Jul 26, 2023 •

edited

Loading

codecov bot commented Jul 26, 2023 •

edited

Loading

cypress bot commented Jul 26, 2023 •

edited

Loading

jbocce commented Aug 8, 2023 •

edited

Loading