Fix shapes mouse click and double click behavior on partial shapes #6912
Conversation
dalthviz
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6912 +/- ##
==========================================
- Coverage 92.48% 92.43% -0.05%
==========================================
Files 612 612
Lines 55145 55169 +24
==========================================
- Hits 51001 50997 -4
- Misses 4144 4172 +28 ☔ View full report in Codecov by Sentry. |
| position_diff = np.linalg.norm(event.pos - layer._last_cursor_position) | ||
| if position_diff: | ||
| # Ensure the new position is actually a new position |
There was a problem hiding this comment.
any reason to use norm instead of just event.pos == layer._last_cursor_position? Latter seems more readable to me.
Also, using position would be more "correct" than pos since it's the world position that shoudln't change, not the canvas pos... In principle we should allow the same canvas pos if it results in a different world pos (i.e: zoom/pan between clicks).
There was a problem hiding this comment.
Used norm since is used for the polygon lasso tool at
napari/napari/layers/shapes/_shapes_mouse_bindings.py
Lines 411 to 421 in 26976b5
I was not sure if a straight forward comparison could be enough but if that is the case then makes sense to change it indeed 👍 Let me know if I should change it!
The logic uses the pos instead of position since, from what I checked, seems like that's the value being stored as layer._last_cursor_position. For example at
napari/napari/layers/shapes/_shapes_mouse_bindings.py
Lines 339 to 340 in 26976b5
Should I try to change in this PR the usage of pos for position for the layer._last_cursor_position logic?
There was a problem hiding this comment.
The norm makes sense in that place because there's a threshold; in this case, you're checking only if the vertex is identical, so I would use == (probably faster as well, though I doubt it matters :P).
Ah, I see; I thought the layer._last_cursor_position was a new thing you created here. I wouldn't change the rest then.
However, this makes me wonder if there's any reason for this value to be stored on the layer and shared between different mouse callbacks? Or worse, can it be a problem if they fight over this value? Maybe there's no need to address in this PR, but we definitely need to make sure we don't end up messing up something else by touching this value.
There was a problem hiding this comment.
However, this makes me wonder if there's any reason for this value to be stored on the layer and shared between different mouse callbacks? Or worse, can it be a problem if they fight over this value? Maybe there's no need to address in this PR, but we definitely need to make sure we don't end up messing up something else by touching this value.
Thinking about this, I would said that the value is stored over the layer so it can be retrieved later to check the difference (in the case of the polygon lasso tool) and now the equivalence (for the polygon tool). Checking, I think that the value layer._last_cursor_position is not being set to None when finishing the drawing, so maybe it could be worthy to add such a "reset" (layer._last_cursor_position = None) as a way to mitigate possible interference between usages when drawing a shape finishes?
There was a problem hiding this comment.
yeah, good idea! Ideally this would be tracke dby the mouse callback itself... but I guess it's not that easy ^^'
| @@ -2595,12 +2595,14 @@ def _finish_drawing(self, event=None) -> None: | |||
| vertices = self._data_view.shapes[index].data | |||
| if len(vertices) <= 2: | |||
| self._data_view.remove(index) | |||
| self.selected_data.clear() | |||
There was a problem hiding this comment.
This is an independend change right?
There was a problem hiding this comment.
Yep, is another thing related but independent indeed. While checking the fix for the initial single click issue (trying to add a vertex over the same position of an already existing vertex), I noticed that doing a double click to finish the polygons/shape drawing for an invalid set of vertices, sometimes causes a index 0 is out of bounds for axis 0 with size 0:
Seems like the issue in that case was caused due to the invalid shape being worked on is at the end removed while still a selection is defined. To prevent that I added the selected_data.clear() call here. Just in case, the traceback:
---------------------------------------------------------------------------
IndexError Traceback (most recent call last)
File ~\anaconda3\envs\napari-dev\lib\site-packages\vispy\app\backends\_qt.py:516, in QtBaseCanvasBackend.mouseDoubleClickEvent(self=<vispy.app.backends._qt.CanvasBackendDesktop object>, ev=<PyQt5.QtGui.QMouseEvent object>)
514 if self._vispy_canvas is None:
515 return
--> 516 self._vispy_mouse_double_click(
self = <vispy.app.backends._qt.CanvasBackendDesktop object at 0x000001ED94DC8EE0>
ev = <PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990>
BUTTONMAP = {0: 0, 1: 1, 2: 2, 4: 3, 8: 4, 16: 5}
517 native=ev,
518 pos=_get_event_xy(ev),
519 button=BUTTONMAP.get(ev.button(), 0),
520 modifiers=self._modifiers(ev),
521 )
File ~\anaconda3\envs\napari-dev\lib\site-packages\vispy\app\base.py:239, in BaseCanvasBackend._vispy_mouse_double_click(self=<vispy.app.backends._qt.CanvasBackendDesktop object>, **kwargs={'button': 1, 'buttons': [], 'last_event': <MouseEvent blocked=False button=1 buttons=[1] d...es=[] time=1715788279.9325497 type=mouse_release>, 'last_mouse_press': None, 'modifiers': (), 'native': <PyQt5.QtGui.QMouseEvent object>, 'pos': (261, 299), 'press_event': None})
235 def _vispy_mouse_double_click(self, **kwargs):
236 # default method for delivering double-click events to the canvas
237 kwargs.update(self._vispy_mouse_data)
--> 239 ev = self._vispy_canvas.events.mouse_double_click(**kwargs)
self._vispy_canvas.events.mouse_double_click = <vispy.util.event.EventEmitter object at 0x000001ED95413F70>
kwargs = {'native': <PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990>, 'pos': (261, 299), 'button': 1, 'modifiers': (), 'buttons': [], 'press_event': None, 'last_event': <MouseEvent blocked=False button=1 buttons=[1] delta=[0. 0.] handled=False is_dragging=True last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=MouseEvent source=None sources=[] time=1715788279.9325497 type=mouse_release>, 'last_mouse_press': None}
self = <vispy.app.backends._qt.CanvasBackendDesktop object at 0x000001ED94DC8EE0>
self._vispy_canvas.events = <vispy.util.event.EmitterGroup object at 0x000001ED95413E50>
self._vispy_canvas = <NapariSceneCanvas (PyQt5) at 0x1ed95411cf0>
240 self._vispy_mouse_data['last_event'] = ev
241 return ev
File ~\anaconda3\envs\napari-dev\lib\site-packages\vispy\util\event.py:453, in EventEmitter.__call__(self=<vispy.util.event.EventEmitter object>, *args=(), **kwargs={'button': 1, 'buttons': [], 'last_event': <MouseEvent blocked=False button=1 buttons=[1] d...es=[] time=1715788279.9325497 type=mouse_release>, 'last_mouse_press': None, 'modifiers': (), 'native': <PyQt5.QtGui.QMouseEvent object>, 'pos': (261, 299), 'press_event': None})
450 if self._emitting > 1:
451 raise RuntimeError('EventEmitter loop detected!')
--> 453 self._invoke_callback(cb, event)
event = <MouseEvent blocked=False button=1 buttons=[] delta=[0. 0.] handled=False is_dragging=False last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=None source=None sources=[] time=1715788279.9958236 type=mouse_double_click>
self = <vispy.util.event.EventEmitter object at 0x000001ED95413F70>
cb = <bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object at 0x000001ED95411CC0>>
454 if event.blocked:
455 break
File ~\anaconda3\envs\napari-dev\lib\site-packages\vispy\util\event.py:471, in EventEmitter._invoke_callback(self=<vispy.util.event.EventEmitter object>, cb=<bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object>>, event=<MouseEvent blocked=False button=1 buttons=[] de... time=1715788279.9958236 type=mouse_double_click>)
469 cb(event)
470 except Exception:
--> 471 _handle_exception(self.ignore_callback_errors,
self = <vispy.util.event.EventEmitter object at 0x000001ED95413F70>
cb = <bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object at 0x000001ED95411CC0>>
event = <MouseEvent blocked=False button=1 buttons=[] delta=[0. 0.] handled=False is_dragging=False last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=None source=None sources=[] time=1715788279.9958236 type=mouse_double_click>
(cb, event) = (<bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object at 0x000001ED95411CC0>>, <MouseEvent blocked=False button=1 buttons=[] delta=[0. 0.] handled=False is_dragging=False last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=None source=None sources=[] time=1715788279.9958236 type=mouse_double_click>)
472 self.print_callback_errors,
473 self, cb_event=(cb, event))
File ~\anaconda3\envs\napari-dev\lib\site-packages\vispy\util\event.py:469, in EventEmitter._invoke_callback(self=<vispy.util.event.EventEmitter object>, cb=<bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object>>, event=<MouseEvent blocked=False button=1 buttons=[] de... time=1715788279.9958236 type=mouse_double_click>)
467 def _invoke_callback(self, cb, event):
468 try:
--> 469 cb(event)
cb = <bound method VispyCanvas._on_mouse_double_click of <napari._vispy.canvas.VispyCanvas object at 0x000001ED95411CC0>>
event = <MouseEvent blocked=False button=1 buttons=[] delta=[0. 0.] handled=False is_dragging=False last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=None source=None sources=[] time=1715788279.9958236 type=mouse_double_click>
470 except Exception:
471 _handle_exception(self.ignore_callback_errors,
472 self.print_callback_errors,
473 self, cb_event=(cb, event))
File E:\Acer\Documentos\Quansight\Napari\napari\napari\_vispy\canvas.py:433, in VispyCanvas._on_mouse_double_click(self=<napari._vispy.canvas.VispyCanvas object>, event=<MouseEvent blocked=False button=1 buttons=[] de... time=1715788279.9958236 type=mouse_double_click>)
410 def _on_mouse_double_click(self, event: MouseEvent) -> None:
411 """Called whenever a mouse double-click happen on the canvas
412
413 Parameters
(...)
431 - mouse_release
432 """
--> 433 self._process_mouse_event(mouse_double_click_callbacks, event)
event = <MouseEvent blocked=False button=1 buttons=[] delta=[0. 0.] handled=False is_dragging=False last_event=MouseEvent modifiers=() native=<PyQt5.QtGui.QMouseEvent object at 0x000001EDA34F9990> pos=[261 299] press_event=None source=None sources=[] time=1715788279.9958236 type=mouse_double_click>
self = <napari._vispy.canvas.VispyCanvas object at 0x000001ED95411CC0>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\_vispy\canvas.py:408, in VispyCanvas._process_mouse_event(self=<napari._vispy.canvas.VispyCanvas object>, mouse_callbacks=<function mouse_double_click_callbacks>, event=<ReadOnlyWrapper at 0x000001EDA6B72240 for MouseEvent>)
406 layer = self.viewer.layers.selection.active
407 if layer is not None:
--> 408 mouse_callbacks(layer, event)
event = <ReadOnlyWrapper at 0x000001EDA6B72240 for MouseEvent at 0x000001EDA6B23A00>
layer = <Shapes layer 'Shapes' at 0x1eda652cc40>
mouse_callbacks = <function mouse_double_click_callbacks at 0x000001ED902EFC70>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\utils\interactions.py:91, in mouse_double_click_callbacks(obj=<Shapes layer 'Shapes'>, event=<ReadOnlyWrapper at 0x000001EDA6B72240 for MouseEvent>)
85 if inspect.isgeneratorfunction(mouse_click_func):
86 raise ValueError(
87 trans._(
88 "Double-click actions can't be generators.", deferred=True
89 )
90 )
---> 91 mouse_click_func(obj, event)
mouse_click_func = <function finish_drawing_shape at 0x000001ED924F1AB0>
obj = <Shapes layer 'Shapes' at 0x1eda652cc40>
event = <ReadOnlyWrapper at 0x000001EDA6B72240 for MouseEvent at 0x000001EDA6B23A00>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\_shapes_mouse_bindings.py:294, in finish_drawing_shape(layer=<Shapes layer 'Shapes'>, event=<ReadOnlyWrapper at 0x000001EDA6B72240 for MouseEvent>)
280 def finish_drawing_shape(layer: Shapes, event: MouseEvent) -> None:
281 """Finish drawing of shape.
282
283 Calls the finish drawing method of the shapes layer which resets all the properties used for shape drawing
(...)
292 double click callback of the shapes layer.
293 """
--> 294 layer._finish_drawing()
layer = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\shapes.py:2625, in Shapes._finish_drawing(self=<Shapes layer 'Shapes'>, event=None)
2618 self.events.data(
2619 value=self.data,
2620 action=ActionType.ADDED,
2621 data_indices=(-1,),
2622 vertex_indices=((),),
2623 )
2624 self._is_creating = False
-> 2625 self._update_dims()
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\base\base.py:841, in Layer._update_dims(self=<Shapes layer 'Shapes'>)
837 self._slice_input = self._slice_input.with_ndim(ndim)
839 self._ndim = ndim
--> 841 self._clear_extents_and_refresh()
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\base\base.py:968, in Layer._clear_extents_and_refresh(self=<Shapes layer 'Shapes'>)
966 self._clear_extent()
967 self._clear_extent_augmented()
--> 968 self.refresh()
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\base\base.py:1448, in Layer.refresh(self=<Shapes layer 'Shapes'>, event=None)
1445 self.events.reload(layer=self)
1446 # Otherwise, slice immediately on the calling thread.
1447 else:
-> 1448 self._refresh_sync()
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\base\base.py:1456, in Layer._refresh_sync(self=<Shapes layer 'Shapes'>, event=None)
1454 self.events.set_data()
1455 self._update_thumbnail()
-> 1456 self._set_highlight(force=True)
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\shapes.py:2581, in Shapes._set_highlight(self=<Shapes layer 'Shapes'>, force=True)
2579 self._value_stored = copy(self._value)
2580 self._drag_box_stored = copy(self._drag_box)
-> 2581 self.events.highlight()
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
self.events.highlight = <napari.utils.events.event.EventEmitter object at 0x000001EDA6A00070>
self.events = <napari.utils.events.event.EmitterGroup object at 0x000001EDA261D6C0>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\utils\events\event.py:764, in EventEmitter.__call__(self=<napari.utils.events.event.EventEmitter object>, *args=(), **kwargs={})
761 self._block_counter.update([cb])
762 continue
--> 764 self._invoke_callback(cb, event if pass_event else None)
event = <Event blocked=False handled=False native=None source=None sources=[] type='highlight'>
self = <napari.utils.events.event.EventEmitter object at 0x000001EDA6A00070>
cb = <bound method VispyShapesLayer._on_highlight_change of <napari._vispy.layers.shapes.VispyShapesLayer object at 0x000001EDA6B23310>>
pass_event = False
765 if event.blocked:
766 break
File E:\Acer\Documentos\Quansight\Napari\napari\napari\utils\events\event.py:802, in EventEmitter._invoke_callback(self=<napari.utils.events.event.EventEmitter object>, cb=<bound method VispyShapesLayer._on_highlight_cha...ri._vispy.layers.shapes.VispyShapesLayer object>>, event=None)
800 self.disconnect(cb)
801 return
--> 802 _handle_exception(
self = <napari.utils.events.event.EventEmitter object at 0x000001EDA6A00070>
event = None
cb = <bound method VispyShapesLayer._on_highlight_change of <napari._vispy.layers.shapes.VispyShapesLayer object at 0x000001EDA6B23310>>
(cb, event) = (<bound method VispyShapesLayer._on_highlight_change of <napari._vispy.layers.shapes.VispyShapesLayer object at 0x000001EDA6B23310>>, None)
803 self.ignore_callback_errors,
804 self.print_callback_errors,
805 self,
806 cb_event=(cb, event),
807 )
File E:\Acer\Documentos\Quansight\Napari\napari\napari\utils\events\event.py:791, in EventEmitter._invoke_callback(self=<napari.utils.events.event.EventEmitter object>, cb=<bound method VispyShapesLayer._on_highlight_cha...ri._vispy.layers.shapes.VispyShapesLayer object>>, event=None)
789 cb(event)
790 else:
--> 791 cb()
cb = <bound method VispyShapesLayer._on_highlight_change of <napari._vispy.layers.shapes.VispyShapesLayer object at 0x000001EDA6B23310>>
792 except Exception as e: # noqa: BLE001
793 # dead Qt object with living python pointer. not importing Qt
794 # here... but this error is consistent across backends
795 if (
796 isinstance(e, RuntimeError)
797 and 'C++' in str(e)
798 and str(e).endswith(('has been deleted', 'already deleted.'))
799 ):
File E:\Acer\Documentos\Quansight\Napari\napari\napari\_vispy\layers\shapes.py:72, in VispyShapesLayer._on_highlight_change(self=<napari._vispy.layers.shapes.VispyShapesLayer object>)
67 self.layer._highlight_color = (
68 settings.appearance.highlight.highlight_color
69 )
71 # Compute the vertices and faces of any shape outlines
---> 72 vertices, faces = self.layer._outline_shapes()
self.layer = <Shapes layer 'Shapes' at 0x1eda652cc40>
self = <napari._vispy.layers.shapes.VispyShapesLayer object at 0x000001EDA6B23310>
74 if vertices is None or len(vertices) == 0 or len(faces) == 0:
75 vertices = np.zeros((3, self.layer._slice_input.ndisplay))
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\shapes.py:2460, in Shapes._outline_shapes(self=<Shapes layer 'Shapes'>)
2457 else:
2458 index = self._value[0]
-> 2460 centers, offsets, triangles = self._data_view.outline(index)
index = [2]
self = <Shapes layer 'Shapes' at 0x1eda652cc40>
self._data_view = <napari.layers.shapes._shape_list.ShapeList object at 0x000001EDA6A01B40>
2461 vertices = centers + (
2462 self._normalized_scale_factor * self._highlight_width * offsets
2463 )
2464 vertices = vertices[:, ::-1]
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\_shape_list.py:1042, in ShapeList.outline(self=<napari.layers.shapes._shape_list.ShapeList object>, indices=[2])
1040 if not isinstance(indices, Sequence):
1041 indices = [indices]
-> 1042 return self.outlines(indices)
indices = [2]
self = <napari.layers.shapes._shape_list.ShapeList object at 0x000001EDA6A01B40>
File E:\Acer\Documentos\Quansight\Napari\napari\napari\layers\shapes\_shape_list.py:1084, in ShapeList.outlines(self=<napari.layers.shapes._shape_list.ShapeList object>, indices=[2])
1082 for i, ind in enumerate(indices):
1083 inds = t_ind == ind
-> 1084 adjust_index = starts[i] - vertices_indices[starts[i]]
vertices_indices = []
starts = array([], dtype=int64)
i = 0
1085 triangles[inds] = triangles[inds] + adjust_index
1087 return centers, offsets, triangles
IndexError: index 0 is out of bounds for axis 0 with size 0There was a problem hiding this comment.
Is there any case in which we can have (or want to have) other shapes being selected while a shape is being added? If so, this will deselect everything, which might not be desired. On the other hand, probably this never happens?
There was a problem hiding this comment.
I think for the moment the selection always resets when a polygon is initialized at
Also, from a quick check via de GUI with latest main, seems like that is the case for all the possible shapes (when creating them any previous selection is discarded):
So, at least for the moment, clearing the selection when removing an invalid shape/vertices should not be a problem 👍
However, just in case, should I add some sort of comment in the code pointing to this PR/these comments for tracebility?
There was a problem hiding this comment.
comments never hurt, but yeah sounds fine by me then :)
|
Thanks @brisvag for all the feedback! |
psobolewskiPhD
added
bugfix
|
Thanks @dalthviz ! |
github-actions
bot
removed
the
ready to merge
…apari#6912) # References and relevant issues Closes napari#6597 # Description Enable proper handling of mouse bindings (single and double click) with unfinished/invalid shapes definitions (not enough vertices to close the shape or trying to add the same vertex multiple times. With the changes: * Ignore addition of a vertex (mouse single click) if it has the same position as the last vertex added (fix for napari#6597) * Trying to finish the drawing (mouse double click) of an invalid shape causes the in progress shape to be removed (fix for napari#6912 (comment)) Preview: 
References and relevant issues
Closes #6597
Description
Enable proper handling of mouse bindings (single and double click) with unfinished/invalid shapes definitions (not enough vertices to close the shape or trying to add the same vertex multiple times. With the changes:
Add polygonsmode for Shapes layer (TypeError: unsupported operand type(s) for +: 'NoneType' and 'int') #6597)Preview: