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

fix(SR): When loading DICOM SR, only one measurement is shown with no way to show others #3228

Merged
merged 12 commits into from
Apr 25, 2023

Conversation

wayfarer3130
Copy link
Contributor

@wayfarer3130 wayfarer3130 commented Mar 8, 2023

Context

The DICOM SR viewport specified in the description that it was a previous of the measurements, however, the actual
implementation displayed only the first series measurements. This change creates a temporary display set containing
all images with measurements on them so that the actual display can be shown.

To implement that, fixes were required in the display set service:

  • Allow updating display sets when new DICOM instances are added which are applicable to the display set.
  • Generate the display sets in O(n log n) time on n images instead of O(n^2) time, making the time to initial images much faster for large display sets.

Changes & Results

Can now see all the measurements on different series when loading the DICOM SR preview for a multi-series measurement store.

Can save measurements again (was broken)

Time to load on study with 2 900 image series is noticeable faster - but hard to measure.

Added a hanging protocol for SR and for Seg @ohif/sr and @ohif/seg

Testing

  • Open up a study with at least 2 series containing images

  • Add markup to two different images in different series, and a second image in one of the series

  • Save the annotations

  • Refresh

  • Load the DICOM SR preview window. You can see the various measurements on the preview.

  • Open up a study with a 2000+ DICOM instance series before

  • Open up the same study after, you should see it is faster - I found it noticeable before/after

Test in the e2e environment with the two URLs:
http://localhost:3000/viewer?StudyInstanceUIDs=1.2.840.113619.2.5.1762583153.215519.978957063.78&hangingProtocolId=@ohif/sr
(this will be blank initially due to a bug in CS3D or perhaps in the dialog service)

http://localhost:3000/viewer?StudyInstanceUIDs=1.3.12.2.1107.5.2.32.35162.30000015050317233592200000046&hangingProtocolId=@ohif/seg

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:

@wayfarer3130 wayfarer3130 changed the base branch from master to v3-stable March 8, 2023 23:09
@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #3228 (981ac74) into v3-stable (9f1e813) will increase coverage by 12.38%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           v3-stable    #3228       +/-   ##
==============================================
+ Coverage      25.15%   37.54%   +12.38%     
==============================================
  Files            119       83       -36     
  Lines           2862     1377     -1485     
  Branches         555      305      -250     
==============================================
- Hits             720      517      -203     
+ Misses          1856      692     -1164     
+ Partials         286      168      -118     

see 76 files with indirect coverage changes


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 dc61d87...981ac74. Read the comment docs.

@cypress
Copy link

cypress bot commented Mar 8, 2023

Passing run #3131 ↗︎

0 36 0 0 Flakiness 0

Details:

PR - documentation
Project: Viewers Commit: 981ac743d9
Status: Passed Duration: 03:42 💡
Started: Apr 21, 2023 7:59 PM Ended: Apr 21, 2023 8:03 PM

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

@netlify
Copy link

netlify bot commented Mar 9, 2023

Deploy Preview for ohif-platform-docs canceled.

Name Link
🔨 Latest commit 981ac74
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/6442e96d9a832200086c1c01

@netlify
Copy link

netlify bot commented Mar 9, 2023

Deploy Preview for ohif-platform-viewer ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-viewer/deploys/644843ece844542e74386f39
😎 Deploy Preview https://deploy-preview-3228--ohif-platform-viewer.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 settings.

@wayfarer3130 wayfarer3130 force-pushed the fix/update-measurements-set branch 4 times, most recently from 9571b90 to 0cbb02c Compare March 13, 2023 20:09
@wayfarer3130 wayfarer3130 changed the title [WIP] fix: When loading DICOM SR, only one measurement is shown with no way to show others fix: When loading DICOM SR, only one measurement is shown with no way to show others Mar 13, 2023
@wayfarer3130 wayfarer3130 force-pushed the fix/update-measurements-set branch 3 times, most recently from c9c1484 to ab4e6f0 Compare March 20, 2023 21:43
@wayfarer3130 wayfarer3130 force-pushed the fix/update-measurements-set branch 2 times, most recently from ff03a87 to b08fd22 Compare March 22, 2023 14:02
@wayfarer3130
Copy link
Contributor Author

@jbocce - addressed your concerns and updated the PR with a merge.

@jbocce
Copy link
Contributor

jbocce commented Apr 18, 2023

