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

feat(nifti): Add nifti volume loader to cornerstone 3D repo #696

Merged
merged 15 commits into from
Sep 7, 2023

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Jul 18, 2023

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

  • BasicNifti: Loads and renders a nifti file.
  • NiftiWithTools: Loads a nifti file, enables tools, and displays a progress bar during the rendering process.

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

  1. Prepend nifti: to the URL for the volume
const niftiURL = 'https://ohif-assets.s3.us-east-2.amazonaws.com/nifti/MRHead.nii.gz';
const volumeId = 'nifti:' + niftiURL;

2.1 create rendering engine and enable viewports like every other use case in cs3d

  1. Create and Cache the volume, this will also load the volume as opposed to our dicom volume loader which needs a final .load (since in dicom we can fetch the metadata, create the volume, and stream data in)
  const volume = await volumeLoader.createAndCacheVolume(volumeId);

  1. assign it to viewports
setVolumesForViewports(
    renderingEngine,
    [{ volumeId, callback: setCtTransferFunctionForVolumeActor }],
    viewportIds
  );

Changes & Results

Add nifti loader

Testing

Test the two examples with these data

  1. https://ohif-assets.s3.us-east-2.amazonaws.com/nifti/CTACardio.nii
  2. https://ohif-assets.s3.us-east-2.amazonaws.com/nifti/CTACardio.nii.gz
  3. https://ohif-assets.s3.us-east-2.amazonaws.com/nifti/MRBrainTumor1.nii.gz
  4. https://ohif-assets.s3.us-east-2.amazonaws.com/nifti/MRHead.nii.gz

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

@netlify
Copy link

netlify bot commented Jul 18, 2023

Deploy Preview for cornerstone-3d-docs canceled.

Name Link
🔨 Latest commit f294015
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/64f9d1428542430008e4763e

@sedghi sedghi changed the title initial commit for nifti reading feat(nifti): Add nifti volume loader to cornerstone 3D repo Jul 18, 2023

return {
promise: niftiVolumePromise,
cancel: () => {
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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?

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.

Looks good so far. See my comments.

@@ -0,0 +1,8 @@
type AffineMatrix = [
Copy link
Collaborator

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.

Copy link
Member Author

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

@longuto
Copy link

longuto commented Aug 9, 2023

Why not continue with this PR

@sedghi
Copy link
Member Author

sedghi commented Aug 9, 2023

@longuto it is still under review (@wayfarer3130)

* Event fired when a Nifti volume is loaded completely
*
*/
NIFTI_VOLUME_LOADED = 'CORNERSTONE_NIFTI_VOLUME_LOADED',
Copy link
Collaborator

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.

Copy link
Member Author

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?

import { parseAffineMatrix } from './affineUtilities';

/**
* This converts scalar data: LPS to RAS and RAS to LPS
Copy link
Collaborator

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)) {
Copy link
Collaborator

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([
Copy link
Collaborator

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;
Copy link
Collaborator

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++) {
Copy link
Collaborator

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.

Copy link
Member Author

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",
Copy link
Collaborator

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?

Copy link
Member Author

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

Copy link
Collaborator

@wayfarer3130 wayfarer3130 left a 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.

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.

Looks good.

@sedghi sedghi merged commit c9c2e83 into main Sep 7, 2023
10 checks passed
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.

4 participants