Skip to content
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

WIP: Update build system introducing Slicer_BUILD_SEGMENTATION_SUPPORT option #849

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jcfr
Copy link
Member

@jcfr jcfr commented Nov 30, 2017

This allows to build Slicer without segmentation support. This is
particularly useful when building custom application based on Slicer.

…RT option

This allows to build Slicer without segmentation support. This is
particularly useful when building custom application based on Slicer.
@jcfr
Copy link
Member Author

jcfr commented Nov 30, 2017

Cc: @lassoan @pieper

@lassoan
Copy link
Contributor

lassoan commented Nov 30, 2017

I don't really mind making it optional other than this is yet another thing to keep in mind when writing Slicer core code: we cannot assume that Segmentation classes are available but have to add #ifdefs around each usage. It is particularly error-prone in Python code, where you don't get compile-time errors.

What was the main motivation? Is Segmentation that big, takes too long to build, causes slowdown anywhere when it is not used, or it is just nicely separated from the rest of the application and so it is easy to make it optional?

We still have unused/rarely used modules and features in the Slicer core. To make Slicer leaner, we could remove those.

@pieper
Copy link
Member

pieper commented Nov 30, 2017

Yes, this strikes me as a bit odd - with this patch if I turn of segmentation support I'll still build classes like Libs/MRML/Widgets/qMRMLSegmentSelectorWidget.cxx but with methods like void qMRMLSegmentSelectorWidget::setSelectedSegmentIDs(QStringList segmentIDList)
that are now noops? What's the point of that?

If we really want to be able to build without segments it seems we'd want to move these classes into a different library and just exclude it from the build.

Speaking a bit from my experience the past few days at RSNA I can say that the segment editor is very popular and very general purpose so I don't see a lot of motivation for excluding it from a build. Things like FreeSurfer support or even diffusion volume support are much less "core" in comparison.

@jcfr
Copy link
Member Author

jcfr commented Nov 30, 2017

or it is just nicely separated from the rest of the application and so it is easy to make it optional?

yes.

qMRMLSegmentSelectorWidget::setSelectedSegmentIDs(QStringList segmentIDList)
that are now noops?

This was the simplest way to get a PR working for review. Without having to update the UI file associated with the Slice controller.

segment editor is very popular

Agreed. When the custom Slicer application is used for segmentation, it is definitively relevant to have the corresponding module available.

I may streamline the Pr and simply exclude the modules without doing anything to MRML

For now, I am marking the Pr as "work in progress"

@jcfr jcfr added the wip label Nov 30, 2017
@jcfr jcfr changed the title COMP: Update build system introducing Slicer_BUILD_SEGMENTATION_SUPPORT option WIP: Update build system introducing Slicer_BUILD_SEGMENTATION_SUPPORT option Nov 30, 2017
@jcfr jcfr added wip and removed wip labels Feb 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants