From 076f3dda2036c5833dec35ff4479d3ae1c7f7f55 Mon Sep 17 00:00:00 2001 From: Liezl Maree <38435167+roomrys@users.noreply.github.com> Date: Wed, 7 Aug 2024 10:40:29 -0700 Subject: [PATCH] Do not apply offset when double clicking a `PredictedInstance` (#1888) * Add offset argument to newInstance and AddInstance * Apply offset of 10 for Add Instance menu button (Ctrl + I) * Add offset for docks Add Instance button * Make the QtVideoPlayer context menu unit-testable * Add test for creating a new instance * Add test for "New Instance" button in `InstancesDock` * Fix typo in docstring * Add docstrings and typehinting * Remove unused imports and sort imports --- sleap/gui/app.py | 6 +- sleap/gui/commands.py | 41 +++++++------ sleap/gui/widgets/docks.py | 2 +- sleap/gui/widgets/video.py | 72 ++++++++++++---------- tests/gui/test_commands.py | 103 ++++++++++++++++++++++++++++++++ tests/gui/widgets/test_docks.py | 36 +++++++++-- 6 files changed, 206 insertions(+), 54 deletions(-) diff --git a/sleap/gui/app.py b/sleap/gui/app.py index e872ce9a6..e2396948a 100644 --- a/sleap/gui/app.py +++ b/sleap/gui/app.py @@ -704,13 +704,17 @@ def prev_vid(): ) def new_instance_menu_action(): + """Determine which action to use when using Ctrl + I or menu Add Instance. + + We always add an offset of 10. + """ method_key = [ key for (key, val) in instance_adding_methods.items() if val == self.state["instance_init_method"] ] if method_key: - self.commands.newInstance(init_method=method_key[0]) + self.commands.newInstance(init_method=method_key[0], offset=10) labelMenu = self.menuBar().addMenu("Labels") add_menu_item( diff --git a/sleap/gui/commands.py b/sleap/gui/commands.py index fce6458f5..e3ef8522d 100644 --- a/sleap/gui/commands.py +++ b/sleap/gui/commands.py @@ -504,6 +504,7 @@ def newInstance( init_method: str = "best", location: Optional[QtCore.QPoint] = None, mark_complete: bool = False, + offset: int = 0, ): """Creates a new instance, copying node coordinates as appropriate. @@ -513,6 +514,8 @@ def newInstance( init_method: Method to use for positioning nodes. location: The location where instance should be added (if node init method supports custom location). + mark_complete: Whether to mark the instance as complete. + offset: Offset to apply to the location if given. """ self.execute( AddInstance, @@ -520,6 +523,7 @@ def newInstance( init_method=init_method, location=location, mark_complete=mark_complete, + offset=offset, ) def setPointLocations( @@ -2858,6 +2862,7 @@ def do_action(cls, context: CommandContext, params: dict): init_method = params.get("init_method", "best") location = params.get("location", None) mark_complete = params.get("mark_complete", False) + offset = params.get("offset", 0) if context.state["labeled_frame"] is None: return @@ -2881,6 +2886,7 @@ def do_action(cls, context: CommandContext, params: dict): init_method=init_method, location=location, from_prev_frame=from_prev_frame, + offset=offset, ) # Add the instance @@ -2898,6 +2904,7 @@ def create_new_instance( init_method: str, location: Optional[QtCore.QPoint], from_prev_frame: bool, + offset: int = 0, ) -> Instance: """Create new instance.""" @@ -2915,6 +2922,7 @@ def create_new_instance( mark_complete=mark_complete, init_method=init_method, location=location, + offset=offset, ) if has_missing_nodes: @@ -2988,6 +2996,7 @@ def set_visible_nodes( mark_complete: bool, init_method: str, location: Optional[QtCore.QPoint] = None, + offset: int = 0, ) -> bool: """Sets visible nodes for new instance. @@ -2996,6 +3005,9 @@ def set_visible_nodes( copy_instance: The instance to copy from. new_instance: The new instance. mark_complete: Whether to mark the instance as complete. + init_method: The initialization method. + location: The location of the mouse click if any. + offset: The offset to apply to all nodes. Returns: Whether the new instance has missing nodes. @@ -3014,24 +3026,19 @@ def set_visible_nodes( scale_width = new_size_width / old_size_width scale_height = new_size_height / old_size_height - # Default the offset is 0 - offset_x = 0 - offset_y = 0 - - # Using the menu or the hotkey - if init_method == "best": - offset_x = 10 - offset_y = 10 + # The offset is 0, except when using Ctrl + I or Add Instance button. + offset_x = offset + offset_y = offset - # Using right click and context menu - if location is not None: - reference_node = next( - (node for node in copy_instance if not node.isnan()), None - ) - reference_x = reference_node.x - reference_y = reference_node.y - offset_x = location.x() - (reference_x * scale_width) - offset_y = location.y() - (reference_y * scale_height) + # Using right click and context menu with option "best" + if (init_method == "best") and (location is not None): + reference_node = next( + (node for node in copy_instance if not node.isnan()), None + ) + reference_x = reference_node.x + reference_y = reference_node.y + offset_x = location.x() - (reference_x * scale_width) + offset_y = location.y() - (reference_y * scale_height) # Go through each node in skeleton. for node in context.state["skeleton"].node_names: diff --git a/sleap/gui/widgets/docks.py b/sleap/gui/widgets/docks.py index 43e218adb..3375e4713 100644 --- a/sleap/gui/widgets/docks.py +++ b/sleap/gui/widgets/docks.py @@ -557,7 +557,7 @@ def create_table_edit_buttons(self) -> QWidget: hb = QHBoxLayout() self.add_button( - hb, "New Instance", lambda x: main_window.commands.newInstance() + hb, "New Instance", lambda x: main_window.commands.newInstance(offset=10) ) self.add_button( hb, "Delete Instance", main_window.commands.deleteSelectedInstance diff --git a/sleap/gui/widgets/video.py b/sleap/gui/widgets/video.py index 04965bbbb..949703020 100644 --- a/sleap/gui/widgets/video.py +++ b/sleap/gui/widgets/video.py @@ -240,6 +240,8 @@ def __init__( self._register_shortcuts() + self.context_menu = None + self._menu_actions = dict() if self.context: self.setContextMenuPolicy(QtCore.Qt.CustomContextMenu) self.customContextMenuRequested.connect(self.show_contextual_menu) @@ -358,44 +360,54 @@ def add_shortcut(key, step): def setSeekbarSelection(self, a: int, b: int): self.seekbar.setSelection(a, b) - def show_contextual_menu(self, where: QtCore.QPoint): - if not self.is_menu_enabled: - return + def create_contextual_menu(self, scene_pos: QtCore.QPointF) -> QtWidgets.QMenu: + """Create the context menu for the viewer. - scene_pos = self.view.mapToScene(where) - menu = QtWidgets.QMenu() + This is called when the user right-clicks in the viewer. This function also + stores the menu actions in the `_menu_actions` attribute so that they can be + accessed later and stores the context menu in the `context_menu` attribute. - menu.addAction("Add Instance:").setEnabled(False) + Args: + scene_pos: The position in the scene where the menu was requested. - menu.addAction( - "Default", - lambda: self.context.newInstance(init_method="best", location=scene_pos), - ) + Returns: + The created context menu. + """ - menu.addAction( - "Average", - lambda: self.context.newInstance( - init_method="template", location=scene_pos - ), - ) + self.context_menu = QtWidgets.QMenu() + self.context_menu.addAction("Add Instance:").setEnabled(False) + + self._menu_actions = dict() + params_by_action_name = { + "Default": {"init_method": "best", "location": scene_pos}, + "Average": {"init_method": "template", "location": scene_pos}, + "Force Directed": {"init_method": "force_directed", "location": scene_pos}, + "Copy Prior Frame": {"init_method": "prior_frame"}, + "Random": {"init_method": "random", "location": scene_pos}, + } + for action_name, params in params_by_action_name.items(): + self._menu_actions[action_name] = self.context_menu.addAction( + action_name, lambda params=params: self.context.newInstance(**params) + ) - menu.addAction( - "Force Directed", - lambda: self.context.newInstance( - init_method="force_directed", location=scene_pos - ), - ) + return self.context_menu - menu.addAction( - "Copy Prior Frame", - lambda: self.context.newInstance(init_method="prior_frame"), - ) + def show_contextual_menu(self, where: QtCore.QPoint): + """Show the context menu at the given position in the viewer. - menu.addAction( - "Random", - lambda: self.context.newInstance(init_method="random", location=scene_pos), - ) + This is called when the user right-clicks in the viewer. This function calls + `create_contextual_menu` to create the menu and then shows the menu at the + given position. + + Args: + where: The position in the viewer where the menu was requested. + """ + if not self.is_menu_enabled: + return + + scene_pos = self.view.mapToScene(where) + menu = self.create_contextual_menu(scene_pos) menu.exec_(self.mapToGlobal(where)) def load_video(self, video: Video, plot=True): diff --git a/tests/gui/test_commands.py b/tests/gui/test_commands.py index 899b1f4a0..ffd382ab1 100644 --- a/tests/gui/test_commands.py +++ b/tests/gui/test_commands.py @@ -3,11 +3,15 @@ import sys import time +import numpy as np from pathlib import PurePath, Path +from qtpy import QtCore from typing import List from sleap import Skeleton, Track, PredictedInstance +from sleap.gui.app import MainWindow from sleap.gui.commands import ( + AddInstance, CommandContext, ExportAnalysisFile, ExportDatasetWithImages, @@ -922,3 +926,102 @@ def no_gui_ask(cls, context, params): # Case 3: Export all frames and suggested frames with image data. context.exportFullPackage() assert_loaded_package_similar(path_to_pkg, sugg=True, pred=True) + + +def test_newInstance(qtbot, centered_pair_predictions: Labels): + + # Get the data + labels = centered_pair_predictions + lf = labels[0] + pred_inst = lf.instances[0] + video = labels.video + + # Set-up command context + main_window = MainWindow(labels=labels) + context = main_window.commands + context.state["labeled_frame"] = lf + context.state["frame_idx"] = lf.frame_idx + context.state["skeleton"] = labels.skeleton + context.state["video"] = labels.videos[0] + + # Case 1: Double clicking a prediction results in no offset for new instance + + # Double click on prediction + assert len(lf.instances) == 2 + main_window._handle_instance_double_click(instance=pred_inst) + + # Check new instance + assert len(lf.instances) == 3 + new_inst = lf.instances[-1] + assert new_inst.from_predicted is pred_inst + assert np.array_equal(new_inst.numpy(), pred_inst.numpy()) # No offset + + # Case 2: Using Ctrl + I (or menu "Add Instance" button) + + # Connect the action to a slot + add_instance_menu_action = main_window._menu_actions["add instance"] + triggered = False + + def on_triggered(): + nonlocal triggered + triggered = True + + add_instance_menu_action.triggered.connect(on_triggered) + + # Find which instance we are going to copy from + ( + copy_instance, + from_predicted, + from_prev_frame, + ) = AddInstance.find_instance_to_copy_from( + context, copy_instance=None, init_method="best" + ) + + # Click on the menu action + assert len(lf.instances) == 3 + add_instance_menu_action.trigger() + assert triggered, "Action not triggered" + + # Check new instance + assert len(lf.instances) == 4 + new_inst = lf.instances[-1] + offset = 10 + np.nan_to_num(new_inst.numpy() - copy_instance.numpy(), nan=offset) + assert np.all( + np.nan_to_num(new_inst.numpy() - copy_instance.numpy(), nan=offset) == offset + ) + + # Case 3: Using right click and "Default" option + + # Find which instance we are going to copy from + ( + copy_instance, + from_predicted, + from_prev_frame, + ) = AddInstance.find_instance_to_copy_from( + context, copy_instance=None, init_method="best" + ) + + video_player = main_window.player + right_click_location_x = video.shape[2] / 2 + right_click_location_y = video.shape[1] / 2 + right_click_location = QtCore.QPointF( + right_click_location_x, right_click_location_y + ) + video_player.create_contextual_menu(scene_pos=right_click_location) + default_action = video_player._menu_actions["Default"] + default_action.trigger() + + # Check new instance + assert len(lf.instances) == 5 + new_inst = lf.instances[-1] + reference_node_idx = np.where( + np.all( + new_inst.numpy() == [right_click_location_x, right_click_location_y], axis=1 + ) + )[0][0] + offset = ( + new_inst.numpy()[reference_node_idx] - copy_instance.numpy()[reference_node_idx] + ) + diff = np.nan_to_num(new_inst.numpy() - copy_instance.numpy(), nan=offset) + assert np.all(diff == offset) diff --git a/tests/gui/widgets/test_docks.py b/tests/gui/widgets/test_docks.py index 69fe56a56..d5c16a763 100644 --- a/tests/gui/widgets/test_docks.py +++ b/tests/gui/widgets/test_docks.py @@ -1,15 +1,17 @@ """Module for testing dock widgets for the `MainWindow`.""" from pathlib import Path -import pytest + +import numpy as np + from sleap import Labels, Video from sleap.gui.app import MainWindow -from sleap.gui.commands import OpenSkeleton +from sleap.gui.commands import AddInstance, OpenSkeleton from sleap.gui.widgets.docks import ( InstancesDock, + SkeletonDock, SuggestionsDock, VideosDock, - SkeletonDock, ) @@ -99,11 +101,35 @@ def test_suggestions_dock(qtbot): assert dock.wgt_layout is dock.widget().layout() -def test_instances_dock(qtbot): +def test_instances_dock(qtbot, centered_pair_predictions: Labels): """Test the `DockWidget` class.""" - main_window = MainWindow() + main_window = MainWindow(labels=centered_pair_predictions) + labels = main_window.labels + context = main_window.commands + lf = context.state["labeled_frame"] dock = InstancesDock(main_window) assert dock.name == "Instances" assert dock.main_window is main_window assert dock.wgt_layout is dock.widget().layout() + + # Test new instance button + + offset = 10 + + # Find instance that we will copy from + ( + copy_instance, + from_predicted, + from_prev_frame, + ) = AddInstance.find_instance_to_copy_from( + context, copy_instance=None, init_method="best" + ) + n_instance = len(lf.instances) + dock.main_window._buttons["new instance"].click() + + # Check that new instance was added with offset + assert len(lf.instances) == n_instance + 1 + new_inst = lf.instances[-1] + diff = np.nan_to_num(new_inst.numpy() - copy_instance.numpy(), nan=offset) + assert np.all(diff == offset)