ENH: Add widget to specify labelmap geometry in segmentations #975
Conversation
cpinter
commented
Jun 20, 2018
cpinter
commented
- Add qMRMLSegmentationGeometryWidget that allows the user to specify a volume/segmentation/model/ROI as source, and set geometry details
- For segmentation with labelmap master it is possible to set isotropic spacing and oversampling factor. Spacing cannot be set manually in these cases
- For model, ROI, or segmentation with poly data master the parent transform is used for orientation, user input for spacing, and the bounding box for origin and extent
- Add qMRMLSegmentationGeometryDialog that has a geometry widget in it, and OK and Cancel buttons if editing is enabled, and an OK button otherwise. If resampling is requested, then besides setting the reference image geometry conversion parameter, the existing segments are resampled to the specified geometry
- Add button in row of master volume in Segment Editor widget that opens the geometry dialog
- Replace button in conversion parameters dialog for specifying reference image geometry. Before it was a simple dialog that allowed selecting a volume, now the new geometry dialog shows up
- Add python test for testing all kinds of scenarios for segmentation labelmap geometry calculation
- Crash fixed in vtkCalculateOversamplingFactor: when factor was very low, invalid extent was calculated and then crashed when calculating spacing based on that
|
Button in Segment Editor. Brings up the geometry dialog. When OK is clicked then resampling happens: Button in conversion parameters dialog. Brings up the geometry dialog. Only the conversion parameter is set when OK is clicked: When opening the dialog, the current geometry is shown: If volume or segmentation with labelmap master is chosen as source. Spacing and directions come from the volume, but isotropic spacing or oversampling can be specified: Segmentation with closed surface master, model node, or ROI node can also be selected. Spacing can be manually specified, and the orientation (parent transform) of the source node is considered: |
|
Thank you, this will be awesome! I've found a couple of smaller problems:
|
|
This looks great! |
|
I can definitely see how this can be a very helpful functionality! 👍 Did you consider making this widget not limited to segmentations? I think it might also be helpful sometime to do this kind of resampling operations for image volumes, and I don't think there is a straightforward way to do it right now in Slicer. |
For volume nodes, Crop volume module can do almost the same (I've completely reworked it about a year ago). Is there anything specific that you think should be added there? |
|
@lassoan yes, I agree it can do it, but it requires annotation ROI, while this widget would be more efficient for the tasks when you just want to resample the volume within the original FOV. It is just a more streamlined approach. I really don't want to stall this PR, just wanted to mention this another potential utility - if you agree, and if it is not a lot of work, might make sense to add. But should not be a stopper, and can be considered later. |
|
Yes, this discussion does not affect this PR (maybe we should move it to Discourse or the issue tracker). If you don't specify a ROI then Crop volume module creates the ROI automatically and fits it to the input volume. Resample scalar volume can be used for this, too, but we've found that Crop volume is a better place, as when you oversample an image, you usually also want to crop it, to keep number of voxels under control. I think the main problem is that there are about 4 modules for resampling volumes and you can also resample using Crop volume and Transforms modules. So there are 6 modules in total, with slightly different capabilities to do the same thing, which is of course quite confusing. |
|
Right, so the issues with CropVolume:
My suggestion was exactly along the lines of reducing the confusion. Geometry update operation is not uncommon for volumes, there is no straightforward way to do it, and now we introduce yet another way to do resampling, but it would only work for segmentations. :-/
|
|
Thanks for the feedback! @lassoan I'm not sure what's the problem with comment 7. What would you like to see instead? Elongated matrix widget looks very strange, I think the empty space is much better. I'm not sure when I can work on this, as we leave in a few hours, but hopefully on the bus/plane. |
For me, it looks ugly. If there is no element that would look OK as expanded then we should prevent expansion. This is certainly not a blocking issue, it can be addressed later. |
OK, you can use this to resample a segmentation, but it is not intended for that. It is for defining geometry of labelmap representation before segmentation is started. |
|
I could rename the widget to qMRMLVolumeGeometryWidget, and put it to Libs/MRML/Widgets, so that later when we add this to other modules, then it's more proper. |
I cannot really specify a maximum height because of the different DPIs that we support now. If it's ugly, then you shouldn't resize it to so high. Or did it end up like this on its own? |
|
We can make the dialog fixed height, with size retrieved from layout->totalMinimumSize().height(). It is done like this in QMessageBox (in QMessageBoxPrivate::updateSize()) when you click the "Show details" button:
|
6, I'd probably just disallow 0 or negative values for spacing. How about changing the minimum value to 0.05mm? +1. I cannot move the widget as is to MRML/Widgets, because of the ROI support (the ROI node is defined in the Annotations module). I have to deal with this when I get to doing this refactoring. I would hesitate to move any Annotations class into Libs, because of the "temporary state" of the module. I'm open to suggestions. +2. I wanted to make the MRMLCoordinateWidgets show mm, as it is done in the Volumes module, but the obvious ways didn't work. Does anyone happen to know what's the trick? I'll address the rest of Andras' comments now. |
cpinter
force-pushed
the
segment-editor-geometry-widget
branch
from
e21cd6d
to
394c182
Compare
+1. We might be able to use displayable node and GetBounds() method, but I think we can generalize later, when we actually need the widget somewhere else. +2. You need to set the MRML scene (and of course quantity). Fix whatever you can and let me know when you've run out of time and then I'll take over. |
|
There are some build warnings and errors on CI: /usr/src/Slicer/Modules/Loadable/Segmentations/Widgets/qMRMLSegmentationConversionParametersWidget.cxx: In member function ‘void qMRMLSegmentationConversionParametersWidget::onSpecifyGeometryButtonClicked()’: |
cpinter
force-pushed
the
segment-editor-geometry-widget
branch
2 times, most recently
from
34c133f
to
a61216f
Compare
|
I believe I addressed all issues. Let me know if further changes are needed. Thanks! |
| @@ -463,7 +463,7 @@ void vtkCalculateOversamplingFactor::ApplyOversamplingOnImageGeometry(vtkOriente | |||
| { | |||
| int dimension = extent[axis*2+1] - extent[axis*2] + 1; | |||
| int extentMin = static_cast<int>(ceil(oversamplingFactor * extent[axis * 2])); | |||
There was a problem hiding this comment.
shouldn't this be floor for minimum extent; and ceil for maximum extent?
There was a problem hiding this comment.
I'm not sure as I did it long ago, but back then I tested everything with the tiny patient and checked voxel by voxel, so this should be OK. If you want to double check, feel free to do it, write a test, or add a ticket.
There was a problem hiding this comment.
I was just wondering if it may crop boundary voxels. I don't remember having seen any unwanted cropping, but also I did not use oversampling that much. Now that it'll be more easily available, I'll have more chance to use it and confirm that it works well.
| static const char* GetSegmentationClosedSurfaceRepresentationName() { return "Closed surface"; }; | ||
| static const char* GetSegmentationPlanarContourRepresentationName() { return "Planar contour"; }; | ||
| static const char* GetSegmentationClosedSurfaceRepresentationName() { return "Closed surface"; }; | ||
| static const char* GetBinaryLabelmapRepresentationName() { return GetSegmentationBinaryLabelmapRepresentationName(); }; |
There was a problem hiding this comment.
Maybe we should make the old methods deprecated? We should at least document which methods should be used, but even better, we could log a warning if the deprecated methods are used to discourage their usage.
There was a problem hiding this comment.
OK I can do it, but later (when I'm back) and in a separate commit, as these functions are called from several extensions as well, and I don't want to introduce warnings in them.
I did this because I realized that the "segmentation" in their name was unnecessary and they were super long.
There was a problem hiding this comment.
OK. For now, it is enough to add comments that tell which method names are preferred.
| return False | ||
| import numpy | ||
| tolerance = 0.0001 | ||
| actualSpacing = orientedImageData.GetSpacing() |
There was a problem hiding this comment.
spacing, orientation, and directions comparison could be simplified by using
static bool vtkAddonMathUtilities::MatrixAreEqual(const vtkMatrix4x4* m1, const vtkMatrix4x4* m2, double tolerance = 1e-3);
There was a problem hiding this comment.
I know. However I wanted to be able to log error messages that are more punctual and are easier to interpret.
There was a problem hiding this comment.
OK. If this comes up again then we may create a shared helper function in vtkAddon or slicer.util that can provide these more descriptive messages.
| #TODO: Come up with proper geometry baselines for these too | ||
| vtkSegmentationCore.vtkOrientedImageDataResample.ResampleOrientedImageToReferenceOrientedImage( | ||
| segmentOrientedImageData, geometryImageData, geometryImageData, False, True) | ||
| self.assertTrue(self.imageDataContainsData(geometryImageData)) |
There was a problem hiding this comment.
Would it be possible to compare the number of non-empty voxels? It would catch much more potential bugs for sure, but of course it is only usable if it is not too fragile.
There was a problem hiding this comment.
OK. This should be easy to add.
| this, SLOT(onSetReferenceImageGeometryFromVolumeClicked())); | ||
| geometryLayout->addWidget(setGeometryFromVolumeButton); | ||
| QPushButton* specifyGeometryButton = new QPushButton(tr("Specify geometry"), geometryWidget); | ||
| //setGeometryFromVolumeButton->setFixedWidth(160); |
There was a problem hiding this comment.
if not needed then it should be probably deleted
There was a problem hiding this comment.
I left it there because it was not obvious what will be the proper solution. You hated the long widget after resizing it manually, but I'm not sure this is better.
There was a problem hiding this comment.
It would be great if you could add a comment explaining why the code was left there, what it can be used for, etc.
There was a problem hiding this comment.
I'll remove this. I thought this is for the height issue, but it's not. Thanks
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| bool qMRMLSegmentationGeometryWidgetPrivate::isSourceASegmentationWithBinaryLabelmapMaster() |
There was a problem hiding this comment.
Should we keep "A" after isSource? I've seen "IsA" in code, but usually there is no "A" or "The" in names. It could be isSourceSegmentationWithBinaryLabelmapMaster.
| std::string binaryLabelmapName = vtkSegmentationConverter::GetBinaryLabelmapRepresentationName(); | ||
| vtkSmartPointer<vtkOrientedImageData> sourceBinaryLabelmap; | ||
| if ( sourceSegmentationNode | ||
| && sourceSegmentationNode->GetSegmentation()->GetNumberOfSegments() > 0 |
There was a problem hiding this comment.
sourceSegmentationNode->GetSegmentation() can be nullptr, so it would be safer to check that
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| void qMRMLSegmentationGeometryWidgetPrivate::matchInputAndSourceAxes(vtkMRMLNode* sourceNode) |
There was a problem hiding this comment.
"match" does not indicate what this method actually does. "ComputeSourceAxisIndexForInputAxis" or something like that would be more informative.
| { | ||
| qDebug() << Q_FUNC_INFO << "Master representation '" << masterRepresentationName.c_str() << "' in each segment of segmentation " | ||
| << d->SegmentationNode->GetName() << " has been resampled to specified geometry"; | ||
| d->SegmentationNode->Modified(); |
There was a problem hiding this comment.
don't we need to do this even if some of the resamplings fail?
| Q_D(qMRMLSegmentationGeometryWidget); | ||
| if (!outputGeometry) | ||
| { | ||
| return; |
There was a problem hiding this comment.
should we set d->GeometryImageData to identity or something like that?
There was a problem hiding this comment.
This is a get function. I don't understand why we should reset it after getting it.
cpinter
force-pushed
the
segment-editor-geometry-widget
branch
from
a61216f
to
96e332b
Compare
|
Thanks Andras! I'll think about the logic functions and issue a PR depending on what seems most reasonable. |
|
@lassoan How would you imagine adding logic access? A new logic class in Segmentations/Logic or MRML/Logic that the widget would call? |
- Add qMRMLSegmentationGeometryWidget that allows the user to specify a volume/segmentation/model/ROI as source, and set geometry details - For segmentation with labelmap master it is possible to set isotropic spacing and oversampling factor. Spacing cannot be set manually in these cases - For model, ROI, or segmentation with poly data master the parent transform is used for orientation, user input for spacing, and the bounding box for origin and extent - Add qMRMLSegmentationGeometryDialog that has a geometry widget in it, and OK and Cancel buttons if editing is enabled, and an OK button otherwise. If resampling is requested, then besides setting the reference image geometry conversion parameter, the existing segments are resampled to the specified geometry - Add button in row of master volume in Segment Editor widget that opens the geometry dialog - Replace button in conversion parameters dialog for specifying reference image geometry. Before it was a simple dialog that allowed selecting a volume, now the new geometry dialog shows up - Add python test for testing all kinds of scenarios for segmentation labelmap geometry calculation - Crash fixed in vtkCalculateOversamplingFactor: when factor was very low, invalid extent was calculated and then crashed when calculating spacing based on that
cpinter
force-pushed
the
segment-editor-geometry-widget
branch
from
96e332b
to
ed284b2
Compare
|
I made a logic class that does the calculation. A quick test didn't bring out any problems, and the automatic test passes so all the cases should still work. I think I'll only generalize it to volumes later. It won't be as simple, as in MRML/Widgets there are some classes that are not accessible, so it needs some more thought. If there are no objections, I'll integrate it tomorrow. |
|
Integrated in Slicer/Slicer@6a8012e |
