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.

Sign up for GitHub

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

Jump to bottom

feat(voi): added support for sigmoid voiLUTFunction for StackViewport and VolumeViewport #224

Merged
merged 7 commits into from
Feb 13, 2023
Merged

feat(voi): added support for sigmoid voiLUTFunction for StackViewport and VolumeViewport #224

merged 7 commits into from
Feb 13, 2023

Conversation

Ouwen
Copy link
Contributor

@Ouwen Ouwen commented Sep 22, 2022 •
edited
Loading

  • CPU fallback is unimplemented

@netlify
Copy link

netlify bot commented Sep 22, 2022 •
edited
Loading

✅ Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 348409a
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/63e992456260f70008faeb5e
😎 Deploy Preview https://deploy-preview-224--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.

@Ouwen Ouwen mentioned this pull request Sep 22, 2022
Merged
@Ouwen
Copy link
Contributor Author

Ouwen commented Sep 22, 2022

@sedghi

sedghi
sedghi reviewed Sep 23, 2022
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Great contribution as always, please see my comments

@Ouwen Ouwen mentioned this pull request Sep 26, 2022
Merged
@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from 4e89d65 to de3155c Compare October 2, 2022 21:26
@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 2, 2022

@sedghi added VOI type as an enum and added documentation

@sedghi
Copy link
Member

sedghi commented Oct 3, 2022

All looks good, two last things

  1. This only adds it for StackViewport right? maybe we rename the PR to include such information?
  2. Can you please post here a before/after image for LINEAR vs SIGMOID voi for reference (since we are not adding new docs)

Thanks

@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 3, 2022

Sure thing, I think I can also try implementing this in the volume viewer

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from de3155c to df36464 Compare October 7, 2022 18:55
@Ouwen Ouwen changed the title feat: added support for sigmoid voiLUTFunction feat: added support for sigmoid voiLUTFunction for StackViewport Oct 7, 2022
@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 7, 2022

Linear
Screen Shot 2022-10-07 at 9 07 54 PM

Sigmoid
Screen Shot 2022-10-07 at 9 05 50 PM

@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 7, 2022 •
edited
Loading

@sedghi
I've added the changes you suggested. Also scaffolding for the VolumeViewport. One thing that came to mind while looking at the volume viewport is if the voiLUTfunction should exist at the viewportInput level vs setVOI in the stackviewport as well as volume viewport

Thoughts?

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from df36464 to c904e30 Compare October 7, 2022 19:39
@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 10, 2022

I noticed that we have the following:

voiLUTFunction: seems to refer to cornerstoneWADO image object/metadata properties
voiLutFunction: property of viewports
VOILUTFunction: dcmjs naming convention

Is this intentional?

@sedghi
Copy link
Member

sedghi commented Oct 10, 2022

I hate this a lot and didn't notice it, good catch

voiLUTFunction: seems to refer to cornerstoneWADO image object/metadata properties
voiLutFunction: property of viewports
VOILUTFunction: dcmjs naming convention

Can we remove voiLutFunction and use either of those? I think we can live with the dcmjs one only

Were you able to run the example i added? It seems to be not working as I expected

cd packages/core
yarn run example voiSigmoid

@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 14, 2022

@sedghi this PR requires making this change to vtkjs
(gradienthealth/vtk-js@623f5ec)

VOILUTFunction should be supported in volumes and stack viewport now.

There is some setProperties logic that I believe is still TODO from previous commits, but I think it might be beyond scope of this PR.

@Ouwen Ouwen changed the title feat: added support for sigmoid voiLUTFunction for StackViewport feat: added support for sigmoid voiLUTFunction for StackViewport and VolumeViewport Oct 14, 2022
@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch 3 times, most recently from baf2aac to d22d1b1 Compare October 20, 2022 04:03
@Ouwen
Copy link
Contributor Author

Ouwen commented Oct 20, 2022

