-
Notifications
You must be signed in to change notification settings - Fork 285
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(4D): added support for 4D data rendering #438
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/StreamingImageVolume.ts
Outdated
Show resolved
Hide resolved
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.
Good start, there is a lot of methods for the 3D which doesn't make sense such as imageIdIndexToFrameIndex
, getScalarDataByImageIdIndex
, getActiveScalarData
. I guess the design of the inheritance hierarchy is not optimal. Maybe we need a BaseStreamingImageVolume
and then StreamingImageVolume3D
and StreamingImageVolume4D
. What do you think
instanceMetaData = JSON.parse(JSON.stringify(instanceMetaData)); | ||
|
||
// It was using JSON.parse(JSON.stringify(...)) before but it is 8x slower | ||
instanceMetaData = removeInvalidTags(instanceMetaData); |
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 sure why we were doing the json strigify either, do we really need this? does it really throw error if we have null or undefiend?
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.
Yes, it does fail when there are null/undefined values but at least it is taking 110ms vs 936ms before for the PT/CT demo dataset we are using to test it.
The only issue that I see after removing JSON.stringify
is related to this change because it adds isMultiFrame: undefined
. We would have to remove it from the json and keep it private to the metadataManager
context or make sure it's set to false
instead of undefined
. But even after making this change I would not be 100% sure if it would work for all scenarios because JSON.stringify
was added one year ago while that isMultiFrame
property was added only one month ago. It may have another reason for calling it.
packages/streaming-image-volume-loader/src/helpers/splitImageIdsBy4DTags.ts
Show resolved
Hide resolved
packages/streaming-image-volume-loader/src/helpers/splitImageIdsBy4DTags.ts
Show resolved
Hide resolved
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.
almost there, see my comments
const scalarData = | ||
'getScalarData' in image ? image.getScalarData() : image.scalarData; |
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.
image
is returned by getTargetIdImage(...)
. This function returns IImageData | CPUIImageData | IImageVolume
but only IImageData
has getScalarData()
.
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 great, thanks a lot !
Can you please update the PR with listing the breaking changes?
- scalarData is now private and need to be accessed via getScalarData
- ....
cornerstone now loads 4D volumes when using
cornerstoneStreamingDynamicImageVolume
schema. Currently it is implemented only for (0054,1300) FrameReferenceTime tag. Minor changes are needed to add support for some other 4D tags as follow:changes:
scalarData
is now private toImageVolume
and need to be accessed viagetScalarData()