Fix QtInteractor and BackgroundPlotter issues

[banesullivan]

#548 introduced issues with threading between VTK and PyQt. These changes fix those issues and attempt to address #600

Detail

• Fix closing issues where BasePlotter.close() was being called multiple times. It should only be called once.
• Fix camera lagging issue by removing the Modified callback when moving the camera (this one was on me) and calling Modified on the Renderer anytime the camera's position is set (not moved interactively)
• Attempt a fix for the q key on the BackgroundPlotter it was totally messed up and even these changes aren't great... I get random segfaults that crash the kernel from time to time with this (but not every time so this is better). Its mostly mitigated.
• Thread the render call for the QtInteractor class - this was the cause of the segfaults happening during cell picking and other times where VTK modified the renderer.
• Add segfault handler on __init__ of PyVista to yield tracebacks when segfaults occur (I believe faulthandler is a standard lib package, but I put it in a try statement just in case)
• Remove all observers of the Renderer and its camera when closing
• Fix Qt example in the docs to reflect API changes from Duplicated figures with BackgroundPlotter in sphinx #548
• Make sure the _first_time flag is set to false after creating the QtInteractor to make sure render calls aren't blocked (relevant to Interacting with QT : Graphics are not displaying until clicking on graph area pyvista-support#86)
• repurpose auto_update to take float time interval for update rate

[banesullivan]

#548 changed the inheritance of render window interactor through the new QVTKRenderWindowInteractorAdapter class. Unfortunately, this breaks downstream projects... (and breaks the code in pyvista/pyvista-support#86). The changes were needed, so we may have to throw a big ole deprecation warning or figure out how to support backward compatibility

[codecov]

Codecov Report

Merging #603 into master will increase coverage by 1.51%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #603      +/-   ##
==========================================
+ Coverage   83.91%   85.42%   +1.51%     
==========================================
  Files          32       32              
  Lines        9074    10274    +1200     
==========================================
+ Hits         7614     8777    +1163     
- Misses       1460     1497      +37

[banesullivan]

import faulthandler
faulthandler.enable()

import pyvista as pv
mesh = pv.Sphere()

p = pv.BackgroundPlotter()
p.add_mesh(mesh)
p.enable_cell_picking(through=True, start=True)

results in the following traceback

python(41022,0x112165dc0) malloc: *** error for object 0x690: pointer being freed was not allocated
python(41022,0x112165dc0) malloc: *** set a breakpoint in malloc_error_break to debug
Fatal Python error: Aborted

Thread 0x0000700011792000 (most recent call first):
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/parentpoller.py", line 39 in run
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 917 in _bootstrap_inner
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 885 in _bootstrap

Thread 0x0000700011000000 (most recent call first):
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 296 in wait
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 552 in wait
  File "/Users/bane/.local/lib/python3.7/site-packages/IPython/core/history.py", line 829 in run
  File "/Users/bane/.local/lib/python3.7/site-packages/IPython/core/history.py", line 58 in needs_sqlite
  File "</Users/bane/.local/lib/python3.7/site-packages/decorator.py:decorator-gen-24>", line 2 in run
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 917 in _bootstrap_inner
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 885 in _bootstrap

Thread 0x00007000109f7000 (most recent call first):
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/heartbeat.py", line 61 in run
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 917 in _bootstrap_inner
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 885 in _bootstrap

Thread 0x00007000104f4000 (most recent call first):
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/selectors.py", line 558 in select
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/asyncio/base_events.py", line 1739 in _run_once
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/asyncio/base_events.py", line 539 in run_forever
  File "/Users/bane/.local/lib/python3.7/site-packages/tornado/platform/asyncio.py", line 148 in start
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/iostream.py", line 78 in _thread_main
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 865 in run
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 917 in _bootstrap_inner
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/threading.py", line 885 in _bootstrap