pending Kitware/vtk-js#2603

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from d22d1b1 to 4cf652c Compare October 28, 2022 06:42
@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch 2 times, most recently from 79c6e6a to e1d8637 Compare November 20, 2022 04:44
wayfarer3130 added a commit that referenced this pull request Jan 20, 2023
@wayfarer3130
fix(dcmjs): Add a set of accessors to the sequence list so the API is…
9dad6c5 
… more consistent (#224)

* fix(dcmjs): Add a set of accessors to the sequence list so the API is more consistent.

* fix(dcmjs): Added documentation on the addAccessors as requested
@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch 2 times, most recently from 98962a4 to 9d70fbe Compare February 3, 2023 01:50
@Ouwen
Copy link
Contributor Author

Ouwen commented Feb 3, 2023 •
edited
Loading

@sedghi hmmm unsure why these doc issues are popping up. As far as I know, I ran yarn run build:update-api
nvm it is from the examples, I know the fix...

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from c396e93 to f9e5dd2 Compare February 4, 2023 02:39
@Ouwen
Copy link
Contributor Author

Ouwen commented Feb 4, 2023

@sedghi ready for review

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch 2 times, most recently from 07be113 to 804ac5c Compare February 7, 2023 05:47
@Ouwen
Copy link
Contributor Author

Ouwen commented Feb 7, 2023

@sedghi just another ping since there are a lot of branches flying around...

sedghi
sedghi reviewed Feb 8, 2023
View reviewed changes
packages/core/src/RenderingEngine/BaseVolumeViewport.ts Outdated
Comment on lines 413 to 421
const voiRange = actor
.getProperty()
.getRGBTransferFunction(0)
// @ts-ignore: vtk d ts problem
.getRange();
this.voiRange = {
lower: voiRange[0],
upper: voiRange[1],
};
Copy link
Member

Choose a reason for hiding this comment

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

we basically do this inside createVolumeActor, how is it different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the voiRange is saved to the volume anywhere, here we set the voiRange on the BaseVolumeViewport instance. I guess this could be moved into create volume actor, but it would require passing this into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess one way to do this is to have BaseVolumeViewport implement a getter for voiRange. This getter takes in an index for the volume actor of interest with default being 0. A VOIRange object is returned.

Thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

I would say let's take out the triggerVOIModified from inside the createVolumeActor, and put it inside the base

sedghi
sedghi reviewed Feb 8, 2023
View reviewed changes
packages/core/src/RenderingEngine/StackViewport.ts
@@ -78,6 +80,7 @@ interface ImagePixelModule {
pixelRepresentation: string;
windowWidth: number;
windowCenter: number;
voiLUTFunction: VOILUTFunctionType;
Copy link
Member

Choose a reason for hiding this comment

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

can you please change this to be at least the same as the volume viewport? VOILUTFunction

Copy link
Member

Choose a reason for hiding this comment

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

or this is different? i'm confused again :()

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is coming from cswil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be the same, I try to keep the same convention as the surrounding code. The voiLUTFunction is from cswil. Probably makes sense to change the other vars

  pixelRepresentation => PixelRepresentation ;
  windowWidth: number; => I think this is derived dicom tag from voi
  windowCenter: number; =>  I think this is derived dicom tag from voi

Copy link
Member

Choose a reason for hiding this comment

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

Ok good

sedghi
sedghi reviewed Feb 8, 2023
View reviewed changes
packages/core/src/RenderingEngine/StackViewport.ts
}

// make sure the VOI LUT function is valid in the VOILUTFunctionType which is enum
if (Object.values(VOILUTFunctionType).indexOf(voiLUTFunction) === -1) {
Copy link
Member

Choose a reason for hiding this comment

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

This logic is used 3 times Object.values(VOILUTFunctionType).indexOf(voiLUTFunction) maybe refactor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check can probably be removed actually since the VOILUTFunctionType is forced in the function signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other places are due to CSWIL passing in a string which needs to be properly checked. Once we convert to typed CSWIL these logic can be removed.

sedghi
sedghi reviewed Feb 8, 2023
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Minor comment about the if (i === 0 && !this.voiRange) {

@Ouwen Ouwen force-pushed the gradienthealth/added_sigmoid_lut branch from f0f45bf to a899143 Compare February 12, 2023 17:52
@Ouwen
Copy link
Contributor Author

Ouwen commented Feb 13, 2023

@sedghi this is ready for review. Instead of moving the triggerEvent function outside of the createVolumeActor I decide to remove the need for this.voiRange instead this is something that can be derived from the volume actor's look up table. The logic for this is in the getProperties section.

sedghi
sedghi approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution as always :D

@sedghi sedghi changed the title feat: added support for sigmoid voiLUTFunction for StackViewport and VolumeViewport feat(voi): added support for sigmoid voiLUTFunction for StackViewport and VolumeViewport Feb 13, 2023
@sedghi sedghi merged commit 2fcec22 into cornerstonejs:main Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedghi sedghi sedghi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ouwen @sedghi