From 2ab1f3d215674b7f8c5aac0474bd40526dd6484e Mon Sep 17 00:00:00 2001 From: Joeycho Date: Thu, 6 Aug 2026 22:50:46 +0200 Subject: [PATCH] fix(adapters): do not drop segmentation slices when merging sparse segment data `getSegmentData()` builds its result as a SPARSE array -- `segmentData[i]` is assigned only for images that actually hold voxels: if (hasWrittenSegmentationData) { segmentData[currentLabelMapImageIndex] = segmentationDataForImageId; } `compactMergeSegmentDataWithoutInformationLoss` then merged two of those arrays with `largerArray.forEach(...)`. `Array.prototype.forEach` SKIPS HOLES, so the merge only visited indices where `largerArray` had an element. When the existing layer is the longer of the two, `largerArray` IS that layer -- and every image where only the INCOMING segment had data was never visited, and was silently discarded. This surfaces when two segments do not collide in-plane (so they merge into one layer rather than being pushed as separate layers) and the incoming segment extends beyond the existing layer's populated index range. Observed on a 694-slice CT: segment A spanned images [375..525] (length 526), segment B spanned [241..453] (length 454). B merged into A's layer, `largerArray` became A, and B came back as only [375..453] -- 213 slices reduced to 79. Reloading a saved DICOM-SEG returned a truncated mask, and re-exporting wrote the truncation back, so the loss compounded on every save/reload cycle. Nothing raised an error. Note the survivor is always A.start..B.end regardless of how far B extends below it, so masks of different sizes collapse to the same size. Fixed by iterating the index RANGE instead of one array's populated entries. `checkHasOverlapping` iterates the same way but is CORRECT and is left unchanged: an overlap requires both arrays to be populated at the same index, so a hole can never be an overlap. The existing tests all use DENSE arrays -- `[undefined, [0, 1]]` holds an `undefined` ELEMENT, which `forEach` does visit -- so none of them exercised a real hole. Added a regression test that builds sparse arrays the way `getSegmentData()` does; it fails before this change and passes after, with the eight existing cases unaffected. --- .../Segmentation/compactMergeSegData.ts | 30 ++++++++++++----- .../adapters/test/compactMergeSegData.jest.js | 33 +++++++++++++++++++ 2 files changed, 55 insertions(+), 8 deletions(-) diff --git a/packages/adapters/src/adapters/Cornerstone3D/Segmentation/compactMergeSegData.ts b/packages/adapters/src/adapters/Cornerstone3D/Segmentation/compactMergeSegData.ts index 163cc3cf02..7862930224 100644 --- a/packages/adapters/src/adapters/Cornerstone3D/Segmentation/compactMergeSegData.ts +++ b/packages/adapters/src/adapters/Cornerstone3D/Segmentation/compactMergeSegData.ts @@ -46,20 +46,34 @@ export const compactMergeSegmentDataWithoutInformationLoss = ({ continue; } - largerArray.forEach((_, currentImageIndex) => { + // Iterate the index RANGE rather than one array's populated entries. + // + // getSegmentData() builds its result as a SPARSE array — segmentData[i] is assigned + // only for images that actually hold voxels — and Array.prototype.forEach SKIPS HOLES. + // Iterating `largerArray` therefore visited only the indices where that array had an + // element. When the existing layer is the longer one, `largerArray` IS the existing + // layer, so every image where only the INCOMING segment had data was never visited, + // and was silently dropped. + const mergeLength = Math.max( + currentTestedArray.length, + newSegmentData.length + ); + + for ( + let currentImageIndex = 0; + currentImageIndex < mergeLength; + currentImageIndex++ + ) { const originalImagePixelData = currentTestedArray[currentImageIndex]; const newImagePixelData = newSegmentData[currentImageIndex]; - if ( - (!originalImagePixelData && !newImagePixelData) || - !newImagePixelData - ) { - return; + if (!newImagePixelData) { + continue; } if (!originalImagePixelData) { currentTestedArray[currentImageIndex] = newImagePixelData; - return; + continue; } const mergedPixelData = originalImagePixelData.map( @@ -70,7 +84,7 @@ export const compactMergeSegmentDataWithoutInformationLoss = ({ ); currentTestedArray[currentImageIndex] = mergedPixelData; - }); + } return; } diff --git a/packages/adapters/test/compactMergeSegData.jest.js b/packages/adapters/test/compactMergeSegData.jest.js index 887ed480b9..cc5043c919 100644 --- a/packages/adapters/test/compactMergeSegData.jest.js +++ b/packages/adapters/test/compactMergeSegData.jest.js @@ -161,4 +161,37 @@ describe('compactMergeSegmentDataWithoutInformationLoss', () => { ], ]); }); + + // getSegmentData() assigns only the indices that hold voxels, so its result is a SPARSE + // array (real holes), not a dense one containing `undefined`. That distinction matters: + // Array.prototype.forEach visits an `undefined` ELEMENT but skips a HOLE. Every test above + // uses dense arrays, so none of them exercise this. + it('should keep new data at indices the existing layer does not span (sparse arrays)', () => { + // Existing layer holds images 3..4 — length 5, holes at 0..2. + const existingLayer = []; + existingLayer[3] = [1, 0]; + existingLayer[4] = [1, 0]; + + // New segment holds images 1..3 — length 4, so the EXISTING layer is the longer one + // and becomes `largerArray`. Indices 1 and 2 are outside the existing layer's + // populated set and used to be dropped. + const newSegmentData = []; + newSegmentData[1] = [0, 2]; + newSegmentData[2] = [0, 2]; + newSegmentData[3] = [0, 2]; + + const arrayOfSegmentData = [existingLayer]; + + compactMergeSegmentDataWithoutInformationLoss({ + arrayOfSegmentData, + newSegmentData, + }); + + // Merged into the single existing layer, keeping every image from both. + expect(arrayOfSegmentData).toHaveLength(1); + expect(arrayOfSegmentData[0][1]).toEqual([0, 2]); + expect(arrayOfSegmentData[0][2]).toEqual([0, 2]); + expect(arrayOfSegmentData[0][3]).toEqual([1, 2]); + expect(arrayOfSegmentData[0][4]).toEqual([1, 0]); + }); });