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

Adding support of Qt5 for QGLViewer that was compiled with it #983

Merged
merged 8 commits into from
Apr 1, 2015
Merged

Adding support of Qt5 for QGLViewer that was compiled with it #983

merged 8 commits into from
Apr 1, 2015

Conversation

naubry
Copy link

@naubry naubry commented Mar 25, 2015

Hi,

I'm trying to add the support of Qt5 in order to use QGLViewer that was compiled with it. I'm not sure that is the best way to integrate it but I made the maximum to keep both functionnalities (The possibility to use Qt4 and/or Qt5).

I changed two files : CheckDGtalOptionalDependencies.cmake and Viewer3D.h.

In the Viewer3D.h file, I only changed one line and I think it's working on both Qt4 and Qt5 without a conditionnal test...

I remain at your disposal for any questions or modifications.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 90.25% when pulling d824816 on naubry:Qt5AndQGLViewer into 57adfd8 on DGtal-team:master.

@kerautret
Copy link
Member

Great, I look it, and by the way have you check if DGtalTools is also fine with QT5 ?

@naubry
Copy link
Author

naubry commented Mar 25, 2015

No, sorry I haven't verified if DGtalTools works with Qt5 and my modification.

@naubry
Copy link
Author

naubry commented Mar 25, 2015

I made some modifications in DGtal and DGtaltools and I have compiled 99% of DGtalTools for the moment..

@kerautret
Copy link
Member

I just tested on my config and from the construction i obtain:


CMake Error at cmake/CheckDGtalOptionalDependencies.cmake:340 (find_package):
By not providing "FindQt5.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "Qt5", but
CMake did not find one.

Could not find a package configuration file provided by "Qt5" with any of
the following names:

 Qt5Config.cmake
 qt5-config.cmake

Add the installation prefix of "Qt5" to CMAKE_PREFIX_PATH or set "Qt5_DIR"
to a directory containing one of the above files. If "Qt5" provides a
separate development package or SDK, be sure it has been installed.
Call Stack (most recent call first):
CMakeLists.txt:40 (INCLUDE)

@naubry
Copy link
Author

naubry commented Mar 25, 2015

You must specify by hand the path where cmake can find qmake and where are the Qt5config files.
Variables are : QT_QMAKE_EXECUTABLE and Qt5_DIR

For me for instance, qmake is here : /Users/naubry/Qt/5.4/clang_64/bin/qmake
and the Qt5config files : /Users/naubry/Qt/5.4/clang_64/lib/cmake/Qt5

I have used cmake-gui to add these variables, it's more convenient :)

@kerautret
Copy link
Member

My last message was from my current build, so I tried with an empty one I have no error message but with your change it doesn't consider QT 4 so when QGLviewer is activated with ccmake construct as all as if it was not present. ;(

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 90.35% when pulling d730f3b on naubry:Qt5AndQGLViewer into 57adfd8 on DGtal-team:master.

@@ -21,7 +21,7 @@ OPTION(WITH_MAGICK "With GraphicsMagick++." OFF)
OPTION(WITH_ITK "With Insight Toolkit ITK." OFF)
OPTION(WITH_CAIRO "With CairoGraphics." OFF)
OPTION(WITH_HDF5 "With HDF5." OFF)
OPTION(WITH_QGLVIEWER "With LibQGLViewer for 3D visualization (Qt required)." OFF)
OPTION(WITH_QGLVIEWER_QT "With LibQGLViewer for 3D visualization (Qt required)." OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the WITH_QGLVIEWER flag, this flag is used at many places in the code.

@dcoeurjo
Copy link
Member

I would suggest

  • to keep QT4 as a (default) dependency (with REQUIRED) when WITH_LIBQGLVIEWER is on
  • to create an additional WITH_QGLVIEWER_QT5 flag for advanced users (default "off") such that if the flag is set to true, then the FIND will search for QT5 instead of QT4
  • if needed in the code, transfer the WITH_QGLVIEWER_QT5 to the compiler (add_definition) such that you can do some #ifdefif necessary

@dcoeurjo
Copy link
Member

@naubry does it make sense for you ?

@naubry
Copy link
Author

naubry commented Mar 26, 2015

That makes sense to me. I have already started working in this direction last night.

@naubry
Copy link
Author

naubry commented Mar 26, 2015

I have added an option to activate Qt5.
DGtal lib, examples and tests compile and run on both system Linux and OSX.
I have also modified DGtalTools with success. So, I can make a PR for this modifications soon.

@naubry
Copy link
Author

naubry commented Mar 26, 2015

Sorry, I made a mistake when I merged my new code with this branch...

@kerautret
Copy link
Member

Looks fine for me, tested with default QT4 and QT5 (we need to specify the qt5 dir:Qt5_DIR=/Users/kerautre/Qt/5.4/clang_64/lib/cmake/Qt5/) .
It looks also ok in DGTalTools (by the way Nicolas you can PR in main DGtalTools ? )
@dcoeurjo for DGtalTools I needed to apply :--DQt5_DIR=/Users/kerautre/Qt/5.4/clang_64/lib/cmake/Qt5/
I don't remember how it can be transmitted automatically in DGtaTools ?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 90.14% when pulling 2e70f15 on naubry:Qt5AndQGLViewer into 57adfd8 on DGtal-team:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.22%) to 90.12% when pulling 2e70f15 on naubry:Qt5AndQGLViewer into 57adfd8 on DGtal-team:master.

@dcoeurjo
Copy link
Member

Thanks @naubry. There is still something that puzzles me. The QT includes:

 #ifdef WITH_QT5
    #include <QApplication>
#else
   #include <QtGui/qapplication.h> 
#endif

in the examples seem to be duplicated since there are already included in Viewer3D. I know some of them were already there before your QT5 thing but maybe we should cleanup this up.

@dcoeurjo
Copy link
Member

@kerautret See the the DGtalConfig.cmake.in line 144, maybe just add a set(QT5_DIR @QT5_DIR@) ?

@kerautret can you make sure that in the doc, we only require to include "Viewer3D.h" and not the Qt stuff when using the viewer ? (Viewer3D should be in charge to include all qt deps)

@kerautret
Copy link
Member

thanks @dcoeurjo yes I confirm it works (just lower case for the name).
@naubry I PR on your branch

@kerautret
Copy link
Member

I look it for the doc

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 90.14% when pulling a0cbb0f on naubry:Qt5AndQGLViewer into 57adfd8 on DGtal-team:master.

@kerautret
Copy link
Member

@dcoeurjo I confirm the doc don''t refer to any QT include...

@dcoeurjo
Copy link
Member

dcoeurjo commented Apr 1, 2015

Perfect @naubry thanks a lot..
Congrats for your first PR !*
(I'll add you to the AUTHOR list)

dcoeurjo added a commit that referenced this pull request Apr 1, 2015
Adding support of Qt5 for QGLViewer that was compiled with it
@dcoeurjo dcoeurjo merged commit acfd210 into DGtal-team:master Apr 1, 2015
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.

4 participants