From fd61b709124b62572087ef4b05a9c19041742d52 Mon Sep 17 00:00:00 2001 From: Sean Martin Date: Tue, 9 Jun 2026 16:00:00 +0200 Subject: [PATCH 1/2] fix: retain overlay segments in merge/split and undo also supress browse if last node deleted, and allow to unsupress browse. Catmaid generates new IDs so generally not a problem. But good to cover just in case it ever changes to reuse IDs. --- .../catmaid/spatial_skeleton_commands.ts | 17 +++++++++++++++++ src/skeleton/frontend.ts | 14 ++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/datasource/catmaid/spatial_skeleton_commands.ts b/src/datasource/catmaid/spatial_skeleton_commands.ts index c6103c6f0..57711c96a 100644 --- a/src/datasource/catmaid/spatial_skeleton_commands.ts +++ b/src/datasource/catmaid/spatial_skeleton_commands.ts @@ -787,6 +787,7 @@ function applyCreatedNodeToCache( ); } ensureVisibleSegment(layer, newNode.segmentId); + skeletonLayer.unsuppressBrowseSegment(newNode.segmentId); if (options.selectSegment ?? true) { selectSegment(layer, newNode.segmentId, options.pinSegment); } @@ -916,6 +917,13 @@ async function commitAndApplyDeleteNode( if (options.invalidateSourceCells) { invalidateDeletedNodeSourceCells(resolvedNode.skeletonLayer, deleteContext); } + const remainingNodes = + layer.spatialSkeletonState.getCachedSegmentNodes( + resolvedNode.node.segmentId, + ) ?? []; + if (remainingNodes.length === 0) { + resolvedNode.skeletonLayer.suppressBrowseSegment(resolvedNode.node.segmentId); + } return { resolvedNode }; } @@ -1853,6 +1861,10 @@ class SplitCommand implements SpatialSkeletonCommand { [existingSkeletonId, newSkeletonId], collectUniqueNodePositions(getSplitAffectedNodes(resolvedNode)), ); + resolvedNode.skeletonLayer.unsuppressBrowseSegment(existingSkeletonId); + resolvedNode.skeletonLayer.unsuppressBrowseSegment(newSkeletonId); + resolvedNode.skeletonLayer.retainOverlaySegment(existingSkeletonId); + resolvedNode.skeletonLayer.retainOverlaySegment(newSkeletonId); StatusMessage.showTemporaryMessage( `${statusPrefix} skeleton ${existingSkeletonId}. New skeleton: ${newSkeletonId}.`, ); @@ -1932,6 +1944,7 @@ class SplitCommand implements SpatialSkeletonCommand { formerParent, ), ); + splitNode.skeletonLayer.retainOverlaySegment(resultSkeletonId); StatusMessage.showTemporaryMessage( `${statusPrefix} split at node ${splitNode.node.nodeId}.`, ); @@ -2126,6 +2139,7 @@ class MergeCommand implements SpatialSkeletonCommand { [resultSkeletonId, deletedSkeletonId], getMergeAffectedPositions(result.deletedSegmentId, firstNode, secondNode), ); + firstNode.skeletonLayer.retainOverlaySegment(resultSkeletonId); const swapSuffix = result.directionAdjusted ? " Merge direction was adjusted by the active source." : ""; @@ -2223,6 +2237,9 @@ class MergeCommand implements SpatialSkeletonCommand { `Only the split completed. ${formatErrorMessage(error)}`; } } + attachedNode.skeletonLayer.unsuppressBrowseSegment(restoredSegmentId); + attachedNode.skeletonLayer.retainOverlaySegment(survivingSegmentId); + attachedNode.skeletonLayer.retainOverlaySegment(restoredSegmentId); this.layer.selectSpatialSkeletonNode( attachedNode.node.nodeId, this.layer.manager.root.selectionState.pin.value, diff --git a/src/skeleton/frontend.ts b/src/skeleton/frontend.ts index 6d850f128..cf0e6e970 100644 --- a/src/skeleton/frontend.ts +++ b/src/skeleton/frontend.ts @@ -2194,6 +2194,20 @@ export class SpatiallyIndexedSkeletonLayer return true; } + unsuppressBrowseSegment(segmentId: number) { + const normalizedSegmentId = Math.round(Number(segmentId)); + if ( + !Number.isSafeInteger(normalizedSegmentId) || + normalizedSegmentId <= 0 || + !this.suppressedBrowseSegmentIds.has(normalizedSegmentId) + ) { + return false; + } + this.suppressedBrowseSegmentIds.delete(normalizedSegmentId); + this.redrawNeeded.dispatch(); + return true; + } + private getOverlayRenderSegmentIds() { return mergeSpatiallyIndexedSkeletonOverlaySegmentIds( this.getActiveEditableSegmentIds(), From 19305c41a6dcd0f456e6ddd0c9a1ddaab1c31096 Mon Sep 17 00:00:00 2001 From: Sean Martin Date: Tue, 9 Jun 2026 17:29:01 +0200 Subject: [PATCH 2/2] fix: remove hard ties between memory cache and overlay segments Replaces soft/hard supression duplicates with one source of overlay IDs. Also always add to this set of overlay IDs and don't clear it on refetch --- .../catmaid/spatial_skeleton_commands.ts | 10 +--- .../spatial_skeleton_commands.spec.ts | 28 +++++---- src/skeleton/frontend.spec.ts | 7 +-- src/skeleton/frontend.ts | 58 ++----------------- src/ui/skeleton_edit_tools.spec.ts | 5 +- src/ui/skeleton_edit_tools.ts | 1 + 6 files changed, 34 insertions(+), 75 deletions(-) diff --git a/src/datasource/catmaid/spatial_skeleton_commands.ts b/src/datasource/catmaid/spatial_skeleton_commands.ts index 57711c96a..14dc8d918 100644 --- a/src/datasource/catmaid/spatial_skeleton_commands.ts +++ b/src/datasource/catmaid/spatial_skeleton_commands.ts @@ -787,7 +787,6 @@ function applyCreatedNodeToCache( ); } ensureVisibleSegment(layer, newNode.segmentId); - skeletonLayer.unsuppressBrowseSegment(newNode.segmentId); if (options.selectSegment ?? true) { selectSegment(layer, newNode.segmentId, options.pinSegment); } @@ -922,7 +921,7 @@ async function commitAndApplyDeleteNode( resolvedNode.node.segmentId, ) ?? []; if (remainingNodes.length === 0) { - resolvedNode.skeletonLayer.suppressBrowseSegment(resolvedNode.node.segmentId); + resolvedNode.skeletonLayer.markSegmentEdited(resolvedNode.node.segmentId); } return { resolvedNode }; } @@ -1861,8 +1860,6 @@ class SplitCommand implements SpatialSkeletonCommand { [existingSkeletonId, newSkeletonId], collectUniqueNodePositions(getSplitAffectedNodes(resolvedNode)), ); - resolvedNode.skeletonLayer.unsuppressBrowseSegment(existingSkeletonId); - resolvedNode.skeletonLayer.unsuppressBrowseSegment(newSkeletonId); resolvedNode.skeletonLayer.retainOverlaySegment(existingSkeletonId); resolvedNode.skeletonLayer.retainOverlaySegment(newSkeletonId); StatusMessage.showTemporaryMessage( @@ -1926,7 +1923,7 @@ class SplitCommand implements SpatialSkeletonCommand { this.layer.displayState.segmentStatedColors.value.delete( BigInt(deletedSkeletonId), ); - splitNode.skeletonLayer.suppressBrowseSegment(deletedSkeletonId); + splitNode.skeletonLayer.markSegmentEdited(deletedSkeletonId); } this.layer.selectSpatialSkeletonNode( splitNode.node.nodeId, @@ -2131,7 +2128,7 @@ class MergeCommand implements SpatialSkeletonCommand { BigInt(deletedSkeletonId), ); if (deletedSkeletonId !== resultSkeletonId) { - firstNode.skeletonLayer.suppressBrowseSegment(deletedSkeletonId); + firstNode.skeletonLayer.markSegmentEdited(deletedSkeletonId); } this.layer.clearSpatialSkeletonMergeAnchor(); await refreshTopologySegments( @@ -2237,7 +2234,6 @@ class MergeCommand implements SpatialSkeletonCommand { `Only the split completed. ${formatErrorMessage(error)}`; } } - attachedNode.skeletonLayer.unsuppressBrowseSegment(restoredSegmentId); attachedNode.skeletonLayer.retainOverlaySegment(survivingSegmentId); attachedNode.skeletonLayer.retainOverlaySegment(restoredSegmentId); this.layer.selectSpatialSkeletonNode( diff --git a/src/layer/segmentation/spatial_skeleton_commands.spec.ts b/src/layer/segmentation/spatial_skeleton_commands.spec.ts index 2e0724b8e..3a4cfe69d 100644 --- a/src/layer/segmentation/spatial_skeleton_commands.spec.ts +++ b/src/layer/segmentation/spatial_skeleton_commands.spec.ts @@ -1247,7 +1247,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: makeDisplayState([originalSegmentId]), @@ -1386,7 +1387,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: { @@ -1437,7 +1439,7 @@ describe("spatial_skeleton_commands", () => { segmentId: originalSegmentId, }); - skeletonLayer.suppressBrowseSegment.mockClear(); + skeletonLayer.markSegmentEdited.mockClear(); deleteSegmentColor.mockClear(); layer.selectSpatialSkeletonNode.mockClear(); layer.markSpatialSkeletonNodeDataChanged.mockClear(); @@ -1458,7 +1460,7 @@ describe("spatial_skeleton_commands", () => { }), ); expect(deleteSegmentColor).toHaveBeenCalledWith(BigInt(splitSegmentId)); - expect(skeletonLayer.suppressBrowseSegment).toHaveBeenCalledWith( + expect(skeletonLayer.markSegmentEdited).toHaveBeenCalledWith( splitSegmentId, ); expect(layer.selectSpatialSkeletonNode).toHaveBeenCalledWith( @@ -1607,7 +1609,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: { @@ -1860,7 +1863,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: { @@ -2150,7 +2154,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: { @@ -2349,7 +2354,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: { @@ -2509,7 +2515,8 @@ describe("spatial_skeleton_commands", () => { source: skeletonSource, getNode: vi.fn((nodeId: number) => cacheByNode.get(nodeId)), invalidateSourceCellsForPositions: vi.fn(), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), }; const layer = { displayState: makeDisplayState([firstSegmentId, secondSegmentId]), @@ -2603,7 +2610,8 @@ describe("spatial_skeleton_commands", () => { if (nodeId === secondNode.nodeId) return secondNode; return undefined; }), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), invalidateSourceCellsForPositions: vi.fn(), }; const layer = { diff --git a/src/skeleton/frontend.spec.ts b/src/skeleton/frontend.spec.ts index d6670a78d..7b9aa56b3 100644 --- a/src/skeleton/frontend.spec.ts +++ b/src/skeleton/frontend.spec.ts @@ -299,19 +299,18 @@ describe("SpatiallyIndexedSkeletonLayer targeted source invalidation", () => { }); describe("SpatiallyIndexedSkeletonLayer browse exclusions", () => { - it("includes suppressed browse segments even when no overlay segment is loaded", () => { + it("includes edited segments in browse exclusions regardless of cache state", () => { const layer = Object.assign( Object.create(SpatiallyIndexedSkeletonLayer.prototype), { - suppressedBrowseSegmentIds: new Set(), + editedSegmentIds: new Set(), browseExcludedSegments: new Uint64Set(), browseExcludedSegmentsKey: undefined, redrawNeeded: { dispatch: vi.fn() }, - getLoadedOverlaySegmentIds: () => [], }, ); - expect(layer.suppressBrowseSegment(29)).toBe(true); + expect(layer.markSegmentEdited(29)).toBe(true); expect(layer.redrawNeeded.dispatch).toHaveBeenCalledTimes(1); const excludedSegments = (layer as any).getBrowsePassExcludedSegments(); diff --git a/src/skeleton/frontend.ts b/src/skeleton/frontend.ts index cf0e6e970..177ae1205 100644 --- a/src/skeleton/frontend.ts +++ b/src/skeleton/frontend.ts @@ -2071,7 +2071,7 @@ export class SpatiallyIndexedSkeletonLayer private browseExcludedSegments = new Uint64Set(); private gpuBrowseExcludedSegmentsHashTable: GPUHashTable; private browseExcludedSegmentsKey: string | undefined; - private suppressedBrowseSegmentIds = new Set(); + private readonly editedSegmentIds = new Set(); private retainedOverlaySegmentIds: number[] = []; private maxRetainedOverlaySegments: number; private readonly selectedNodeOutlineColor = vec3.clone( @@ -2159,6 +2159,7 @@ export class SpatiallyIndexedSkeletonLayer } retainOverlaySegment(segmentId: number) { + this.markSegmentEdited(segmentId); const nextRetainedOverlaySegmentIds = retainSpatiallyIndexedSkeletonOverlaySegment( this.retainedOverlaySegmentIds, @@ -2180,30 +2181,16 @@ export class SpatiallyIndexedSkeletonLayer return true; } - suppressBrowseSegment(segmentId: number) { + markSegmentEdited(segmentId: number) { const normalizedSegmentId = Math.round(Number(segmentId)); if ( !Number.isSafeInteger(normalizedSegmentId) || normalizedSegmentId <= 0 || - this.suppressedBrowseSegmentIds.has(normalizedSegmentId) + this.editedSegmentIds.has(normalizedSegmentId) ) { return false; } - this.suppressedBrowseSegmentIds.add(normalizedSegmentId); - this.redrawNeeded.dispatch(); - return true; - } - - unsuppressBrowseSegment(segmentId: number) { - const normalizedSegmentId = Math.round(Number(segmentId)); - if ( - !Number.isSafeInteger(normalizedSegmentId) || - normalizedSegmentId <= 0 || - !this.suppressedBrowseSegmentIds.has(normalizedSegmentId) - ) { - return false; - } - this.suppressedBrowseSegmentIds.delete(normalizedSegmentId); + this.editedSegmentIds.add(normalizedSegmentId); this.redrawNeeded.dispatch(); return true; } @@ -2215,41 +2202,8 @@ export class SpatiallyIndexedSkeletonLayer ); } - private getLoadedOverlaySegmentIds( - segmentIds: readonly number[] = this.getOverlayRenderSegmentIds(), - ) { - if (this.inspectionState === undefined) { - return []; - } - return segmentIds.filter( - (segmentId) => - this.inspectionState?.getCachedSegmentNodes(segmentId) !== undefined, - ); - } - private getNormalizedBrowsePassExcludedSegmentIds() { - const segmentIds = new Set(); - for (const segmentId of this.getLoadedOverlaySegmentIds()) { - const normalizedSegmentId = Math.round(Number(segmentId)); - if ( - !Number.isSafeInteger(normalizedSegmentId) || - normalizedSegmentId <= 0 - ) { - continue; - } - segmentIds.add(normalizedSegmentId); - } - for (const segmentId of this.suppressedBrowseSegmentIds) { - const normalizedSegmentId = Math.round(Number(segmentId)); - if ( - !Number.isSafeInteger(normalizedSegmentId) || - normalizedSegmentId <= 0 - ) { - continue; - } - segmentIds.add(normalizedSegmentId); - } - return [...segmentIds].sort((a, b) => a - b); + return [...this.editedSegmentIds].sort((a, b) => a - b); } private getBrowsePassExcludedSegments() { diff --git a/src/ui/skeleton_edit_tools.spec.ts b/src/ui/skeleton_edit_tools.spec.ts index a1e6b6c19..d84ce9941 100644 --- a/src/ui/skeleton_edit_tools.spec.ts +++ b/src/ui/skeleton_edit_tools.spec.ts @@ -564,7 +564,8 @@ describe("spatial_skeleton_edit_tool", () => { if (nodeId === secondNode.nodeId) return secondNode; return undefined; }), - suppressBrowseSegment: vi.fn(), + markSegmentEdited: vi.fn(), + retainOverlaySegment: vi.fn(), invalidateSourceCellsForPositions: vi.fn(), }; const commandHistory = new SpatialSkeletonCommandHistory(); @@ -634,7 +635,7 @@ describe("spatial_skeleton_edit_tool", () => { segmentId: 17, }); expect(deleteSegmentColor).toHaveBeenCalledWith(11n); - expect(skeletonLayer.suppressBrowseSegment).toHaveBeenCalledWith(11); + expect(skeletonLayer.markSegmentEdited).toHaveBeenCalledWith(11); expect(markSpatialSkeletonNodeDataChanged).toHaveBeenCalledWith({ invalidateFullSkeletonCache: false, }); diff --git a/src/ui/skeleton_edit_tools.ts b/src/ui/skeleton_edit_tools.ts index fe5e88f5f..234bc6189 100644 --- a/src/ui/skeleton_edit_tools.ts +++ b/src/ui/skeleton_edit_tools.ts @@ -1024,6 +1024,7 @@ export class SpatialSkeletonEditModeTool extends SpatialSkeletonToolBase { ) return; dragStarted = true; + skeletonLayer.markSegmentEdited(nodeInfo.segmentId); } dragPanel.translateDataPointByViewportPixels( this.dragGlobalPosition,