-
Notifications
You must be signed in to change notification settings - Fork 297
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: WSI Viewport - basic framework to display WSI images with annotations #944
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -45,6 +45,11 @@ module.exports = { | |||
'../../../node_modules/@cornerstonejs/dicom-image-loader/dist/dynamic-import', | |||
to: '${destPath.replace(/\\/g, '/')}', | |||
}, | |||
{ | |||
from: | |||
'../../../node_modules/dicom-microscopy-viewer/dist/dynamic-import/index.worker.min.worker.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider randomizing the UID in dicomImageLoader. This is important because conflicts may arise if another library also has index.worker.js, since we copy this to serve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really prefer to change this to just load from a nested path, but I can't figure out the webpack mumbo jumbo to enable that. Even better would be to direct load as a script external. I THINK I know how to do that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loads on nested path. Does not need randomization any longer.
@@ -382,24 +380,6 @@ class RenderingEngine implements IRenderingEngine { | |||
return viewports.filter(isStackViewport) as Array<IStackViewport>; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why you are removing video viewport
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method was unused, and I don't see a need for methods for each type when we have the generic ones.
console.log('Loaded DICOMMicroscopyViewer', DicomMicroscopyViewer); | ||
this.frameOfReferenceUID = null; | ||
const metadata = this.imageIds.map((imageId) => { | ||
const imageMetadata = client.getDICOMwebMetadata(imageId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we prob should go from metadata manager get eventually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but this is the required format right now internally, so getting it direct. That avoids refetching the data, which makes things noticeably faster on first render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a lot of work to use dicom metadata manager.
} | ||
}); | ||
// Construct viewer instance | ||
const viewer = new DicomMicroscopyViewer.viewer.VolumeImageViewer({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
didn't know the api for DMV has VolumeImageViewer, what is volume for WSI? the tiles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will have to look it up - it might be the fact that it actually can be a volume, with various focal depths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not adding focal depth support right now, although it is something we might want to consider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good first draft
Added a branch ImagingDataCommons/dicom-microscopy-viewer#111 with changes required to run this basic WSI view |
Deals with an ugly bug in netlify which doesn't permit tarballs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot
Context
Show whole slide imaging DICOM in a CS3D viewport.
Changes & Results
Testing
Run wsi and wsiAnnotationTools
Basic viewing and simple annotations should be supported
Fixes for [#1090]
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment