Skip to content
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

feat(thumbnail highlight): Thumbnails of hydrated series are now highlighted #3594

Merged

Conversation

jbocce
Copy link
Collaborator

@jbocce jbocce commented Aug 10, 2023

Context

Thumbnails representing hydratable series (i.e. SR, SEG, RTSTRUCT, etc.) are now highlighted to indicate when the item is hydrated in OHIF.
Addresses OHIF issue #3421.
Also, thumbnails with no image that are 'active' (i.e. the viewport is selected in the viewer) are now also highlighted appropriately with a border just like the image thumbnails.
The highlighting is per as described here

Changes & Results

SEG and RTSTRUCT display sets now have their isHydrated flag set to true when they are hydrated. Hydrated SR display sets for tracked measurements already had this flag set appropriately.

The ThumbnailNoImage modality label colours and the "list-bullets" icon color when the thumbnail is hydrated are now set based on whether the associated display set is hydrated or not. Also a change was made in ThumbnailNoImage so that it indicates an active series (i.e. when visible and highlighted in an OHIF viewport) with a border just like the Thumbnail (with an image).

Testing

A few test cases to consider...

Test A

  1. View a study with tracked measurements.

  2. Double click or drag and drop the SR into a viewport.

  3. Opt NOT to track the measurements and simply view the SR in the viewport.

  4. The thumbnail for the SR should have a border indicating that it is the active viewport. Activating a different viewport should remove the border around the thumbnail.
    image

  5. Similar behaviour should occur when loading a study with SEG or RTSTRUCT and opting NOT to hydrate the segmentation.

Test B

  1. View a study with tracked measurements.

  2. Double click or drag and drop the SR into a viewport.

  3. Opt to track the measurements.

  4. The thumbnail for the hydrated SR should look similar to below...
    image

  5. Similar behaviour should occur when loading a study with SEG or RTSTRUCT after opting to hydrate the segmentation.

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: Windows 11
  • Node version: 16.14.0
  • Browser: Chrome 115.0.5790.171

…(i.e. SR, SEG, RTSTRUCT, etc.) are

now highlighted to indicate when the item is hydrated in OHIF.
Addresses OHIF issue 3421.
Also, thumbnails with no image that are 'active' (i.e. the viewport is selected in the viewer)
are now also highlighted appropriately with a border just like the image thumbnails.
@jbocce jbocce requested a review from sedghi August 10, 2023 20:11
@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit 5bef6ce
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/64dcfab7df195400081bb3ec
😎 Deploy Preview https://deploy-preview-3594--ohif-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Aug 10, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit 5bef6ce
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64dcfab7db8ad30007e3908a
😎 Deploy Preview https://deploy-preview-3594--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #3594 (5bef6ce) into master (f845f87) will not change coverage.
Report is 2 commits behind head on master.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3594   +/-   ##
=======================================
  Coverage   42.58%   42.58%           
=======================================
  Files          80       80           
  Lines        1463     1463           
  Branches      340      340           
=======================================
  Hits          623      623           
  Misses        675      675           
  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 3fe6766...5bef6ce. Read the comment docs.

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So something came to my mind to try again and seems to be a bug. it is where a segment is hydrated then removed, it still shows it as hydrated

@jbocce
Copy link
Collaborator Author

jbocce commented Aug 11, 2023

So something came to my mind to try again and seems to be a bug. it is where a segment is hydrated then removed, it still shows it as hydrated

Good catch. I will have a look. Thanks so much.

Moved the setting of the displaySet.isHydrated flag to the SegmentationService.
The DisplaySetService event is fired when the isHydrated flag is updated.
} = this.servicesManager.services;
const displaySet = displaySetService.getDisplaySetByUID(displaySetUID);
displaySet.isHydrated = isHydrated;
displaySetService.setDisplaySetMetadataInvalidated(displaySetUID);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is the best way, but at the very least it will start a discussion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm, I see why you need this, but at the same time we have another listener for this at the OHIFCornerstoneViewport which invalidates the data. Maybe we need another event to decouple this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a new DisplaySetService event called DISPLAY_SET_UPDATED?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure really, it is kind of the same event we are invalidating a metadata, but at the same time we don't want to hit that other listener in the OHIFCornerstoneViewport, aaaaah
I hate having DISPLAY_SET_UPDATED and DISPLAY_SET_SERIES_METADATA_INVALIDATED
Let's think till we meet

@@ -36,6 +36,7 @@ const ThumbnailList = ({
imageSrc,
messages,
imageAltText,
isHydrated,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is kind of weird that we have isHydrated even for regular imagees (e.g., CT, PT, etc.), can we rename it to something else? I don't know what but basically it is is this display set hydrated given that it is derived (rt, seg, sr)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calling it isADerivedAndHydratedDisplaySet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about isHydratedForDerivedDisplaySet

@@ -248,9 +248,29 @@ function PanelStudyBrowserTracking({
}
);

const SubscriptionDisplaySetMetaDataInvalidated = displaySetService.subscribe(
displaySetService.EVENTS.DISPLAY_SET_SERIES_METADATA_INVALIDATED,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add the same logic to the PanelStudyBrowswer too

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments

@jbocce jbocce requested a review from sedghi August 16, 2023 13:37
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. thanks a lot!

@igoroctaviano
Copy link
Contributor

@fedorov with this change, do you think we could close #3421?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants