VTK 9.0 Support #4096

pionex · 2020-05-14T03:07:12Z

This code allows PCL master to compile and generally run with VTK 9.0.0. Some attempt was made to allow this code to be backwards compatible with older versions of VTK, although that has not been fully tested.

The main cmake has been updated to work with the breaking new conventions and has some backwards compatibility. It will not currently work with QT support with an older version of VTK, however.

The visualization cmake has been modified to call the new vtk_module_autoinit functionality. In my testing, so far I have only found that this needs to occur to make the visualizer work, but its possible other vtk modules need to be initialized for additional targets.

The changes made vtkCellArray usage in pcl_visualizer in the add/update polygon mesh functions are still being tested. There was some commented code that I used to make this compile, but I don't know if this fully works or offers the best performance.

The vtkIdType * were changed to vtkIdType const * where necessary to support the new API, although the VTK documentation recommends to use iterators instead if possible.

Version 2 of vtkCellArray deprecates the write pointer and requires const pointers in in some api calls that return pointers to shared memory. If the VTK_CELL_ARRAY_V2 macro is defined, this code uses the appropriate const pointers and uses alternative methods to insert cells into the cell array. This commit also adds some missing vtk headers that were causing incomplete type errors.

kunaltyagi · 2020-05-14T03:08:10Z

Closes #4095

kunaltyagi

CMake not reviewed

kunaltyagi · 2020-05-14T03:12:46Z

visualization/src/pcl_visualizer.cpp

@@ -3611,7 +3642,12 @@ pcl::visualization::PCLVisualizer::renderViewTesselatedSphere (
  // * Compute area of the mesh
  //////////////////////////////
  vtkSmartPointer<vtkCellArray> cells = mapper->GetInput ()->GetPolys ();
-  vtkIdType npts = 0, *ptIds = nullptr;
+  vtkIdType npts = 0;
+#ifdef VTK_CELL_ARRAY_V2


Please extract out this type. Sample code

Is the point of extracting this out to eliminate multiple ifdefs everywhere its used?

I was searching for a good common place to define this type, but there doesn't appear to be any common header file. Should it be (re)defined in each module where its necessary?

I'm assuming you mean something like:

#ifdef VTK_CELL_ARRAY_V2 using vtkCellPtsPtr = vtkIdType const *; #else using vtkCellPtsPtr = vtkIdType*;

I am also looking into using iterators and it seems straightforward, but this change is still required

Sorry, I seemed to have missed this conversation.

Is the point of extracting this out to eliminate multiple ifdefs

👍

Should it be (re)defined in each module where its necessary

That wouldn't be ideal. A single new header somewhere would be better, but I also don't know where.

surface/src/vtk_smoothing/vtk_utils.cpp

kunaltyagi · 2020-05-14T03:34:14Z

VTK documentation recommends to use iterators instead if possible

Perhaps using iterators is a better way to go forwards. We can split the logic using macros from when iterators first came to VTK API and use the current logic for older VTK. This can allow us to make decisions such as stopping support for old versions: VTK 7 and higher are available on all platforms being targeted.

It will not currently work with QT support with an older version of VTK

~~Should I mark this PR as draft or are you looking for feedback from PCL to add this support?~~

PS: The CI will be red since we look for VTK 6 and 7 on Ubuntu

I don't know if this fully works or offers the best performance

Note for others: Needs testing

larshg · 2020-05-14T13:22:02Z

Some work was also done here #3637

pionex · 2020-05-14T20:30:19Z

Some work was also done here #3637

How should we collaborate to consolidate these changes?

larshg · 2020-05-16T20:15:39Z

Hey Pionex

I'm testing your branch out and I have done a few cmake changes.

But I can't get it to compile. I get a bunch of:
'vtkSmartPointerBase &vtkSmartPointerBase::operator =(const vtkSmartPointerBase &)': cannot convert argument 1 from 'T *' to 'vtkObjectBase *'

Did you get this?

Edit: I'm on win10 using VS2019.

larshg · 2020-05-16T20:37:59Z

It was missing includes, that probably was transparently included... VS is not so forgiving 🙄

Added PR to @pionex 's PR.

kunaltyagi · 2020-05-16T21:24:05Z

The PR made by @larshg

Reorganise to hopefully support pre 9.0.

Move RenderingUI as it is not available vtk < 9.0

larshg · 2020-05-21T20:50:51Z

@pionex I have tested with my vtk 8.2 system and made a few changes. Please merge it and lets fix the last things mentioned by @kunaltyagi.

larshg · 2020-05-26T19:32:45Z

@pionex ~~where is VTK_CELL_ARRAY_V2 defined?~~ - Nm - found it.

pionex · 2020-05-26T19:44:50Z

I can have a look at this in a few. How much longer will you be working on it today? I’m in GMT+12, so day is just starting.

larshg · 2020-05-26T19:56:10Z

I'm off to bed now so go ahead. You can look if the ci is green with my latest push to my pr also.

I was thinking of making a compat header in visualization and then those that can use this will(visualization and all that depends on visualization).
The rest ie I/O and surface.. maybe more cant remember I would leave as is with the if defs.

There is no common sub module which imports vtk and it seems overkill to make such project for a few extra if defs.

Morwenn · 2020-05-28T13:25:53Z

cmake/pcl_find_vtk.cmake

@@ -87,7 +87,9 @@ else()
  list(APPEND PCL_VTK_COMPONENTS RenderingContext${VTK_RENDERING_BACKEND})
 endif()

+message("WITH_QT is:${WITH_QT}")


Maybe add a space after the colon 🙂

It was more for 'debugging' - but I guess the information is nice to have, but could probably be consolidated to a single line summerizing most of the VTK settings 😄

There was an issue about producing a full-blown CMake configuration report for PCL, maybe it's the kind of information that could go there when it happens.

https://cmake.org/cmake/help/v3.17/module/FeatureSummary.html

That's something nice and available in 3.5 too (Ubuntu 16.04's CMake version)