Current thread 0x0000000112165dc0 (most recent call first):
  File "/Users/bane/Software/pyvista/pyvista/pyvista/plotting/plotting.py", line 955 in update
  File "/Users/bane/Software/pyvista/pyvista/pyvista/plotting/qt_plotting.py", line 442 in <lambda>
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/site-packages/vtkmodules/qt/QVTKRenderWindowInteractor.py", line 460 in mouseReleaseEvent
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/eventloops.py", line 106 in _loop_qt
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/eventloops.py", line 122 in loop_qt4
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/eventloops.py", line 134 in loop_qt5
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/kernelbase.py", line 263 in enter_eventloop
  File "/Users/bane/.local/lib/python3.7/site-packages/tornado/ioloop.py", line 743 in _run_callback
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/asyncio/events.py", line 88 in _run
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/asyncio/base_events.py", line 1775 in _run_once
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/asyncio/base_events.py", line 539 in run_forever
  File "/Users/bane/.local/lib/python3.7/site-packages/tornado/platform/asyncio.py", line 148 in start
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel/kernelapp.py", line 497 in start
  File "/Users/bane/.local/lib/python3.7/site-packages/traitlets/config/application.py", line 658 in launch_instance
  File "/Users/bane/.local/lib/python3.7/site-packages/ipykernel_launcher.py", line 16 in <module>
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/runpy.py", line 85 in _run_code
  File "/Users/bane/anaconda3/envs/dev/lib/python3.7/runpy.py", line 193 in _run_module_as_main

So it appears that the callback of the picker is triggering a modified event on the renderer which then triggers the update event on the BackgroundPlotter to occur since we add those in with auto_update.

[akaszynski]

Core API is failing unit tests. Could you review and potentially merge #611 and then merge with master? Will make unit testing easier.

[banesullivan]

Okay, a hacky conditional decorator was added to thread the render call on Mac and not on Windows/Linux.

I've tested this on my Mac and on my Linux VM. I'm going to see if my Windows VM still works and see if all is good there.

[banesullivan]

Okay, nevermind. this still has some serious issues.

[GuillaumeFavelier]

The whole point of #293

[akaszynski]

I left in:

self.app_window.signal_close.connect(lambda: QtInteractor.close(self))

And menu exits work fine. However, pressing "q" still causes segfaults more often than not. This is despite the fact that both Exit and "q" call the exact same function. Even when changing the key event to:

self.add_key_event("q", lambda: self.app_window.close())

There are still issues. This is the same as the Exit callback of:

file_menu.addAction('Exit', self.app_window.close)

The only thing I can think is that there's something funky going on with how vtk handles the key events and that it doesn't play well with pyqt.

To fix this, I added in @threading to close in 77901b1

    @threaded
    def close(self):
        """Close the plotter.

        This function closes the window which in turn will
        close the plotter through `signal_close`.

        """
        self.app_window.close()

With this, I can't get it to segfault/crash/freeze while closing with "q" or the menu or by hand.

I think vtk calls need to be threaded so they don't interfere with the main pyqt thread. Let's see if Linux on Azure likes it.

[akaszynski]

The Azure gods seem to be pleased. @banesullivan, @GuillaumeFavelier, since this seems sensitive to the environment, could you please pull and test?

[GuillaumeFavelier]

All good on my end and thanks for the @threaded on close() too.

[GuillaumeFavelier]

I think vtk calls need to be threaded so they don't interfere with the main pyqt thread.

This is good to know especially for pyvista-gui

[akaszynski]

Actually, in my closed source gui, just about all calls to vtk are threaded. This lets me update the plotter in the background, doesn't make the gui freeze when called, and ensures errors (if not caught, and there's a wrapper for that too), will just kill the child thread and not the main one with a ugly segfault.

[akaszynski]

Thanks all for the great team effort. This was not an easy PR.

[banesullivan]

So uh... the q-key isn't working for me. It freezes the window and then blocks the process

All else works excellently though

[GuillaumeFavelier]

It probably does the opposite this time: closes the interactor but not the window. That's why there was 2 versions of close before

[GuillaumeFavelier]

Reference to: #388

[GuillaumeFavelier]

I think the q-key should be connected to app_window.close

[akaszynski]

