Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. #2051

frozar · 2017-11-03T08:34:23Z

When I try to compile and execute the example doc/tutorials/content/sources/qt_visualizer, I got a mysterious and immediat error concerning realloc(). After some research I found this comment that describes the issue:
https://stackoverflow.com/questions/36725763/c-realloc-invalid-pointer#comments-36725978

It's not possible to have a program that links to both Qt4 and Qt5 libraries. The dependance from Qt4 comes from the file CMakeLists.txt given as example, and the dependance from Qt5 comes from VTK library.

The purpose of this pull request is to provide an example of PCL using Qt5 as graphical interface, instead of Qt4.

Documentation ressources:
https://github.com/euler0/mini-cmake-qt/blob/master/CMakeLists.txt
http://doc.qt.io/qt-5/cmake-manual.html
https://wiki.qt.io/Using_CMake_build_system

taketwo · 2017-11-03T23:16:18Z

But VTK can be compiled with both Qt4 and Qt5?

frozar · 2017-11-06T08:23:00Z

In fact, it's possible that VTK can be compile with Qt4 or Qt5, but you can have trouble when you use binary packages available from Ubuntu repositories.

When I look at the dependancies of the packages pcl-tools, I get:

apt-cache depends pcl-tools
pcl-tools
  [...]
  Dépend: libqtcore4
  Dépend: libqtgui4
  [...]
  Dépend: libvtk6.2-qt
  [...]

When I look at the dependancies of the packages libvtk6.2-qt, I get:

libvtk6.2-qt
  [...]
  Dépend: libqt5core5a
 |Dépend: libqt5gui5
  Dépend: libqt5gui5-gles
  Dépend: libqt5network5
 |Dépend: libqt5opengl5
  Dépend: libqt5opengl5-gles
  Dépend: libqt5webkit5
  Dépend: libqt5widgets5
  [...]

So both Qt4 and Qt5 is installed.

When I try to compile the example doc/tutorials/content/sources/qt_visualizer, if remove the link against the VTK library uin the CMakeLists.txt file, it compiles, but at the execution I get the foolowing error:

