Fix performance bottleneck when deleting a lot of trees at once (#8176)

* Fix performance bottleneck when deleting a lot of trees at once. Also fix a bug when importing an NML with groups when only groups but no trees exist in an annotation. Also fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree. * update changelog * apply PR feedback * Better error message for non-existing key in diffable map and don't throw an error in getNodeAndTree if a node with nodeId does not exist.
scalableminds · Nov 13, 2024 · 8ff5b67 · 8ff5b67
1 parent 1ec9b19
commit 8ff5b67
Show file tree

Hide file tree

Showing 10 changed files with 87 additions and 27 deletions.
diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md
@@ -16,6 +16,9 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Reading image files on datastore filesystem is now done asynchronously. [#8126](https://github.com/scalableminds/webknossos/pull/8126)
 
 ### Fixed
+- Fix performance bottleneck when deleting a lot of trees at once. [#8176](https://github.com/scalableminds/webknossos/pull/8176)
+- Fix a bug when importing an NML with groups when only groups but no trees exist in an annotation. [#8176](https://github.com/scalableminds/webknossos/pull/8176)
+- Fix a bug where trying to delete a non-existing node (via the API, for example) would delete the whole active tree. [#8176](https://github.com/scalableminds/webknossos/pull/8176)
 
 ### Removed
 

diff --git a/frontend/javascripts/libs/diffable_map.ts b/frontend/javascripts/libs/diffable_map.ts
@@ -54,7 +54,7 @@ class DiffableMap<K extends number, V> {
     if (value !== undefined) {
       return value;
     } else {
-      throw new Error("Get empty");
+      throw new Error(`Key '${key}' does not exist in diffable map.`);
     }
   }
 

diff --git a/frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts b/frontend/javascripts/oxalis/model/accessors/skeletontracing_accessor.ts
@@ -175,7 +175,7 @@ export function getNodeAndTree(
     let node = null;
 
     if (nodeId != null) {
-      node = tree.nodes.getOrThrow(nodeId);
+      node = tree.nodes.getNullable(nodeId);
     } else {
       const { activeNodeId } = skeletonTracing;
 

diff --git a/frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx b/frontend/javascripts/oxalis/model/actions/skeletontracing_actions.tsx
@@ -39,6 +39,7 @@ type CreateTreeAction = ReturnType<typeof createTreeAction>;
 type SetEdgeVisibilityAction = ReturnType<typeof setTreeEdgeVisibilityAction>;
 type AddTreesAndGroupsAction = ReturnType<typeof addTreesAndGroupsAction>;
 type DeleteTreeAction = ReturnType<typeof deleteTreeAction>;
+type DeleteTreesAction = ReturnType<typeof deleteTreesAction>;
 type ResetSkeletonTracingAction = ReturnType<typeof resetSkeletonTracingAction>;
 type SetActiveTreeAction = ReturnType<typeof setActiveTreeAction>;
 type SetActiveTreeByNameAction = ReturnType<typeof setActiveTreeByNameAction>;
@@ -65,7 +66,11 @@ type UpdateNavigationListAction = ReturnType<typeof updateNavigationListAction>;
 export type LoadAgglomerateSkeletonAction = ReturnType<typeof loadAgglomerateSkeletonAction>;
 type NoAction = ReturnType<typeof noAction>;
 
-export type BatchableUpdateTreeAction = SetTreeGroupAction | DeleteTreeAction | SetTreeGroupsAction;
+export type BatchableUpdateTreeAction =
+  | SetTreeGroupAction
+  | DeleteTreeAction
+  | DeleteTreesAction
+  | SetTreeGroupsAction;
 export type BatchUpdateGroupsAndTreesAction = {
   type: "BATCH_UPDATE_GROUPS_AND_TREES";
   payload: BatchableUpdateTreeAction[];
@@ -93,6 +98,7 @@ export type SkeletonTracingAction =
   | SetEdgeVisibilityAction
   | AddTreesAndGroupsAction
   | DeleteTreeAction
+  | DeleteTreesAction
   | ResetSkeletonTracingAction
   | SetActiveTreeAction
   | SetActiveTreeByNameAction
@@ -139,6 +145,7 @@ export const SkeletonTracingSaveRelevantActions = [
   "SET_EDGES_ARE_VISIBLE",
   "ADD_TREES_AND_GROUPS",
   "DELETE_TREE",
+  "DELETE_TREES",
   "SET_ACTIVE_TREE",
   "SET_ACTIVE_TREE_BY_NAME",
   "SET_TREE_NAME",
@@ -337,6 +344,19 @@ export const deleteTreeAction = (treeId?: number, suppressActivatingNextNode: bo
     suppressActivatingNextNode,
   }) as const;
 
+export const deleteTreesAction = (treeIds: number[], suppressActivatingNextNode: boolean = false) =>
+  // If suppressActivatingNextNode is true, the trees will be deleted without activating
+  // another node (nor tree). Use this in cases where you want to avoid changing
+  // the active position (due to the auto-centering). One could also suppress the auto-centering
+  // behavior, but the semantics of changing the active node might also be confusing to the user
+  // (e.g., when proofreading). So, it might be clearer to not have an active node in the first
+  // place.
+  ({
+    type: "DELETE_TREES",
+    treeIds,
+    suppressActivatingNextNode,
+  }) as const;
+
 export const resetSkeletonTracingAction = () =>
   ({
     type: "RESET_SKELETON_TRACING",
@@ -555,11 +575,15 @@ export const deleteNodeAsUserAction = (
 
       return deleteNodeAction(node.id, tree.treeId);
     }) // If the tree is empty, it will be deleted
-    .getOrElse(deleteTreeAction(treeId));
+    .getOrElse(
+      getTree(skeletonTracing, treeId)
+        .map((tree) => (tree.nodes.size() === 0 ? deleteTreeAction(tree.treeId) : noAction()))
+        .getOrElse(noAction()),
+    );
 };
 
 // Let the user confirm the deletion of the initial node (node with id 1) of a task
-function confirmDeletingInitialNode(treeId?: number) {
+function confirmDeletingInitialNode(treeId: number) {
   Modal.confirm({
     title: messages["tracing.delete_tree_with_initial_node"],
     onOk: () => {
@@ -573,12 +597,15 @@ export const deleteTreeAsUserAction = (treeId?: number): NoAction => {
   const skeletonTracing = enforceSkeletonTracing(state.tracing);
   getTree(skeletonTracing, treeId).map((tree) => {
     if (state.task != null && tree.nodes.has(1)) {
-      confirmDeletingInitialNode(treeId);
+      confirmDeletingInitialNode(tree.treeId);
     } else if (state.userConfiguration.hideTreeRemovalWarning) {
-      Store.dispatch(deleteTreeAction(treeId));
+      Store.dispatch(deleteTreeAction(tree.treeId));
     } else {
       renderIndependently((destroy) => (
-        <RemoveTreeModal onOk={() => Store.dispatch(deleteTreeAction(treeId))} destroy={destroy} />
+        <RemoveTreeModal
+          onOk={() => Store.dispatch(deleteTreeAction(tree.treeId))}
+          destroy={destroy}
+        />
       ));
     }
   });

diff --git a/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts b/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts
@@ -13,7 +13,7 @@ import {
   deleteBranchPoint,
   createNode,
   createTree,
-  deleteTree,
+  deleteTrees,
   deleteNode,
   deleteEdge,
   shuffleTreeColor,
@@ -897,10 +897,16 @@ function SkeletonTracingReducer(state: OxalisState, action: Action): OxalisState
             .getOrElse(state);
         }
 
-        case "DELETE_TREE": {
-          const { treeId, suppressActivatingNextNode } = action;
-          return getTree(skeletonTracing, treeId)
-            .chain((tree) => deleteTree(skeletonTracing, tree, suppressActivatingNextNode))
+        case "DELETE_TREE":
+        case "DELETE_TREES": {
+          const { suppressActivatingNextNode } = action;
+          const treeIds =
+            action.type === "DELETE_TREE"
+              ? getTree(skeletonTracing, action.treeId) // The treeId in a DELETE_TREE action can be undefined which will select the active tree
+                  .map((tree) => [tree.treeId])
+                  .getOrElse([])
+              : action.treeIds;
+          return deleteTrees(skeletonTracing, treeIds, suppressActivatingNextNode)
             .map(([trees, newActiveTreeId, newActiveNodeId, newMaxNodeId]) =>
               update(state, {
                 tracing: {

diff --git a/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts b/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
@@ -561,7 +561,10 @@ export function addTreesAndGroups(
   );
   const hasInvalidNodeIds = getMinimumNodeId(trees) < Constants.MIN_NODE_ID;
   const needsReassignedIds =
-    Object.keys(skeletonTracing.trees).length > 0 || hasInvalidTreeIds || hasInvalidNodeIds;
+    Object.keys(skeletonTracing.trees).length > 0 ||
+    skeletonTracing.treeGroups.length > 0 ||
+    hasInvalidTreeIds ||
+    hasInvalidNodeIds;
 
   if (!needsReassignedIds) {
     // Without reassigning ids, the code is considerably faster.
@@ -631,20 +634,22 @@ export function addTreesAndGroups(
 
   return Maybe.Just([newTrees, treeGroups, newNodeId - 1]);
 }
-export function deleteTree(
+export function deleteTrees(
   skeletonTracing: SkeletonTracing,
-  tree: Tree,
+  treeIds: number[],
   suppressActivatingNextNode: boolean = false,
 ): Maybe<[TreeMap, number | null | undefined, number | null | undefined, number]> {
-  // Delete tree
-  const newTrees = _.omit(skeletonTracing.trees, tree.treeId);
+  if (treeIds.length === 0) return Maybe.Nothing();
+  // Delete trees
+  const newTrees = _.omit(skeletonTracing.trees, treeIds);
 
   let newActiveTreeId = null;
   let newActiveNodeId = null;
 
   if (_.size(newTrees) > 0 && !suppressActivatingNextNode) {
-    // Setting the tree active whose id is the next highest compared to the id of the deleted tree.
-    newActiveTreeId = getNearestTreeId(tree.treeId, newTrees);
+    // Setting the tree active whose id is the next highest compared to the ids of the deleted trees.
+    const maximumTreeId = _.max(treeIds) || Constants.MIN_TREE_ID;
+    newActiveTreeId = getNearestTreeId(maximumTreeId, newTrees);
     // @ts-expect-error ts-migrate(2571) FIXME: Object is of type 'unknown'.
     newActiveNodeId = +_.first(Array.from(newTrees[newActiveTreeId].nodes.keys())) || null;
   }

diff --git a/frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts b/frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts
@@ -468,6 +468,7 @@ export function* watchSkeletonTracingAsync(): Saga<void> {
       "DELETE_BRANCHPOINT",
       "SELECT_NEXT_TREE",
       "DELETE_TREE",
+      "DELETE_TREES",
       "BATCH_UPDATE_GROUPS_AND_TREES",
       "CENTER_ACTIVE_NODE",
     ],

diff --git a/frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx b/frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/skeleton_tab_view.tsx
@@ -53,7 +53,7 @@ import { setDropzoneModalVisibilityAction } from "oxalis/model/actions/ui_action
 import {
   setTreeNameAction,
   createTreeAction,
-  deleteTreeAction,
+  deleteTreesAction,
   deleteTreeAsUserAction,
   shuffleAllTreeColorsAction,
   selectNextTreeAction,
@@ -458,11 +458,11 @@ class SkeletonTabView extends React.PureComponent<Props, State> {
     });
     checkAndConfirmDeletingInitialNode(treeIdsToDelete).then(() => {
       // Update the store at once
-      const deleteTreeActions: BatchableUpdateTreeAction[] = treeIdsToDelete.map((treeId) =>
-        deleteTreeAction(treeId),
-      );
       this.props.onBatchUpdateGroupsAndTreesAction(
-        updateTreeActions.concat(deleteTreeActions, [setTreeGroupsAction(newTreeGroups)]),
+        updateTreeActions.concat([
+          deleteTreesAction(treeIdsToDelete),
+          setTreeGroupsAction(newTreeGroups),
+        ]),
       );
     });
   };
@@ -510,8 +510,7 @@ class SkeletonTabView extends React.PureComponent<Props, State> {
     if (selectedTreeCount > 0) {
       const deleteAllSelectedTrees = () => {
         checkAndConfirmDeletingInitialNode(selectedTreeIds).then(() => {
-          const deleteTreeActions = selectedTreeIds.map((treeId) => deleteTreeAction(treeId));
-          this.props.onBatchActions(deleteTreeActions, "DELETE_TREE");
+          this.props.onDeleteTrees(selectedTreeIds);
           this.setState({
             selectedTreeIds: [],
           });
@@ -1033,6 +1032,10 @@ const mapDispatchToProps = (dispatch: Dispatch<any>) => ({
     dispatch(deleteTreeAsUserAction());
   },
 
+  onDeleteTrees(treeIds: number[]) {
+    dispatch(deleteTreesAction(treeIds));
+  },
+
   onBatchActions(actions: Array<Action>, actionName: string) {
     dispatch(batchActions(actions, actionName));
   },

diff --git a/frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx b/frontend/javascripts/oxalis/view/right-border-tabs/trees_tab/tree_hierarchy_renderers.tsx
@@ -140,6 +140,7 @@ const createMenuForTree = (tree: Tree, props: Props, hideContextMenu: () => void
         onClick: () => {
           props.deselectAllTrees();
           Store.dispatch(deleteTreeAction(tree.treeId));
+          hideContextMenu();
         },
         title: "Delete Tree",
         disabled: isEditingDisabled,

diff --git a/frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts b/frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts
@@ -888,6 +888,20 @@ test("SkeletonTracing should delete several trees", (t) => {
   t.deepEqual(_.size(newSkeletonTracing.trees), 0);
   t.not(newSkeletonTracing.trees, initialSkeletonTracing.trees);
 });
+test("SkeletonTracing should delete several trees at once", (t) => {
+  const createTreeAction = SkeletonTracingActions.createTreeAction();
+  const deleteTreesAction = SkeletonTracingActions.deleteTreesAction([1, 2, 3]);
+  // create trees and delete them
+  const newState = ChainReducer<OxalisState, Action>(initialState)
+    .apply(SkeletonTracingReducer, createTreeAction)
+    .apply(SkeletonTracingReducer, createTreeAction)
+    .apply(SkeletonTracingReducer, deleteTreesAction)
+    .unpack();
+  t.not(newState, initialState);
+  const newSkeletonTracing = enforceSkeletonTracing(newState.tracing);
+  t.deepEqual(_.size(newSkeletonTracing.trees), 0);
+  t.not(newSkeletonTracing.trees, initialSkeletonTracing.trees);
+});
 test("SkeletonTracing should set a new active tree", (t) => {
   const createTreeAction = SkeletonTracingActions.createTreeAction();
   const setActiveTreeAction = SkeletonTracingActions.setActiveTreeAction(2);