COMP: Remove "vtk" prefix from VTK COMPONENTS #242

phcerdan · 2021-03-17T16:03:49Z

In modern VTK (> 8.90) the COMPONENT names don't have the "vtk" prefix.

This fix get the VTK version first, and then look for the COMPONENTS
with the right name, prepending ${_vtk_prefix} to the names.

Fix #241

phcerdan · 2021-03-17T16:34:34Z

Windows-2019 error in CI is one of those filepath is too long from Windows...

aylward · 2021-03-17T17:17:21Z

This brings up the issue of whether or not ITKVtkGlue should be a requirement or not. At the top level CMakeLists.txt, the find_package(VTK) is conditional on ITKVtkGlue being enabled...and ITKVtkGlue requires QuickView.h which is part of ITKVtkGlue. I think we need to add ITKVtkGlue as a dependency in itk-modules so that order of building works.

@thewtex @dzenanz

thewtex · 2021-03-17T17:33:19Z

This brings up the issue of whether or not ITKVtkGlue should be a requirement or not. At the top level CMakeLists.txt, the find_package(VTK) is conditional on ITKVtkGlue being enabled. We can either remove this condition or we need to make certain examples conditional on ITKVtkGlue being enabled (since they require VTK).

Checking this:

InsightSoftwareConsortium/ITKSoftwareGuide#150

