Skip to content

Integrate QVTKOpenGLWidget from VTK 8#523

Closed
sankhesh wants to merge 1 commit intoRobotLocomotion:masterfrom
patmarion:vtk8-new-qvtk-widget
Closed

Integrate QVTKOpenGLWidget from VTK 8#523
sankhesh wants to merge 1 commit intoRobotLocomotion:masterfrom
patmarion:vtk8-new-qvtk-widget

Conversation

@sankhesh
Copy link
Copy Markdown
Contributor

QVTKOpenGLWidget changes ported from #508

@patmarion
Copy link
Copy Markdown
Member

Hi Sankhesh,

I am very excited to have this code in master, but I withheld it from #518 because I think there are a few more cleanups required before it's ready:

  1. Need to enable Qt5 and vtk8 testing on CI

  2. Should the QVTKOpenGLWidget widget only be used for qt5 and vtk8 with OpenGL2 backend? Current the code only checks the qt version. We could put the version checking into the cmake code and use a configured header to explicitly say whether or not to use QVTKOpenGLWidget, rather than doing the repeating the version check in all the places in the c++ code.

  3. The initialization that happens before QApplication could be moved into a reusable class and called from all the locations that construct QApplication. CI tests use directorPython from consoleApp.cpp, but drakeVisualizerApp.cpp.

@jamiesnape
Copy link
Copy Markdown

FYI @stonier. We need this pretty urgently for Drake.

@patmarion
Copy link
Copy Markdown
Member

I thought you built the current set of binaries from the #508 branch instead of master. Can you do that for the next round of packages so that you aren't blocked by this?

@sankhesh
Copy link
Copy Markdown
Contributor Author

Hi Pat,

  1. Need to enable Qt5 and vtk8 testing on CI

@jamiesnape has some ideas for getting xvfb to open an OpenGL 3.2 core context.

  1. Should the QVTKOpenGLWidget widget only be used for qt5 and vtk8 with OpenGL2 backend?

Yes, the `QVTKOpenGLWidget would only be available for the OpenGL2 backend. See https://gitlab.kitware.com/vtk/vtk/blob/72296e8d8f43fad32f08afefd81d471c4d8548b5/GUISupport/Qt/CMakeLists.txt#L37-41

Current the code only checks the qt version. We could put the version checking into the cmake code and use a configured header to explicitly say whether or not to use QVTKOpenGLWidget, rather than doing the repeating the version check in all the places in the c++ code.

That's a good idea. It would make the C++ code much cleaner. Although, would you mind handling it as part of the CMake code cleanup that you proposed during our conversation?

The initialization that happens before QApplication could be moved into a reusable class and called from all the locations that construct QApplication. CI tests use directorPython from consoleApp.cpp, but drakeVisualizerApp.cpp.

Sure. It could be. But since its just two consumers (and a three lines of code), we could do it as part of a separate follow-on PR. What do you think?

@jamiesnape
Copy link
Copy Markdown

We want to switch off VTK 5 support, so the CMake build including Director needs VTK 8 support. At present there are rendering issues without this.

@jamiesnape
Copy link
Copy Markdown

jamiesnape commented Jun 16, 2017

@jamiesnape has some ideas for getting xvfb to open an OpenGL 3.2 core context.

sudo apt-get install libgl1-mesa-glx-lts-xenial, I think.

@sankhesh
Copy link
Copy Markdown
Contributor Author

sudo apt-get install libgl1-mesa-glx-lts-xenial, I think.

I thought Travis-ci is already compiling against mesa (or I think I tried it). I'll try it again to be sure.

@jamiesnape
Copy link
Copy Markdown

Not the Xenial one.

@patmarion
Copy link
Copy Markdown
Member

Of the 3 cleanups I listed, only 2) and 3) are important code changes. 1) is for CI testing, but I don't mind delaying that. I think 2) and 3) are quick to implement.

@sankhesh
Copy link
Copy Markdown
Contributor Author

@jamiesnape @patmarion #528 supersedes this PR.

@sankhesh sankhesh closed this Jun 28, 2017
@patmarion patmarion deleted the vtk8-new-qvtk-widget branch December 29, 2025 21:59
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.

3 participants