[Rebase] Make fixes for VTK 6 #363

fran6co · 2013-11-14T12:47:35Z

I needed to rebase the PR #218 to use it in the homebrew pcl recipe.

richmattes · 2013-11-14T14:03:29Z

Thanks for going through the effort to rebase these patches! Two comments:

There are still two hard-coded vtk libraries in PCLConfig.cmake.in. You can probably replace them with ${VTK_LIBRARIES} (or @VTK_LIBRARIES@ since it's an .in file)
Did you try building/running the test suite? I seem to remember running into some vtk issues there too, but I won't be able to verify until tonight.

fran6co · 2013-11-14T14:09:27Z

@jpgr87 I don't see the hardcoded vtk libraries in PCLConfig.cmake.in, it's using ${VTK_LIBRARIES} already.

I can't run the test suite, I'm on Mavericks and I'm having some issues to get it working.

richmattes · 2013-11-14T14:15:26Z

Sorry, I should have been more clear. The problem is this line:

set(VTK_LIBRARIES vtkCommon vtkRendering vtkHybrid vtkCharts)

These libraries don't exist in vtk6, causing sadness for programs that try to build againt pcl. The VTK_LIBRARIES variable should be set based on the output of find_package(VTK ${QUIET_}) instead of being hard-coded.

fran6co · 2013-11-14T14:38:04Z

@jpgr87 not sure if it works. I'm fixing my environment still =/

fran6co · 2013-11-14T16:19:35Z

Uhm, I'm getting this error:

visualization/include/pcl/visualization/interactor.h:54:12: fatal error: 'vtkXRenderWindowInteractor.h' file not found
#  include <vtkXRenderWindowInteractor.h>

nizar-sallem · 2013-11-15T06:07:11Z

@jspricke could we have a vtk6 branch on PCL so that more can participate in this ?

jspricke · 2013-11-15T09:13:36Z

@nizar-sallem it wouldn't make sense to have it in the main repository (because we can't give push rights based on branches and it would pollute the global namespace), but you can have it in your fork easily. Just send pull requests against @fran6co branch or you could add each other as collaborators in your fork and push directly.

nizar-sallem · 2013-11-15T11:30:16Z

@jspricke to whom should I send the PR exactly ? PCL or @fran6co or @jpgr87 it would be nice to have only one fork that gathers all.

fran6co · 2013-11-15T11:36:14Z

You can commit directly to my vtk6-fixes branch and it's going to show up here.

nizar-sallem · 2013-11-15T12:42:35Z

Thanks @fran6co

nizar-sallem · 2013-11-19T15:03:11Z

Sorry the git refs got all mixed up (my fault). Anyway whenever we are done I will do the clean up.

fran6co · 2013-11-19T15:15:24Z

I did put it back.

nizar-sallem · 2013-11-19T15:19:26Z

Thanks

nizar-sallem · 2013-11-19T15:20:10Z

On my side everything is fine I tested apps and visualization with built VTK-6.0

richmattes · 2013-11-19T15:53:22Z

I pulled the PR, built, and ran the unit tests on Fedora 20 x86_64. Everything looks like it works (minus the first unit test segfaulting, not sure if that has to do with this PR or not yet)

nizar-sallem · 2013-11-19T15:54:57Z

which unit test exactly ?

richmattes · 2013-11-19T16:05:03Z

a_recognition_ism_test fails. I caught it in gdb when built with RelWithDebInfo, but when I rebuilt as Debug to get more output the test didn't crash. It seemed like it was stuck though, after running for about 5 minutes I killed it. I'll rebuild RelWithDebInfo minus optimization flags and try again soon.

nizar-sallem · 2013-11-19T16:10:27Z

Thanks @jpgr87 if it is related to VTK-6.0 support post it here please :)

fran6co · 2013-11-19T16:21:10Z

@nizar-sallem, I'm going to rebase this branch.

nizar-sallem · 2013-11-19T17:31:22Z

@fran6co Thanks that cleaned it

fran6co · 2013-11-19T18:53:12Z

Now it's failing with this error. Probably something is off with the include paths.

In file included from /tmp/pcl-Ou0c/macbuild/apps/include/pcl/apps/moc_openni_passthrough.cxx:9:
In file included from /tmp/pcl-Ou0c/macbuild/apps/include/pcl/apps/../../../../../apps/include/pcl/apps/openni_passthrough.h:42:
In file included from /tmp/pcl-Ou0c/apps/include/pcl/apps/openni_passthrough_qt.h:44:
/tmp/pcl-Ou0c/macbuild/apps/ui_openni_passthrough.h:21:10: fatal error: 'QVTKWidget.h' file not found
#include "QVTKWidget.h"

