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

ENH: VTK Superbuild Update #193

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

mseng10
Copy link
Collaborator

@mseng10 mseng10 commented Feb 9, 2021

Description

This PR will be the first step in getting the Superbuild's libraries updated to their respective latest versions. In order to avoid a heavy PR, this will strictly contain ITK and VTK changes.

Versions Changed

  • ITK abd38d5a0040b9a8fbb0ad3127089dbb72a93342 (same as GitHub Actions)
  • VTK 9.0.1

LevelSetV4Visualization

The examples located in src/Bridge/VtkGlue that utilize the (deprecated in ITK v5.0) ITKLevelSetV4Visualization module, now require the ITK version to be less than 5.0.

VTK Configuration

Several of the examples needed to have their CMakeLists.txt reconfigured. In addition, a few of them did not even have VTK utilized in their respective CMakeLists.txt, despite utilizing VTK in their cxx script.

After reconfiguring, I am able to get a clean build, but it has resulted in 63 failing tests. However, many of these can be attributed to a single error when using the VtkGlue module:

7987B]vtkImageSliceMapper.cxx:36    WARN| Error: no override found for 'vtkImageSliceMapper'.

I saw a mention of this issue in InsightSoftwareConsortium/ITK#739. I am unsure of the exact steps to fix this, but I will continue to investigate. If anyone has knowledge of this error, please reach out.

Note: The PR will remain a draft until the Superbuild builds cleanly and passes all tests for VTK 9.0.1.

@dzenanz
Copy link
Member

dzenanz commented Feb 9, 2021

I had problems with VTKGlue for a long time, and it is broken in my nightly build.

@dzenanz
Copy link
Member

dzenanz commented Feb 10, 2021

Perhaps @mathstuf could provide some advice?

Copy link

@mathstuf mathstuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should provide COMPONENTS to your find_package(VTK) calls. Note that without components and use of ${VTK_LIBRARIES}, you are linking to all of VTK, not just what you need.

src/Core/Common/FindMaxAndMinInImage/CMakeLists.txt Outdated Show resolved Hide resolved
@mseng10 mseng10 changed the title ENH: ITK and VTK Superbuild Update ENH: VTK Superbuild Update Feb 11, 2021
These examples only compile with ITK 4.13 and below as they incorporate the ITKLevelSetV4Visualization module. This modules was moved in ITK 5.0.
Presently, all of the examples that utilize QuickView are disabled if
QuickView is not enabled. However, these examples strictly use QuickView
for visualization purposes and don't contribute to the core
functionality of the program.

These changes primarily change 2 types of CMakeList.txt files: the ones
for a module and those for a specific example.

For modules, it will remove if(ENABLE_QUICKVIEW) to simply just enable
the example. If some of the examples strictly used VTK and not
QuickView, I replaced it with if(VTK_DIR).

For examples, it will first check if QuickView is enabled. If so, that
will find the VTK package and only utilize the components we need which
primarily is: vtkRenderingCore and vtkRenderingGL2PSOpenGL2. If the VTK
version is less than 8.9, then the VTK_USE_FILE is included. Finally, it
links the VTK_LIBRARIES, and intiates the VTK module if the VTK version is atleast 8.9.
Many of the examples that utilize VTK were not configured correctly or
needed to be updated to accomodate VTK 9.1.0.

This primarily included, finding the VTK package and only using the
components that specific example needed. If the VTK version is less than
8.9, then the VTK_USE_FILE is included. Finally, the VTK_LIBRARIES are
linked and if the VTK version is atleast 8.9, then the module is
inialized using VTK's vtk_module_autoinit().
@mseng10
Copy link
Collaborator Author

mseng10 commented Feb 23, 2021

@thewtex @dzenanz After some time, I was able to make significant changes to accommodate examples that utilize VTK/QuickView in some form. In addition to my initial comments in this PR, I was able to successfully configure all of the examples that use VTK and/or QuickView.

For examples that strictly utilize VTK, I was able to configure their respective CMakeList.txt to look for the VTK package and only use components that are needed by the example. Then I linked the VTK_LIBRARIES and initialized the VTK module using VTK's vtk_auto_init().

For examples that strictly use QuickView, but nothing else, I noticed they weren't being built unless ENABLE_QUICKVIEW was set to true. However, all of these examples use the QuickView for viewing purposes and not for the core functionality of the example. Instead, I removed the if(ENABLE_QUICKVIEW) to enable all of these examples, then in each of their individual CMakeList.txt, I find VTK and the components it would need if ENABLE_QUICKVIEW was set to true. If not set, the example would just build with ITK_LIBRARIES. This will greatly improve our test coverage for examples that just use ITK by about 46 examples.

Finally, many of the examples needed to have their test input updated to properly pass. In addition, their documentation was updated to contain the new input/output.

A few of the examples and their respective tests contained errors
that were marked with a TODO, for a future PR. These included:

  1. Baseline comparisons that cannot work due to the output not ever
    being stored. In a future PR, the examples should be modified to
    save their output, so we can test against it.
    + VTKImageToITKImageBaseline
    + InPlaceFilterOfImageBaseline
    + GeometricPropertiesOfRegionBaseline
  2. The DemonstrateThresholdAlgorithm example has it's
    KittlerIllingworth filter commented out with a TODO. This was the
    only filter to not work, and will be fixed in a future PR.

@mseng10 mseng10 marked this pull request as ready for review February 23, 2021 17:58
After fixing the CMakeList.txt scipts for examples that use VTK library
in some form, many of the examples needed there test input and
documentation updated.

This primarily added an input image for an examples test, and updated
it's respective documentation.txt with the input and output.

A few of the examples and/or their respective tests contained errors
that were marked with a TODO, for a future PR. These included:
  1) Baseline comparisons, that cannot work due to the output not ever
     being stored. In a future PR, the examples should be modified to
     save their output, so we can test against it.
       + VTKImageToITKImageBaseline
       + InPlaceFilterOfImageBaseline
       + GeometricPropertiesOfRegionBaseline
  2) The DemonstrateThresholdAlgorithm example has it's
     KittlerIllingworth filter commented out with a TODO. This was the
     only filter to not work, and will be documented in a future issue.
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mseng10 fantastic work! 👍

@thewtex thewtex merged commit 0db8e1d into InsightSoftwareConsortium:master Feb 24, 2021
@thewtex
Copy link
Member

thewtex commented Feb 24, 2021

CC: @wschroed @phcerdan

@mseng10 mseng10 deleted the superbuild branch March 2, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants