After merge, remove segment item of id that doesn't exist anymore (#7729

) * after merge, remove segment item of id that doesn't exist anymore * update changelog * remove Maybe usage from mergeTrees reducer helper * swap source and target in mergeTreesAction so that it is consistent with proofread merging (the active tree/segment should always survive the merge operation) * update changelog * fix tests * fix tests (II) * Update CHANGELOG.unreleased.md Co-authored-by: Daniel <daniel.werner@scalableminds.com> --------- Co-authored-by: Daniel <daniel.werner@scalableminds.com>
scalableminds · Apr 8, 2024 · 1649a35 · 1649a35
1 parent 6c25924
commit 1649a35
Show file tree

Hide file tree

Showing 7 changed files with 81 additions and 64 deletions.
diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md
@@ -18,6 +18,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Changed the time-tracking overview to show times spent in annotations and tasks and filter them by teams and projects. In the linked detail view, the tracked times can also be filtered by type (annotations or tasks) and project. [#7524](https://github.com/scalableminds/webknossos/pull/7524)
 - The time tracking api route `/api/users/:id/loggedTime`, which is used by the webknossos-libs client, and groups the times by month, now uses UTC when determining month limits, rather than the server’s local timezone. [#7524](https://github.com/scalableminds/webknossos/pull/7524)
 - Duplicated annotations are opened in a new browser tab. [#7724](https://github.com/scalableminds/webknossos/pull/7724)
+- When proofreading segments and merging two segments, the segment item that doesn't exist anymore after the merge is automatically removed. [#7729](https://github.com/scalableminds/webknossos/pull/7729)
 - Changed some internal APIs to use spelling dataset instead of dataSet. This requires all connected datastores to be the latest version. [#7690](https://github.com/scalableminds/webknossos/pull/7690)
 
 ### Fixed
@@ -29,3 +30,4 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 ### Removed
 
 ### Breaking Changes
+- When merging two trees or segments, the active item will always "survive" the merge operation (the clicked item will be merged *into* the active one). This was not consistent for certain skeleton-based operations (i.e., merging skeletons with a shortcut and proofreading segmentations with agglomerate skeletons). [#7729](https://github.com/scalableminds/webknossos/pull/7729)
diff --git a/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts b/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer.ts
@@ -939,29 +939,30 @@ function SkeletonTracingReducer(state: OxalisState, action: Action): OxalisState
           const isProofreadingActive =
             state.uiInformation.activeTool === AnnotationToolEnum.PROOFREAD;
           const treeType = isProofreadingActive ? TreeTypeEnum.AGGLOMERATE : TreeTypeEnum.DEFAULT;
-          const trees = getTreesWithType(skeletonTracing, treeType);
-          return mergeTrees(trees, sourceNodeId, targetNodeId)
-            .map(([trees, newActiveTreeId, newActiveNodeId]) =>
-              update(state, {
-                tracing: {
-                  skeleton: {
-                    trees: {
-                      $set: trees,
-                    },
-                    activeNodeId: {
-                      $set: newActiveNodeId,
-                    },
-                    activeTreeId: {
-                      $set: newActiveTreeId,
-                    },
-                    activeGroupId: {
-                      $set: null,
-                    },
-                  },
+          const oldTrees = getTreesWithType(skeletonTracing, treeType);
+          const mergeResult = mergeTrees(oldTrees, sourceNodeId, targetNodeId);
+          if (mergeResult == null) {
+            return state;
+          }
+          const [trees, newActiveTreeId, newActiveNodeId] = mergeResult;
+          return update(state, {
+            tracing: {
+              skeleton: {
+                trees: {
+                  $set: trees,
                 },
-              }),
-            )
-            .getOrElse(state);
+                activeNodeId: {
+                  $set: newActiveNodeId,
+                },
+                activeTreeId: {
+                  $set: newActiveTreeId,
+                },
+                activeGroupId: {
+                  $set: null,
+                },
+              },
+            },
+          });
         }
 
         case "SET_TREE_NAME": {

diff --git a/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts b/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
@@ -651,44 +651,45 @@ export function mergeTrees(
   trees: TreeMap,
   sourceNodeId: number,
   targetNodeId: number,
-): Maybe<[TreeMap, number, number]> {
-  const sourceTree = findTreeByNodeId(trees, sourceNodeId);
-  const targetTree = findTreeByNodeId(trees, targetNodeId); // should be activeTree
+): [TreeMap, number, number] | null {
+  // targetTree will be removed (the content will be merged into sourceTree).
+  const sourceTree = findTreeByNodeId(trees, sourceNodeId); // should be activeTree, so that the active tree "survives"
+  const targetTree = findTreeByNodeId(trees, targetNodeId);
 
-  if (sourceTree == null || targetTree == null || sourceTree === targetTree) {
-    return Maybe.Nothing();
+  if (targetTree == null || sourceTree == null || targetTree === sourceTree) {
+    return null;
   }
 
   const newEdge: Edge = {
     source: sourceNodeId,
     target: targetNodeId,
   };
 
-  let newTrees = _.omit(trees, sourceTree.treeId);
+  let newTrees = _.omit(trees, targetTree.treeId);
 
-  const newNodes = targetTree.nodes.clone();
+  const newNodes = sourceTree.nodes.clone();
 
-  for (const [id, node] of sourceTree.nodes.entries()) {
+  for (const [id, node] of targetTree.nodes.entries()) {
     newNodes.mutableSet(id, node);
   }
 
   newTrees = update(newTrees, {
-    [targetTree.treeId]: {
+    [sourceTree.treeId]: {
       nodes: {
         $set: newNodes,
       },
       edges: {
-        $set: targetTree.edges.addEdges(sourceTree.edges.asArray().concat(newEdge)),
+        $set: sourceTree.edges.addEdges(targetTree.edges.asArray().concat(newEdge)),
       },
       comments: {
-        $set: targetTree.comments.concat(sourceTree.comments),
+        $set: sourceTree.comments.concat(targetTree.comments),
       },
       branchPoints: {
-        $set: targetTree.branchPoints.concat(sourceTree.branchPoints),
+        $set: sourceTree.branchPoints.concat(targetTree.branchPoints),
       },
     },
   });
-  return Maybe.Just([newTrees, targetTree.treeId, targetNodeId]);
+  return [newTrees, sourceTree.treeId, sourceNodeId];
 }
 export function shuffleTreeColor(
   skeletonTracing: SkeletonTracing,

diff --git a/frontend/javascripts/oxalis/model/sagas/proofread_saga.ts b/frontend/javascripts/oxalis/model/sagas/proofread_saga.ts
@@ -12,6 +12,7 @@ import {
 } from "oxalis/model/actions/skeletontracing_actions";
 import {
   initializeEditableMappingAction,
+  removeSegmentAction,
   setMappingIsEditableAction,
 } from "oxalis/model/actions/volumetracing_actions";
 import type { ProofreadAtPositionAction } from "oxalis/model/actions/proofread_actions";
@@ -238,6 +239,8 @@ function* handleSkeletonProofreadingAction(action: Action): Saga<void> {
   // Actually, action is MergeTreesAction | DeleteEdgeAction | MinCutAgglomerateAction,
   // but the takeEveryUnlessBusy wrapper does not understand this.
   // Handles split, merge and min-cut actions on agglomerates.
+  // Note that the skeletontracing reducer already mutated the skeletons according to the
+  // received action.
   if (
     action.type !== "MERGE_TREES" &&
     action.type !== "DELETE_EDGE" &&
@@ -394,12 +397,16 @@ function* handleSkeletonProofreadingAction(action: Action): Saga<void> {
     ),
   );
   if (sourceTree !== targetTree) {
+    // A split happened, because the new trees are not identical.
     yield* put(
       setTreeNameAction(
         getTreeNameForAgglomerateSkeleton(newTargetAgglomerateId, volumeTracing.mappingName),
         targetTree.treeId,
       ),
     );
+  } else {
+    // A merge happened. Remove the segment that doesn't exist anymore.
+    yield* put(removeSegmentAction(targetAgglomerateId, volumeTracing.tracingId));
   }
 
   const pack = (agglomerateId: number, newAgglomerateId: number, nodePosition: Vector3) => ({
@@ -633,6 +640,11 @@ function* handleProofreadMergeOrMinCut(action: Action) {
     bucket.containsValue(targetAgglomerateId),
   );
 
+  if (action.type === "PROOFREAD_MERGE") {
+    // Remove the segment that doesn't exist anymore.
+    yield* put(removeSegmentAction(targetAgglomerateId, volumeTracing.tracingId));
+  }
+
   const [newSourceAgglomerateId, newTargetAgglomerateId] = yield* all([
     call(getDataValue, sourcePosition),
     call(getDataValue, targetPosition),

diff --git a/frontend/javascripts/oxalis/view/context_menu.tsx b/frontend/javascripts/oxalis/view/context_menu.tsx
@@ -470,7 +470,7 @@ function getNodeContextMenuOptions({
             disabled: areInSameTree,
             onClick: () =>
               activeNodeId != null
-                ? Store.dispatch(mergeTreesAction(clickedNodeId, activeNodeId))
+                ? Store.dispatch(mergeTreesAction(activeNodeId, clickedNodeId))
                 : null,
             label: (
               <>

diff --git a/frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts b/frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts
@@ -931,7 +931,7 @@ test("SkeletonTracing should merge two trees", (t) => {
     viewport,
     resolution,
   );
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 3);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(3, 1);
   // create a node in first tree, then create a second tree with three nodes and merge them
   const newState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -955,8 +955,8 @@ test("SkeletonTracing should merge two trees", (t) => {
       target: 4,
     },
     {
-      source: 1,
-      target: 3,
+      source: 3,
+      target: 1,
     },
   ]);
 });
@@ -987,7 +987,7 @@ test("SkeletonTracing should merge two trees with comments and branchPoints", (t
     viewport,
     resolution,
   );
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 3);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(3, 1);
   const createCommentAction = SkeletonTracingActions.createCommentAction("foo");
   const createBranchPointAction = SkeletonTracingActions.createBranchPointAction();
   // create a node in first tree, then create a second tree with three nodes and merge them
@@ -1016,8 +1016,8 @@ test("SkeletonTracing should merge two trees with comments and branchPoints", (t
       target: 4,
     },
     {
-      source: 1,
-      target: 3,
+      source: 3,
+      target: 1,
     },
   ]);
   t.is(newSkeletonTracing.trees[2].comments.length, 2);
@@ -1217,6 +1217,7 @@ test("SkeletonTracing should delete a comment for a node", (t) => {
   const newSkeletonTracing = enforceSkeletonTracing(newState.tracing);
   t.deepEqual(newSkeletonTracing.trees[1].comments.length, 0);
 });
+
 test("SkeletonTracing should only delete the comment for the active node", (t) => {
   const commentText = "Wow such test comment";
   const createNodeAction = SkeletonTracingActions.createNodeAction(

diff --git a/frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts b/frontend/javascripts/test/sagas/skeletontracing_saga.spec.ts
@@ -297,7 +297,7 @@ test("SkeletonTracingSaga should emit createNode and createTree update actions",
   });
 });
 test("SkeletonTracingSaga should emit first deleteNode and then createNode update actions", (t) => {
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(2, 1);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 2);
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
     .apply(SkeletonTracingReducer, createTreeAction)
@@ -329,8 +329,8 @@ test("SkeletonTracingSaga should emit first deleteNode and then createNode updat
     name: "createEdge",
     value: {
       treeId: 1,
-      source: 2,
-      target: 1,
+      source: 1,
+      target: 2,
     },
   });
 });
@@ -481,7 +481,7 @@ test("SkeletonTracingSaga should emit an updateTree update actions (branchpoints
   });
 });
 test("SkeletonTracingSaga should emit update actions on merge tree", (t) => {
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 3);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(3, 1);
   // create a node in first tree, then create a second tree with three nodes and merge them
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -521,13 +521,13 @@ test("SkeletonTracingSaga should emit update actions on merge tree", (t) => {
     name: "createEdge",
     value: {
       treeId: 2,
-      source: 1,
-      target: 3,
+      source: 3,
+      target: 1,
     },
   });
 });
 test("SkeletonTracingSaga should emit update actions on split tree", (t) => {
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 3);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(3, 1);
   // create a node in first tree, then create a second tree with three nodes and merge them
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -612,13 +612,13 @@ test("SkeletonTracingSaga should emit update actions on split tree", (t) => {
     name: "deleteEdge",
     value: {
       treeId: 2,
-      source: 1,
-      target: 3,
+      source: 3,
+      target: 1,
     },
   });
 });
 test("compactUpdateActions should detect a tree merge (1/3)", (t) => {
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 4);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(4, 1);
   // Create three nodes in the first tree, then create a second tree with one node and merge them
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -661,15 +661,15 @@ test("compactUpdateActions should detect a tree merge (1/3)", (t) => {
     name: "createEdge",
     value: {
       treeId: 2,
-      source: 1,
-      target: 4,
+      source: 4,
+      target: 1,
     },
   });
   t.is(simplifiedFirstBatch.length, 3);
 });
 test("compactUpdateActions should detect a tree merge (2/3)", (t) => {
   // In this test multiple diffs are performed and concatenated before compactUpdateActions is invoked
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 5);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(5, 1);
   // Create three nodes in the first tree, then create a second tree with one node
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -742,16 +742,16 @@ test("compactUpdateActions should detect a tree merge (2/3)", (t) => {
     name: "createEdge",
     value: {
       treeId: 2,
-      source: 1,
-      target: 5,
+      source: 5,
+      target: 1,
     },
   });
   t.is(simplifiedSecondBatch.length, 5);
 });
 test("compactUpdateActions should detect a tree merge (3/3)", (t) => {
   // In this test multiple merges and diffs are performed and concatenated before compactUpdateActions is invoked
-  const firstMergeTreesAction = SkeletonTracingActions.mergeTreesAction(4, 1);
-  const secondMergeTreesAction = SkeletonTracingActions.mergeTreesAction(6, 1);
+  const firstMergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 4);
+  const secondMergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 6);
   // Create three nodes in the first tree, then create a second tree with one node
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)
@@ -823,8 +823,8 @@ test("compactUpdateActions should detect a tree merge (3/3)", (t) => {
     name: "createEdge",
     value: {
       treeId: 1,
-      source: 4,
-      target: 1,
+      source: 1,
+      target: 4,
     },
   });
   t.is(simplifiedFirstBatch.length, 3);
@@ -855,8 +855,8 @@ test("compactUpdateActions should detect a tree merge (3/3)", (t) => {
     name: "createEdge",
     value: {
       treeId: 1,
-      source: 6,
-      target: 1,
+      source: 1,
+      target: 6,
     },
   });
   t.is(simplifiedThirdBatch.length, 3);
@@ -1077,7 +1077,7 @@ test("compactUpdateActions should do nothing if it cannot compact", (t) => {
   // the right spot (see code comments for why)
   // This case cannot happen currently as there is no action in webknossos that results in such a diff,
   // it could however exist in the future and this test makes sure things won't break then
-  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(1, 2);
+  const mergeTreesAction = SkeletonTracingActions.mergeTreesAction(2, 1);
   // Create three nodes in the first tree, then create a second tree with one node and merge them
   const testState = ChainReducer<OxalisState, Action>(initialState)
     .apply(SkeletonTracingReducer, createNodeAction)