Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Plotter and children for proper garbage collection #3037

Merged
merged 6 commits into from
Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions pyvista/plotting/charts.py
Original file line number Diff line number Diff line change
Expand Up @@ -4370,7 +4370,16 @@ def __init__(self, renderer):
# needed.
self._scene = None
self._actor = None
self._renderer = renderer

# a weakref.proxy would be nice here, but that doesn't play
# nicely with SetRenderer, so instead we'll use a weak reference
# plus a property to call it
self.__renderer = weakref.ref(renderer)

@property
def _renderer(self):
"""Return the weakly dereferenced renderer, maybe None."""
return self.__renderer()

def _setup_scene(self):
"""Set up a new context scene and actor for these charts."""
Expand Down Expand Up @@ -4459,6 +4468,6 @@ def __iter__(self):
for chart in self._charts:
yield chart

def __del__(self): # pragma: no cover
def __del__(self):
Comment on lines -4462 to +4471
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how this all started. I naively added a __del__ when fixing #2991, only to realise that this is never called. Had to skip this for coverage then, since you can't call __del__ on an object that refuses to die.

"""Clean up before being destroyed."""
self.deep_clean()
3 changes: 2 additions & 1 deletion pyvista/plotting/renderer.py
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ def __init__(self, parent, border=True, border_color='w', border_width=2.0):
"""Initialize the renderer."""
super().__init__()
self._actors = {}
self.parent = parent # the plotter
self.parent = parent # weakref.proxy to the plotter from Renderers
self._theme = parent.theme
self.camera_set = False
self.bounding_box_actor = None
Expand Down Expand Up @@ -2760,6 +2760,7 @@ def deep_clean(self, render=False):
self.disable_shadows()
if self.__charts is not None:
self.__charts.deep_clean()
self.__charts = None

self.remove_floors(render=render)
self.remove_legend(render=render)
Expand Down
3 changes: 2 additions & 1 deletion pyvista/plotting/renderers.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Organize Renderers for ``pyvista.Plotter``."""
import collections
from weakref import proxy

import numpy as np

Expand All @@ -26,7 +27,7 @@ def __init__(
):
"""Initialize renderers."""
self._active_index = 0 # index of the active renderer
self._plotter = plotter
self._plotter = proxy(plotter)
self._renderers = []

# by default add border for multiple plots
Expand Down
21 changes: 21 additions & 0 deletions tests/plotting/test_collection.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""This module contains any tests which cause memory leaks."""
import gc
import weakref

import numpy as np
Expand Down Expand Up @@ -40,3 +41,23 @@ def test_add_array(sphere):
"""Ensure data added dynamically to a plotter is collected."""
pl = pv.Plotter()
pl.add_mesh(sphere, scalars=range(sphere.n_points))


def test_plotting_collection():
"""Ensure that we don't leak Plotter, Renderer and Charts instances."""
pl = pv.Plotter()
ref_plotter = weakref.ref(pl)
ref_renderers = weakref.ref(pl.renderers)
ref_renderer = weakref.ref(pl.renderer)
ref_charts = weakref.ref(pl.renderer._charts) # instantiated on the fly

# delete known references to Plotter
del pv.plotting._ALL_PLOTTERS[pl._id_name]
del pl

# check that everything is eventually destroyed
gc.collect() # small reference cycles allowed
assert ref_plotter() is None, gc.get_referrers(ref_plotter())
assert ref_renderers() is None
assert ref_renderer() is None
assert ref_charts() is None