-
Notifications
You must be signed in to change notification settings - Fork 453
fix(SegmentationState): only activate first segment if it exists #2479
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
| const segmentKeys = Object.keys(segmentation.segments); | ||
| if (segmentKeys.length > 0) { | ||
| firstSegmentIndex = segmentKeys.map((k) => Number(k)).sort()[0]; | ||
| setActiveSegmentIndex(segmentationId, firstSegmentIndex); |
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.
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.
Hey @nagychris, can you tell me more about the problem you're running into, without getting too deep into the code details? For example, are you loading a labelmap, and then seeing something unexpected happen?
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.
Hi @sedghi, the issue is that my application expects the initial state of a segmentation to have no segments, because the user should first select which type of segment they want to create before they can draw. So the issue is related to how the workflow works in my application.
The current workaround I see is to remove this empty segment as soon as the segmentation is loaded, but it's quite hacky, so I wondered if we can remove this again.
But if you say that this empty / default segment is required for other cornerstone3D features, then we can also keep it. Just wanted to raise this topic, as it might also be an issue for other applications using cornerstone3D.
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.
So the issue with your currrent proposal is that sometimes (specifically in RTSS) we skip segments, so the first one becomes segment 2 for instance
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 but that sounds like a more general bug to me, that should not be related to whether you add a default segment here?
Context
This change was already there in a previous version:
cornerstone3D/packages/tools/src/stateManagement/segmentation/internalAddSegmentationRepresentation.ts
Lines 37 to 44 in b1979f6
But was removed again in the current
mainbranch (via a1ed218):cornerstone3D/packages/tools/src/stateManagement/segmentation/internalAddSegmentationRepresentation.ts
Lines 53 to 60 in e7b0cea
-> this breaks the expected behavior of segmentations, as they now always load with an empty segment with index 1, because
activateSegmentIndexis called and this creates the non-existing segment with an empty label''. We should not do this, and rather expect the application to handle the case of an initially empty segmentation.If we add an empty segment automatically, it is harder for the application to handle the initial state for empty segmentations -> they might need to always remove this segment if it exists, e.g., if they want to check if no segment exists yet. Otherwise the user might start drawing with a non-existent segment index.
Changes & Results
Before: empty segmentations are loaded with an empty segment with index 1.
After: empty segmentations are empty after loading (contain no segments).
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment