Updating proper values for ChangeType parameter in AnnotationModifiedEventDetail. #1821
Conversation
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I think this look ok, wdyt @wayfarer3130 |
| @@ -685,7 +685,7 @@ class PlanarFreehandROITool extends ContourSegmentationBaseTool { | |||
| const { data } = annotation; | |||
| if ( | |||
| !data.cachedStats[targetId] || | |||
There was a problem hiding this comment.
How about !data.cachedStates[targetId]?.areaUnit
The areaUnit should be a non empty string, even for unknown units as that will be pixels
There was a problem hiding this comment.
It seems there is an existing issue that calculateCachedStats is always called for each mouse movement of open freenad annotation. This is due to the areaUnit for open freehand ROIs in cachedStates is always undefined.
As this condition is always true for Open freehand annotations, resulting in the unintended activation of an annotationModified event, we are restricting the second condition for closed freehand only.
There was a problem hiding this comment.
Then we should probably check on unit rather than areaUnit, and always make sure we specify unit in it - that should be available for both closed and open contours. I believe it is not currently set for open contours, but it should be.
There was a problem hiding this comment.
There should be an equivalent check for open contours, and I'd really rather have a single check that works for both.
| @@ -81,6 +81,8 @@ type Annotation = { | |||
| [key: string]: unknown; | |||
| /** Cached Annotation statistics which is specific to the tool */ | |||
| cachedStats?: Record<string, unknown>; | |||
| /** Label of an annotation */ | |||
| label?: string; | |||
There was a problem hiding this comment.
This is inconsistent with the label in LengthTool - label belongs to the data element. Note the difference between CS3D and OHIF - in CS3D the data is in the data object, while in OHIF it is at the top level.
There was a problem hiding this comment.
The label is already defined inside the data element as you suggested.
There was a problem hiding this comment.
Sorry, was seeing the nesting wrong. Could you move the handles to it's own type - I think that is sufficiently long to be worth having by itself. That would resolve the issue with having it hard to see the nesting.
|
The initial creation should leave the label changed event out |
At present, the label change event is not triggered upon the creation of an annotation. We will trigger this event from the OHIF side when an annotation label is added or modified. |
There was a problem hiding this comment.
Generally looks good - just two changes:
- Can you ensure that open and closed contours both define unit?
- Change the planar freehand roi to use unit instead of areaUnit so that both open and closed contours correctly detect not initialized
- Move the handles into it's own type definition so that it is obvious what belongs with what parts. Or, at a minimum, define all the short items above handles so that when the handles ends, it also ends the data element. But, I'd really like a separate type.
Context
Updated ChangeType parameter value in AnnotationModifiedEventDetail for LengthTool
Changes & Results
The
ChangeTypeparameter inAnnotationModifiedEventDetailnow returnsStatsUpdatedfollowing the execution of the_calculateCachedStatsfunction, it returnsHandlesUpdatedwhen the handle position is modified.The
initialSetupchange type can be fired from addNewAnnotation(), we currently left it out. Please let us know if you think it should be there as cs3d already has a annotation created event for it. Should we remove the change type InitialSetup ?Testing
Checklist
PR
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-apito update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment