From bba665ddec9fc5a6744fca5ff91f1033c5842ff4 Mon Sep 17 00:00:00 2001 From: Sean Martin Date: Mon, 8 Jun 2026 17:21:35 +0200 Subject: [PATCH 1/2] fix: correct highlight logic --- src/layer/segmentation/index.ts | 12 ++++++++++++ src/skeleton/frontend.ts | 14 +++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/layer/segmentation/index.ts b/src/layer/segmentation/index.ts index 92dae493e..1eb97aaa9 100644 --- a/src/layer/segmentation/index.ts +++ b/src/layer/segmentation/index.ts @@ -799,6 +799,9 @@ export class SegmentationUserLayer extends Base { readonly selectedSpatialSkeletonNodeId = new WatchableValue< number | undefined >(undefined); + readonly selectedSpatialSkeletonNodeSegmentId = new WatchableValue< + number | undefined + >(undefined); readonly selectedSpatialSkeletonNodeInfo = new WatchableValue< SelectedSpatialSkeletonNodeInfo | undefined >(undefined); @@ -931,6 +934,7 @@ export class SegmentationUserLayer extends Base { position: copyOptionalSpatialSkeletonPosition(selectedNodePosition), sourceState, }; + this.selectedSpatialSkeletonNodeSegmentId.value = segmentId; this.captureSpatialSkeletonSelectionState( (state) => { state.nodeId = normalizedNodeId.toString(); @@ -1030,6 +1034,7 @@ export class SegmentationUserLayer extends Base { pin: boolean | "toggle" | "force-unpin" = false, ) => { this.selectedSpatialSkeletonNodeInfo.value = undefined; + this.selectedSpatialSkeletonNodeSegmentId.value = undefined; this.captureSpatialSkeletonSelectionState((state) => { state.nodeId = undefined; state.value = undefined; @@ -1099,6 +1104,9 @@ export class SegmentationUserLayer extends Base { if (this.selectedSpatialSkeletonNodeId.value !== nextSelectedNodeId) { this.selectedSpatialSkeletonNodeId.value = nextSelectedNodeId; } + if (this.selectedSpatialSkeletonNodeSegmentId.value !== nextSelectedSegmentId) { + this.selectedSpatialSkeletonNodeSegmentId.value = nextSelectedSegmentId; + } const selectedNodeInfo = this.selectedSpatialSkeletonNodeInfo.value; if ( selectedNodeInfo !== undefined && @@ -1599,6 +1607,8 @@ export class SegmentationUserLayer extends Base { { sources2d: slicePanelSources, selectedNodeId: this.selectedSpatialSkeletonNodeId, + selectedNodeSegmentId: + this.selectedSpatialSkeletonNodeSegmentId, pendingNodePositionVersion: this.spatialSkeletonState.pendingNodePositionVersion, getPendingNodePosition: (nodeId) => @@ -1632,6 +1642,8 @@ export class SegmentationUserLayer extends Base { displayState, { selectedNodeId: this.selectedSpatialSkeletonNodeId, + selectedNodeSegmentId: + this.selectedSpatialSkeletonNodeSegmentId, pendingNodePositionVersion: this.spatialSkeletonState.pendingNodePositionVersion, getPendingNodePosition: (nodeId) => diff --git a/src/skeleton/frontend.ts b/src/skeleton/frontend.ts index 24c772ac8..bee21eefa 100644 --- a/src/skeleton/frontend.ts +++ b/src/skeleton/frontend.ts @@ -1820,6 +1820,7 @@ type SpatiallyIndexedSkeletonSourceEntry = interface SpatiallyIndexedSkeletonLayerOptions { sources2d?: SpatiallyIndexedSkeletonSourceEntry[]; selectedNodeId?: WatchableValueInterface; + selectedNodeSegmentId?: WatchableValueInterface; pendingNodePositionVersion?: WatchableValueInterface; getPendingNodePosition?: (nodeId: number) => ArrayLike | undefined; getCachedNode?: (nodeId: number) => SpatiallyIndexedSkeletonNode | undefined; @@ -2049,6 +2050,9 @@ export class SpatiallyIndexedSkeletonLayer private selectedNodeId: | WatchableValueInterface | undefined; + private selectedNodeSegmentId: + | WatchableValueInterface + | undefined; private pendingNodePositionVersion: | WatchableValueInterface | undefined; @@ -2137,7 +2141,14 @@ export class SpatiallyIndexedSkeletonLayer if (isCacheValid) { return this.selectedNodeOutlineColor; } - const segmentId = this.displayState.segmentSelectionState.baseValue; + // Prefer the explicitly-tracked segment ID (correct for all selection paths: + // picking, tab, and history navigation). Fall back to the hover-driven + // segmentSelectionState only if no tracked segment is available. + const trackedSegmentId = this.selectedNodeSegmentId?.value; + const segmentId = + trackedSegmentId !== undefined + ? BigInt(trackedSegmentId) + : this.displayState.segmentSelectionState.baseValue; if (segmentId === undefined) { return SELECTED_NODE_OUTLINE_FALLBACK_COLOR; } @@ -2380,6 +2391,7 @@ export class SpatiallyIndexedSkeletonLayer ), ); this.selectedNodeId = options.selectedNodeId; + this.selectedNodeSegmentId = options.selectedNodeSegmentId; this.pendingNodePositionVersion = options.pendingNodePositionVersion; this.getPendingNodePositionOverride = options.getPendingNodePosition; this.getCachedNodeInfo = options.getCachedNode; From f1a539e04912d1092091042b7f1afb32baf0084b Mon Sep 17 00:00:00 2001 From: Sean Martin Date: Mon, 8 Jun 2026 17:41:14 +0200 Subject: [PATCH 2/2] refactor: directly use the node info instead of duping we were prevouisly duplicating because the two had different lifetimes. Now we just try make node info have the full desired behaviour instead. --- src/layer/segmentation/index.ts | 44 ++++++++++++------------------ src/skeleton/frontend.ts | 44 ++++++++++++------------------ src/ui/skeleton_edit_tools.spec.ts | 16 ++++------- src/ui/skeleton_edit_tools.ts | 18 ++++++------ src/ui/skeleton_tab.ts | 8 +++--- 5 files changed, 54 insertions(+), 76 deletions(-) diff --git a/src/layer/segmentation/index.ts b/src/layer/segmentation/index.ts index 1eb97aaa9..70c602250 100644 --- a/src/layer/segmentation/index.ts +++ b/src/layer/segmentation/index.ts @@ -796,12 +796,6 @@ export class SegmentationUserLayer extends Base { readonly spatialSkeletonState = this.registerDisposer( new SpatialSkeletonState(), ); - readonly selectedSpatialSkeletonNodeId = new WatchableValue< - number | undefined - >(undefined); - readonly selectedSpatialSkeletonNodeSegmentId = new WatchableValue< - number | undefined - >(undefined); readonly selectedSpatialSkeletonNodeInfo = new WatchableValue< SelectedSpatialSkeletonNodeInfo | undefined >(undefined); @@ -934,7 +928,6 @@ export class SegmentationUserLayer extends Base { position: copyOptionalSpatialSkeletonPosition(selectedNodePosition), sourceState, }; - this.selectedSpatialSkeletonNodeSegmentId.value = segmentId; this.captureSpatialSkeletonSelectionState( (state) => { state.nodeId = normalizedNodeId.toString(); @@ -998,7 +991,7 @@ export class SegmentationUserLayer extends Base { }; ensureSpatialSkeletonInspectionFromSelection = () => { - const selectedNodeId = this.selectedSpatialSkeletonNodeId.value; + const selectedNodeId = this.selectedSpatialSkeletonNodeInfo.value?.nodeId; const selectedNode = selectedNodeId === undefined ? undefined @@ -1034,7 +1027,6 @@ export class SegmentationUserLayer extends Base { pin: boolean | "toggle" | "force-unpin" = false, ) => { this.selectedSpatialSkeletonNodeInfo.value = undefined; - this.selectedSpatialSkeletonNodeSegmentId.value = undefined; this.captureSpatialSkeletonSelectionState((state) => { state.nodeId = undefined; state.value = undefined; @@ -1101,19 +1093,23 @@ export class SegmentationUserLayer extends Base { const nextSelectedSegmentId = getSegmentIdFromLayerSelectionValue( nextLayerSelectionState, ); - if (this.selectedSpatialSkeletonNodeId.value !== nextSelectedNodeId) { - this.selectedSpatialSkeletonNodeId.value = nextSelectedNodeId; - } - if (this.selectedSpatialSkeletonNodeSegmentId.value !== nextSelectedSegmentId) { - this.selectedSpatialSkeletonNodeSegmentId.value = nextSelectedSegmentId; - } const selectedNodeInfo = this.selectedSpatialSkeletonNodeInfo.value; - if ( - selectedNodeInfo !== undefined && - (selectedNodeInfo.nodeId !== nextSelectedNodeId || - selectedNodeInfo.segmentId !== nextSelectedSegmentId) + if (nextSelectedNodeId === undefined) { + if (selectedNodeInfo !== undefined) { + this.selectedSpatialSkeletonNodeInfo.value = undefined; + } + } else if ( + selectedNodeInfo?.nodeId !== nextSelectedNodeId || + selectedNodeInfo?.segmentId !== nextSelectedSegmentId ) { - this.selectedSpatialSkeletonNodeInfo.value = undefined; + // Preserve rich info (position, sourceState) when only the segment ID + // changed for the same node; otherwise replace with minimal state so + // the render layer always has a valid nodeId+segmentId even after + // history navigation where we have no position or sourceState. + this.selectedSpatialSkeletonNodeInfo.value = + selectedNodeInfo?.nodeId === nextSelectedNodeId + ? { ...selectedNodeInfo, segmentId: nextSelectedSegmentId } + : { nodeId: nextSelectedNodeId, segmentId: nextSelectedSegmentId }; } }; this.registerDisposer( @@ -1606,9 +1602,7 @@ export class SegmentationUserLayer extends Base { displayState, { sources2d: slicePanelSources, - selectedNodeId: this.selectedSpatialSkeletonNodeId, - selectedNodeSegmentId: - this.selectedSpatialSkeletonNodeSegmentId, + selectedNodeInfo: this.selectedSpatialSkeletonNodeInfo, pendingNodePositionVersion: this.spatialSkeletonState.pendingNodePositionVersion, getPendingNodePosition: (nodeId) => @@ -1641,9 +1635,7 @@ export class SegmentationUserLayer extends Base { mesh, displayState, { - selectedNodeId: this.selectedSpatialSkeletonNodeId, - selectedNodeSegmentId: - this.selectedSpatialSkeletonNodeSegmentId, + selectedNodeInfo: this.selectedSpatialSkeletonNodeInfo, pendingNodePositionVersion: this.spatialSkeletonState.pendingNodePositionVersion, getPendingNodePosition: (nodeId) => diff --git a/src/skeleton/frontend.ts b/src/skeleton/frontend.ts index bee21eefa..6d850f128 100644 --- a/src/skeleton/frontend.ts +++ b/src/skeleton/frontend.ts @@ -1817,10 +1817,14 @@ export abstract class MultiscaleSpatiallyIndexedSkeletonSource extends Multiscal type SpatiallyIndexedSkeletonSourceEntry = SliceViewSingleResolutionSource; +interface SelectedSkeletonNodeInfo { + readonly nodeId: number; + readonly segmentId?: number; +} + interface SpatiallyIndexedSkeletonLayerOptions { sources2d?: SpatiallyIndexedSkeletonSourceEntry[]; - selectedNodeId?: WatchableValueInterface; - selectedNodeSegmentId?: WatchableValueInterface; + selectedNodeInfo?: WatchableValueInterface; pendingNodePositionVersion?: WatchableValueInterface; getPendingNodePosition?: (nodeId: number) => ArrayLike | undefined; getCachedNode?: (nodeId: number) => SpatiallyIndexedSkeletonNode | undefined; @@ -2047,11 +2051,8 @@ export class SpatiallyIndexedSkeletonLayer computeTextureFormat(new TextureFormat(), dataType, numComponents), )); } - private selectedNodeId: - | WatchableValueInterface - | undefined; - private selectedNodeSegmentId: - | WatchableValueInterface + private selectedNodeInfo: + | WatchableValueInterface | undefined; private pendingNodePositionVersion: | WatchableValueInterface @@ -2131,28 +2132,21 @@ export class SpatiallyIndexedSkeletonLayer } private getSelectedNodeOutlineColor() { - const selectedNodeId = this.selectedNodeId?.value; - if (selectedNodeId === undefined) { + const nodeInfo = this.selectedNodeInfo?.value; + if (nodeInfo === undefined) { return SELECTED_NODE_OUTLINE_FALLBACK_COLOR; } const currentGeneration = this.selectedNodeOutlineColorGeneration; - const isCacheValid = - this.cachedSelectedNodeOutlineColorGeneration === currentGeneration; - if (isCacheValid) { + if (this.cachedSelectedNodeOutlineColorGeneration === currentGeneration) { return this.selectedNodeOutlineColor; } - // Prefer the explicitly-tracked segment ID (correct for all selection paths: - // picking, tab, and history navigation). Fall back to the hover-driven - // segmentSelectionState only if no tracked segment is available. - const trackedSegmentId = this.selectedNodeSegmentId?.value; const segmentId = - trackedSegmentId !== undefined - ? BigInt(trackedSegmentId) + nodeInfo.segmentId !== undefined + ? BigInt(nodeInfo.segmentId) : this.displayState.segmentSelectionState.baseValue; if (segmentId === undefined) { return SELECTED_NODE_OUTLINE_FALLBACK_COLOR; } - this.cachedSelectedNodeOutlineColorGeneration = currentGeneration; return computeHighVisibilityContrastColor( this.selectedNodeOutlineColor, @@ -2390,8 +2384,7 @@ export class SpatiallyIndexedSkeletonLayer this.displayState.transform, ), ); - this.selectedNodeId = options.selectedNodeId; - this.selectedNodeSegmentId = options.selectedNodeSegmentId; + this.selectedNodeInfo = options.selectedNodeInfo; this.pendingNodePositionVersion = options.pendingNodePositionVersion; this.getPendingNodePositionOverride = options.getPendingNodePosition; this.getCachedNodeInfo = options.getCachedNode; @@ -2499,10 +2492,9 @@ export class SpatiallyIndexedSkeletonLayer ); this.nodeIdAttributeIndex = nodeIdIndex >= 0 ? nodeIdIndex : undefined; const requestRedraw = () => this.redrawNeeded.dispatch(); - const selectedNodeWatchable = this.selectedNodeId; - if (selectedNodeWatchable?.changed) { + if (this.selectedNodeInfo?.changed) { this.registerDisposer( - selectedNodeWatchable.changed.add(() => { + this.selectedNodeInfo.changed.add(() => { invalidateSelectedNodeOutlineColor(); requestRedraw(); }), @@ -2940,7 +2932,7 @@ export class SpatiallyIndexedSkeletonLayer ); gl.uniform1i( nodeShader.uniform("uSelectedNodeId"), - this.selectedNodeId?.value ?? -1, + this.selectedNodeInfo?.value?.nodeId ?? -1, ); const chunkOrigin = vec3.create(); @@ -3063,7 +3055,7 @@ export class SpatiallyIndexedSkeletonLayer ); gl.uniform1i( nodeShader.uniform("uSelectedNodeId"), - this.selectedNodeId?.value ?? -1, + this.selectedNodeInfo?.value?.nodeId ?? -1, ); if (renderContext.emitPickID) { diff --git a/src/ui/skeleton_edit_tools.spec.ts b/src/ui/skeleton_edit_tools.spec.ts index eb132a76c..a1e6b6c19 100644 --- a/src/ui/skeleton_edit_tools.spec.ts +++ b/src/ui/skeleton_edit_tools.spec.ts @@ -664,7 +664,7 @@ describe("spatial_skeleton_edit_tool", () => { }; const tool = { layer: { - selectedSpatialSkeletonNodeId: { value: undefined }, + selectedSpatialSkeletonNodeInfo: { value: undefined }, spatialSkeletonState: { mergeAnchorNodeId: { value: 101 }, }, @@ -736,11 +736,7 @@ describe("spatial_skeleton_edit_tool", () => { }, }, spatialSkeletonMergeMode: makeModeWatchable(), - selectedSpatialSkeletonNodeId: { - value: selectedNode.nodeId, - changed: makeChangedSignal(), - }, - selectedSpatialSkeletonNodeInfo: { value: selectedNode }, + selectedSpatialSkeletonNodeInfo: { value: selectedNode, changed: makeChangedSignal() }, spatialSkeletonState: { mergeAnchorNodeId, getCachedNode: vi.fn(), @@ -834,11 +830,10 @@ describe("spatial_skeleton_edit_tool", () => { }, }, spatialSkeletonMergeMode: makeModeWatchable(), - selectedSpatialSkeletonNodeId: { - value: selectedNode.nodeId as number | undefined, + selectedSpatialSkeletonNodeInfo: { + value: selectedNode as typeof selectedNode | undefined, changed: selectedNodeChanged, }, - selectedSpatialSkeletonNodeInfo: { value: selectedNode }, spatialSkeletonState: { mergeAnchorNodeId, getCachedNode: vi.fn(), @@ -884,7 +879,7 @@ describe("spatial_skeleton_edit_tool", () => { metaKey: false, }, }); - layer.selectedSpatialSkeletonNodeId.value = undefined; + layer.selectedSpatialSkeletonNodeInfo.value = undefined; selectedNodeChanged.dispatch(); expect(selectSegment).toHaveBeenCalledWith(17n, true); @@ -924,7 +919,6 @@ describe("spatial_skeleton_edit_tool", () => { }, }, spatialSkeletonSplitMode: makeModeWatchable(), - selectedSpatialSkeletonNodeId: { value: selectedNode.nodeId }, selectedSpatialSkeletonNodeInfo: { value: selectedNode }, spatialSkeletonState: { commandHistory: new SpatialSkeletonCommandHistory(), diff --git a/src/ui/skeleton_edit_tools.ts b/src/ui/skeleton_edit_tools.ts index 1b90f7252..2f6fa8048 100644 --- a/src/ui/skeleton_edit_tools.ts +++ b/src/ui/skeleton_edit_tools.ts @@ -286,7 +286,7 @@ abstract class SpatialSkeletonToolBase extends LayerTool const isVisible = this.isSpatialSkeletonSegmentVisible(pickedSegmentId); if (isVisible) { this.removeVisibleSegmentByNumber(pickedSegmentId, { deselect: true }); - const selectedNodeId = this.layer.selectedSpatialSkeletonNodeId.value; + const selectedNodeId = this.layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; const selectedNode = selectedNodeId === undefined ? undefined @@ -421,7 +421,7 @@ abstract class SpatialSkeletonToolBase extends LayerTool sourceState?: SpatialSkeletonSourceState; } | undefined { - const nodeId = this.layer.selectedSpatialSkeletonNodeId.value; + const nodeId = this.layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; if ( typeof nodeId !== "number" || !Number.isSafeInteger(nodeId) || @@ -450,7 +450,7 @@ abstract class SpatialSkeletonToolBase extends LayerTool } protected getSelectedSpatialSkeletonNodeSummary() { - const nodeId = this.layer.selectedSpatialSkeletonNodeId.value; + const nodeId = this.layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; if (nodeId === undefined) { return undefined; } @@ -533,7 +533,7 @@ abstract class SpatialSkeletonToolBase extends LayerTool event.detail.preventDefault(); const pinnedSelection = this.layer.manager.root.selectionState.value; const hasSpatialSkeletonSelection = - this.layer.selectedSpatialSkeletonNodeId.value !== undefined || + this.layer.selectedSpatialSkeletonNodeInfo.value?.nodeId !== undefined || (pinnedSelection?.layers.some( ({ layer, state }) => layer === this.layer && hasSpatialSkeletonNodeSelection(state), @@ -807,7 +807,7 @@ export class SpatialSkeletonEditModeTool extends SpatialSkeletonToolBase { layer.spatialSkeletonState.clearPendingNodePositions(); }); activation.registerDisposer( - layer.selectedSpatialSkeletonNodeId.changed.add(renderStatus), + layer.selectedSpatialSkeletonNodeInfo.changed.add(renderStatus), ); activation.registerDisposer( layer.manager.root.selectionState.changed.add(renderStatus), @@ -862,7 +862,7 @@ export class SpatialSkeletonEditModeTool extends SpatialSkeletonToolBase { ); return; } - const selectedParentNodeId = layer.selectedSpatialSkeletonNodeId.value; + const selectedParentNodeId = layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; const addNodeBlockedReason = this.getAddNodeBlockedReason( skeletonLayer, selectedParentNodeId, @@ -902,7 +902,7 @@ export class SpatialSkeletonEditModeTool extends SpatialSkeletonToolBase { return; } const selectedParentNodeId = - layer.selectedSpatialSkeletonNodeId.value; + layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; const addNodeBlockedReason = this.getAddNodeBlockedReason( skeletonLayer, selectedParentNodeId, @@ -1196,9 +1196,9 @@ export class SpatialSkeletonMergeModeTool extends SpatialSkeletonToolBase { ), ); activation.registerDisposer( - this.layer.selectedSpatialSkeletonNodeId.changed.add(() => { + this.layer.selectedSpatialSkeletonNodeInfo.changed.add(() => { if ( - this.layer.selectedSpatialSkeletonNodeId.value === undefined && + this.layer.selectedSpatialSkeletonNodeInfo.value?.nodeId === undefined && this.layer.spatialSkeletonState.mergeAnchorNodeId.value !== undefined ) { anchorSelection = undefined; diff --git a/src/ui/skeleton_tab.ts b/src/ui/skeleton_tab.ts index be24369f3..337387bd3 100644 --- a/src/ui/skeleton_tab.ts +++ b/src/ui/skeleton_tab.ts @@ -551,7 +551,7 @@ export class SpatialSkeletonEditTab extends Tab { const applyRowInteractionState = ( options: { scrollSelectedIntoView?: boolean } = {}, ) => { - const selectedNodeId = layer.selectedSpatialSkeletonNodeId.value; + const selectedNodeId = layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; nodesList.forEachRenderedItem((entry, index) => { const item = virtualItems[index]; if (item?.kind !== "node") return; @@ -706,7 +706,7 @@ export class SpatialSkeletonEditTab extends Tab { } // Extract selected node and segment ID - const selectedId = layer.selectedSpatialSkeletonNodeId.value; + const selectedId = layer.selectedSpatialSkeletonNodeInfo.value?.nodeId; const selectedNode = selectedId === undefined ? undefined @@ -1178,7 +1178,7 @@ export class SpatialSkeletonEditTab extends Tab { const entry = document.createElement("div"); entry.className = "neuroglancer-spatial-skeleton-tree-entry"; entry.dataset.selected = String( - node.nodeId === layer.selectedSpatialSkeletonNodeId.value, + node.nodeId === layer.selectedSpatialSkeletonNodeInfo.value?.nodeId, ); entry.dataset.viewerHovered = String(node.nodeId === hoveredViewerNodeId); entry.dataset.listHovered = String(node.nodeId === hoveredListNodeId); @@ -1717,7 +1717,7 @@ export class SpatialSkeletonEditTab extends Tab { }, layer.displayState.segmentationColorGroupState), ); this.registerDisposer( - layer.selectedSpatialSkeletonNodeId.changed.add(() => { + layer.selectedSpatialSkeletonNodeInfo.changed.add(() => { pendingScrollToSelectedNode = true; applyRowInteractionState({ scrollSelectedIntoView: true }); }),