Page MenuHomePhabricator

[MultiLabel Segmentation] 3D Threshold tool does not work with multiple labels / layers
Closed, ResolvedPublic

Description

Using the Multilabel segmentation plugin view and the 3D Threshold tool, a label set image with multiple layers / labels is not correctly handled.

If using multiple labels on the same layer, the color of the first label is always used for a segmentation mask.
If using multiple layers, a Threshold segmentation on a layer != 0 leads to a workbench crash.

To reproduce:

  1. Load a 3D image, open the multilabel segmentation plugin view, create a new label set image
  2. create a new label on the first layer (automatic popup dialog)
  3. create another label on the first layer (distinct color)
  4. create a threshold segmentation mask on the first label
  5. create a threshold segmentation mask on the second label
    1. the second label will have the same color as the first label
  6. create a new layer and create a third label on that layer (automatic popup dialog)
  7. select a specific color for the third label
  8. use the threshold tool on the third label (second layer)
    1. MITK workbench crashes in mitk::BaseProperty *mitk::PropertyList::GetProperty(const std::string &propertyKey) const with this being a nullptr.

I did not look closer into it but I suspect that this is similar to other recently discovered bugs where the current layer is not considered and the current pixel value / label does not exist on the first layer.
I'm not exactly sure if this is related, but maybe these layer / label problems need to be kept in mind while working on T28118: Refactoring of SegTool2D.

Revisions and Commits

rMITK MITK
Restricted Differential Revision
Restricted Differential Revision
Restricted Differential Revision

Related Objects

StatusAssignedTask
OpenNone
Resolvedkalali
Resolvedfloca
Openfloca
Resolvedfloca
Resolvedfloca
Resolvedfloca
DuplicateNone
Resolvedfloca
OpenNone
Resolvedfloca
WontfixNone
Resolvedkislinsk
Resolvedkislinsk
WontfixNone
Openfloca
Resolvedfloca
Openfloca
Resolvedfloca

Event Timeline

kalali triaged this task as Normal priority.Dec 22 2020, 12:53 PM
kalali created this task.

@floca I sarted looking into this and found out the following:
The threshold tool used is QmitkBinaryThresholdToolGUIBase, which is a subclass of QmitkAutoSegmentationToolGUIBase. This class uses the AutoSegmentationTool, a subclass of the mitkTool. As far as I can see the target node that will be connected with this tool upon activating the tool is done inside mitk::AutoSegmentationTool::GetTargetSegmentationNode().
However, there is no information about any layer or even the information that this can be used with multilabel segmentation nodes.

This probably went unnoticed (was not mentioned in T28118: Refactoring of SegTool2D) because the 3D tools work with the classic segmentation and the 2D tools work with the multi label segmentation. There is only this single 3D multi label segmentation tool: 3D Threshold. This 3D Threshold tool seems to be the only AutoSegmentationTool available for multi label segmentation.
Before refactoring this one I think we should discuss T28142: [Segmentation] Remove plugin redundancy with MultiLabelSegmentation.

Great you are looking into that matter.
The problem is caused by mitk::AutoSegmentationWithPreviewTool::TransferImageAtTimeStep() more precisely in the private helper template function ITKSetVolume (same cpp).

This helper is used in case of 3D tools and bluntly overwrites the whole volume. This works with simple images, but obviously makes problems when using a real mult lable situations.
So this is something that should be reworked. As there seems to be now valid code for our 3D segmentations how to correctly integrate a segmentation (preview) result in to a multi label image.
The only code that does it correct (as far as I can see for now; but might not be optimally efficient) is mitk::FeedbackContourTool::WriteBackFeedbackContourAsSegmentationResult(...). It takes a contour, converts it into a segmentation, generates the content regarding the labels and so and correctly adds it to the multi label image.

But in the end, even after first refactoring the tools, most of this "get the segmentation finally into the (label) image is still quite a mess. I not changed it a lot, only made some changes, as I wanted not to shake up to much at once, to have a chance to find sources of problems if they arise in the refactoring.
So it totally would make sense (also in the light of T28142) to tackle now that array and have definitive functions (for 3D and 2D-Slices) that do the updates/overwrite of segmentations in a LabelSetImage and use it in all program paths of segmentation.
Remark: The very code that takes a binary segmentation and adds it compliant to the labels set rules to the label set, can be found here: ContourModelUtils::FillSliceInSlice().
I personally find it misplaced there, but while I was refactoring the tools last year I just wanted to reduce the number of code parts doing the same; without shaking up to much things.

Hope that helps a bit. If not we can have a session together to dig into the whole complex, so that you can plan your victory. ;)

Thanks for the clarification. I will look into the FeedbackCntourTool and ContourModelUtils and see if I can make sense of all this :D
Depending on what I find out I think we should really talk about the future of the (multilabel) segmentation modules / plugins and how we can simplify the code base (generalize it) and remove redundancy as well as decouple classes etc.

