Unfortunately there are multiple other errors too when building Plastimatch with ITKv5.
I wish some more testing had been done before making the switch, at least trying to build the most downloaded extensions. Now we have to scrape to have a working SlicerRT, which is by far the most downloaded extension. I already got emails about why it is not available. This is an emergency for us that I think would have been avoidable.
For others struggling with the same thing, here is the ITK 5 migration guide: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Documentation/ITK5MigrationGuide.md
Nightly Slicer will be unstable for a while due to many major changes that we are planning to do (Python3…). Can your users stay on the latest stable version?
That’s what I told them. There are not a lot of changes in SlicerRT so they don’t lose much. But we might lose users who try the nightly and they don’t find what they want.
After taking a better look, these errors, although there are many, do not seem hard to fix. At least for us what took the long time was to investigate what broke exactly and to find the time to start the fixes (deadlines etc). Doing the fixes should be now easy, and can be expected in a day or two.
I don’t think it is needed to revert the change unless there are multiple other extensions that rely heavily on ITK as much as SlicerRT (due to Plastimatch mainly).
Based on your finding and implemented fixes, could you add entry to https://www.slicer.org/wiki/Documentation/Nightly/Developers/Tutorials/MigrationGuide#Transition_from_ITK4_to_ITK5
Some of the things are specific to Plastimatch (e.g. I don’t think anyone else uses BioCell), and I find that ITK migration guide is pretty good. Since there is a link already to the full migration guide I think it is enough.
I just saw this post about release of ITK 4.13.2, which made me quite confused about the discussion in this thread. Quoting that post,
The next feature pre-release for ITK 5, ITK 5 Release Candidate 2, is anticipated in a few weeks
If ITK 5 is not yet released, we only have release candidate 1, why is Slicer switching to ITK 5 now?
Part of the motivation was that @phcerdan was available to do the troubleshooting for the needed C++ changes. Also this has been on the roadmap for a long time to upgrade all the dependencies and we planned that the nightly would be undergoing a lot of slicer5 changes (python3 is coming soon too) and that was accepted since we have a 4.10.1 already and plans for a 4.10.2 soon for people who aren’t ready to change. I think it’s fine that we are a bit ahead of the game on ITK. I doubt it’ll change much between RC and release.
I think C++ changes is different from upgrading to a dependency that is in a pre-release stage. I did not find ITK 5 upgrade in the roadmap, maybe I missed it. Maybe it all makes sense to the developers closely involved, but for someone a bit on the outside, switch to a prerelease of a key dependency looks strange.
Personally I don’t think it’s strange at all that the pre-release version of Slicer would be using the almost released version of ITK. You are right that the roadmap does not specifically callout ITK5, but it makes it clear that disruptive changes are coming…
Hi @fedorov, I updated the wiki with the steps taken on the transition. ITKv4->ITKv5 and the ITKv5 Migration Guide
Upgrading external modules should be a matter of upgrading to ITKv5 new threads,
if the ThreadedGenerateData makes no use of
void ThreadedGenerateData( const OutputRegionType& threadRegion, ThreadIdType threadId )
void DynamicThreadedGenerateData( const OutputRegionType& threadRegion )
In other cases, where
threadId is involved, the ITK migration guide has examples, but to just use the ITKv4 threading system add to the ITK class constructor:
that will probably solve 99% of the problems upgrading to ITKv5.
I am sorry the upgrade forced external plugins to catch up…
I went through the extensions on the latest nightly dashboard that did not build. These are the extensions that fail to build due to ITK and the failure seems to be related to version 5 (and not some older issue like itkFactoryRegistration):
- VirtualFractureReconstruction (also failed before for different reasons)
All of these issues are related to these few things:
- Changed constants such as ITK_THREAD_RETURN_TYPE
- The abovementioned multi-threading changes
- Changed vnl functions (e.g. vnl_math_isinf or vnl_math_abs)
- Deprecated vcl functions (e.g. vcl_cstdio.h or vcl_vector)
- Changed image initialization ( conversion from ‘int’ to ‘const itk::SmartPointer<itk::Image<float, 3> >’ is ambiguous) - I haven’t met this one when fixing SlicerRT but I think this should be new as well
I volunteer fixing SlicerElastix as I’m planning to work on it regarding the node references I’m adding to all registration modules.
Thanks @cpinter! You probably know this, but @lassoan checked the elastix repo and they were actively porting to itk5 so maybe it’s just a matter of changing the superbuild hash.
Just an update that I issued a PR to Elastix to fix the remaining build errors. Once this is integrated I’ll update the hash.
This is great, thank you! Until they integrate the fix, you can fork the Elastix repository and use that fork for nightly builds. It would allow us to start testing Elastix build on all operating systems.
Looks like there’s a small conflict between @cpinter’s patch and the one @N-Dekker did. If it doesn’t get resolved in a few days then yes, doing temporary Slicer-specific elastix fork would be a good idea.
Just returned from travelling. I’ll take care of this tomorrow. Thanks for the patience