@Morwenn is there a issue for this CMake reporting?

@kunaltyagi I thought so but apparently it was merely a comment in issue #3603, my memory failed me. Such a summary is definitely something valuable when dealing with build issues (and especially good when users report issues, you don't have to ask again and again for the information). I didn't know that CMake had a built-in feature to report the options, that's nice.

larshg · 2020-06-13T18:36:10Z

@pionex have you done any work - feel like we are close to make it possible to merge? If you are busy I don't mind picking it up and try to complete it.

pionex · 2020-06-15T20:42:12Z

@pionex have you done any work - feel like we are close to make it possible to merge? If you are busy I don't mind picking it up and try to complete it.

Please go ahead. I wasn't sure where to put the new headers.

This warning might also be something to look into:
vtkOpenGLPolyDataMapper::SetFragmentShaderCode was deprecated for VTK 9.0 and will be removed in a future version. Use vtkOpenGLShaderProperty::SetFragmentShaderCode instead

Do I need to do anything on github for you to pick it up?

SunBlack · 2020-06-19T12:50:54Z

cmake/pcl_find_vtk.cmake

+#Change to use list Transform when cmake is >= 3.12
+if(NOT (${VTK_VERSION} VERSION_LESS 9.0))
+set(PCL_VTK_COMPONENTS
+  ChartsCore
+  CommonColor
+  CommonCore
+  CommonDataModel
+  CommonExecutionModel
+  CommonMath
+  CommonMisc
+  CommonTransforms
+  FiltersCore
+  FiltersExtraction
+  FiltersGeneral
+  FiltersGeometry
+  FiltersModeling
+  FiltersSources
+  ImagingCore
+  ImagingSources
+  InteractionStyle
+  InteractionWidgets
+  IOCore
+  IOGeometry
+  IOImage
+  IOLegacy
+  IOPLY
+  RenderingAnnotation
+  RenderingCore
+  RenderingContext2D
+  RenderingLOD
+  RenderingFreeType
+  RenderingOpenGL2
+  ViewsCore
+  ViewsContext2D
+)
+else()
+set(PCL_VTK_COMPONENTS
+  vtkChartsCore
+  vtkCommonCore
+  vtkCommonDataModel
+  vtkCommonExecutionModel
+  vtkFiltersCore
+  vtkFiltersExtraction
+  vtkFiltersModeling
+  vtkImagingCore
+  vtkImagingSources
+  vtkInteractionStyle
+  vtkInteractionWidgets
+  vtkIOCore
+  vtkIOGeometry
+  vtkIOImage
+  vtkIOLegacy
+  vtkIOPLY
+  vtkRenderingAnnotation
+  vtkRenderingLOD
+  vtkViewsContext2D
+)
+endif()


Indentation of set is missing.