richmattes · 2013-11-19T19:14:30Z

You might not have built vtk with qt support. It's packaged separately as libvtk5-qt4-dev on ubuntu, not sure how you installed vtk on the mac.

fran6co · 2013-11-19T19:22:49Z

I'm using homebrew, the formula installs 6 and it did build with qt support, but it could be something wrong with homebrew. After Mavericks everything went bonkers =/

Or that vtk6 changed a lot of things.

nizar-sallem · 2013-11-20T15:19:29Z

Hi,
Try to find libvtkGUISupportQt, libvtkGUISupportQtOpenGL, libvtkGUISupportQtWebkit, libvtkRenderingQt and libvtkViewsQt on your system. If they are not all there then something is wron g with the brew formula.

nizar-sallem · 2013-11-28T14:02:39Z

the surface/src/vtk_smoothing/vtk_utils.cpp looks fine in here.

nizar-sallem · 2013-11-28T14:03:59Z

also about #include "QVTKWidget.h", that is relateed to Qt5. VTK compiles only against Qt4 so I dropped Qt5 finding from PCL in this branch.

nizar-sallem · 2013-12-05T21:56:11Z

@fran6co Are we done here ?

fran6co · 2013-12-05T21:58:13Z

Didn't test it yet. I'll give it a try one of these days.

jspricke · 2014-05-24T12:35:35Z

