search

SlicerRt / SlicerRT

Open-source toolkit for radiation therapy research, an extension of 3D Slicer. Features include DICOM-RT import/export, dose volume histogram, dose accumulation, external beam planning (TPS), structure comparison and morphology, isodose line/surface generation, etc.
https://slicerrt.org
127 stars 60 forks source link

ENH: Add support for merged binary labelmap segments #117

Closed Sunderlandkyl closed 4 years ago

Sunderlandkyl commented 4 years ago

Binary labelmap representations can now support multiple segments in the same vtkOrientedImageData. This commit replaces some instances of segment->GetRepresentation("Binary labelmap") with segmentationNode->GetBinaryLabelmapRepresentation(), or the use of vtkImageThreshold to isolate the binary labelmaps for individual segments.

To be merged once https://github.com/Slicer/Slicer/pull/1221 has been merged.

cpinter commented 4 years ago

Minor comments, otherwise looks good (I assume you ran the tests and at least tried the DVH module manually with different settings). Can be committed after the Slicer PR went in. Thank you!

Sunderlandkyl commented 4 years ago

Thanks for reviewing! I ran all of the SlicerRT tests and everything passed.

cpinter commented 4 years ago

If you haven't done it already, please do some manual testing in DVH, trying all options. Thanks!

Sunderlandkyl commented 4 years ago

The function signature for GetBinaryLabelmapRepresentation and GetClosedSurfaceRepresentation is now:

 virtual void GetBinaryLabelmapRepresentation(const std::string segmentId, vtkOrientedImageData* outputBinaryLabelmap);
cpinter commented 4 years ago

Thanks! I think it will be safer this way

lassoan commented 4 years ago

@cpinter Do you use this same branch for Slicer-4.10?

lassoan commented 4 years ago

Merged in https://github.com/SlicerRt/SlicerRT/commit/2f77010cf96d6d9fef27c8bf02ca319810e9555a. Added #ifdefs to make it work with both Slicer-4.10 and Slicer-4.11 - I did not have a chance to test it, so it would be great if @Sunderlandkyl you could have a look at the dashboard tomorrow and/or test if it works as expected.

cpinter commented 4 years ago

Thanks for merging! @lassoan There has been no need for a separate branch so far. The ifdefs are a good solution, but I wouldn't have minded fixing to the commit before this one; the next stable is around the corner.