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

feat(annotations): rework annotation manager api and enable multi-manager setup #442

Merged
merged 17 commits into from
Mar 1, 2023

Conversation

sedghi
Copy link
Member

@sedghi sedghi commented Feb 21, 2023

Changes to Annotation Manager

Annotation state

we can now switch between different annotation managers via csTools.annotations.state.setAnnotationManager(manager) to set the annotation manager to get used to filter/add/show/save annotations (by default it is our FrameOfReferenceSpecificAnnotationManager) that stores and group annotations based on the FrameOfReferenceUID

Some methods in the annotation state has been modified to add annotationGroupSelector which can be element or a string that is passed inside the annotation manager and let the manager map those to a key (GroupKey) that is used to group annotations together. In the default annotation manager, the groupKey is the FrameOfReferenceUID; however, you can write custom managers that store annotations separately per viewportId, or timepoints (prior vs current reading) or readerId, etc.

Here are the changes

  • getAnnotations(element, toolName) is now getAnnotations(toolName, annotationGroupSelector)
  • addAnnotation(element, annotation) is now addAnnotation(annotation, annotationGroupSelector)
  • getNumberOfAnnotations(toolName, frameOfReferenceUID) is now getNumberOfAnnotations(toolName, annotationGroupSelector)
  • removeAnnotation(annotationUID, element) is now removeAnnotation(annotationUID)
  • getAnnotation(annotationUID, element?) is now getAnnotation(annotationUID)
  • removeAllAnnotations(element?) is now removeAllAnnotations() without argument

FrameOfReferenceSpecificAnnotationManager

  • annotationManagers should implement a getGroupKey that receives the annotationGroupSelector and returns the key
  • old get(FrameOfReferenceUID, toolName) has been replaced by getAnnotations(groupKey, toolName?)
  • old getAnnotation now only needs annotationUID and not more arguments
  • old getNumberOfAnnotations(toolName, frameOfReferenceUID) is now getNumberOfAnnotations(groupKey, toolName?)
  • old addAnnotation(annotation) is now addAnnotation(annotation, groupKey)
  • old removeAnnotation now only requires annotationUID
  • old saveAnnotations(FrameOfReferenceUID, toolName?) is now saveAnnotations(groupKey?, toolName?)
  • new method removeAnnotations(groupKey, toolName?)

@netlify
Copy link

netlify bot commented Feb 21, 2023

Deploy Preview for cornerstone-3d-docs ready!

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

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.

Please add a function that takes an element and returns an annotation manager id to use rather than assuming that it is FOR or default. That really gives you the ability to add other annotation manager types in the future, or even switch between them on a viewport, eg add some FOR annotations, and then some volume specific annotations. Could then have a combined one for viewing/rendering, and a specific one depending on the state for adding/udpating. Or, two of them for two different clinicians interpretations for comparison purposes.

jmannau pushed a commit to jmannau/cornerstone3D-beta that referenced this pull request Feb 23, 2023
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.

Almost there - just add the function
getAnnotationManagerId: (element) => getEnabledElement(element).FrameOfReferenceUID
in a single file, and export it/use it.
The idea is to avoid exposing any knowledge of what the ID is in all these files, so that it can be changed in a single place, or even injected as a custom function at some point externally

@sedghi sedghi changed the title fix(annotations): rework the add and getAnnotations surface API fix(annotations): rework annotation manager and making it useful for other managers Feb 28, 2023
@sedghi sedghi changed the title fix(annotations): rework annotation manager and making it useful for other managers fix(annotations): rework annotation manager api and enable multi-manager setup Feb 28, 2023
@sedghi sedghi changed the title fix(annotations): rework annotation manager api and enable multi-manager setup feat(annotations): rework annotation manager api and enable multi-manager setup Feb 28, 2023
return annotationManager.get(FrameOfReferenceUID, toolName);
const manager = getAnnotationManager();
const groupKey = manager.getGroupKey(options);
return manager.getAnnotations(groupKey, toolName) as Annotations;
Copy link

Choose a reason for hiding this comment

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

It seems here that the output of type of manager.getAnnotations could be undefined, but is being coerced into Annotations. Would it make sense to return an empty Array if annotations are undefined, to respect to return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks!

@sedghi sedghi merged commit 60bd013 into main Mar 1, 2023
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