./pcl_visualizer 
*** Error in `./pcl_visualizer': realloc(): invalid pointer: 0x00007f90faf48820 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f90f635d7e5]
/lib/x86_64-linux-gnu/libc.so.6(realloc+0x348)[0x7f90f636aa08]
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5(_ZN9QListData7reallocEi+0x1f)[0x7f90e8d009cf]
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5(_ZN9QListData6appendEi+0x81)[0x7f90e8d00aa1]
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5(+0x1d6d78)[0x7f90e8dccd78]
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5(_Z21qRegisterResourceDataiPKhS0_S0_+0x2e6)[0x7f90e8dc8b16]
/usr/lib/x86_64-linux-gnu/libQt5Core.so.5(+0x7bcc3)[0x7f90e8c71cc3]
/lib64/ld-linux-x86-64.so.2(+0x106ba)[0x7f90fbc4e6ba]
/lib64/ld-linux-x86-64.so.2(+0x107cb)[0x7f90fbc4e7cb]
/lib64/ld-linux-x86-64.so.2(+0xc6a)[0x7f90fbc3ec6a]

Another user already identifies this issue. Roughly it said that you cannot have an executable that both link against Qt4 and Qt5.

He chose to downgrade his version of VTK to be able to execute but I prefer to upgrade the example CMakeLists.txt to be able to compile with Qt5.

PS: it's surprising that binaries available in the pcl-tools package doesn't have this issue...

taketwo · 2017-11-06T09:03:49Z

Ideally, we should check whether VTK was built with Qt4 or Qt5 and use the same. This information should be available in the VTK_QT_VERSION variable after VTK finder script has been executed.

That being said, I do not think that maintaining Qt4 compatibility is a priority for PCL and thus I'm fine with making Qt5 a strict requirement, at least in a tutorial code. @SergioRAgostinho opinions?

SergioRAgostinho · 2017-11-06T12:05:08Z

I would actually migrate everything to Qt5, not only the tutorial code.

frozar · 2017-11-06T12:52:26Z

I think that I can help to migrate all tutorial code (I think there is only two examples) in this merge request, but what do you mean exactly when you say "migrate everything"? Is it a big stuff?

SergioRAgostinho · 2017-11-06T13:07:02Z

We have some apps and tools which are only QT4 compatible and I meant porting those. I would need to dig into all of our cmake files to be sure, but everything within CMake conditional blocks checking for QT4_FOUND.

frozar · 2017-11-06T13:21:57Z

I don't know if it helps, but I did a grep:

grep -rn "QT4_FOUND" ~/wk/pcl_repo 
/home/frozar/wk/pcl_repo/CMakeLists.txt:350:    if (QT4_FOUND)
/home/frozar/wk/pcl_repo/CMakeLists.txt:352:    endif (QT4_FOUND)
/home/frozar/wk/pcl_repo/gpu/people/tools/CMakeLists.txt:34:#  if(QT4_FOUND AND VTK_USE_QVTK)
/home/frozar/wk/pcl_repo/apps/CMakeLists.txt:75:    if (QT4_FOUND AND VTK_USE_QVTK)
/home/frozar/wk/pcl_repo/apps/CMakeLists.txt:88:    endif (QT4_FOUND AND VTK_USE_QVTK)
/home/frozar/wk/pcl_repo/apps/CMakeLists.txt:149:      if (QT4_FOUND AND VTK_USE_QVTK)
/home/frozar/wk/pcl_repo/apps/modeler/CMakeLists.txt:18:if(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/modeler/CMakeLists.txt:24:endif(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/cloud_composer/CMakeLists.txt:21:if(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/cloud_composer/CMakeLists.txt:27:endif(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/in_hand_scanner/CMakeLists.txt:8:if(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/point_cloud_editor/CMakeLists.txt:59:IF(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/point_cloud_editor/CMakeLists.txt:62:ELSE(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/apps/point_cloud_editor/CMakeLists.txt:65:ENDIF(NOT QT4_FOUND)
/home/frozar/wk/pcl_repo/cmake/pcl_find_qt5.cmake:77:    set(QT4_FOUND TRUE)

Question:
What do you want to do for this merge request? Just update two tutorial cmake files or do the huge stuff?

taketwo · 2017-11-06T13:23:33Z

I'd say lets keep this PR scoped in tutorial code.
More serious things like complete depreciation and removal of Qt4 support can be handled later.

SergioRAgostinho · 2017-11-06T13:24:51Z

I'd say lets keep this PR scoped in tutorial code.

Same here.

taketwo · 2017-11-06T13:32:10Z

Besides, I'm not convinced that we need to remove Qt4 support at all. Right now it seems to work without issues, so why fixing something that is not broken :)

frozar · 2017-11-06T13:35:38Z

Ok, I'll convert the example doc/tutorials/content/sources/qt_colorize_cloud to use Qt5 soon.

I think it's the only other Qt example, isn't it?

SergioRAgostinho · 2017-11-06T13:38:42Z

The only, yet annoying, "issue" I faced so far was that Homebrew dropped support for Qt4, so on Mac, if you want meet this dependency you have to go compile it yourself. On trusty and xenial I think it's still there. On vcpkg it's only Qt5 as well.

taketwo · 2017-11-06T13:40:41Z

I think it's the only other Qt example, isn't it?

Seems so.

The only, yet annoying, "issue" I faced so far was that Homebrew dropped support for Qt4, so on Mac, if you want meet this dependency you have to go compile it yourself.

But PCL can be compiled with Qt5, what is the problem?

SergioRAgostinho · 2017-11-06T13:43:00Z

But PCL can be compiled with Qt5, what is the problem?

Some of our shipped apps and tools only support Qt4.

taketwo · 2017-11-06T13:46:58Z

Ah, OK. Then I'd say the goal should be to update those to work with both versions (which is absolutely trivial in most cases).

frozar · 2017-11-06T17:03:52Z

I adapted the CMake files of tutorials qt_visualizer and qt_colorize_cloud and rebase the branch on top of master.

taketwo · 2017-11-06T17:08:16Z

doc/tutorials/content/sources/qt_colorize_cloud/CMakeLists.txt


-TARGET_LINK_LIBRARIES (colorize_cloud ${QT_LIBRARIES} ${PCL_LIBRARIES} ${VTK_LIBRARIES})
+TARGET_LINK_LIBRARIES (${PROJECT_NAME} ${QT_LIBRARIES} ${PCL_LIBRARIES})


I believe explicitly linking against QT_LIBRARIES is not needed, the qt5_use_modules command below should take care of this.

Also, since you are rewriting most of the content anyway, can you please convert CMake function calls to lowercase? Uppercasing is an outdated practice.

Ok. Should I also take care of spaces before parenthesis?

taketwo

Thanks

SergioRAgostinho

I would like to convert everything regarding include_directories, libraries and definitions to be target specific, i.e.target_include_directories, target_link_libraries and target_add_definitions, because these allow to specify visibility to other things down the line. In this case it makes no difference because it's a super simple example but our newbie users simply copy things blindly and this would get them used to constraint definitions/headers/libraries to the intended targets.

SergioRAgostinho · 2017-11-06T20:21:51Z

doc/tutorials/content/sources/qt_colorize_cloud/CMakeLists.txt


-TARGET_LINK_LIBRARIES (colorize_cloud ${QT_LIBRARIES} ${PCL_LIBRARIES} ${VTK_LIBRARIES})
+include_directories(${PCL_INCLUDE_DIRS})
+link_directories(${PCL_LIBRARY_DIRS})


This should not be required.

Effectively, the link_directories instruction is not required.

SergioRAgostinho · 2017-11-06T20:34:54Z

doc/tutorials/content/sources/qt_visualizer/CMakeLists.txt


-TARGET_LINK_LIBRARIES (pcl_visualizer ${QT_LIBRARIES} ${PCL_LIBRARIES} ${VTK_LIBRARIES})
+include_directories(${PCL_INCLUDE_DIRS})
+link_directories(${PCL_LIBRARY_DIRS})


I'm not sure this is necessary

taketwo · 2017-11-07T08:28:45Z

I'd argue that we as a library should provide our exported module targets with properly set up interface include directories, definitions, etc. so that the only thing the users need to do is write target_link_libraries(user_target pcl_common pcl_io ...).

SergioRAgostinho · 2017-11-07T08:36:11Z

That is true, but that requires a massive rework in our CMake files. Nothing is adhering to that.. :/

…t tutorials.

…ointCloudLibrary#2051) Migration of qt_visualizer and qt_colorize_cloud tutorials to Qt5.

frozar and others added 2 commits November 6, 2017 18:00

[TUTO] Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5.

bb0eb9d

[TUTO] Fix the tutorial qt_colorize_cloud compilation issue: qt4 -> qt5.

56b6306

frozar force-pushed the pcl_qt5_compilation branch from 4fa5c74 to 56b6306 Compare November 6, 2017 17:00

taketwo reviewed Nov 6, 2017

View reviewed changes

frozar added 2 commits November 6, 2017 18:16

[TUTO] WIP: Cmake

6ee355b

[TUTO] Coding style in CMake files.

db630a6

taketwo approved these changes Nov 6, 2017

View reviewed changes

SergioRAgostinho reviewed Nov 6, 2017

View reviewed changes

frozar added 2 commits November 7, 2017 09:46

[CMAKE] Remove the 'link_directories' instruction.

ad0e25a

[TUTO] Remove the need to be in a directory '../build' to build the Q…

6cb6424

…t tutorials.

SergioRAgostinho merged commit c9ae6d7 into PointCloudLibrary:master Nov 8, 2017

frozar deleted the pcl_qt5_compilation branch November 8, 2017 09:12

UnaNancyOwen pushed a commit to UnaNancyOwen/pcl that referenced this pull request Nov 24, 2017

[TUTO] Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. (P…

a587697

…ointCloudLibrary#2051) Migration of qt_visualizer and qt_colorize_cloud tutorials to Qt5.

SergioRAgostinho mentioned this pull request Sep 2, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

taketwo changed the title ~~[TUTO] Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5.~~ Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. Sep 2, 2018

taketwo added the module: tutorials label Sep 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. #2051

Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. #2051

frozar commented Nov 3, 2017

taketwo commented Nov 3, 2017

frozar commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

frozar commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

frozar commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

frozar commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

frozar commented Nov 6, 2017

taketwo Nov 6, 2017

frozar Nov 6, 2017

taketwo left a comment

SergioRAgostinho left a comment •

edited

Loading

SergioRAgostinho Nov 6, 2017

frozar Nov 7, 2017

SergioRAgostinho Nov 6, 2017

frozar Nov 7, 2017

taketwo commented Nov 7, 2017

SergioRAgostinho commented Nov 7, 2017


		TARGET_LINK_LIBRARIES (colorize_cloud ${QT_LIBRARIES} ${PCL_LIBRARIES} ${VTK_LIBRARIES})
		TARGET_LINK_LIBRARIES (${PROJECT_NAME} ${QT_LIBRARIES} ${PCL_LIBRARIES})

Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. #2051

Fix the tutorial qt_visualizer compilation issue: qt4 -> qt5. #2051

Conversation

frozar commented Nov 3, 2017

taketwo commented Nov 3, 2017

frozar commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

frozar commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

frozar commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

frozar commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

SergioRAgostinho commented Nov 6, 2017

taketwo commented Nov 6, 2017

frozar commented Nov 6, 2017

taketwo Nov 6, 2017

Choose a reason for hiding this comment

frozar Nov 6, 2017

Choose a reason for hiding this comment

taketwo left a comment

Choose a reason for hiding this comment

SergioRAgostinho left a comment • edited Loading

Choose a reason for hiding this comment

SergioRAgostinho Nov 6, 2017

Choose a reason for hiding this comment

frozar Nov 7, 2017

Choose a reason for hiding this comment

SergioRAgostinho Nov 6, 2017

Choose a reason for hiding this comment

frozar Nov 7, 2017

Choose a reason for hiding this comment

taketwo commented Nov 7, 2017

SergioRAgostinho commented Nov 7, 2017

SergioRAgostinho left a comment •

edited

Loading