Ok I have to admit: The whole code base that is related to this topic is totally unclear to me and I find it very hard to understand what is going on. I don't know why the code is like that but I would highly suggest to simplify and clean the relevant classes and functions as much as possible before it becomes problematic (e.g. more complex cases with 4D-dynamic segmentations with multi-layer multi-label segmentation masks or similar). Also, I highly doubt that this is future proof when it comes to changes in the following years so as already mentioned in my last comment: I think we really need to discuss this topic - maybe it helps to have a clearer picture by creating (class) diagrams with relations etc.

What I found out: There is a function mitk::SegTool2D::WriteSliceToVolume that is used by the mitk::AutoSegmentationWithPreviewTool::TransferImageAtTimeStep function to write the slice information. However, there is nothing written about any pixel value. The only occurrence of pixelValue is inside mitk::SegTool2D::WritePreviewOnWorkingImage, but this function is not used anywhere in the code base :D I don't know why it has been created in the first place.

Ok I have to admit: The whole code base that is related to this topic is totally unclear to me and I find it very hard to understand what is going on.[...]

Told you so. ;) As proposed we can have a session together.

I don't know why the code is like that

You haven't seen it before the first refactoring round. Beliefe me, it was even more entangled, dublicated and (quasi-)redundant. 🙈

but I would highly suggest to simplify and clean the relevant classes and functions

100% agreed. Never said somethin else.

it becomes problematic (e.g. more complex cases with 4D-dynamic segmentations with multi-layer multi-label segmentation masks or similar).

One should seperate these to problem areas. At least in the refactored tools there is always a base class that soundly manages 4D cases. Only the low level function have to be handled with care.
But e.g. every class derived from AutoSegmentationWithPreviewTool or FeedbackContourTool should be pretty much handled correctly if they use the public/protected interface correctly.
Remark: All left 4D problems known to mee, are comming from tools that are not refactored.

Multi-layer / multi-label support thats the major not throughly handled point with too much threads/styles left. This is the next big point to address, I think.

I think we really need to discuss this topic

Honestly, I think there is enough awareness in the core team for the relevance of the situation. So that is not the case. It is about starting somewhere to cut through the gordian knot. My next step would be to reduce the update of a multilable segmentation to one optimized piece of code, that is used by every tool (directly or beneath several abstraction levels) or ControurModel.

  • maybe it helps to have a clearer picture by creating (class) diagrams with relations etc.

Here it would help to get concrete feedback what are you missing in the documentation of the refactored class. In the end this is what doxygen should provide now. If it does not, we should try to improve the class documentation/naming directly instead of drafting diagrams.

What I found out: There is a function mitk::SegTool2D::WriteSliceToVolume that is used by the mitk::AutoSegmentationWithPreviewTool::TransferImageAtTimeStep function to write the slice information. However, there is nothing written about any pixel value.

It is, but hidden. It took me some time to understand/see it last year.
It happens in the mitk::ExtractSliceFilter because the mitkVtkImageOverwrite member was set to overwrites. This in the end turns the extract filter into an overwrite filter and the slice pixel values are copied into the input. But this is all done with out taking care of multi lable etc...

The only occurrence of pixelValue is inside mitk::SegTool2D::WritePreviewOnWorkingImage,

You also have occurences in the FeedbackContourTool.

but this function is not used anywhere in the code base :D I don't know why it has been created in the first place.

Because it became obsolete while the refactoring and it seems like I have forgotten to remove it know.
As said there very plenty of redundancies you haven't even seen and I tried to steadly making my way trough and improving it bit by bit.

Don't get me wrong - I'm not saying that you or someone "verschlimmbesserte" anything :D I understand and appreciate all the effort that has been made by you.
I am more thinking of a different approach architecture wise - you already mentioned "abstraction levels". For me it seems as if different classes and functions are on the same "level" of doing something but they do it differently - which makes it even harder to grasp the logic behind it. That's why I mentioned "class diagrams". Also, often classes and functions are way too overloaded (e.g. SegTool2D) with way too many responsibilities.

So basically we are on the same page, I suggest to address this tomorrow. When I say "we should discuss this" I was thinking in the direction: Who does it and where to start :D

Thanks for the clarification. I agree 100%.

Unknown Object (User) added a subscriber: Unknown Object (User).May 5 2021, 7:53 AM
This comment was removed by kislinsk.

Was also meantioned several times be testers this week.

kalali added a revision: Restricted Differential Revision.Feb 18 2022, 5:49 PM

This task is fixed with the related differential. The other topic that was discussed here (overwriting labels) will be addressed here: T28991: [Segmentation] 3D tools overwrite / remove previous mask if reconfirmed.