Is it really necessary to have two lists? I had started more than one moth ago same test (just didn't had time to continue and as this PR also exists)...

I had solved it by:

function(checkVTKComponents) cmake_parse_arguments(PARAM "" "MISSING_COMPONENTS" "COMPONENTS" ${ARGN}) set(vtkMissingComponents) if (VTK_VERSION VERSION_GREATER "8.2") foreach(vtkComponent ${PARAM_COMPONENTS}) if (NOT TARGET VTK::${vtkComponent}) list(APPEND vtkMissingComponents ${vtkComponent}) endif() endforeach() else() foreach(vtkComponent ${PARAM_COMPONENTS}) set(vtkComponent "vtk${vtkComponent}") if (NOT TARGET ${vtkComponent}) list(APPEND vtkMissingComponents ${vtkComponent}) endif() endforeach() endif() set(${PARAM_MISSING_COMPONENTS} ${vtkMissingComponents} PARENT_SCOPE) endfunction() set(PCL_VTK_COMPONENTS ChartsCore CommonCore CommonDataModel CommonExecutionModel FiltersCore FiltersExtraction FiltersModeling ImagingCore ImagingSources InteractionStyle InteractionWidgets IOCore IOGeometry IOImage IOLegacy IOPLY RenderingAnnotation RenderingLOD ViewsContext2D ) # Start with a generic call to find any VTK version we are supporting, so we retrieve # the version of VTK. As the module names were changed from VTK 8.2 to 9.0, we don't # search explicitly for modules. Furthermore we don't pass required minimum version 6.2 # to find_package because then it only accept versions with same major version. find_package(VTK) if(VTK_FOUND AND ("${VTK_VERSION}" VERSION_LESS 6.2)) message(WARNING "The minimum required version of VTK is 6.2, but found ${VTK_VERSION}") set(VTK_FOUND FALSE) return() endif() # We still require vtkOpenGLExtensionManager and vtkOpenGLRenderWindow # Before VTK 8.2 they were part of VTK compontent # Since VTK 8.2 they were part of VTK compontent vtkOpenGLRenderWindow => "OpenGL2", vtkOpenGLExtensionManager => missing if("${VTK_VERSION}" VERSION_LESS 8.2) set(VTK_RENDERING_BACKEND "OpenGL") else() set(VTK_RENDERING_BACKEND "OpenGL2") endif() # Check if requested modules are available checkVTKComponents(COMPONENTS ${PCL_VTK_COMPONENTS} MISSING_COMPONENTS vtkMissingComponents) message(STATUS "${vtkMissingComponents}") if (vtkMissingComponents) set(VTK_FOUND FALSE) message(WARNING "Missing vtk modules: ${vtkMissingComponents}") return() endif()

I'll look into using your snippet.

Originally it was using List transform, but that wasn't available. But an "ordinary" foreach could be used ofc.

tkircher · 2020-07-01T19:41:10Z

Thank you for this! It's a strange feeling having a build of PCL that isn't years out of date.

StefanBruens · 2020-07-11T17:09:41Z

cmake/pcl_find_vtk.cmake

+
+message("WITH_QT is:${WITH_QT}")
+if(WITH_QT)
+  message("VTK_MODULES_ENABLED is: ${VTK_MODULES_ENABLED}")


VTK 9.0 has "VTK_AVAILABLE_COMPONENTS" instead of "VTK_MODULES_ENABLED"

Thanks for mentioning it. Saw it yesterday as well :-)

bpoebiapl · 2020-07-30T03:32:27Z

any update?

larshg · 2020-07-30T07:40:46Z

I'm doing slowly progress in #4262 instead of this one.

kaitou1419 · 2021-10-21T20:42:26Z

please fix ERROR: pcl\visualiztion is missing when install with vcpkg

mbmiddleton added 4 commits May 14, 2020 14:53

Add missing header and namespace specifier

f807763

Add VTK9 module auto initialization to visualizer cmake

cd81974

Update main project Cmake to support VTK9 conventions

8230bc6

kunaltyagi added this to In progress in Update dependencies via automation May 14, 2020

kunaltyagi requested changes May 14, 2020

View reviewed changes

Update dependencies automation moved this from In progress to Review in progress May 14, 2020

kunaltyagi added changelog: enhancement Meta-information for changelog generation module: cmake module: visualization needs: code review Specify why not closed/merged yet needs: more work Specify why not closed/merged yet labels May 14, 2020

pionex marked this pull request as draft May 14, 2020 03:24

kunaltyagi added needs: testing Specify why not closed/merged yet needs: author reply Specify why not closed/merged yet labels May 14, 2020

kunaltyagi removed needs: author reply Specify why not closed/merged yet needs: code review Specify why not closed/merged yet labels May 16, 2020

larshg added 3 commits May 17, 2020 21:01

Move findVTK to a module file.

4ad0cfc

Reorganise to hopefully support pre 9.0.

Added missing includes.

e548e78

Link only to necessary vtk modules.

9c0ef43

kunaltyagi mentioned this pull request May 21, 2020

[ci] Add surface module back on the CI (depends on VTK PRs: 4046, 4096) #4126

Closed

Fix list transform with appending VTK.

382ef31