It looks like it would make more sense to make ENABLE_QUICKVIEW a CMake cache option, then change the condition from if(Module_ITKVtkGlue to if(ENABLE_QUICKVIEW.

aylward · 2021-03-17T17:35:18Z

@thewtex -- QuickView.h is required by the code in SphinxExamples that are used by the VTK examples. QuickView.h is in ITK/Bridge/VTKGlue. I believe that means that we need to add VTKGlue as a dependency, but perhaps not?

thewtex · 2021-03-17T17:39:08Z

QuickView.h is in ITK/Bridge/VTKGlue. I believe that means that we need to add VTKGlue as a dependency, but perhaps not?

Good catch. In that case the current setup seems desirable, correct? If we have ITKVtkGlue enabled, we can use the QuickView functionality, but we are not required to have a working ITKVtkGlue / VTK build.

aylward · 2021-03-17T17:41:28Z

I was getting QuickView.h not found, unless I added ITKVtkGlue as a depends in itk-modules.cmake. Perhaps that depends is needed for the path to be specified?

phcerdan · 2021-03-17T17:48:12Z

Maybe there are some examples that require a if(ENABLE_QUICKVIEW) conditional.

ITKSphinxExamples/src/Bridge/CMakeLists.txt

Line 2 in 4efbf33

add_subdirectory_if_module_enabled(VtkGlue)

phcerdan · 2021-03-17T17:56:48Z

I cannot reproduce the QuickView.h not found after switching OFF the module VtkGlue in ITK.
The ENABLE_QUICKVIEW turns off in that case, but everything compiles.

aylward · 2021-03-17T18:00:30Z

I wonder if building from scratch would have the same result.

…

On Wed, Mar 17, 2021 at 1:57 PM Pablo Hernandez-Cerdan < ***@***.***> wrote: I cannot reproduce the QuickView.h not found after switching OFF the module VtkGlue in ITK. The ENABLE_QUICKVIEW turns off in that case, but everything compiles. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#242 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEJL5BFH5ODEWU7EYMGW3TEDUPJANCNFSM4ZK4ABWQ> .

-- Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives --- Kitware: *Delivering innovative, open source, scientific software.*

aylward · 2021-03-17T18:03:08Z

Is the module's name "VtkGlue" or "ITKVtkGlue" ?

…

On Wed, Mar 17, 2021 at 1:48 PM Pablo Hernandez-Cerdan < ***@***.***> wrote: Maybe there are some examples that require a if(ENABLE_QUICKVIEW) conditional. https://github.com/InsightSoftwareConsortium/ITKSphinxExamples/blob/4efbf33490efbb0f6baafb18511df6585b739e1c/src/Bridge/CMakeLists.txt#L2 — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#242 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEJL5D5RGLF2MFIHNBHJ3TEDTO7ANCNFSM4ZK4ABWQ> .

-- Stephen R. Aylward, Ph.D. Senior Director of Strategic Initiatives --- Kitware: *Delivering innovative, open source, scientific software.*

dzenanz · 2021-03-17T18:09:01Z

There is also https://github.com/InsightSoftwareConsortium/ITKVTKGlue, so there could be some confusion between them. The remote one does not have ITK prefix in the name, the built-in one does.

thewtex · 2021-03-17T18:18:17Z

To avoid complexity and modernize, I am wondering if we should replace the QuickView usage in the examples with Jupyter notebooks (and eventually vtk.js)? What do you think?

phcerdan · 2021-03-17T19:14:41Z

The name is ITKVtkGlue. I was referring to that one.

phcerdan · 2021-03-17T19:16:35Z

I think this could be simplified even more, we already do a find_package(VTK) in the top CMakeLists, where ENABLE_QUICKVIEW is set. Here:

ITKSphinxExamples/src/CMakeLists.txt

Line 18 in 4efbf33

set(ENABLE_QUICKVIEW ON)

We can set _vtk_prefix there.

dzenanz · 2021-03-17T19:21:47Z

Then individual examples would not be self-contained.

phcerdan · 2021-03-17T19:35:35Z

Last push re-use _vtk_prefix defined at top level when ITK has ITKVtkGlue (to reuse the find_package(VTK) there)

Individual examples define their own _vtk_prefix with the extra find_package(VTK)

aylward · 2021-03-17T19:50:04Z

Do we need to declare a conditional depends on itkvtkglue in itk-modules.init?

…

--- Sent via phone.

On Wed, Mar 17, 2021, 3:35 PM Pablo Hernandez-Cerdan < ***@***.***> wrote: Last push re-use _vtk_prefix defined at top level when ITK has ITKVtkGlue (to reuse the find_package(VTK)) Individual example define their own _vtk_prefix. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#242 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACEJLYJFVOYTZTTSMEHN2DTEEABTANCNFSM4ZK4ABWQ> .

dzenanz · 2021-03-17T19:54:11Z

src/Bridge/VtkGlue/VTKImageToITKImage/CMakeLists.txt

 find_package(ITK REQUIRED)
 include(${ITK_USE_FILE})

+find_package(VTK REQUIRED)


I think this will change stand-alone build behavior of this and other similar examples. They used to work fine with VTK8- and throw a configure warning with VTK9+. Now they will work fine with VTK9+, but fail to work with VTK8-.

Assuming that _vtk_prefix will be undefined (and therefore empty) if being built stand-alone.

Why do you think this first find_package will change that? VTK is required anyway in these cases.

thewtex · 2021-03-17T21:20:29Z

Do we need to declare a conditional depends on itkvtkglue in
itk-modules.init?

That may help when built as a remote module, but would need to be tested.

thewtex · 2021-03-17T21:21:00Z

We may just want to support the latest released version of VTK, i.e. VTK 9, here to keep things simple.

phcerdan · 2021-03-18T07:05:24Z

Do we need to declare a conditional depends on itkvtkglue in itk-modules.init?

I am not sure anything related to ITKVtkGlue has changed here, if this is tackled, should be in other PR.
These changes work with and without ITKVtkGlue (tested in remote/external build defining -DITK_DIR=...)

phcerdan · 2021-03-18T07:11:00Z

This PR seems long, but is not complex, it defines _vtk_prefix based on VTK_VERSION. This requires an extra find_package(VTK) without components.
In the case ITK has the module ITKVtkGlue, we already find_package(VTK) in the top CMakelists.txt, so we take advantage and define _vtk_prefix there, at the top level.

The standalone examples define their own _vtk_prefix with an extra find_package(VTK).

thewtex · 2021-03-18T09:02:10Z

@phcerdan could this please be rebased on master for a clean CI so we can get it merged? Gracias!

In modern VTK (> 8.90) the COMPONENT names don't have the "vtk" prefix. This fix get the VTK version first, and then look for the COMPONENTS with the right name, prepending ${_vtk_prefix} to the names. The examples using ITKVtkGlue (`ENABLE_QUICKVIEW` = ON) re-use the variable `_vtk_prefix` defined in the top CMakeLists. The examples that depend on `VTK` and `ITK without ITKVTKGlue` set `_vtk_prefix` by their own. Fix InsightSoftwareConsortium#241

phcerdan · 2021-03-18T11:02:57Z

Rebased on top of master @thewtex

thewtex

@phcerdan thanks!

phcerdan force-pushed the update_vtk_module_names_9 branch from 81195e0 to 4efbf33 Compare March 17, 2021 16:09

thewtex requested a review from aylward March 17, 2021 16:45

thewtex mentioned this pull request Mar 17, 2021

Update name of VTK components to be aware of VTK 9.0 changes. #241

Closed

dzenanz approved these changes Mar 17, 2021

View reviewed changes

phcerdan marked this pull request as draft March 17, 2021 19:16

phcerdan force-pushed the update_vtk_module_names_9 branch from 4efbf33 to 3172250 Compare March 17, 2021 19:33

phcerdan marked this pull request as ready for review March 17, 2021 19:36

dzenanz reviewed Mar 17, 2021

View reviewed changes

phcerdan force-pushed the update_vtk_module_names_9 branch from 3172250 to 222bd3d Compare March 18, 2021 11:01

phcerdan force-pushed the update_vtk_module_names_9 branch from 222bd3d to 9c045a1 Compare March 18, 2021 11:02

thewtex approved these changes Mar 18, 2021

View reviewed changes

thewtex merged commit ddbd361 into InsightSoftwareConsortium:master Mar 18, 2021

COMP: Remove "vtk" prefix from VTK COMPONENTS #242

COMP: Remove "vtk" prefix from VTK COMPONENTS #242

Uh oh!

Conversation

phcerdan commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aylward commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thewtex commented Mar 17, 2021

Uh oh!

aylward commented Mar 17, 2021

Uh oh!

thewtex commented Mar 17, 2021

Uh oh!

aylward commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021

Uh oh!

aylward commented Mar 17, 2021 via email

Uh oh!

aylward commented Mar 17, 2021 via email

Uh oh!

dzenanz commented Mar 17, 2021

Uh oh!

thewtex commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021

Uh oh!

dzenanz commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aylward commented Mar 17, 2021 via email

Uh oh!

dzenanz Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

dzenanz Mar 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phcerdan Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

thewtex commented Mar 17, 2021

Uh oh!

thewtex commented Mar 17, 2021

Uh oh!

phcerdan commented Mar 18, 2021

Uh oh!

phcerdan commented Mar 18, 2021

Uh oh!

thewtex commented Mar 18, 2021

Uh oh!

phcerdan commented Mar 18, 2021

Uh oh!

thewtex left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

phcerdan commented Mar 17, 2021 •

edited

Loading

aylward commented Mar 17, 2021 •

edited

Loading

phcerdan commented Mar 17, 2021 •

edited

Loading

dzenanz Mar 17, 2021 •

edited

Loading