Skip to content

Port vtk7 qt5#508

Closed
sankhesh wants to merge 42 commits intoRobotLocomotion:masterfrom
sankhesh:port-vtk7-qt5
Closed

Port vtk7 qt5#508
sankhesh wants to merge 42 commits intoRobotLocomotion:masterfrom
sankhesh:port-vtk7-qt5

Conversation

@sankhesh
Copy link
Copy Markdown
Contributor

Support VTK 7 / 8 and Qt 5

set(PythonQt_TAG)
if(${DD_QT_VERSION} VERSION_GREATER "4")
set(PythonQt_REPO https://github.com/patmarion/PythonQt.git)
set(PythonQt_TAG fix-compile-error-on-qt5.8)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@patmarion Mind creating a PR on PythonQt for branch fix-compile-error-on-qt5.8 and mention this PR (#508)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@patmarion ping!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes, eventually we will get this in. upstream PythonQt is actually a svn repo. The CommonToolkit group manages some unofficial patched branches on Github. I think this is already fixed upstream in svn but I'm not sure if CTK's branches are up to date or not. I will try to look into it. In the meantime, I have no problem referencing this branch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sigh... commontk/PythonQt#50 (apparently I can create a PR of Pat's branch).

@sankhesh, does patched-7 still work with Qt4?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i think patched-7 has removed qt4 support. the src dir generated_cpp_47 was removed in that branch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay... but at least commontk/PythonQt#50 would let us use the same repository in either case, and just a different branch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks @patmarion @mwoehlke-kitware

I'm not sure why pythonqt decided to drop the generated sources for Qt4 because the CMake code still seems to support both Qt4 and Qt5. Compilation fails when it can't find the generated sources with Qt4.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It can definitively be re-added. It would be nice to work with Florian (from mevislab) and push the patches upstream. I do not have the bandwidth currently ...

set(ctkPythonConsole_TAG)
if(${DD_QT_VERSION} VERSION_GREATER "4")
set(ctkPythonConsole_REPO https://github.com/mwoehlke-kitware/ctkPythonConsole)
set(ctkPythonConsole_TAG qt5)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mwoehlke-kitware Mind creating a PR for branch qt5 on ctkPythonConsole and mention this PR (#508) ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed; I think we should be able to use a single repo/tag here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On closer inspection, it seems this would need tweaking to work with either Qt4 or Qt5. I don't know that I'll be able to do that; @sankhesh, can you do it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mwoehlke-kitware I'll look into it.

@sankhesh
Copy link
Copy Markdown
Contributor Author

@patmarion @mwoehlke-kitware Please review

@patmarion
Copy link
Copy Markdown
Member

Thanks, I'll try to take a look today and review!

@patmarion
Copy link
Copy Markdown
Member

By the way, can you tell me which versions of VTK and Qt (and OS) you're testing with?

@sankhesh
Copy link
Copy Markdown
Contributor Author

Thank you @patmarion

I compiled and tested with Kitware/VTK@28deb56 (OpenGL2 backend)and Qt versions 4.8, 5.5.1, 5.7.1 and 5.8.0

Note My testing was limited to running ctest and loading a simple OBJ file.

@patmarion
Copy link
Copy Markdown
Member

it looks like the tests requiring opengl are failing with the new vtk bottles due to the vtk rendering backend. did something change related to opengl between the new bottles and the previous bottles where tests were passing?

@jamiesnape
Copy link
Copy Markdown

We are using VTK 8.0.0 RC2 instead of master. I have no idea if that introduced changes.

@sankhesh
Copy link
Copy Markdown
Contributor Author

sankhesh commented Jun 9, 2017

@patmarion Nothing has changed in terms of OpenGL between master and VTK RC2. It seems like xvfb is not returning an OpenGL context newer than 2.1. The minimum that VTK requires is an OpenGL core 3.2

@patmarion
Copy link
Copy Markdown
Member

Hmm, here are the failed tests today, they are segfaulting on exit:

http://128.30.27.135/viewTest.php?onlyfailed&buildid=614

Last week, these tests passed. They actually printed the same error messages, but did not segfault on exit:

http://128.30.27.135/viewTest.php?onlypassed&buildid=569

@patmarion
Copy link
Copy Markdown
Member

Is drake's jenkins CI running tests with VTK using xvfb?

@jamiesnape
Copy link
Copy Markdown

jamiesnape commented Jun 9, 2017

Not at present. It did briefly for VTK 5 and VTK 7.1.1. We had to run sudo apt-get install libgl1-mesa-glx-lts-xenial on Trusty.

@sankhesh
Copy link
Copy Markdown
Contributor Author

sankhesh commented Jun 9, 2017

@patmarion @jamiesnape

There are two issues at play here:

  • Xvfb creates an OpenGL 2.1 context which is older than the minimum supported by VTK (3.2)
    • Try compiling VTK against a newer mesa for Travis-CI and/or
    • Using a different X server (vnc, perhaps)
  • Segfault at exit
    • I'm bringing up the docker container locally to see if I can debug this.

@jamiesnape
Copy link
Copy Markdown

@jamiesnape
Copy link
Copy Markdown

jamiesnape commented Jun 9, 2017

Try compiling VTK against a newer mesa for Travis-CI

We did not have to recompile on Jenkins.

@patmarion
Copy link
Copy Markdown
Member

Another option is to provide a VTK package with the previous rendering backend for use on CI servers and by users without modern hardware.

@jamiesnape
Copy link
Copy Markdown

Another option is to provide a VTK package with the previous rendering backend for use on CI servers and by users without modern hardware.

I would not support that.

@sankhesh
Copy link
Copy Markdown
Contributor Author

sankhesh commented Jun 9, 2017

Another option is to provide a VTK package with the previous rendering backend for use on CI servers and by users without modern hardware.

The OpenGL backend support in VTK is short-lived post v8.0. We'd have to eventually move away from it.

@sankhesh
Copy link
Copy Markdown
Contributor Author

sankhesh commented Jun 9, 2017

@sankhesh This issue? RobotLocomotion/drake#6259 (comment)

Thanks for pointing that out @jamiesnape. It looks like a valid issue.
@patmarion Do you know where it originates from?

@patmarion
Copy link
Copy Markdown
Member

Another option is to provide a VTK package with the previous rendering backend for use on CI servers and by users without modern hardware.

I would not support that.

I understand Drake has a specific set of VTK versions and packages it uses, but that doesn't have to dictate the versions that are used by Director's Travis-CI.

@jamiesnape
Copy link
Copy Markdown

I will defer to @stonier to comment on that, but there is a maintenance cost to creating packages, which is why we are only supporting certain configurations. If we start having to support multiple backends for users without modern hardware, then the workload is doubled.

@patmarion
Copy link
Copy Markdown
Member

Are you planning to use the packages produced by director travis-cs? I thought I saw some comments about making drake's director packages on jenkins?

@jamiesnape
Copy link
Copy Markdown

Yes, they will be made on Jenkins.

@patmarion
Copy link
Copy Markdown
Member

Ok perfect. So after this PR is completed and merged, I will probably invest some time to make my own vtk packages to cover the new variety of support configurations so those can all get tested on director's travis-ci, but this shouldn't need to affect any drake usage.

@jamiesnape
Copy link
Copy Markdown

Sounds good.

@patmarion
Copy link
Copy Markdown
Member

patmarion commented Jun 10, 2017

@sankhesh it looks like the mac build is timing out while compiling PyQt from source during the brew install stage.

1). Please consider making VTK packages that do not require PyQt to be compiled from source.
2). If you must do a long running brew command, you can use the brew_install.sh script which prints progress, this prevents TravisCI from timing out. The brew install script is here:

https://github.com/RobotLocomotion/director/blob/master/distro/travis/brew_install.sh

Just call that script brew_install.sh instead of brew install. If you use this script, you should edit line 11 to remove the --build-bottle flag, it looks like I added that a while back but should have removed it.

@patmarion
Copy link
Copy Markdown
Member

I am closing this in favor of #518. In #518 I updated the vtk packages and travis-ci scripts to get the build passing. It also has everything from this branch except the work in progress to integrate the QVTKOpenGLWidget of VTK 8. I put the QVTKOpenGLWidget work on this branch: https://github.com/patmarion/director/tree/vtk8-new-qvtk-widget

CDash link: http://128.30.27.135/index.php?project=director

@patmarion patmarion closed this Jun 12, 2017
@sankhesh
Copy link
Copy Markdown
Contributor Author

@patmarion Btw, were you able to reproduce the segfault at exit on a local build? I wasn't.

@patmarion patmarion mentioned this pull request Jun 12, 2017
@patmarion
Copy link
Copy Markdown
Member

@patmarion Btw, were you able to reproduce the segfault at exit on a local build? I wasn't.

No, but I am building with VTK 7.1 and OpenGL backend. I plan to add testing for OpenGL2 backend but it depends on fixing the Xvfb/glx issue we were seeing in #508.

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.

6 participants