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: remove the need for slabThickness in volumeAPI for tools #113

Merged
merged 9 commits into from
May 27, 2022

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented May 26, 2022

  • Removes the need for slabThicknessEnabled
  • Simplifies the API for setSlabThickness
  • Fix bugs for volumeNewImageEventDispatcher
  • Fix a bug for Synchronizer that triggered source viewport event when it didn't belong to the synchronizer anymore
  • Fix RectangleROIStartEndThreshold tool various bugs with the new API
  • Fix arrow tool to accept external api for change/set text

@sedghi sedghi changed the title Fix/segmentation fix: remove the need for slabThicknessEnabled May 26, 2022
@sedghi sedghi requested review from Punzo and JamesAPetts May 26, 2022 02:32
@Punzo
Copy link
Contributor

Punzo commented May 26, 2022

Nice fix. One comment: you set the infrastructure in crosshair to set the slabthickness by volumeIDs, but then in the petCt example you don't use it. You should exclude the pet volume from the third row of viewports.

i.e. now the example renders like:

image

but it should be (i.e., no slab thickness for the pet):

image

Copy link
Member

@JamesAPetts JamesAPetts left a comment

Choose a reason for hiding this comment

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

Minor comments, I think the all-by-default or just use these isn't very clear in the API, and minor changes could fix that.

Otherwise I agree with Davide, the petCt example needs to be updated to reflect these changes.

packages/core/src/RenderingEngine/Viewport.ts Outdated Show resolved Hide resolved
packages/core/src/RenderingEngine/Viewport.ts Outdated Show resolved Hide resolved
packages/core/src/RenderingEngine/VolumeViewport.ts Outdated Show resolved Hide resolved
packages/core/src/RenderingEngine/VolumeViewport.ts Outdated Show resolved Hide resolved
packages/tools/src/tools/CrosshairsTool.ts Show resolved Hide resolved
packages/tools/src/tools/CrosshairsTool.ts Show resolved Hide resolved
packages/tools/src/tools/CrosshairsTool.ts Outdated Show resolved Hide resolved
@sedghi sedghi changed the title fix: remove the need for slabThicknessEnabled fix: remove the need for providing slabThickness in volumeAPI May 26, 2022
@sedghi sedghi requested a review from JamesAPetts May 26, 2022 17:36
@sedghi sedghi changed the title fix: remove the need for providing slabThickness in volumeAPI fix: remove the need for slabThickness in volumeAPI for tools May 27, 2022
Copy link
Member

@JamesAPetts JamesAPetts 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 on demo, otherwise LGTM.

packages/tools/examples/crossHairs/index.ts Show resolved Hide resolved
@sedghi sedghi merged commit a5e431d into main May 27, 2022
@swederik swederik deleted the fix/segmentation branch July 8, 2022 09:32
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