@jbocce - addressed your concerns and updated the PR with a merge.

Thank you kindly.

throw new Error(`The comparator ${comparator} was not a function`);
}

this.getActiveDisplaySets().forEach(displaySet => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use .filter here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In their infinite wisdom, the JS gods decided that iterators don't have the filter method :-(

@@ -441,6 +441,7 @@ class MetadataProvider {
}

getUIDsFromImageID(imageId) {
if (!imageId) throw new Error('MetadataProvider::Empty imageId');
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we are throwing this Error do we need to handle it somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was previously a NPE, so this just gets an error message when it happens. Yes, it needs to be handled, but I haven't gotten a hold of a good stack trace that shows what is happening when it is null. I think it is a race condition.

Copy link
Contributor

@jbocce jbocce left a comment

Choose a reason for hiding this comment

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

A few minor comments. Looks good in general.

@jbocce
Copy link
Contributor

jbocce commented Apr 19, 2023

One issue I noticed is that the spacing between the SR and key image note series is not consistent with the others in the series tray. It would be great if you could address that...
image

@jbocce
Copy link
Contributor

jbocce commented Apr 19, 2023

Double click to one up is also broken. However, I suspect it is due to the changes made for this PR cornerstonejs/cornerstone3D#560 in cornerstone. I think we need to check evt.button === 0 instead of evt.button ===1, but I am not 100% certain.

@sedghi sedghi changed the title fix: When loading DICOM SR, only one measurement is shown with no way to show others fix(SR): When loading DICOM SR, only one measurement is shown with no way to show others Apr 25, 2023
@sedghi sedghi merged commit 69d8e6a into v3-stable Apr 25, 2023
sedghi pushed a commit that referenced this pull request May 5, 2023
…ical-4d-base-merge (#3368)

* fix(volumeLoad): should not have missing slices when loading (#3287)

* fix(volumeLoad): should not have missing slices when loading

* add review comments

* feat(DoubleClick): double click a viewport to one up and back (#3285)

* feat(DoubleClick): double click a viewport to one up and back

Added a toggleOneUp command that puts the active viewport into a 1x1 grid layout
and it toggles out of 'one-up' by restoring its saved 'toggleOneUpViewportGridStore'
from the StateSyncService.
Added double click customization for the Cornerstone extension with the
default double click handling being the toggleOneUp command.
Added a cypress test for the double click functionality.

* PR feedback:
- tracked viewport measurements no longer show as dashed when toggling one up
- disallowed double clicking near a measurement
- updated cornerstone3D dependencies to fix double click of TMTV and volume viewport 3D
- created ViewportGridService.getLayoutOptionsFromState

* Updated the ViewportGridService docs.

* Switched to using 'cornerstoneViewportClickCommands' and consistency with the context menu clicks.

* feat(tmtv): add more stages to pt/ct (#3290)

* feat(tmtv): add more stages to pt/ct

* make error stage change to info

* apply review comments

* fix(viewports): The display of linked viewports during drag and drop has a race (#3286)

* fix: The display of linked viewports during drag and drop has a race

* PR review comments

* fix: Segmentation display two up

* Removing console logs

* Fix the blank viewport can have stuff added to it

* Fix the null name on HP module

* Fix the navigate to initial image

* Fix the nth interleave loader

* Fix the unit tests

* PR comments - docs mostly

* fix: Exception thrown on change displayset after double click

* feat(multiframe): enhanced support for multiframe dicom (#3164)

* Changes in cswil version and multiframe

* Minor changes

* wip

* Adding support for NM multiframe images

* Applying PR suggestions

* fixing package versions

* Restoring default.js config file

* Check if NM subtype is reconstructable

* Restore default.js values

* refactore code

* feat: add flag for strict zspacing

---------

Co-authored-by: Alireza <[email protected]>

* feat(URL): add param for initial series and sop uids to display (#3265)

* feat: Allow navigating to a specified series and sop instance

This was a feature in OHIF v2, so adding it to v3, albeit with new
parameters.

feat: Allow comma separated as well as repeated args params

* docs

* Test fixes

* feat: Navigate to SOP selected - PR fixes

* Updated docs

* PR fixes

* fix(crosshairs): suppressed error for crosshair (#3237)

* fix(SRTool): Ellipse Display for DICOMSR Tool (#3307)

* feat(App): support async config function (#3313)

* fix(URL): allow multi filter query (#3314)

* fix(viewport): Initial blank image on SR/SEG initial display (#3304)

* fix: Blank display area on initial DICOM SR load

* Docs

* Fix a NPE

* fix(ROIThreshold): fix setWeight not updating properly for ROIThreshold panel (#3315)

* fix(hp): Add displaySetMatchDetails and displaySets to options in (#3320)

The sameAs function requires displaySetMatchDetails and displaySets to compare attributes between display sets, but these properties were not being passed into the options object. This commit adds the necessary lines of code to include displaySetMatchDetails and displaySets in the options object, fixing the issue where the  custom attribute function 'sameAs' was missing required data.

* feat(measurements): add CircleROI Tool (#3211)

* [refactor] measurement service mapping files - rename to .ts files

* [fix] RectangleROI - measurement service mapping _getReport() function - tool name fix

* [feat] added CircleROI tool from updated cornerstone3D

* [refactor] fix for typescript strong typing warnings

* [feat] cornerstone-dicom-sr extension - added CircleROI tool

* [feat] added toolbar button for CircleROI tool

* [doc] doc updates for CircleROI tool

* [update] library - dcmjs to 0.29.5

* [fix] roundNumber() function when given strings

* [fix] refactor after upgrading conerstonejs library 0.49.0

* yarn.lock file change

* fix: Service consistency typing (#3309)

* Use more consistent type/structure for services

* Fix restoring SR so the tests described work

* One more typed service

* Added types for the Cornerstone library

* Couple more type fixes

* feat: Add a new hanging protocol @ohif/mn (#3305)

* feat: Add a new hanging protocol @ohif/mn

* Add @ohif/seg example

* PR comments and a couple more fixes to make things cleaner

* PR comment

* Added an example to cause the PR checks to rerun

* chore(version): updated Orthanc from 1.5.7 to 1.11.0 (#3330)

* fix(SR): When loading DICOM SR, only one measurement is shown with no way to show others (#3228)

* fix: Make the cornerstone sR viewport show all measurements

* PR fixes

* PR fixes

* Add a DICOM SR hanging protocol

* Duplicate the hanging protocol for seg as well

* PR requested change

* PR requested changes

* PR fixes plus merge update fixes

* PR fixes and integration test fix

* PR - documentation

* feat(RT): add dicom RT support via volume viewports (#3310)

* feat: initial RT support

* make the segmentation service work with representation data

* feat: make segmentation service work with representations

* fix rtss vis

* fix: rt hydration

* fix the rendering of rt names

* fix imports

* refactor: Modify status and click handling for hydration of RTStructures

Modify status and click handling for hydration of RTStructures by renaming `onPillClick` to `onStatusClick` in `OHIFCornerstoneRTViewport.tsx` and `_getStatusComponent.tsx` files. Also, update initial segmentation configurations in `PanelSegmentation.tsx` and simplify configuration changes and values for segmentation service in `SegmentationService.ts`. Finally, remove console debug in `CornerstoneViewportService.ts`.

* wip for highlighting contours

* refactor rt displayset code

* review code update

* update cornerstone dependencies

* refactor: Update license year, version number, and minor code cleanup

This commit updates the license year in several files, updates the version number in package.json, and contains minor code cleanup in two files.

* add bulkdataURI retrieve for RT

* fix package version

* apply review comments

* apply review comments

* apply review comments

* feat(panels): refactor and streamline segmentation configuration and inputs

Rewrote state hooks and streamlined the configuration input for `PanelSegmentation` to be more verbose and reusable. Included several new input types, including the `InputRange` component which now shows a fixed floating value based on the step provided. The `SegmentationConfig` component now works with dynamic values controlled by `initialConfig`. These changes should improve function usability and make the code more maintainable going forward.

* fix various bugs

* fix contour delete by upgrade cs3d version

* feat(viewport, inputNumber, segmentationConfig, orthanc): Implement minimum and maximum values for input number components, and useBulkDataURI for Orthanc configuration. Compare measurement view planes with absolute viewport view planes in Cornerstone viewport.

* update yarn lock

* feat(dicomImageLoader): replace wado image loader with the new library (#3339)

* use new dicom image loader instead of cswil

new dicom image loader fails

update dicom image loader

update yarn lock

modify webpack

* update webpack

* fix webpack

* update package versions

* update package versions

* fix(Browser history): fixed NPE when navigating a study via browser history and history navigation is now available via the navigateHistory command (#3337)

* fix(Browser history):
- fixed an NPE when navigating to a different study via the URL
- exposed browser history navigation via a command

* Added documentation for the navigateHistory command.
Moved the history object from UI to viewer.

* feat(storybook): Refactor Storybook to use Typescript (#3341)

* feat(storybook): Refactor Storybook to use Typescript

This commit updates the Storybook configuration to use Typescript, including renaming `main.js` to `main.ts`, updating `core.builder` and `framework.name` to use `@storybook/builder-webpack5` and `@storybook/react-webpack5`, respectively. The code also adds options to `addon-docs` to enable adding GFM support to the generated docs. Additionally, the commit modifies `staticDir`, removes outdated addons, and makes various devDependencies and PostCSS updates to align with Storybook 7.x.x. Finally, the commit removes some unused code and relocates `Button` and `AboutModal` story files.

* apply review comments

* update yarn lock

* feat(microscopy): add dicom microscopy extension and mode (#3247)

* skeleton for dicom-microscopy extension

* skeleton for microscopy mode

* [feat] ported @radicalimaging/microscopy-dicom to OHIF's default extension

* [feat] ported microscopy mode from private repo

* added new definitions to the package.json

* webpack configuration update for microscopy extension

* register new icons for microscopy tools

* fixes to the microscopy extension and mode

* trivial error fix - typescript type import error

* link microscopy extension with default OHIF app plugin config

* demo config fix

* fix logs

* remove unsed imports

* [fix] loading of microscopy

* [fix] webworker script loading, normalizing denaturalized dataset

* found the latest version of dicom-microscopy-viewer that works with current OHIF extension : 0.35.2

* hide thumbnail pane by default, as we have issues with

* comments

* remove unused code

* [feat] microscopy - annotation selection

* [feat] microscopy - edit annotation label

* wip

* [bugfix] dicom-microscopy tool

* [bugfix] dicom microscopy annotations

* [fix] mixed-content blocking caused by BulkDataURI

* [fix] microscopy measurements panel - center button

* [fix] microscopy measurements panel - styling

* [fix] microscopy - controls

* fix local loading of microscopy

* fix local loading of dicom microscopy

* fix typo - indexof to indexOf

* [fix] remove unused icons

* remove commented out lines from webpack configuration

* platform/viewer/public/config/default.js - revert accidental changes

* [refactor] redirecting to microscopy mode on Local mode

* attribution to DMV and SLIM viewer

* [fix] code review feedbacks

* fix thumbnails

* [fix] microscopy - fix old publisher.publish() to PubSubService._broadcastEvent()

* [fix] interactions, webpack config, roi selection

* [feat] microscopy - remove select tool  from UI

* microscopy author

* [fix] saving and loading SR

* [bugfix] - missing publish() function in MicroscopyService, replace with _broadcastEvent

* remove author section from readme

* refactor SR saving feature

* fix webpack config after merge

* [bugfix] repeated import

* fix e2e

* webpack configuration

* hide "Create report" button for microscopy

* fix(ui): updated input fields to match new color scheme (#3323)

* Updated input fields to match new color scheme

Added small input text fields and fixed coloring

* Added tailwind changes to viewers file

* feat(cst): Add new command to get nearby annotation tools (#3327)

* feat(cst): Add new command to get nearby annotation tools
This commit adds a new command `getNearbyAnnotation` to the `commandsModule.ts` file that identifies nearby annotation tools excluding Crosshairs and ReferenceLines. It also removes an unused mapping to `Crosshairs` in `initMeasurementService.js` and changes the command run in `findNearbyToolData.ts` to `getNearbyAnnotation`.

* perf(cornerstone): Improve performance by optimizing tools

Removes unnecessary imports to improve app's performance. Changes how annotations are detected on the `commandsModule.js` file by fetching the `isAnnotation` property of the tool instance if available. Updates `CrosshairsTool` and `ReferenceLinesTool` to not be annotations anymore. Adjusts the `localhost` URL ports for the `dcm4chee-arc` service interface on `local_dcm4chee.js` to allow for more efficient server communication.

* feat(cornerstone): Add CornerstoneServices type and ToolGroupService.getToolGroup to get tool group by ID or active viewport

This commit introduces the new CornerstoneServices interface to the code and adds the ToolGroupService.getToolGroup method, which can retrieve a specific tool group by ID or from the active viewport. The older _getToolGroup method has also been removed from commandsModule. When no tool group ID is provided, this method now retrieves the tool group from the currently active viewport with the help of getActiveViewportEnabledElement. Additionally, the required reference to @ohif/core has been removed.

* fix(viewportDialog): viewportDialoge not appearing in non-tracked viewports (#3071)

* fix: viewportdialoge not appearing in non-tracked viewports

* feat(viewports): Introduce useViewportDialog and remove deprecated API

This commit introduces the `useViewportDialog` hook and replaces the deprecated `viewportDialogApi` with the new `viewportDialogState`. Additionally, the notifications in `OHIFCornerstoneRTViewport` and `OHIFCornerstoneViewport` have been removed. Finally, the `CinePlayer` component now accepts optional parameters.

* fix tests

* feat(DICOM Upload): Added new DICOM upload dialogue launched from the worklist page (#3326)

* feat(DICOM Upload)
OHIF #3297
- DicomWebDataSource.store.dicom now accepts an ArrayBuffer of data sets to store
- DicomWebDataSource.store.dicom now also accepts optional callbacks to track upload and an AbortSignal object to cancel upload
- Added DicomFileUploader class that performs and tracks the upload of a DICOM file
- Added various UI pieces for the upload: DicomUpload, DicomUploadProgress, DicomUploadProgressItem
- ProgressLoadingBar was extracted from LoadingIndicatorProgress so it can be reused
- Modal dialogues can now optionally prevent an outside click from closing the Modal

* Passing an XMLHttpRequest to the dataSource.store.dicom method instead of callbacks and AbortSignal.
Cleanup of various UI pieces to minimize code and increase readability.

* Made the DicomUpload component a customization module exported by the cornerstone extension.

* Exposed a copy of the data source configuration via the IWebApiDataSource interface.
Added dicomUploadEnabled to a data source's configuration.

* Code clean up.

* Upgraded the dicomweb-client version to one that provides the ability to
pass a custom HTTP request. DICOM upload uses that custom HTTP request
to track progress and cancel requests.

* Distinguished between failed and cancelled uploads.

* Allow no selection in the upload dialogue.
Fixed the styling of various progress information so that everything aligns.

* Switched from cornerstone wado image loader to cornerstone dicom image loader for DICOM upload.

* Added special cancelled icon to differentiate from failed.

* Added a bit of spacing between the upload progress bar and percentage.

* Fixed minor issue with upload rejection.

* Performance improvement for cancel all uploads:
- use React memo for each upload item progress (row)
- do not await each request of a cancel all

* Fixed various padding/spacing for the DICOM upload drop zone component.
Changed the border dashing for the DICOM upload drop zone to be a background image gradient.
Added hover and active effects to the 'Cancel All Uploads' text.

* fix(jump): Jump to measurement wasn't jumping when presentation already existed (#3318)

* fix: Jump to measurement after presentation store

* Fix initial load of DICOM SR

* Fix measurement highlight issues

* Clear measurements changed to array

* Fix merge issues

* PR comments

* Removed unused formatting.  Change to single event

* Fix the unit test

* PR review comments

* refactor(viewport): simplify imports, align destructuring

Simplify import statements by removing unused imports, organize existing imports, and align destructured imports consistently across the component.

---------

* feat(proxy datasource): JSON server configuration launch (#2791)

* feat: delegating dicom web proxy datasource

* fix: configurably allow WADO image loader to issue preflight OPTIONS request

* Added documentation and fixed up some error handling from PR review

* review comments changes

* another small code review fix

* chore(lerna): upgrades lerna to 6.x (#3215)

* feat(core): upgrades lerna to 6.x.x

* feat(core): upgrades lerna to 6.5.1

* feat(core): updates lerna to 6.x.x

* feat(core): updates default config

* fix(DICOM Upload): Decreased the minimum height for the upload dialog contents to better fit some laptops. (#3347)

* fix(HangingProtocolService): Add callback property & update description (#3349)

The description for the numberOfDisplaySetsWithImages property was corrected. The update also involves adding a callback property that returns several objects (matchedViewports, viewportMatchDetails, displaySetMatchDetails) and returning them from the method 'findViewportProtocols'. The refactored code also has better readability.

* chore(lerna): add lerna caching to build and tests (#3350)

* chore(bump cs3d): bump package and add caching to webpack (#3353)

* chore(bump cs3d): bump package and add caching to webpack

* remove test for now

* fix: conflicts

---------
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