-
Notifications
You must be signed in to change notification settings - Fork 326
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
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
packages/tools/src/stateManagement/annotation/annotationState.ts
Outdated
Show resolved
Hide resolved
packages/tools/src/stateManagement/annotation/annotationState.ts
Outdated
Show resolved
Hide resolved
packages/tools/src/stateManagement/annotation/annotationState.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
There was a problem hiding this 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
return annotationManager.get(FrameOfReferenceUID, toolName); | ||
const manager = getAnnotationManager(); | ||
const groupKey = manager.getGroupKey(options); | ||
return manager.getAnnotations(groupKey, toolName) as Annotations; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks!
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 theFrameOfReferenceUID
Some methods in the
annotation state
has been modified to addannotationGroupSelector
which can beelement
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, thegroupKey
is theFrameOfReferenceUID
; however, you can write custom managers that store annotations separately perviewportId
, ortimepoints
(prior vs current reading) orreaderId
, etc.Here are the changes
getAnnotations(element, toolName)
is nowgetAnnotations(toolName, annotationGroupSelector)
addAnnotation(element, annotation)
is nowaddAnnotation(annotation, annotationGroupSelector)
getNumberOfAnnotations(toolName, frameOfReferenceUID)
is nowgetNumberOfAnnotations(toolName, annotationGroupSelector)
removeAnnotation(annotationUID, element)
is nowremoveAnnotation(annotationUID)
getAnnotation(annotationUID, element?)
is nowgetAnnotation(annotationUID)
removeAllAnnotations(element?)
is nowremoveAllAnnotations()
without argumentFrameOfReferenceSpecificAnnotationManager
getGroupKey
that receives the annotationGroupSelector and returns the keyget(FrameOfReferenceUID, toolName)
has been replaced bygetAnnotations(groupKey, toolName?)
getAnnotation
now only needsannotationUID
and not more argumentsgetNumberOfAnnotations(toolName, frameOfReferenceUID)
is nowgetNumberOfAnnotations(groupKey, toolName?)
addAnnotation(annotation)
is nowaddAnnotation(annotation, groupKey)
removeAnnotation
now only requires annotationUIDsaveAnnotations(FrameOfReferenceUID, toolName?)
is nowsaveAnnotations(groupKey?, toolName?)
removeAnnotations(groupKey, toolName?)