Re-reset key events in Plotter.__init__()

[adeak]

Preface

I've noticed a few peculiarities with the way my plotters (on a debian linux system) close:

1. when I press 'q', the plotter closes even if I use plotter.show(auto_close=False), with the warning telling me that I shouldn't have closed the window by clicking exit button (which I didn't)
2. after plotter.show() closes, I don't have a plotter.last_image screenshot, even though I should
3. and now the new public before_close_callback functionality is similarly broken on my system:

import pyvista as pv                                                     
                                                                         
plotter = pv.Plotter()                                                   
plotter.add_mesh(pv.Sphere())                                            
#plotter.show(before_close_callback=lambda pl: pl.screenshot('tmp.png')) 
plotter.show(auto_close=False)                                           
print(type(plotter.last_image))                                          

The above version kills two birds with one stone: when I run it the plot appears, and on pressing the 'q' key on my keyboard it prints (on main)

/home/adeak/pyvista/pyvista/plotting/plotting.py:6205: UserWarning: `auto_close` ignored: by clicking the exit button, you have destroyed the render window and we have to close it out.
  warnings.warn(
<class 'NoneType'>

If I uncomment the before_close_callback line, I get

Traceback (most recent call last):
  File "/home/adeak/pyvista/example.py", line 5, in <module>
    plotter.show(before_close_callback=lambda pl: pl.screenshot('tmp.png'))
  File "/home/adeak/pyvista/pyvista/plotting/plotting.py", line 6220, in show
    self.close()
  File "/home/adeak/pyvista/pyvista/plotting/plotting.py", line 4033, in close
    self._before_close_callback(self)
  File "/home/adeak/pyvista/example.py", line 5, in <lambda>
    plotter.show(before_close_callback=lambda pl: pl.screenshot('tmp.png'))
  File "/home/adeak/pyvista/pyvista/plotting/plotting.py", line 5083, in screenshot
    raise RuntimeError('This plotter is closed and unable to save a screenshot.')
RuntimeError: This plotter is closed and unable to save a screenshot.

Again this looks as if the plotter was closed prematurely.

---

With this patch the first case prints

/home/adeak/pyvista/pyvista/plotting/plotting.py:6205: UserWarning: `auto_close` ignored: by clicking the exit button, you have destroyed the render window and we have to close it out.
  warnings.warn(
<class 'numpy.ndarray'>

So for some reason the plotter still gets more killed than I intended, but now plotter.last_image works.

And the before_close_callback case runs successfully, silently creating the screenshot.

This PR

I've realised that what must be going on is that the 'q' key is bound to an event that kills my render window, and this happens in a platform-dependent way which is why others don't see this. We have this in BasePlotter:

pyvista/pyvista/plotting/plotting.py

Lines 1753 to 1768 in 3b97cab

	def reset_key_events(self):
	"""Reset all of the key press events to their defaults."""
	if hasattr(self, 'iren'):
	self.iren.clear_key_event_callbacks()
	self.add_key_event('q', self._prep_for_close) # Add no matter what
	b_left_down_callback = lambda: self.iren.add_observer(
	'LeftButtonPressEvent', self.left_button_down
	)
	self.add_key_event('b', b_left_down_callback)
	self.add_key_event('v', lambda: self.isometric_view_interactive())
	self.add_key_event('C', lambda: self.enable_cell_picking())
	self.add_key_event('Up', lambda: self.camera.Zoom(1.05))
	self.add_key_event('Down', lambda: self.camera.Zoom(0.95))
	self.add_key_event('plus', lambda: self.increment_point_size_and_line_width(1))
	self.add_key_event('minus', lambda: self.increment_point_size_and_line_width(-1))

And this gets called in BasePlotter.__init__():

pyvista/pyvista/plotting/plotting.py

Line 276 in 3b97cab

self.reset_key_events()

Now, the issue is that BasePlotter has no render window interactor, and when Plotter.__init__() calls super().__init__(), neither does it. Calling reset_key_events() after self.iren is created helps in the way explained above.

Open issue

I don't know why auto_close=False still doesn't work for me. The fact that I get the warning means that pressing the 'q' key still (even with this patch) brings me to

pyvista/pyvista/plotting/plotting.py

Lines 6192 to 6207 in 3b97cab

	# In the event that the user hits the exit-button on the GUI (on
	# Windows OS) then it must be finalized and deleted as accessing it
	# will kill the kernel.
	# Here we check for that and clean it up before moving on to any of
	# the closing routines that might try to still access that
	# render window.
	if not self.ren_win.IsCurrent():
	self._clear_ren_win() # The ren_win is deleted
	# proper screenshots cannot be saved if this happens
	if not auto_close:
	warnings.warn(
	"`auto_close` ignored: by clicking the exit button, "
	"you have destroyed the render window and we have to "
	"close it out."
	)
	auto_close = True

I'm wondering if this check might somehow be inadequate on my system (i.e. I have an alive render window despite not self.ren_win.IsCurrent()). If I temporarily remove this entire block of code from the library, then

plotter.show(auto_close=False)
# interact with the plotter, close it
plotter.show(screenshot='tmp.png')

works almost as expected: the screenshot is created in the position that the first call to show() ended up with. The only weirdness is that the second call to show() returns immediately, with no chance to further interact with the plot. But there's no error, no segfault, and the example script continues execution fine. Not sure what to make of this all.

[adeak]

Since all the other self.add_key_event() calls are no-ops in case self.iren doesn't exist, it seemed more readable to skip the whole method in case self.iren is missing. It's not likely that self will pick up the attribute mid-execution of the method.

[codecov]

Codecov Report

Merging #3622 (eec066b) into main (853715e) will decrease coverage by 0.03%.
The diff coverage is 64.00%.

@@            Coverage Diff             @@
##             main    #3622      +/-   ##
==========================================
- Coverage   94.07%   94.03%   -0.04%     
==========================================
  Files          83       83              
  Lines       18618    18630      +12     
==========================================
+ Hits        17515    17519       +4     
- Misses       1103     1111       +8     

[adeak]

If I'm right that reset_key_events() wasn't really running, this might be a breaking change if there are any built-in key events that we'll be clearing and inadvertently not restoring.

[banesullivan]

Oof. yeah, I'm concerned this will clear out existing key events from VTK which most users have come to expect. Perhaps reset_key_events() needs to re-add those as well. I'll pull these changes soon and see which key-events are missing/needed

[adeak]

Thanks, @banesullivan!

[adeak]

Oh, and at least some VTK key events are impervious to this method, see #3647 (comment).

[banesullivan]

I added a detailed comment in #3647 about how resetting PyVista's key events have no effect on the VTK key events (though they may still conflict)

[banesullivan]

Personally, I'd like for this line to be right after the self.iren.add_observer("KeyPressEvent", self.key_press_event) statement a few lines later but this should be fine.

[adeak]

Wouldn't putting that line right ater the add_observer() call clear the newly added event as well?

[banesullivan]

I don't think so. The reset_key_events() method is PyVista-specific and registers callbacks with the render window interactor's _key_press_event_callbacks. Observers are not modified by reset_key_events(). These callbacks are then handled, at event-time, by key_press_event() so we can register self.iren.add_observer("KeyPressEvent", self.key_press_event) before or after reset_key_events()