Move RenderingUI as it is not available vtk < 9.0

larshg added 6 commits May 24, 2020 21:21

Fix qtmodules

e3fb9fc

Add check if vtk is found in IO

ae0b7cb

Check if VTK is present

965e2f1

Check if VTK found in surface.

3a522f4

Define PCL_SHARED_LIBS for findVTK script to work

2b68fcf

Forgot pull first...

c8e4e83

Remove FindVTK and move it to PCL_only_find so it doesn't get shipped.

5b8544e

larshg mentioned this pull request May 28, 2020

Support vtk 9.0 #4138

Closed

Morwenn reviewed May 28, 2020

View reviewed changes

kunaltyagi mentioned this pull request May 29, 2020

[cmake] Add a comprehensive build summary #4149

Open

Neumann-A mentioned this pull request Jun 2, 2020

vcpkg install pcl:x64-windows, but visualization include not installed #include <pcl/visualization/pcl_visualizer.h> missing from pcl installed include microsoft/vcpkg#11601

Closed

kunaltyagi linked an issue Jun 7, 2020 that may be closed by this pull request

[CMake] VTK 9.0 deprecation warnings #4046

Closed

SunBlack reviewed Jun 19, 2020

View reviewed changes

larshg mentioned this pull request Jun 20, 2020

[compile error] "error: no matching function for call to ‘vtkSmartPointer<vtkRenderWindow>::operator=(vtkRenderWindow*)’" #4207

Open

kunaltyagi linked an issue Jun 22, 2020 that may be closed by this pull request

[compile error] "error: no matching function for call to ‘vtkSmartPointer<vtkRenderWindow>::operator=(vtkRenderWindow*)’" #4207

Open

Neumann-A mentioned this pull request Jun 30, 2020

PCL (Point Cloud Lib) pcl_visualization.h not existing microsoft/vcpkg#12156

Closed

Neumann-A mentioned this pull request Jul 3, 2020

[pcl] Update to 1.10.0 microsoft/vcpkg#10042

Merged

StefanBruens reviewed Jul 11, 2020

View reviewed changes

larshg mentioned this pull request Jul 11, 2020

Add support for VTK 9 #4262

Merged

larshg closed this in #4262 Feb 16, 2021

Update dependencies automation moved this from Review in progress to Done Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VTK 9.0 Support #4096

VTK 9.0 Support #4096

pionex commented May 14, 2020

kunaltyagi commented May 14, 2020

kunaltyagi left a comment

kunaltyagi May 14, 2020

pionex May 14, 2020

pionex May 14, 2020

kunaltyagi May 16, 2020

kunaltyagi commented May 14, 2020 •

edited

larshg commented May 14, 2020

pionex commented May 14, 2020

larshg commented May 16, 2020 •

edited

larshg commented May 16, 2020 •

edited

kunaltyagi commented May 16, 2020

larshg commented May 21, 2020

larshg commented May 26, 2020 •

edited

pionex commented May 26, 2020

larshg commented May 26, 2020

Morwenn May 28, 2020

larshg May 28, 2020

Morwenn May 28, 2020

Neumann-A May 28, 2020

kunaltyagi May 29, 2020

Morwenn May 29, 2020

larshg commented Jun 13, 2020

pionex commented Jun 15, 2020 •

edited

SunBlack Jun 19, 2020 •

edited

larshg Jun 23, 2020

tkircher commented Jul 1, 2020

StefanBruens Jul 11, 2020

larshg Jul 11, 2020

bpoebiapl commented Jul 30, 2020

larshg commented Jul 30, 2020

kaitou1419 commented Oct 21, 2021

VTK 9.0 Support #4096

VTK 9.0 Support #4096

Conversation

pionex commented May 14, 2020

kunaltyagi commented May 14, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunaltyagi commented May 14, 2020 • edited

larshg commented May 14, 2020

pionex commented May 14, 2020

larshg commented May 16, 2020 • edited

larshg commented May 16, 2020 • edited

kunaltyagi commented May 16, 2020

larshg commented May 21, 2020

larshg commented May 26, 2020 • edited

pionex commented May 26, 2020

larshg commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larshg commented Jun 13, 2020

pionex commented Jun 15, 2020 • edited

SunBlack Jun 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkircher commented Jul 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpoebiapl commented Jul 30, 2020

larshg commented Jul 30, 2020

kaitou1419 commented Oct 21, 2021

kunaltyagi commented May 14, 2020 •

edited

larshg commented May 16, 2020 •

edited

larshg commented May 16, 2020 •

edited

larshg commented May 26, 2020 •

edited

pionex commented Jun 15, 2020 •

edited

SunBlack Jun 19, 2020 •

edited