From 1649a35b5acff71bb645912bcfc654acb9d116bf Mon Sep 17 00:00:00 2001 From: Philipp Otto Date: Mon, 8 Apr 2024 15:49:02 +0200 Subject: [PATCH] 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 --------- Co-authored-by: Daniel --- CHANGELOG.unreleased.md | 2 + .../model/reducers/skeletontracing_reducer.ts | 45 ++++++++++--------- .../skeletontracing_reducer_helpers.ts | 27 +++++------ .../oxalis/model/sagas/proofread_saga.ts | 12 +++++ .../javascripts/oxalis/view/context_menu.tsx | 2 +- .../reducers/skeletontracing_reducer.spec.ts | 13 +++--- .../test/sagas/skeletontracing_saga.spec.ts | 44 +++++++++--------- 7 files changed, 81 insertions(+), 64 deletions(-) diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md index ee6ae5c7e54..e4bc5388280 100644 --- 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 index 24fd8bb5226..ed9e7dc6970 100644 --- 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 index 83a56bb347f..1ff6f0791cc 100644 --- a/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts +++ b/frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts @@ -651,12 +651,13 @@ 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 = { @@ -664,31 +665,31 @@ export function mergeTrees( 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 index 0983497e7dc..68d8caef7a3 100644 --- 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 { // 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 { ), ); 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 index 167cfad2152..0e475c405a7 100644 --- 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 index 0403d79a9b0..934dcf161eb 100644 --- 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(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 index 3bdccdb22b1..d1cc6862577 100644 --- 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(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(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(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(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(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(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(initialState) .apply(SkeletonTracingReducer, createNodeAction)