-
Notifications
You must be signed in to change notification settings - Fork 435
fix(Annotation Tools): Fix active volume to show correct cachedStats (especially on fusion viewport) #2418
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
base: main
Are you sure you want to change the base?
Conversation
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.
LGTM, let's see what Bill says
|
I'm not at all keen on having more volume id specific tools - that means you have to know exactly what volumes to use when you create creating the tool group, and when you make any changes to the volumes available, they have to apply to the tool group. |
|
I like the idea, but asking people to do this would be a breaking change since i think we already say use volumeId. I suggest we implement it in the next version. |
|
Hi there I agree with both of you, the current implementation is too complex. I would suggest to use image index in the viewport, as you can control it in the hanging protocol I think it might be possible to specify which image index is used by the tool group. |
| * Used for deciding which set of cached stats is appropriate to display | ||
| * for a given viewport. | ||
| */ | ||
| public static isSpecifiedTargetId(desiredTargetId: string) { |
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.
@Celian-abd - my preference is to provide a function that determines the preferred target id rather than hard coding the value. That way it is possible to have more flexible determination and to work with multiple merged viewports at a single time by creating a custom function that works against both viewports.
| * for a given viewport. | ||
| */ | ||
| public static isSpecifiedTargetId(desiredTargetId: string) { | ||
| return (_viewport, { targetId }) => targetId.includes(desiredTargetId); |
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.
@sedghi - this code is wrong as the target id doesn't have to include the desired target id as a substring in general. The right solution is to get the volume id associated with the given image id, and check for equality. HOwever, without passing in the set of image ids associated with the volume, I don't see an easy way of doing that. Should I just pass in a Set of the image ids that are preferred and use imageIds.has(imageId)?
|
@sedghi - I've updated the PR with changes that I think should work with the API generally. It isn't perfect, but I think is acceptable as the API should be fairly future compatible. |
|
@wayfarer3130 Looks good to me, thanks a lot ! |
|
@wayfarer3130 can you fix the volumeId and imageid thing i sent in slack and we merge this |
@sedghi - it really wasn't clear what you wanted changed from the comment in slack |
Context
(Remake of #2408)
Currently, even if we set the PT volumeId into the configuration of Rectangle/Circle/EllipticalRoiTools, it will always show the cachedStats of the CT on the fusion viewport.
As we can see here :
Changes & Results
If we set PT volumeId in the tool configuration :

Testing
Example PET-CT
You can modify or delete the prefered volumeId in the tool configuration :
fusionToolGroup.addTool(RectangleROITool.toolName, { volumeId: ptVolumeId });Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment