Codecov Report

Merging #200 (07eb6fd) into main (7157bbb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #200   +/-   ##
=======================================
  Coverage   97.22%   97.22%           
=======================================
  Files           8        8           
  Lines         648      648           
  Branches       90       90           
=======================================
  Hits          630      630           
  Partials       18       18           

Looks like segfaults in almost every CI. Things were green on #202. Can you replicate locally @adam-grant-hendry ?

@larsoner Yes, let me confirm where it’s happening.

@larsoner I think the segfaults were from trying to add the window container within QVTKRenderWindowInteractor:

window_ = QtGui.QWindow.fromWinId(self.plotter.winId())
self.container = self.createWindowContainer(window_, self)

We may just have to teach users to add this to their MainWindow classes. I cannot find a good place to put this, unless we explicitly make MainWindow subclass from QMainWindow and one of QtInteractor or BackgroundPlotter.

@larsoner @tkoyama010 Can I request one of you please review?

I should be able to review in about a week. One comment without looking at the changes though:

We may just have to teach users to add this to their MainWindow classes.

We use a MainWindow subclass that gets used for BackgroundPlotter:

pyvistaqt/window.py:10:class MainWindow(QMainWindow):

Maybe it could be added there? That would benefit PyVistaQt users who use BackgroundPlotter already (rather than the QtInteractor directly). This would also then test that your change works for BackgroundPlotter, and hence should work for users as well.

And +1 for modifying docs/usage.rst: class MyMainWindow(MainWindow): with this advice

I should be able to review in about a week. One comment without looking at the changes though:

I took a quick look and this does look good so far. Hopefully the MainWindow changes are not too painful to implement and just work immediately!

@larsoner @tkoyama010 I was wrong: the errors occur when QVTKRenderWindowInteractor subclasses from anything other than QWidget.

On Windows, X11, and MacOS, with Qt>5.4 the primary paint engine for QWidget-based classes is the raster paint engine. When the raster engine is used, Windows and macOS ignore WA_PaintOnScreen, but since we call winId (which works on all platforms), we get a native widget with the raster engine, so returning None from QWidget::paintEngine() doesn't cause an error.

Using an opengl paint engine, however, causes a problem. Since Qt4, all QWidget's are double-buffered, and WA_PaintOnScreen was added for backwards-compatibility to allow setting a widget to use single buffering. However, single buffering will not work with QOpenGLWidgets as the Qt docs clearly state:

QOpenGLWidget always renders offscreen, using framebuffer objects. QGLWidget on the other hand uses a native window and surface.

Hence QOpenGLWidget is always going to be double-buffered.

The QVTKOpenGL widgets were introduced to work with Qt's QOpenGLWidget, but these have not been ported to Python VTK (i.e. vtkmodules).

In PyQt5>5.4 and all of PyQt6 (and their PySide equivalents), the QGL classes are gone and replaced by the QOpenGL classes. So, if we want to support the latest versions of PyQt and PySide, let users fix rendering glitches on their hardware (esp. non-desktop versions) we need DLL's for the QVTKOpenGL widgets ported to vtkmodules.

Alternatively, if those DLL's can't be ported, we could try to implement these classes using the Python Wrappers developer notes.

we need DLL's for the QVTKOpenGL widgets ported to vtkmodules.

This change would need to be made at the VTK end... and this would not be easy. Also I'm still concerned that, even if they added these, they would necessarily be specific to one flavor of Qt (at least 6 vs 5, if not even as specific as 6.x.y vs 6.x.z).

It might be worth searching the VTK issue tracker to see if anyone has brought up the idea of them shipping a Qt 6 variant of a QVTKOpenGL widget in vtkmodules -- that seems the most future compatible (moreso than Qt 5 at least), and if not, opening a new issue for discussion:

https://gitlab.kitware.com/vtk/vtk/-/issues

Feel free to ping me over there if you find an issue or open a new one (same username as GH)!

we need DLL's for the QVTKOpenGL widgets ported to vtkmodules.

This change would need to be made at the VTK end... and this would not be easy. Also I'm still concerned that, even if they added these, they would necessarily be specific to one flavor of Qt (at least 6 vs 5, if not even as specific as 6.x.y vs 6.x.z).

It might be worth searching the VTK issue tracker to see if anyone has brought up the idea of them shipping a Qt 6 variant of a QVTKOpenGL widget in vtkmodules -- that seems the most future compatible (moreso than Qt 5 at least), and if not, opening a new issue for discussion:

https://gitlab.kitware.com/vtk/vtk/-/issues

Feel free to ping me over there if you find an issue or open a new one (same username as GH)!

I spoke with David Gobbi (@dgobbi), who created the first QVTKRenderWindowInteractor, in VTK Issue #18606. We actually agreed making DLL's for the QVTK classes won't help: we would need to also wrap PyQtX libraries with sip and PySideX libraries with shiboken. Due to licensing and binding differences, this would require creating 4 separate packages (vtk-qt5, vtk-qt6, vtk-pyside2, and vtk-pyside6), which is not maintainable nor desired.

Instead, we agreed a pure python approach would be best. To that end, I've already begun writing all the QVTK C++ classes in pure python (using the binding guides from the Python Wrapper notes). I'm also adding a Testing subfolder with a CMakeLists.txt that will add unittest-style tests (this pattern is consistent with other such tests in the VTK library).

QOpenGLWidget is needed because the current QVTKRenderWindowInteractor implementation using WA_PaintOnScreen forces single-buffering, which causes issues. For one, when loading in a large file and displaying a progress bar dialog, both the progress bar and interactor try to draw to the screen simultaneously, causing a deadlock (i.e. blue-circle/rainbow-wheel of death). In all GUI frameworks (OpenGL, Metal, Direct3D, etc.), only one thread may draw to the screen at a time (i.e. the application main thread).

This is just one example, but there are several other rendering issues that support for QOpenGLWidget will fix. With a back buffer, we can render to it with multiple QThreads (see the official Qt example). Not only should this fix deadlock issues, but help us render faster and better support offscreen rendering activities.

Just requesting that our MainWindow subclass be modified if possible (and if it still makes sense)

Yes, it still makes sense. I should push the QVTK python classes in an MR to VTK Issue #18606 today or tomorrow. The intent of these is to replace the original QVTKRenderWindowInteractor (perhaps a deprecation warning will be added?), so some pyvista and mayavi code will need to change (but likely not all that much).

Sounds good, thanks for tackling this @adam-grant-hendry !

Sounds good, thanks for tackling this @adam-grant-hendry !

Of course, my pleasure.

The VTK team has requested I get reviewers from the pyvista and mayavi maintainers. Would you be willing to be a reviewer if I pinged you when I am ready?

Yes, gladly!