Skip to content

Commit 7d7dfb8

Browse files
sedghiwayfarer3130
andauthored
fix(segmentation): undoable overlap labelmap strokes, faster eager stroke resolution, brush hover guard (#2785)
* fix(segmentation): make overlap labelmap strokes fully undoable A stroke that forces a segment move to a private labelmap layer changed three things outside its history memo: the bulk voxel move, the layer registration, and the segment rebinding. Cross-layer overwrite erases were also unrecorded. Undo then restored only the stroke's own voxels, leaving the moved voxels stranded on a layer that should not exist at that point in history. Labelmap memos now carry ordered restore steps (priorSteps/postSteps): moveSegmentToPrivateLabelmap emits a step that reverses/replays the whole move, cross-layer erases are recorded per layer through crossLayerEraseCallback, and a mid-stroke voxel-manager swap rides the committed earlier memo along. Undo walks steps in reverse-chronological order, redo chronologically, so each stroke restores as one unit. * perf(segmentation): memoize labelmap image reference scans Every eager (non-lazy) brush or eraser drag event re-ran isReferenceViewable over ALL labelmap imageIds via updateLabelmapSegmentationImageReferences, each call doing full metadata and slice-basis resolution. On a two-layer, 120-image labelmap this cost about 51ms per drag event (about 19fps while erasing). The resolver now remembers completed scans per segmentation, keyed by the viewport's current image plus a fingerprint of the labelmap imageId set, and replays the stored mapping instead of rescanning. Layer additions or removals change the fingerprint and force a fresh scan; removeSegmentation and reset clear the memo. Same scenario now costs about 2ms per event. * fix(BrushTool): bail out of preMouseDown when hover data cannot be created preMouseDownCallback dereferenced this._hoverData.viewport without guarding the case where createHoverData returns undefined (no active segment index yet, or the viewport camera is not resolvable), crashing with 'Cannot read properties of undefined (reading viewport)'. Every other createHoverData call site already guards this; do the same here and undo the draw activation before bailing. * fix(tools): cache labelmap scan results by viewport * test(tools): assert viewport scan replay coverage --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
1 parent 30f3b60 commit 7d7dfb8

10 files changed

Lines changed: 1016 additions & 20 deletions

File tree

packages/tools/src/stateManagement/segmentation/labelmapModel/labelmapEditTransaction.ts

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,20 @@ import {
1313
getSegmentsOnLabelmap,
1414
} from './labelmapSegmentBindings';
1515
import { moveSegmentToPrivateLabelmap as defaultMoveSegmentToPrivateLabelmap } from './privateLabelmap';
16+
import type { LabelmapRestoreStep } from '../../../utilities/segmentation/createLabelmapMemo';
17+
18+
/** A batch of cross-layer erases performed by one strategy fill call, with
19+
* everything needed to reverse/replay it on undo/redo. */
20+
type CrossLayerEraseRecord = {
21+
voxelManager: Types.IVoxelManager<number>;
22+
labelValue: number;
23+
indices: number[];
24+
};
1625

1726
type MoveSegmentToPrivateLabelmap = (
1827
segmentation: Segmentation,
19-
segmentIndex: number
28+
segmentIndex: number,
29+
options?: { moveStepCallback?: (step: LabelmapRestoreStep) => void }
2030
) => LabelmapLayer | undefined;
2131

2232
type BeginLabelmapEditTransactionOptions = {
@@ -39,6 +49,10 @@ type LabelmapEditTransaction = {
3949
protectedSegmentIndices: number[];
4050
crossLayerEraseBindings: SegmentLabelmapBindingState[];
4151
movedSegment: boolean;
52+
/** Undo/redo step for a segment move performed by this transaction
53+
* (layer registration + bulk voxel move + binding change), for recording
54+
* on the stroke's memo. */
55+
moveStep?: LabelmapRestoreStep;
4256
};
4357

4458
type ResolveLabelmapLayerEditTargetOptions = {
@@ -62,6 +76,9 @@ type EraseLabelmapEditTransactionOptions = {
6276
isInObject: (point: Types.Point3) => boolean;
6377
isInObjectBoundsIJK?: Types.BoundsIJK;
6478
imageId?: string;
79+
/** When provided, receives the erased voxels of each touched layer so the
80+
* caller can record them for undo/redo. */
81+
crossLayerEraseCallback?: (record: CrossLayerEraseRecord) => void;
6582
};
6683

6784
function getProtectedSegmentIndicesForLayer(
@@ -197,13 +214,16 @@ function beginLabelmapEditTransaction(
197214
}
198215
);
199216

217+
let moveStep: LabelmapRestoreStep | undefined;
218+
200219
if (shouldMoveSegment) {
201220
const moveSegmentToPrivateLabelmap =
202221
options.moveSegmentToPrivateLabelmap ??
203222
defaultMoveSegmentToPrivateLabelmap;
204223
const privateLayer = moveSegmentToPrivateLabelmap(
205224
segmentation,
206-
segmentIndex
225+
segmentIndex,
226+
{ moveStepCallback: (step) => (moveStep = step) }
207227
);
208228
const privateBinding = getSegmentBinding(segmentation, segmentIndex);
209229

@@ -234,6 +254,7 @@ function beginLabelmapEditTransaction(
234254
overwriteSegmentIndices
235255
),
236256
movedSegment,
257+
moveStep,
237258
};
238259
}
239260

@@ -329,6 +350,7 @@ function eraseVolumeLayer(
329350
return;
330351
}
331352

353+
const erasedIndices: number[] = [];
332354
volume.voxelManager.forEach(
333355
({ value, index, pointIJK }) => {
334356
if (value !== binding.labelValue) {
@@ -343,13 +365,22 @@ function eraseVolumeLayer(
343365
}
344366

345367
volume.voxelManager.setAtIndex(index, 0);
368+
erasedIndices.push(index);
346369
},
347370
{
348371
imageData: volume.imageData,
349372
boundsIJK: options.isInObjectBoundsIJK,
350373
}
351374
);
352375

376+
if (erasedIndices.length) {
377+
options.crossLayerEraseCallback?.({
378+
voxelManager: volume.voxelManager as Types.IVoxelManager<number>,
379+
labelValue: binding.labelValue,
380+
indices: erasedIndices,
381+
});
382+
}
383+
353384
volume.voxelManager
354385
?.getArrayOfModifiedSlices?.()
355386
?.forEach((sliceIndex) => modifiedSlices.add(sliceIndex));
@@ -373,6 +404,7 @@ function eraseStackLayer(
373404
return;
374405
}
375406

407+
const erasedIndices: number[] = [];
376408
voxelManager.forEach(
377409
({ value, index, pointIJK }) => {
378410
if (value !== binding.labelValue) {
@@ -387,13 +419,22 @@ function eraseStackLayer(
387419
}
388420

389421
voxelManager.setAtIndex(index, 0);
422+
erasedIndices.push(index);
390423
},
391424
{
392425
imageData: options.referenceImageData,
393426
boundsIJK: options.isInObjectBoundsIJK,
394427
}
395428
);
396429

430+
if (erasedIndices.length) {
431+
options.crossLayerEraseCallback?.({
432+
voxelManager,
433+
labelValue: binding.labelValue,
434+
indices: erasedIndices,
435+
});
436+
}
437+
397438
const currentSlice = stackViewport.getCurrentImageIdIndex?.();
398439
if (typeof currentSlice === 'number') {
399440
modifiedSlices.add(currentSlice);
@@ -440,6 +481,7 @@ export {
440481
};
441482
export type {
442483
BeginLabelmapEditTransactionOptions,
484+
CrossLayerEraseRecord,
443485
EraseLabelmapEditTransactionOptions,
444486
LabelmapEditTransaction,
445487
LabelmapLayerEditTarget,

packages/tools/src/stateManagement/segmentation/labelmapModel/labelmapImageReferenceResolver.spec.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,41 @@ describe('LabelmapImageReferenceResolver', () => {
215215
);
216216
expect(result).toBe('lm-a-0');
217217
});
218+
219+
it('replays the cached result for the matching viewport scan key', () => {
220+
const segmentation = makeSegmentation();
221+
getSegmentation.mockReturnValue(segmentation);
222+
223+
const viewport1 = makeStackViewport({
224+
id: 'vp-1',
225+
getCurrentImageId: jest.fn(() => 'ref-0'),
226+
isReferenceViewable: jest.fn(
227+
(ref) => ref.referencedImageId === 'lm-a-0'
228+
),
229+
});
230+
const viewport2 = makeStackViewport({
231+
id: 'vp-2',
232+
getCurrentImageId: jest.fn(() => 'ref-0'),
233+
isReferenceViewable: jest.fn(
234+
(ref) => ref.referencedImageId === 'lm-a-1'
235+
),
236+
});
237+
getEnabledElementByViewportId.mockImplementation((viewportId) => ({
238+
viewport: viewportId === 'vp-1' ? viewport1 : viewport2,
239+
}));
240+
241+
expect(
242+
resolver.updateLabelmapSegmentationImageReferences('vp-1', 'seg-1')
243+
).toBe('lm-a-0');
244+
expect(
245+
resolver.updateLabelmapSegmentationImageReferences('vp-2', 'seg-1')
246+
).toBe('lm-a-1');
247+
expect(
248+
resolver.updateLabelmapSegmentationImageReferences('vp-1', 'seg-1')
249+
).toBe('lm-a-0');
250+
expect(viewport1.isReferenceViewable).toHaveBeenCalledTimes(2);
251+
expect(viewport2.isReferenceViewable).toHaveBeenCalledTimes(2);
252+
});
218253
});
219254

220255
describe('getCurrentLabelmapImageIdsForViewport', () => {

packages/tools/src/stateManagement/segmentation/labelmapModel/labelmapImageReferenceResolver.ts

Lines changed: 87 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,17 @@ class LabelmapImageReferenceResolver {
3131
// labelmapImageIdReferenceMap, so removeSegmentation can purge entries in
3232
// O(k) without scanning the whole map.
3333
private readonly keysBySegmentationId = new Map<string, Set<string>>();
34+
// Memo of already-performed isReferenceViewable scans. Each scan resolves
35+
// every labelmap imageId against the viewport's current image via full
36+
// metadata resolution (image plane, slice basis), which costs tens of ms
37+
// for multi-layer labelmaps - and the segmentation data-modified pipeline
38+
// re-enters this on EVERY brush/erase drag event. Scan keys include the
39+
// viewport id and a signature of the labelmap imageId set, so adding/removing
40+
// a layer (which changes the set) naturally forces a fresh scan.
41+
private readonly completedScanResultsBySegmentationId = new Map<
42+
string,
43+
Map<string, string | undefined>
44+
>();
3445

3546
constructor(getSegmentation: GetSegmentation) {
3647
this.getSegmentation = getSegmentation;
@@ -40,10 +51,12 @@ class LabelmapImageReferenceResolver {
4051
this.stackLabelmapImageIdReferenceMap.clear();
4152
this.labelmapImageIdReferenceMap.clear();
4253
this.keysBySegmentationId.clear();
54+
this.completedScanResultsBySegmentationId.clear();
4355
}
4456

4557
removeSegmentation(segmentationId: string): void {
4658
this.stackLabelmapImageIdReferenceMap.delete(segmentationId);
59+
this.completedScanResultsBySegmentationId.delete(segmentationId);
4760
const keys = this.keysBySegmentationId.get(segmentationId);
4861
if (keys) {
4962
for (const key of keys) {
@@ -142,11 +155,71 @@ class LabelmapImageReferenceResolver {
142155
return;
143156
}
144157

145-
return this.updateLabelmapSegmentationReferences(
158+
const viewport = enabledElement.viewport as Types.IStackViewport;
159+
const scanKey = this.generateScanKey('current', viewport, labelmapImageIds);
160+
if (scanKey && this.hasCompletedScan(segmentationId, scanKey)) {
161+
return this.getCompletedScanResult(segmentationId, scanKey);
162+
}
163+
164+
const result = this.updateLabelmapSegmentationReferences(
146165
segmentationId,
147-
enabledElement.viewport as Types.IStackViewport,
166+
viewport,
148167
labelmapImageIds
149168
);
169+
170+
if (scanKey) {
171+
this.markCompletedScan(segmentationId, scanKey, result);
172+
}
173+
174+
return result;
175+
}
176+
177+
/** Signature of one isReferenceViewable scan: the viewport's current image
178+
* plus a cheap fingerprint of the labelmap imageId set (count + endpoints),
179+
* so layer additions/removals invalidate naturally. */
180+
private generateScanKey(
181+
kind: 'current' | 'all',
182+
viewport: Types.IStackViewport,
183+
labelmapImageIds: string[]
184+
): string | undefined {
185+
const referenceImageId =
186+
typeof viewport.getCurrentImageId === 'function'
187+
? viewport.getCurrentImageId()
188+
: undefined;
189+
if (!referenceImageId) {
190+
return;
191+
}
192+
return `${kind}|${viewport.id}|${referenceImageId}|${
193+
labelmapImageIds.length
194+
}|${labelmapImageIds[0]}|${labelmapImageIds[labelmapImageIds.length - 1]}`;
195+
}
196+
197+
private hasCompletedScan(segmentationId: string, scanKey: string): boolean {
198+
return !!this.completedScanResultsBySegmentationId
199+
.get(segmentationId)
200+
?.has(scanKey);
201+
}
202+
203+
private getCompletedScanResult(
204+
segmentationId: string,
205+
scanKey: string
206+
): string | undefined {
207+
return this.completedScanResultsBySegmentationId
208+
.get(segmentationId)
209+
?.get(scanKey);
210+
}
211+
212+
private markCompletedScan(
213+
segmentationId: string,
214+
scanKey: string,
215+
result?: string
216+
): void {
217+
let results = this.completedScanResultsBySegmentationId.get(segmentationId);
218+
if (!results) {
219+
results = new Map();
220+
this.completedScanResultsBySegmentationId.set(segmentationId, results);
221+
}
222+
results.set(scanKey, result);
150223
}
151224

152225
getCurrentLabelmapImageIdsForViewport(
@@ -382,6 +455,18 @@ class LabelmapImageReferenceResolver {
382455

383456
const stackViewport = enabledElement.viewport as Types.IStackViewport;
384457

458+
const scanKey = this.generateScanKey(
459+
'all',
460+
stackViewport,
461+
labelmapImageIds
462+
);
463+
if (scanKey && this.hasCompletedScan(segmentationId, scanKey)) {
464+
return;
465+
}
466+
if (scanKey) {
467+
this.markCompletedScan(segmentationId, scanKey);
468+
}
469+
385470
this.updateLabelmapSegmentationReferences(
386471
segmentationId,
387472
stackViewport,

0 commit comments

Comments
 (0)