I think the q-key should be connected to app_window.close

self.close calls app_window.close:

pyvista/pyvista/plotting/qt_plotting.py

Lines 739 to 760 in 71dc915

	def reset_key_events(self):
	"""Reset all of the key press events to their defaults.
	Handles closing configuration for q-key.
	"""
	super(BackgroundPlotter, self).reset_key_events()
	if self.allow_quit_keypress:
	self.add_key_event("q", lambda: self.close())
	def scale_axes_dialog(self, show=True):
	"""Open scale axes dialog."""
	return ScaleAxesDialog(self.app_window, self, show=show)
	@threaded
	def close(self):
	"""Close the plotter.
	This function closes the window which in turn will
	close the plotter through `signal_close`.
	"""
	self.app_window.close()

[akaszynski]

I can confirm that calling the action stored in the file menu from the vtk event handler leads to the same segfault. I don't think we can safely have vtk call PyQt events.

[akaszynski]

@banesullivan, please try it now. Seems that removing closeEvent in c996bac let me close without using a thread.

[banesullivan]

All good now! I'm going to do one final peruse through the diff and then merge!

[GuillaumeFavelier]

I'm not very enthusiastic with the current situation:

Now the MainWindow is not needed at all. I designed it originally so that BackgroundPlotter could catch signal_close() and close properly. But now the signal_close is never emitted (created, connected but never emitted):

pyvista/pyvista/plotting/qt_plotting.py

Lines 859 to 867 in c996bac

	class MainWindow(QMainWindow):
	"""Convenience MainWindow that manages the application."""
	signal_close = pyqtSignal()
	def __init__(self, parent=None):
	"""Initialize the main window."""
	super(MainWindow, self).__init__(parent)
	self.signal_close.connect(self.close)

I can only advice that we test thoroughly all the events that close QtInteractor and BackgroundPlotter and I hope that we will find something working for everyone. I note 4 interactions:

• q-key
• Menu > Exit
• X window button
• programmatically with close()

With the current state of the code, I get the exact same segfault I had in #603 (comment) with ipython.

I would be okay to disable temporarily the q shortcut if that means no crash on ipython

[GuillaumeFavelier]

I can reproduce this segfault consistently with any version of PyQt5 after 5.10.1:

5.11.2, 5.11.3, 5.12, 5.12.1, 5.12.2, 5.12.3, 5.13.0, 5.13.1, 5.13.2, 5.14.0, 5.14.1

I can mitigate by using 5.10.1 for now

  Date: Fri Feb 14 13:14:01 2020 CET

             Linux : OS
                 4 : CPU(s)
            x86_64 : Machine
             64bit : Architecture
            7.7 GB : RAM
           IPython : Environment
        GPU Vendor : NVIDIA Corporation
      GPU Renderer : GeForce GTX 960M/PCIe/SSE2
       GPU Version : 4.5.0 NVIDIA 430.64

  Python 3.7.6 (default, Jan  8 2020, 19:59:22)  [GCC 7.3.0]

            0.23.2 : pyvista
             8.1.2 : vtk
            1.18.1 : numpy
             2.6.1 : imageio
             1.4.3 : appdirs
             0.5.1 : scooby
             3.1.3 : matplotlib
            5.10.1 : PyQt5
            7.12.0 : IPython
             1.0.0 : colorcet
             1.4.1 : scipy
--------------------------------------------------------------------------------

[akaszynski]

I'm not very enthusiastic with the current situation:

Me neither. I found that closeEvent in MainWindow was causing the app to crash in a variety of situations on my end when pressing q. Removing closeEvent fixed this issue for me, even on 5.11.3.

@GuillaumeFavelier you mind creating a new branch that addresses closeEvent? For the time being we should also consider setting allow_quit_keypress to False.

[GuillaumeFavelier]

you mind creating a new branch that addresses closeEvent?

I'll do my best!

My first priority right now would be to drastically improve the testing of pyvista so that those issues are detected sooner in the development cycle to avoid regression. I'll try to push in this direction at least.