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

fix(rendering): should still use Float32 when not 16 bit for scaling issues #501

Merged
merged 1 commit into from
Mar 22, 2023

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Mar 22, 2023

We introduced optional 16 bit texture support in the previous pr here, but because of scaling issues in PT, the images were casted to int/uint 16 while they should still be Float.

These changes will get reverted back once we merge the dicomImageLoader which properly handles scaling

FYI @Ouwen

@netlify
Copy link

netlify bot commented Mar 22, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit e4cca5f
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/641b104ad9f09100082e8ca0
😎 Deploy Preview https://deploy-preview-501--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -497,7 +499,7 @@ function createUint16SharedArray(length: number): Uint16Array;
function createUint8SharedArray(length: number): Uint8Array;

// @public (undocumented)
export function createVolumeActor(props: createVolumeActorInterface, element: HTMLDivElement, viewportId: string, suppressEvents?: boolean): Promise<VolumeActor>;
export function createVolumeActor(props: createVolumeActorInterface, element: HTMLDivElement, viewportId: string, suppressEvents?: boolean, use16BitTexture?: boolean): Promise<VolumeActor>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably time to add an options bag here rather than just adding additional parameters.

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 will get removed soon, so can we not do it in this PR?

@@ -61,7 +62,7 @@ async function createVolumeActor(
// types of volumes which might not be composed of imageIds would be e.g., nrrd, nifti
// format volumes
if (imageVolume.imageIds) {
await setDefaultVolumeVOI(volumeActor, imageVolume);
await setDefaultVolumeVOI(volumeActor, imageVolume, use16BitTexture);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option should go in an options object, as there may well be others in the future.

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.

As a temporary fix to resolve rendering issues this looks ok. There are comments we will likely need applied eventually, but looks good in the short term.

@sedghi sedghi merged commit 448baf2 into main Mar 22, 2023
@Ouwen
Copy link
Contributor

Ouwen commented Mar 22, 2023

@sedghi does this change also solve #481 (comment)

@Ouwen
Copy link
Contributor

Ouwen commented Mar 26, 2023

@sedghi, I was going through a rebase and I think there are some breaking changes here. The main issue is that we allow undefined types for the targetBuffer object. However, currently CSWIL expects a type if targetBuffer object is provided.

We can either fix in CSWIL or fix in CS3D. I'm thinking CS3D and can push up a fix.

However, if dicomLoader feature is coming soon, I can hold off on this and stay on older cs3D verison.

bionoone pushed a commit to bionoone/cornerstone3D-beta that referenced this pull request Mar 27, 2023
* feat: add 16 bit data type scale under a decode flag

* fix linter
@sedghi
Copy link
Member Author

sedghi commented Mar 27, 2023

@Ouwen I guess in the next couple of days i will use dicom Image loader

1 similar comment
@sedghi
Copy link
Member Author

sedghi commented Mar 27, 2023

@Ouwen I guess in the next couple of days i will use dicom Image loader

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.

3 participants