Conversation
Add option to choose segmentation editable area
Thibault-Pelletier
left a comment
Thibault-Pelletier
left a comment
There was a problem hiding this comment.
Your PR is also missing at least one unit test:
- example / smoke test
- non regression for the added segment editor API changes
| OUTSIDE_ALL_VISIBLE_SEGMENTS = "EditAllowedOutsideVisibleSegments" | ||
|
|
||
|
|
||
| class SegmentOverwriteMode(enum.Enum): |
There was a problem hiding this comment.
Enum isn't consistent with other enums in the app core esp SegmentationEditableAreaEnum
Enum should use UPPER_SNAKE_CASE
There was a problem hiding this comment.
Is ModificationMode above an exception or should I modify it too ?
There was a problem hiding this comment.
No, modification mode shouldn't be an exception and you can update it too.
To avoid breaking code already using the enum, it should be updated to :
class ModificationMode(enum.IntEnum):
SET = auto()
ADD = auto()
REMOVE = auto()
REMOVE_ALL = auto()
Set = SET #: Deprecated alias for SET
Add = ADD #: Deprecated alias for ADD
Remove = REMOVE #: Deprecated alias for REMOVE
RemoveAll = REMOVE_ALL #: Deprecated alias for REMOVE_ALL
Edit: and in all of trame-slicer, you can update to use the up to date SNAKE_CASE enum
|
|
||
| def set_masking_parameters(self, parameters: SegmentMaskingParameters) -> None: | ||
| if not parameters.segment_id: | ||
| mask_mode = self.active_segmentation.segmentation_node.ConvertMaskModeFromString( |
There was a problem hiding this comment.
Why convert from string and not using directly the right enum values in the enum?
This would remove the need for the active segmentation node since this is a static conversion (segmentation node can be None as it can be inactive and editor node has a active_segmentation_node field BTW).
Logic could then be simplified to:
def set_masking_parameters(self, parameters: SegmentMaskingParameters) -> None:
self.editor_node.SetMaskSegmentID(parameters.segment_id)
self.editor_node.SetMaskMode(parameters.editable_area.value)
self.editor_node.SetOverwriteMode(parameters.overwrite_mode.value)class SegmentationEditableArea(enum.Enum):
EVERYWHERE = vtkMRMLSegmentationNode.EditAllowedEverywhere
INSIDE_SINGLE_SEGMENT = vtkMRMLSegmentationNode.EditAllowedInsideSingleSegment
INSIDE_ALL_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedInsideAllSegments
INSIDE_ALL_VISIBLE_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedInsideVisibleSegments
OUTSIDE_ALL_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedOutsideAllSegments
OUTSIDE_ALL_VISIBLE_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedOutsideVisibleSegments
@dataclass
class SegmentMaskingParameters:
editable_area: SegmentationEditableArea = SegmentationEditableArea.EVERYWHERE
segment_id: str = ""
overwrite_mode: SegmentOverwriteMode = SegmentOverwriteMode.OVERWRITE_ALL
def __post_init__(self):
if self.segment_id:
self.editable_area = SegmentationEditableArea.INSIDE_SINGLE_SEGMENT
There was a problem hiding this comment.
Using the string made the actual use in the example easier as you can see in the commit I just pushed, but I suppose keeping the logic simpler is more important.
| def get_masking_parameters( | ||
| self, | ||
| ) -> SegmentMaskingParameters | None: | ||
| if self.active_segmentation is None: |
There was a problem hiding this comment.
Same, this can be simplified to:
def get_masking_parameters(self) -> SegmentMaskingParameters | None:
return SegmentMaskingParameters(
editable_area=SegmentationEditableArea(self.editor_node.GetMaskMode()),
segment_id=self.editor_node.GetMaskSegmentID(),
overwrite_mode=SegmentOverwriteMode(self.editor_node.GetOverwriteMode()),
)There was a problem hiding this comment.
I don't agree, the editor node could still hold the mask segment ID while the mask mode has been changed from INSIDE_SINGLE_SEGMENT to another value since.
There was a problem hiding this comment.
I think we have two options:
- Have the dataclass be a simple wrapper around the segmentation editor node masking parameters to simplify setting / getting related data at once
- Have the dataclass handle the default behavior when the set parameters will not have an expected result
I think the first case is easier to handle which removes the need for a post_init method as well in the dataclass.
Regarding the dataclass itself, I was mistaken when suggesting to group all 3 values. I think it would make more sense to keep only segment ID and editable area and have overwrite mode separate.
If we do that, then the example can be simplified by using the TypedState.encode method for the values of the SegmentMaskingParameters and construct it with the correct values.
| RemoveAll = auto() | ||
|
|
||
|
|
||
| class SegmentationEditableArea(enum.Enum): |
There was a problem hiding this comment.
This should use the segment editor enum values instead of string.
class SegmentationEditableArea(enum.Enum):
EVERYWHERE = vtkMRMLSegmentationNode.EditAllowedEverywhere
INSIDE_SINGLE_SEGMENT = vtkMRMLSegmentationNode.EditAllowedInsideSingleSegment
INSIDE_ALL_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedInsideAllSegments
INSIDE_ALL_VISIBLE_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedInsideVisibleSegments
OUTSIDE_ALL_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedOutsideAllSegments
OUTSIDE_ALL_VISIBLE_SEGMENTS = vtkMRMLSegmentationNode.EditAllowedOutsideVisibleSegments| overwrite_mode = options_state.overwrite_mode | ||
| if editable_area in self.segmentation_editor.get_segment_ids(): | ||
| parameters = SegmentMaskingParameters( | ||
| editable_area=None, |
There was a problem hiding this comment.
None is not an accepted value in your masking parameter type
The dataclass has default, so instead you should use SegmentMaskingParameters(overwrite_mode=overwrite_mode, segment_id=editable_area)
| ) | ||
| with VCardText(v_if=(self._typed_state.name.is_extended,), classes="align-center"): | ||
| default_editable_area_options = [ | ||
| {"text": label, "value": enum.value} for enum, label in self._default_editable_areas_labels.items() |
There was a problem hiding this comment.
Should use self._typed_state.encode(enum) instead of manual encoding
| VSelect( | ||
| v_model=self._typed_state.name.overwrite_mode, | ||
| items=( | ||
| [{"text": label, "value": enum.value} for enum, label in self._overwrite_mode_labels.items()], |
There was a problem hiding this comment.
Should use self._typed_state.encode(enum) instead of manual encoding
2 participants
Add segmentation "Editable Area" and "Overwrite Control" options