Fix selector garbage collection (#319)

kushalkolar · web-flow · commit f4bab4a3f039 · 2023-10-26T14:19:37.000-04:00
* fix longstanding issue where auto_scale didn't work with zero width or height

* BaseSelector inherits from Graphic, progress on gc, not yet full there

* clear() for Synchronizer
diff --git a/fastplotlib/graphics/_base.py b/fastplotlib/graphics/_base.py
@@ -158,6 +158,15 @@ def __eq__(self, other):
 
         return False
 
+    def _cleanup(self):
+        """
+        Cleans up the graphic in preparation for __del__(), such as removing event handlers from
+        plot renderer, feature event handlers, etc.
+
+        Optionally implemented in subclasses
+        """
+        pass
+
     def __del__(self):
         del WORLD_OBJECTS[self.loc]
 
diff --git a/fastplotlib/graphics/image.py b/fastplotlib/graphics/image.py
@@ -149,7 +149,7 @@ def _get_linear_selector_init_args(self, padding: float, **kwargs):
         if axis == "x":
             offset = self.position_x
             # x limits, number of columns
-            limits = (offset, data.shape[1])
+            limits = (offset, data.shape[1] - 1)
 
             # size is number of rows + padding
             # used by LinearRegionSelector but not LinearSelector
@@ -169,7 +169,7 @@ def _get_linear_selector_init_args(self, padding: float, **kwargs):
         else:
             offset = self.position_y
             # y limits
-            limits = (offset, data.shape[0])
+            limits = (offset, data.shape[0] - 1)
 
             # width + padding
             # used by LinearRegionSelector but not LinearSelector
diff --git a/fastplotlib/graphics/selectors/_base_selector.py b/fastplotlib/graphics/selectors/_base_selector.py
@@ -6,6 +6,8 @@
 
 from pygfx import WorldObject, Line, Mesh, Points
 
+from .._base import Graphic
+
 
 @dataclass
 class MoveInfo:
@@ -31,7 +33,7 @@ class MoveInfo:
 
 
 # Selector base class
-class BaseSelector:
+class BaseSelector(Graphic):
     feature_events = ("selection",)
 
     def __init__(
@@ -42,6 +44,7 @@ def __init__(
         hover_responsive: Tuple[WorldObject, ...] = None,
         arrow_keys_modifier: str = None,
         axis: str = None,
+        name: str = None
     ):
         if edges is None:
             edges = tuple()
@@ -89,6 +92,8 @@ def __init__(
 
         self._pygfx_event = None
 
+        Graphic.__init__(self, name=name)
+
     def get_selected_index(self):
         """Not implemented for this selector"""
         raise NotImplementedError
@@ -341,12 +346,10 @@ def _key_up(self, ev):
 
         self._move_info = None
 
-    def __del__(self):
-        # clear wo event handlers
-        for wo in self._world_objects:
-            wo._event_handlers.clear()
-
-        # remove renderer event handlers
+    def _cleanup(self):
+        """
+        Cleanup plot renderer event handlers etc.
+        """
         self._plot_area.renderer.remove_event_handler(self._move, "pointer_move")
         self._plot_area.renderer.remove_event_handler(self._move_end, "pointer_up")
         self._plot_area.renderer.remove_event_handler(self._move_to_pointer, "click")
@@ -357,6 +360,10 @@ def __del__(self):
         # remove animation func
         self._plot_area.remove_animation(self._key_hold)
 
+        # clear wo event handlers
+        for wo in self._world_objects:
+            wo._event_handlers.clear()
+
         if hasattr(self, "feature_events"):
             feature_names = getattr(self, "feature_events")
             for n in feature_names:
diff --git a/fastplotlib/graphics/selectors/_linear.py b/fastplotlib/graphics/selectors/_linear.py
@@ -18,7 +18,7 @@
 from ._base_selector import BaseSelector
 
 
-class LinearSelector(Graphic, BaseSelector):
+class LinearSelector(BaseSelector):
     @property
     def limits(self) -> Tuple[float, float]:
         return self._limits
@@ -117,9 +117,6 @@ def __init__(
 
         line_data = line_data.astype(np.float32)
 
-        # init Graphic
-        Graphic.__init__(self, name=name)
-
         if thickness < 1.1:
             material = pygfx.LineThinMaterial
         else:
@@ -172,6 +169,7 @@ def __init__(
             hover_responsive=(line_inner, self.line_outer),
             arrow_keys_modifier=arrow_keys_modifier,
             axis=axis,
+            name=name,
         )
 
     def _setup_ipywidget_slider(self, widget):
@@ -189,8 +187,6 @@ def _setup_ipywidget_slider(self, widget):
         # user changes linear selection -> widget changes
         self.selection.add_event_handler(self._update_ipywidgets)
 
-        self._plot_area.renderer.add_event_handler(self._set_slider_layout, "resize")
-
         self._handled_widgets.append(widget)
 
     def _update_ipywidgets(self, ev):
@@ -214,6 +210,12 @@ def _ipywidget_callback(self, change):
 
         self.selection = change["new"]
 
+    def _add_plot_area_hook(self, plot_area):
+        super()._add_plot_area_hook(plot_area=plot_area)
+
+        # resize the slider widgets when the canvas is resized
+        self._plot_area.renderer.add_event_handler(self._set_slider_layout, "resize")
+
     def _set_slider_layout(self, *args):
         w, h = self._plot_area.renderer.logical_size
 
@@ -375,3 +377,11 @@ def _move_graphic(self, delta: np.ndarray):
             self.selection = self.selection() + delta[0]
         else:
             self.selection = self.selection() + delta[1]
+
+    def _cleanup(self):
+        super()._cleanup()
+
+        for widget in self._handled_widgets:
+            widget.unobserve(self._ipywidget_callback, "value")
+
+        self._plot_area.renderer.remove_event_handler(self._set_slider_layout, "resize")
diff --git a/fastplotlib/graphics/selectors/_linear_region.py b/fastplotlib/graphics/selectors/_linear_region.py
@@ -17,7 +17,7 @@
 from .._features._selection_features import LinearRegionSelectionFeature
 
 
-class LinearRegionSelector(Graphic, BaseSelector):
+class LinearRegionSelector(BaseSelector):
     @property
     def limits(self) -> Tuple[float, float]:
         return self._limits
@@ -127,8 +127,6 @@ def __init__(
         #             f"{limits[0]} != {origin[1]} != {bounds[0]}"
         #         )
 
-        Graphic.__init__(self, name=name)
-
         self.parent = parent
 
         # world object for this will be a group
@@ -241,6 +239,7 @@ def __init__(
             hover_responsive=self.edges,
             arrow_keys_modifier=arrow_keys_modifier,
             axis=axis,
+            name=name
         )
 
     def get_selected_data(
diff --git a/fastplotlib/graphics/selectors/_polygon.py b/fastplotlib/graphics/selectors/_polygon.py
@@ -8,15 +8,14 @@
 from .._base import Graphic
 
 
-class PolygonSelector(Graphic, BaseSelector):
+class PolygonSelector(BaseSelector):
     def __init__(
         self,
         edge_color="magenta",
         edge_width: float = 3,
         parent: Graphic = None,
         name: str = None,
     ):
-        Graphic.__init__(self, name=name)
 
         self.parent = parent
 
@@ -31,6 +30,8 @@ def __init__(
 
         self._current_mode = None
 
+        BaseSelector.__init__(self, name=name)
+
     def get_vertices(self) -> np.ndarray:
         """Get the vertices for the polygon"""
         vertices = list()
diff --git a/fastplotlib/graphics/selectors/_sync.py b/fastplotlib/graphics/selectors/_sync.py
@@ -39,8 +39,12 @@ def add(self, selector):
 
     def remove(self, selector):
         """remove a selector"""
-        self._selectors.remove(selector)
         selector.selection.remove_event_handler(self._handle_event)
+        self._selectors.remove(selector)
+
+    def clear(self):
+        for i in range(len(self.selectors)):
+            self.remove(self.selectors[0])
 
     def _handle_event(self, ev):
         if self.block_event:
@@ -81,7 +85,4 @@ def _move_selectors(self, source, delta):
             s._move_graphic(delta)
 
     def __del__(self):
-        for s in self.selectors:
-            self.remove(s)
-
-        self.selectors.clear()
+        self.clear()
diff --git a/fastplotlib/layouts/_base.py b/fastplotlib/layouts/_base.py
@@ -329,12 +329,16 @@ def _add_or_insert_graphic(
             else:
                 self._graphics.append(loc)
 
+        # now that it's in the dict, just use the weakref
+        graphic = weakref.proxy(graphic)
+
         # add world object to scene
         self.scene.add(graphic.world_object)
 
         if center:
             self.center_graphic(graphic)
 
+        # if we don't use the weakref above, then the object lingers if a plot hook is used!
         if hasattr(graphic, "_add_plot_area_hook"):
             graphic._add_plot_area_hook(self)
 
@@ -483,6 +487,9 @@ def delete_graphic(self, graphic: Graphic):
         # remove from list of addresses
         glist.remove(loc)
 
+        # cleanup
+        graphic._cleanup()
+
         if kind == "graphic":
             del GRAPHICS[loc]
         elif kind == "selector":
