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: jumpToSlice and scaling of images in renderToCanvas #78

Merged
merged 9 commits into from
Apr 19, 2022

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Apr 18, 2022

  • Separate resetZoom and resetPan for camera reset as the consumer need to decide which reset is desired (by default reset zoom was applied)
  • Remove the need for passing element to removeAnnotation for the annotation to get removed
  • Fixed the scaling of the images in the renderToCanvas function
  • cleanup the stackScrollTool to use the scrollThroughStack utility and fix the problem with deltaFrames = 0 by caching previous direction
  • Fixed the jumpToSlice bug for imagerIdIndex out of bound

@sedghi sedghi requested a review from JamesAPetts April 19, 2022 00:29
@sedghi sedghi changed the title Fix/image id index fix: jumpToSlice and scaling of images in renderToCanvas Apr 19, 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.

Some minor comments to address, but otherwise good.

How are we doing publishing of cornerstone3D beta btw? We are still currently changing public API a lot during this phase, but we shouldn't increment major versions until its out of beta. Do we make each breaking change a feature change. I.e. 0.X.0.?

): void {
const annotationManager = getViewportSpecificAnnotationManager(element);
let annotationManager = getDefaultAnnotationManager();
if (element) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a great solution that makes sense, allows us to have the annotation-only-api whilst also allowing for element specific managers if we ever need them. I know @swederik is away, but want to tag him for visibility about this decision (sure we can merge before he sees it, can always make another change).

packages/tools/src/tools/StackScrollTool.ts Show resolved Hide resolved
packages/tools/src/types/EventTypes.ts Show resolved Hide resolved
@sedghi sedghi requested a review from JamesAPetts April 19, 2022 13:46
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 above naming of event variable, then good to go.

@sedghi sedghi merged commit bbebf7f into main Apr 19, 2022
@swederik swederik deleted the fix/imageIdIndex branch July 8, 2022 09:33
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.

2 participants