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

Dicom Loader Typescript Conversion #330

Merged

Conversation

jmannau
Copy link
Contributor

@jmannau jmannau commented Dec 7, 2022

Building on #310 I have updated the bulk of the CornerstoneWadoLoad/DicomLoader package to typescript.

I have added @todo comments throughout where there appear to be issues. I have also had to use many as any escape hatches to work around the legacy code.

sedghi and others added 26 commits November 23, 2022 10:09
* fix: annotation highlighted and tooling for ellipticalROI

* update build

* fix tests
 - docs@0.7.8
 - @cornerstonejs/tools@0.29.7
* fix: use focal point for pan cache for stack viewport

* fix: pan dir with flip

* fix pan values while flipped

* update build

* apply review comments

* fix build
 - @cornerstonejs/core@0.21.5
 - docs@0.7.9
 - @cornerstonejs/streaming-image-volume-loader@0.6.6
 - @cornerstonejs/tools@0.29.8
* added basic cursorCrosshairSync tool with example, TODO: for now cursorSync is displayed regardless of distance, create configurable distance and also sync the position of all viewports over which the mouse is not to scroll to a slice that is close to the currentMousePosition in 3d space

* addde stack syncing for StackViewport and syncing for volumeViewport on imageChange events, added configuration for max display distance

* refactored tool functions

* added comment to possible bug

* added configuration options to example

* changed look of crosshair to 4 lines with central space

* undid local tsconfig change

* undid yarn.lock changes

* added tool to example-info.json

* removed from example-runner because it broke build

* readded example and fixed typo

* readded example-info and changed example to trigger rebuild

* added cleanup for mouseoverElement when tool is disabled

* added cleanup when tool gets disabled, this does not get called when toolGroup gets destroyed, might cause remaining listeners

* applied naming changes, reworked adding annotation logic

* removed event listeners and moved logic to check for stack scrolling into rendering logic

* added planeDistanceToPoint to planar utilities

* added getClosestStackImageIndexForPoint

* rewrote logic to use onCameraModified

* updated example-info

* fixed bug with 0 being falsey

* added logic to remove cursor if wanted

* modified toolGroup so that setting a tool active only changes the cursor to default if there is no primary mouse cursor

* fixed bug not updating disable cursor

* fixed missing parentheses from merge

* readded scrollWheel scrolling and api changes

* fixed typos
 - @cornerstonejs/core@0.22.0
 - docs@0.7.10
 - @cornerstonejs/streaming-image-volume-loader@0.6.7
 - @cornerstonejs/tools@0.30.0
 - docs@0.7.11
 - @cornerstonejs/tools@0.30.1
Co-authored-by: edward65 <edward@afxmedical.com>
…qual normals. (cornerstonejs#315)

Co-authored-by: Ramon Emiliani <ramon@afxmedical.com>
* limit disabled element not need to render

* Update BaseTool.ts

fix: get correct viewport when there are multiple viewport with same stack data

Co-authored-by: chendingmiao <cdm@tomtaw.com.cn>
 - docs@0.7.12
 - @cornerstonejs/tools@0.30.2
* fix(htj2k):Support htj2k in the streaming volume loader

* fix(decodeImage):Fix htj2k image decode and mouse key modifiers

* Update for PR

* update ci build
 - docs@0.7.13
 - @cornerstonejs/streaming-image-volume-loader@0.6.8
 - @cornerstonejs/tools@0.30.3
 - @cornerstonejs/core@0.22.1
 - docs@0.7.14
 - @cornerstonejs/streaming-image-volume-loader@0.6.9
 - @cornerstonejs/tools@0.30.4
…js#309)

* fix rotation for handles

* fix: short axis movement

* fix: bidirectional tool incorrect interaction
 - @cornerstonejs/core@0.22.2
 - docs@0.7.15
 - @cornerstonejs/streaming-image-volume-loader@0.6.10
 - @cornerstonejs/tools@0.30.5
…ned (cornerstonejs#323)

While trying to get the volume from the cache, it can be undefined so getting the scaling attribute would throw an error in that case.
This is a quick fix
 - @cornerstonejs/core@0.22.3
 - docs@0.7.16
 - @cornerstonejs/streaming-image-volume-loader@0.6.11
 - @cornerstonejs/tools@0.30.6
 - docs@0.7.17
 - @cornerstonejs/tools@0.30.7
@netlify
Copy link

netlify bot commented Dec 7, 2022

Deploy Preview for cornerstone-3d-docs failed.

Name Link
🔨 Latest commit 09ca22f
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63901797d566a00008532bd2

@sedghi
Copy link
Member

sedghi commented Dec 7, 2022

This is a great start! Thanks
Couple of points

  1. Is it applying my changes for the webpack etc.?
  2. We probably can't just merge into main since other consuming apps are using the old CornerstoneWadoImageLoader (CWIL), I should prepare them also for this significant change. So how about we change this base to the branch I have for the CSWIL Draft PR?

@jmannau jmannau changed the base branch from main to feat/cornerstoneWADO December 7, 2022 20:50
@jmannau
Copy link
Contributor Author

jmannau commented Dec 7, 2022

Thanks @sedghi
This is based on your initial PR and web pack config. The only changes to your web pack config are to include .(ts|js)$ files.
At the moment, typescript error don't fail the build. This is based on the current web pack config. I haven't attempted to change this. Happy to do so.
I have changed the base to be against https://github.com/cornerstonejs/cornerstone3D-beta/tree/feat/cornerstoneWADO

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.

This is a great contribution. Thanks again.
I'm gonna approve this to merge and we can fix bugs in the base branch

@sedghi sedghi merged commit f6efcca into cornerstonejs:feat/cornerstoneWADO Dec 7, 2022
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.

10 participants