feat: Segmentation state restructure to add main representation #19
Conversation
sedghi
commented
Mar 26, 2022
•
sedghi
commented
- re work the segmentation state to include segmentation main representation and derived representations
| getSegmentationState, | ||
| getSegmentationDataByUID, | ||
| getSegmentationRepresentations, | ||
| getSegmentationRepresentationByUID, |
There was a problem hiding this comment.
Is the ByUID necessary? How else would you get it. Don't we have gets notation, not getAnnotationByUID? Let's try to be consistent with whatever we have for annotations
There was a problem hiding this comment.
makes sense, one thing: are we keeping getSegmentationRepresentations or getRepresentations and getRepresentation? I like the getRepresentation (without segmentation), since it is derived from csTools.segmentation.state.getRepresentation, Don't know really .....
Also I guess you have mentioned somewhere else that getSegmentationRepresentations sounds like it will return the representations FOR a segmentation (which is not the case), and it is returning ALL segmentation representations for a ToolGroup.
There was a problem hiding this comment.
James: one end point with optional ids as second argument, getSegmentationRepresentations
| * | ||
| * @param toolGroupId - the UID of the tool group that contains the | ||
| * segmentation | ||
| * @param segmentIndex - the index of the segment to lock/unlock | ||
| * @param locked - boolean | ||
| */ | ||
| // Todo: shouldn't this be a based on a segmentationUID instead of a toolGroupId? | ||
| // Todo: shouldn't this be a based on a segmentationId instead of a toolGroupId? |
There was a problem hiding this comment.
Yes I think it should, since currently we don't allow locking to occur in local toolgroups, only at the global level...
I can imagine it might be easy to have an API to do it for whatever is currently active, so maybe we should keep this and rename it? setSegmentIndexLockedInActiveSegmentation? But I hate how much of a duplicate it would be. Consumer could easily write it themselves
There was a problem hiding this comment.
Yes, I don't like it either, but now sure what is the best way. Tagging @JamesAPetts
There was a problem hiding this comment.
setSegmentIndexLocked that gets the segmentationId, segmentIndex, locked
packages/tools/src/stateManagement/segmentation/addSegmentationRepresentations.ts
Outdated
Show resolved
Hide resolved
| segmentationInputArray: SegmentationPublicInput[] | ||
| ): void { | ||
| if (!segmentationInputArray || !segmentationInputArray.length) { | ||
| throw new Error('The segmentationInputArray undefined or empty array') |
There was a problem hiding this comment.
is undefined or an empty array
packages/tools/src/tools/displayTools/Labelmap/removeLabelmapFromElement.ts
Show resolved
Hide resolved
| @@ -94,7 +94,7 @@ describe('Segmentation State -- ', () => { | |||
| this.renderingEngine.destroy() | |||
| metaData.removeProvider(fakeMetaDataProvider) | |||
| unregisterAllImageLoaders() | |||
| ToolGroupManager.destroyToolGroupByToolGroupId('segToolGroup') | |||
| ToolGroupManager.destroyToolGroupById('segToolGroup') | |||
There was a problem hiding this comment.
do we need ById? What else would you destroy it with?
There was a problem hiding this comment.
@JamesAPetts what do you think