I think we can go without the deprecation thing (it's obscure enough anyway ;) ). @fran6co can you adopt this to the PCL style guide (there are some missing brackets). Then I'm in favor of merging, or what do you think @taketwo?

fran6co · 2014-05-24T15:01:20Z

It's going to take me a while to fix the style, could you pinpoint what brackets are missing?

taketwo · 2014-05-24T15:17:53Z

Then I'm in favor of merging, or what do you think @taketwo?

I am no expert in vtk, but I can try if this will work on my system (Arch).

could you pinpoint what brackets are missing?

I am pretty sure Jochen meant spaces before braces. We also have a rule about wrapping return statements with braces, but your code is okay in that respect.

taketwo · 2014-05-24T17:59:43Z

I installed vtk6.1 from source and started to compile this branch with tests and examples enabled. So far I haven't finished the whole process, but test_outofcore target seems to fail due to some problems with vtkAtomicInt:

vtkAtomicInt.h:307:28: error: reference to ‘detail’ is ambiguous

taketwo · 2014-05-24T19:11:40Z

The actual problem is in the ambiguity between pcl::detail namespace and detail namespace in vtk. This may be resolved by moving using namespace pcl; from line 58 to line 64 in 'test_outofcore.cpp'. The fact that OutofcoreOctreeBaseNode requires visualization/common seems a bit odd though.

Apart from that everything is fine.

fran6co · 2014-05-25T21:14:36Z

Fixed the style issues, fixing outofcore now.

taketwo · 2014-05-25T21:19:03Z

apps/src/render_views_tesselated_sphere.cpp

 trans_filter_center->SetInput (polydata_);
+#else
+ trans_filter_center->SetInputData (polydata_);


Extra indentation spaces here.

taketwo · 2014-05-25T21:25:39Z

visualization/src/image_viewer.cpp

@@ -598,7 +621,11 @@ pcl::visualization::ImageViewer::createLayer (
 rect->set (0, 0, static_cast<float> (width), static_cast<float> (height));
 l.actor->GetScene ()->AddItem (rect);
 }
+#if VTK_MAJOR_VERSION == 6


Earlier (line 45 of this file) a different comparison was used, >= 6.

Done. Used <6 instead, it's a bit more consistent with the rest of the PR.

taketwo · 2014-05-25T21:35:54Z

The style looks good to me now.

The fact that the compilation error in 'test_outofcore' went unnoticed on Travis raises an interesting question. Now that we support two versions of vtk, how should the Travis tasks be organized? Do we want to compile/link/test against both versions? If so, how to minimize the extra burden that we put on Travis?

fran6co · 2014-05-25T21:42:38Z

I think it depends on what version of vtk is the default in the main platforms. Homebrew switched to vtk6 last year. Not sure about linux, but it looks like vtk6 hit Ubuntu for 14.04.

taketwo · 2014-05-25T21:48:11Z

but it looks like vtk6 hit Ubuntu for 14.04

Is it? According to launchpad it's still 5.8 and will be the same in the next release. On Arch the default is 5.10.

fran6co · 2014-05-25T21:50:54Z

The package is https://launchpad.net/ubuntu/+source/vtk6. I think they changed the name to have both versions at the same time.

fran6co · 2014-05-25T22:38:37Z

Fixed test outofcore.

jspricke · 2014-05-26T11:56:50Z

+1 for merging this. Regarding Travis, I'm not aware of any load problems we cause, so I would propose to test both builds. We could do the same for OpenNI[12], what do you think?

fran6co · 2014-05-26T11:59:24Z

I think openni 1 and 2 can coexist, it wouldn't require 2 builds for that, right?

taketwo · 2014-05-26T12:01:50Z

Okay, so I propose to merge this PR. Then we can discuss Travis-related questions in a separate issue.

[Rebase] Make fixes for VTK 6

fran6co mentioned this pull request Nov 14, 2013

Make fixes for VTK 6 #218

Closed

fran6co mentioned this pull request Nov 24, 2013

pcl_sample_consensus error fran6co/homebrew-cv#6

Closed

taketwo reviewed May 25, 2014
View reviewed changes

VTK6 cmake fixes

e256f92

taketwo reviewed May 25, 2014
View reviewed changes

fran6co added 2 commits May 25, 2014 18:36

VTK6 code fixes. VTK5 backward compatibility

4d1ee47

Fixes VTK's definition bleed in PCL_DEFINITIONS

25685ad

Fixes out of core test with VTK6, overlapping namespaces

2c941a5

jspricke added a commit that referenced this pull request May 26, 2014

Merge pull request #363 from fran6co/vtk6-fixes

6b53ae2

[Rebase] Make fixes for VTK 6

jspricke merged commit 6b53ae2 into PointCloudLibrary:master May 26, 2014

taketwo mentioned this pull request May 26, 2014

Modification of the Travis tasks #699

Closed

fran6co mentioned this pull request May 27, 2014

find_package(VTK) should specify a version #662

Closed

asmaloney mentioned this pull request May 28, 2014

pcl_visualization build failure on Mac OS X #712

Closed

[Rebase] Make fixes for VTK 6 #363

[Rebase] Make fixes for VTK 6 #363

Conversation

fran6co commented Nov 14, 2013

richmattes commented Nov 14, 2013

fran6co commented Nov 14, 2013

richmattes commented Nov 14, 2013

fran6co commented Nov 14, 2013

fran6co commented Nov 14, 2013

nizar-sallem commented Nov 15, 2013

jspricke commented Nov 15, 2013

nizar-sallem commented Nov 15, 2013

fran6co commented Nov 15, 2013

nizar-sallem commented Nov 15, 2013

nizar-sallem commented Nov 19, 2013

fran6co commented Nov 19, 2013

nizar-sallem commented Nov 19, 2013

nizar-sallem commented Nov 19, 2013

richmattes commented Nov 19, 2013

nizar-sallem commented Nov 19, 2013

richmattes commented Nov 19, 2013

nizar-sallem commented Nov 19, 2013

fran6co commented Nov 19, 2013

nizar-sallem commented Nov 19, 2013

fran6co commented Nov 19, 2013

richmattes commented Nov 19, 2013

fran6co commented Nov 19, 2013

nizar-sallem commented Nov 20, 2013

nizar-sallem commented Nov 28, 2013

nizar-sallem commented Nov 28, 2013

nizar-sallem commented Dec 5, 2013

fran6co commented Dec 5, 2013

jspricke commented May 24, 2014

fran6co commented May 24, 2014

taketwo commented May 24, 2014

taketwo commented May 24, 2014

taketwo commented May 24, 2014

fran6co commented May 25, 2014

taketwo May 25, 2014

Choose a reason for hiding this comment

fran6co May 25, 2014

Choose a reason for hiding this comment

taketwo May 25, 2014

Choose a reason for hiding this comment

fran6co May 25, 2014

Choose a reason for hiding this comment

taketwo commented May 25, 2014

fran6co commented May 25, 2014

taketwo commented May 25, 2014

fran6co commented May 25, 2014

fran6co commented May 25, 2014

jspricke commented May 26, 2014

fran6co commented May 26, 2014

taketwo commented May 26, 2014