-
Notifications
You must be signed in to change notification settings - Fork 326
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(nifti): Add nifti volume loader to cornerstone 3D repo #696
Conversation
✅ Deploy Preview for cornerstone-3d-docs canceled.
|
|
||
return { | ||
promise: niftiVolumePromise, | ||
cancel: () => { |
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.
If cancel does not actually do anything, then why bother including it. If you keep it, consider removing the commented code.
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.
since it is part of the interface
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.
Perhaps remove the commented code then?
packages/nifti-volume-loader/src/helpers/fetchAndAllocateNiftiVolume.ts
Outdated
Show resolved
Hide resolved
packages/nifti-volume-loader/src/helpers/fetchAndAllocateNiftiVolume.ts
Outdated
Show resolved
Hide resolved
packages/nifti-volume-loader/src/helpers/fetchAndAllocateNiftiVolume.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.
Looks good so far. See my comments.
@@ -0,0 +1,8 @@ | |||
type AffineMatrix = [ |
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.
Do you really need to define yet another matrix format? How about affineMatrix as a function producing a gl-matrix mat4 instance instead, taking four arrays like this as the arguments, and producing the standard mat4 representation.
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.
As discussed in the meeting, I will do it later
Why not continue with this PR |
@longuto it is still under review (@wayfarer3130) |
* Event fired when a Nifti volume is loaded completely | ||
* | ||
*/ | ||
NIFTI_VOLUME_LOADED = 'CORNERSTONE_NIFTI_VOLUME_LOADED', |
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.
Is it necessary to have different events from the dicom volume loader? Really they should be the same events and just distinguish the type of data being loaded. It might be necessary to have them be slightly different because of bulk loading, but ideally making them identical would be useful.
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 think so... , since there is no streaming happening?
…one3D into feat/nifti-loader
…one3D into feat/nifti-loader
import { parseAffineMatrix } from './affineUtilities'; | ||
|
||
/** | ||
* This converts scalar data: LPS to RAS and RAS to LPS |
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.
Could you write out LPS/RAS? At least once
niftiBuffer = NiftiReader.decompress(niftiBuffer); | ||
} | ||
|
||
if (NiftiReader.isNIFTI(niftiBuffer)) { |
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.
This will throw a NPE if it isn't NIFTI data - probably throw an error if it isn't nifti data specifically, and otherwise continue handling it.
|
||
// Spacing goes [1] then [0], as [1] is column spacing (x) and [0] is row spacing (y) | ||
const dimensions = <Types.Point3>[Columns, Rows, numFrames]; | ||
const direction = new Float32Array([ |
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.
Things like this feel like they could be put into a function rather than making this function so long.
// Check if it fits in the cache before we allocate data | ||
// TODO Improve this when we have support for more types | ||
// NOTE: We use 4 bytes per voxel as we are using Float32. | ||
let bytesPerVoxel = 1; |
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.
And a fair bit of this could also be put into functions that provide specific bits of data/usage, and can have their own documentation and maybe even their own unit tests.
return; | ||
} | ||
|
||
for (let i = 0; i < arrayLength; i++) { |
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.
You are modifying this in place - would it be safer to make a copy? What if the data is exposed as a Uint8array and the scaling no longer works? This just feels like it might cause problems, but the memory usage is definitely bigger if you copy the data instead.
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.
This is the same behaviour in the dicom image loader
@@ -0,0 +1,4 @@ | |||
{ | |||
"extends": "../../tsconfig.json", |
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 we find that referencing a global tsconfig file has issues with linking?
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.
no that was something else that I fixed in webpack
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.
There are a number of small comments/issues, nothing that HAS to be addressed, and I know this really should get in soon, so I'm ok with you reviewing the comments and deciding which ones to address.
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 good.
🎉 The work on this PR was made possible thanks to the generous funding from Mayo Clinic, underlining their dedication to fostering open-source community development.
👏 A hearty round of applause for James A. Petts (@JamesAPetts) and Davide Punzo (@Punzo), whose diligent work on the nifti loader proved invaluable.
Context
The nifti format is widely recognized for storing medical images and is the preferred choice for some research labs. This PR introduces a new package,
@cornerstonejs/nifti-volume-loader
, crafted to fetch a nifti file from a URL and load it as a volume within cornerstone3D.We have included two new demonstrations:
Given that the metadata for the nifti file is incorporated within the file header, and there is no standard procedure for servers to return the metadata, both metadata and pixelData will be fetched from the entire binary blob from the URL via an XHR request.
API
nifti:
to the URL for the volume2.1 create rendering engine and enable viewports like every other use case in cs3d
.load
(since in dicom we can fetch the metadata, create the volume, and stream data in)Changes & Results
Add nifti loader
Testing
Test the two examples with these data
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
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