Skip to content

Add options to tool hydrate method#1776

Merged
sedghi merged 7 commits intocornerstonejs:mainfrom
jmhmd:feat/hydrate-tool-opts
Mar 18, 2025
Merged

Add options to tool hydrate method#1776
sedghi merged 7 commits intocornerstonejs:mainfrom
jmhmd:feat/hydrate-tool-opts

Conversation

@jmhmd
Copy link
Contributor

@jmhmd jmhmd commented Jan 23, 2025

Context

hydrate methods of tools use whatever the current viewport camera properties are to populate the new annotation metadata, which may not be desired if adding an annotation to some arbitrary location in the volume or to a specific 2D image.

Changes & Results

  • Optional referencedImageId, viewPlaneNormal, and viewUp to populate annotation metadata rather than use current viewport data.
  • Don't calculate spline polyline coordinates on hydration, as this only works for the current camera position, not any passed in parameters. It appears this gets recalculated on annotation render anyways, so seems to be redundant.

Testing

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:

- Optional referencedImageId, viewPlaneNormal, and viewUp to populate annotation metadata rather than use current viewport data.
- Don't calculate spline polyline coordinates on hydration, as this only works for the current camera position, not any passed in parameters. It appears this gets recalculated on annotation render anyways, so seems to be redundant.
@netlify
Copy link

netlify bot commented Jan 23, 2025

Deploy Preview for cornerstone-3d-docs failed. Why did it fail? →

Name Link
🔨 Latest commit 32bd6b8
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67d8d45b94a15b0008b4e547

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.

see my comments thanks a lot

@jmhmd
Copy link
Contributor Author

jmhmd commented Jan 29, 2025

Changes applied

Copy link
Member

sedghi commented Jan 29, 2025

can we do the refactoring of the base class too?

@jmhmd
Copy link
Contributor Author

jmhmd commented Jan 29, 2025

Which base tool? AnnotationTool.ts doesn't have a hydrate method

@sedghi
Copy link
Member

sedghi commented Jan 29, 2025

Yes, AnnotationTool, so we do something like

protected static hydrateBase(
    ToolClass: new () => T,
    viewportId: string,
    points: Types.Point3[],
    options?: {
      annotationUID?: string;
      instance?: T;
      referencedImageId?: string;
      viewplaneNormal?: Types.Point3;
      viewUp?: Types.Point3;
    }
  ) {
    const enabledElement = getEnabledElementByViewportId(viewportId);
    if (!enabledElement) {
      return;
    }
    const { viewport } = enabledElement;
    const FrameOfReferenceUID = viewport.getFrameOfReferenceUID();

    let { viewPlaneNormal, viewUp } = viewport.getCamera();
    if (options?.viewplaneNormal && options?.viewUp) {
      viewPlaneNormal = options.viewplaneNormal;
      viewUp = options.viewUp;
    }

    const instance = options?.instance || new ToolClass();

    let referencedImageId = instance.getReferencedImageId(
      viewport,
      points[0],
      viewPlaneNormal,
      viewUp
    );

    if (options?.referencedImageId) {
      if (referencedImageId !== options.referencedImageId) {
        viewPlaneNormal = undefined;
        viewUp = undefined;
      }
      referencedImageId = options.referencedImageId;
    }

return those informatino back to the tool 

and in each tool you call it, since you can see the repetition of handling viewUp, viewPlaneNormal with or without options et

@jmhmd
Copy link
Contributor Author

jmhmd commented Mar 14, 2025

Ok, finally got around to this, hopefully this is what you were thinking

@sedghi sedghi requested a review from Copilot March 17, 2025 22:47
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the hydrate methods for a wide range of annotation tools by introducing new optional parameters (referencedImageId, viewplaneNormal, viewUp, and toolInstance) to allow for more flexible annotation metadata population. Key changes include adding a new hydrateBase method in AnnotationTool.ts, updating UI examples to support passing a specific imageId, and revising the API definitions and tool implementations (e.g. SplineROITool, ArrowAnnotateTool, etc.) to incorporate the new options.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

File Description
packages/tools/src/tools/base/AnnotationTool.ts Adds a new hydrateBase method with new optional parameters and updates its internal logic accordingly.
packages/tools/examples/dynamicallyAddAnnotations/* Updates multiple UI examples to parse and utilize an optional third button identifier (imageId) for handling specific image annotations.
packages/tools/src/tools/annotation/* Updates hydrate methods in various annotation tool implementations to accept the new options and use hydrateBase.
common/reviews/api/tools.api.md Updates API documentation to reflect the new optional parameters in tool hydrate signatures.
Comments suppressed due to low confidence (2)

packages/tools/examples/dynamicallyAddAnnotations/ellipticalROIToolUI.ts:89

  • [nitpick] Consider extracting the button ID parsing logic into a shared utility function to reduce duplication and improve readability across the dynamic annotation UI files.
const [type, viewportType, useImageId] = button.id.split('-') as [ 'canvas' | 'image', keyof typeof typeToIdMap, 'imageId'? ];

packages/tools/src/tools/base/AnnotationTool.ts:668

  • [nitpick] Consider renaming the option 'viewplaneNormal' to 'viewPlaneNormal' for consistent camelCase naming with the rest of the codebase.
viewPlaneNormal = options?.viewplaneNormal ?? viewPlaneNormal;

…d options handling and instance management

- Updated the options parameter to ensure proper handling of toolInstance, referencedImageId, viewplaneNormal, and viewUp.
- Refactored the annotation creation logic to utilize the hydrateBase method for better instance management.
- Improved the handling of referencedImageId based on viewport type and provided options.
- Added textBox structure to annotation data for better state management.
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!

@sedghi sedghi merged commit 0df5ce4 into cornerstonejs:main Mar 18, 2025
7 of 11 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.

3 participants