feat(metadata): framework-agnostic display-set splitting + DWI mixed-b-value W/L fix#2777
Closed
sedghi wants to merge 25 commits into
Closed
feat(metadata): framework-agnostic display-set splitting + DWI mixed-b-value W/L fix#2777sedghi wants to merge 25 commits into
sedghi wants to merge 25 commits into
Conversation
…ack and volume viewports (#2717) * fix: closed freehand stat value mismatch * fix: review comments * chore: renamed function * docs: fix MDX rendering issue in snapIndexBounds JSDoc * Fix view issue on stack so that the issue is testable --------- Co-authored-by: Bill Wallace <wayfarer3130@gmail.com>
- isVideoInstance: reuse shared videoUIDs (no MPEG2 misclassification) - buildSeriesInfo + default split rules: guard empty instance lists - numImageFrames: coerce NumberOfFrames with Number() - MetadataModuleType.displaySetModule: type as full IDisplaySet - createDisplaySetFromGroup: denylist reserved data keys - Viewport: extract shared mountDisplaySets template for the 5 overrides - re-export isWsiInstance; add JSDoc to classifiers/viewport-type helpers - demo toBaseImageId: normalize ?frame=N as well as &frame=N - tests for the above
Add the split -> create -> consume pipeline, driving a viewport via setDisplaySets, caching display sets in the metadata layer, the split-rule model (with the DWI mixedDimensionalityBValue worked example), and the reusable instance classifiers.
- GenericViewport: add getDisplaySets() to ViewportController + base impl (derived from live bindings), giving parity with the legacy viewport. - createDisplaySetFromGroup: fold splitNumber into the default displaySetInstanceUID so a split series (DWI) yields unique ids; the viewport displaySetId == the display set instance UID. - demo splitter passes splitNumber; displaySets example keys the registry on displaySetInstanceUID to exercise the convention. - legacy mountDisplaySets records only the mounted source (honest getDisplaySets); documents overlays as a non-breaking future addition. - rename genericViewportDataSet* -> genericViewportDisplaySet* across the registry/access layer, types, and call sites (beta, no alias).
Collapse the displaySetId / displaySetInstanceUID duality onto a single name, displaySetId: - metadata: rename IDisplaySet.displaySetInstanceUID -> displaySetId (and CreateDisplaySetFromGroup option, reserved keys, tests, docs). - generic viewport: rename the DataId type -> DisplaySetId; align the registry/access param names (dataId -> displaySetId). - keep dataId in the public, persisted ViewReference / ViewportDataReference (a generic data-ref pointer) and in loadECGWaveform (an imageId), per the identifier-surface-only scope.
- group buckets are now namespaced by rule and JSON-encoded, so different rules can't collide on a shared key and separator/undefined aliasing is gone (issues 1, 6). - groups are returned in deterministic (split-key sorted) order, making split identity / displaySetId suffixes stable across input ordering (3). - groupInstancesBySplitRules/splitImageIdsBySplitRules accept onUnmatched(d) so instances matching no rule are observable, not silently dropped (5). - updateSeriesInfo typed () => void (return was always ignored) (4). - default rules: singleImageModality split key is now series-scoped (7); comments document the instances[0] homogeneity assumption (2), the multiFrame SliceLocation guard (8); removed the redundant viewportTypes-only customAttributes now that rule-level viewportTypes is applied (9). - tests for rule namespacing, deterministic order, and onUnmatched.
The metadata page had grown to be mostly display-set content. Move it to a dedicated concepts/cornerstone-metadata/display-sets page (pipeline, driving a viewport, caching, split-rule model + DWI example, IDisplaySet model), leave a short pointer on the metadata index, and add it to the sidebar. Also folds in the split-engine guarantees (rule-namespaced keys, deterministic order, onUnmatched, updateSeriesInfo mutation).
Complete the in-progress rename of the SplitRule fields so the engine and the rules line up again: - ruleSelector -> matches (per-instance predicate) - splitKey (rule field) -> groupBy (bucket recipe) - keep updateSeriesInfo; revert the stray augmentSeriesInfo so the series-level hook actually fires The rename had been applied to types.ts and groupInstancesBySplitRules.ts but not to defaultDisplaySetSplitRules.ts/tests, so every rule fell back to a catch-all grouped by SeriesInstanceUID and the series flags were never set - silently disabling the DWI mixed-b-value split. Updates the rules, tests, and docs to the new names.
…ies hook
Series-level facts used to be contributed by a rule's `updateSeriesInfo`
hook, which mutated a shared `seriesInfo` object that `matches`/`groupBy`
then read. Replace that with a pure, rule-scoped `series` hook:
- `series?: ({ instances }) => SeriesFacts` runs once per rule per split and
*returns* that rule's derived facts (no shared mutation).
- `matches`/`groupBy` receive a `RuleContext` ({ series }) carrying only that
rule's own facts.
- `buildSeriesInfo` is now a standalone series-statistics helper, decoupled
from rules; `SeriesInfo` no longer carries rule flags.
Most rules need only matches + groupBy; series is the opt-in for whole-series
facts (multi-frame, volumetric, mixed-b-value DWI). Adds tests covering the
DWI mixed/non-mixed split and updates the display-sets docs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds display-set metadata types and split helpers, migrates generic viewport plumbing to display-set metadata, adds ChangesDisplay set subsystem and provider migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…tadataProvider Three core jest suites still imported/mocked the pre-rename `genericViewportDataSetMetadataProvider` module and its `VIEWPORT_V2_DATA_SET` constant, so they failed to load after the DataSet->DisplaySet rename. Update the module path and constant to the current `genericViewportDisplaySetMetadataProvider` / `VIEWPORT_V2_DISPLAY_SET` names.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/core/src/RenderingEngine/WSIViewport.ts (1)
547-554: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
miniNavigationOverlaywhen replaying WSI display-set metadata.
getGenericViewportWSIDisplaySetnormalizesoptions.miniNavigationOverlay, andWSIViewportLegacyAdapter.setDataIdsstores that option before delegating here. Both of these paths forward onlywebClient, so a registered WSI display set withminiNavigationOverlay: falsesilently falls back to the default overlay behavior after this migration.Suggested fix
public setDataList( entries: Array<{ dataId: string; options?: Record<string, unknown> }> ) { if (!entries.length) { return; } const { dataId } = entries[0]; - const dataSet = metaData.get('genericViewportDisplaySet', dataId) as - | { imageIds?: string[]; options?: { webClient?: WSIClientLike } } - | undefined; + const dataSet = getGenericViewportWSIDisplaySet(dataId); if (dataSet?.imageIds) { - return this.setDataIds(dataSet.imageIds, { - webClient: dataSet.options?.webClient, - } as Parameters<typeof this.setDataIds>[1]); + return this.setDataIds(dataSet.imageIds, dataSet.options); } return this.setDataIds([dataId]); } @@ public async setDisplaySets( ...entries: Array<{ displaySetId: string; options?: unknown }> ): Promise<void> { await this.mountDisplaySets(entries, async (entry) => { const dataSet = getGenericViewportWSIDisplaySet(entry.displaySetId); if (!dataSet?.imageIds?.length || !dataSet.options?.webClient) { throw new Error( `[WSIViewport] No registered WSI dataset (imageIds + webClient) for display set ${entry.displaySetId}` ); } - await this.setWSI(dataSet.imageIds, dataSet.options.webClient); + await this.setDataIds(dataSet.imageIds, dataSet.options); }); }Also applies to: 643-655
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/RenderingEngine/WSIViewport.ts` around lines 547 - 554, `getGenericViewportWSIDisplaySet` is dropping the stored `miniNavigationOverlay` when it replays metadata through `WSIViewport.setDataIds`, so the legacy adapter’s saved overlay setting is lost. Update the metadata shape and forwarding logic in `getGenericViewportWSIDisplaySet` and `WSIViewportLegacyAdapter.setDataIds` to include `options.miniNavigationOverlay` alongside `webClient`, and pass it through unchanged when calling `setDataIds` so registered display sets preserve the original overlay preference.packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts (1)
508-517: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up viewport-owned overlay display sets on teardown.
addImagesnow registersimage/imageDatain the global display-set provider, but the teardown here still only removesrenderImageObjectDataId. Overlays created throughaddImagessurviveremoveData()and viewport destruction, which keeps large buffers reachable and leaves stale metadata behind for futuredisplaySetIdreuse.Suggested fix
+ private readonly ownedDisplaySetIds = new Set<string>(); + async addImages(stackInputs: IStackInput[]): Promise<void> { ... const dataId = this.resolveOverlayDataId(stackInput, image, reference); genericViewportDisplaySetMetadataProvider.add(dataId, { image, imageData: stackInput.imageData, imageIds: [stackInput.imageId], initialImageIdIndex: 0, kind: 'planar', reference, useWorldCoordinateImageData: stackInput.useWorldCoordinateImageData === true, }); + this.ownedDisplaySetIds.add(dataId); await this.addDisplaySet(dataId, { role: 'overlay', }); ... } removeData(dataId: string): void { this.clearResolvedViewCache(); super.removeData(dataId); this.mountedData.handleRemovedData(dataId); + if (this.ownedDisplaySetIds.delete(dataId)) { + genericViewportDisplaySetMetadataProvider.remove(dataId); + } if (!this.isDestroyed && this.getCurrentBinding()) { this.updateBindingsCameraState(); } } protected override onDestroy(): void { this.clearResolvedViewCache(); if (this.renderImageObjectDataId) { genericViewportDisplaySetMetadataProvider.remove( this.renderImageObjectDataId ); this.renderImageObjectDataId = undefined; } + this.ownedDisplaySetIds.forEach((dataId) => { + genericViewportDisplaySetMetadataProvider.remove(dataId); + }); + this.ownedDisplaySetIds.clear(); ... }Also applies to: 1247-1254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts` around lines 508 - 517, The teardown for PlanarViewport is only removing the renderImageObjectDataId entry, so viewport-owned overlay display sets added by addImages remain in genericViewportDisplaySetMetadataProvider and keep buffers/metadata alive. Update the cleanup path in PlanarViewport’s removeData/destroy logic to also unregister every display set created from addImages, using the same dataId or displaySetId bookkeeping used when calling genericViewportDisplaySetMetadataProvider.add. Make sure the fix covers both teardown locations referenced by the review so stale overlay metadata cannot survive viewport destruction or be reused later.packages/tools/examples/videoSegmentation/index.ts (1)
193-223: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSelect the video display set explicitly here.
This sample uses the same mixed series as
packages/core/examples/video/index.ts, soconst [displaySet] = displaySetscan grab the still-image split first. That would drivegetViewportTypeForDisplaySet(displaySet)andvideoIdfrom the wrong display set, breaking the example.Suggested fix
const displaySets = await createDisplaySets({ StudyInstanceUID: '2.25.96975534054447904995905761963464388233', SeriesInstanceUID: '2.25.15054212212536476297201250326674987992', wadoRsRoot: getLocalUrl() || 'https://d14fa38qiwhyfd.cloudfront.net/dicomweb', }); - const [displaySet] = displaySets; + const displaySet = displaySets.find( + (ds) => getViewportTypeForDisplaySet(ds) === Enums.ViewportType.VIDEO + ); if (!displaySet) { - throw new Error('No display set found in series'); + throw new Error('No video display set found in series'); } const videoId = displaySet.instances[0].imageId;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/examples/videoSegmentation/index.ts` around lines 193 - 223, The video segmentation example is picking the first display set from createDisplaySets, which can be the still-image split instead of the actual video display set. Update the selection logic in the createDisplaySets / displaySet handling block to explicitly choose the video display set before using getViewportTypeForDisplaySet(displaySet), displaySet.instances[0].imageId, and viewport.setDisplaySets, so the viewport is configured from the correct video series.packages/tools/examples/videoTools/index.ts (1)
172-228: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t assume the first split result is the video display set.
This example points at the same mixed series handled in
packages/core/examples/video/index.ts. PickingdisplaySets[0]can select the still-image display set, which then makes both the computed viewport type andvideoIdwrong.Suggested fix
const displaySets = await createDisplaySets({ StudyInstanceUID: '2.25.96975534054447904995905761963464388233', SeriesInstanceUID: '2.25.15054212212536476297201250326674987992', wadoRsRoot: getLocalUrl() || 'https://d14fa38qiwhyfd.cloudfront.net/dicomweb', }); - const [displaySet] = displaySets; + const displaySet = displaySets.find( + (ds) => getViewportTypeForDisplaySet(ds) === Enums.ViewportType.VIDEO + ); if (!displaySet) { - throw new Error('No display set found in series'); + throw new Error('No video display set found in series'); } const videoId = displaySet.instances[0].imageId;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tools/examples/videoTools/index.ts` around lines 172 - 228, The example currently assumes `displaySets[0]` is the video series, which can pick the still-image set and break both `getViewportTypeForDisplaySet(displaySet)` and `videoId`. Update the selection logic in the `createDisplaySets`/`displaySet` block to explicitly find the video display set using its identifying metadata or type, then use that chosen set for the viewport setup and `viewport.setDisplaySets` instead of relying on the first array item.
🧹 Nitpick comments (1)
packages/core/src/RenderingEngine/BaseVolumeViewport.ts (1)
1536-1541: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAttach a rejection handler to the fire-and-forget
volume.load().
volume.load()is intentionally not awaited (streaming), but the returned promise has no.catch. A load failure surfaces as an unhandled promise rejection rather than a viewport-scoped error. Consider catching and routing it (e.g. log / event) so failures are observable.🛡️ Suggested handling
if (!cache.getVolume(volumeId)) { const volume = await createAndCacheVolume(volumeId, { imageIds: dataSet.imageIds, }); - volume.load(); + volume.load().catch((error) => { + console.error( + `[VolumeViewport] Failed to load volume ${volumeId} for display set ${entry.displaySetId}`, + error + ); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/RenderingEngine/BaseVolumeViewport.ts` around lines 1536 - 1541, Attach a rejection handler to the fire-and-forget load path in BaseVolumeViewport by handling the promise returned from volume.load() after createAndCacheVolume. Keep the non-awaited streaming behavior, but add a .catch on volume.load() so failures are routed through viewport-scoped logging or events instead of becoming unhandled promise rejections. Use the unique symbols createAndCacheVolume, cache.getVolume, and volume.load() to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/examples/displaySets/index.ts`:
- Around line 181-185: The metadata rendering in the displaySets example is
using row.detailsElement.innerHTML and line.innerHTML, which can inject
untrusted DICOM values into the page. Update the logic in the row/details
rendering path to build each line with DOM nodes instead of HTML, using
textContent for the metadata label and value. Keep the fix localized around the
loop that processes lines and appends to row.detailsElement.
In `@packages/docs/docs/concepts/cornerstone-metadata/display-sets.md`:
- Around line 46-53: The getNaturalizedInstance example references metaData
without showing its source, so update the snippet to either add the missing
metaData import from `@cornerstonejs/metadata` or explicitly note in the
surrounding comment that metaData is the default export. Keep the example
consistent with the rest of the page by making the dependency visible near
getNaturalizedInstance and metaData.get('instance', imageId).
In `@packages/metadata/src/displayset/BaseDisplaySet.ts`:
- Around line 27-34: Clone the incoming viewportTypes and instances in the
BaseDisplaySet constructor instead of storing the caller’s arrays by reference.
Update the BaseDisplaySet initialization so viewportTypes is copied before
calling getPreferredViewportType, and instances is also assigned from a new
array copy, keeping the display set immutable after construction just like the
imageIds and underlyingImageIds fields.
In `@packages/metadata/src/displayset/buildSeriesInfo.ts`:
- Around line 14-31: Deduplicate the resolved instances before computing series
aggregates in buildSeriesInfo, because imageIds can expand to the same
naturalized instance multiple times and inflate the series facts. Update the
logic around NumberOfSeriesRelatedInstances, numberOfFrames,
numberOfNonImageObjects, and numberOfSOPInstanceUIDsPerSeries to operate on a
unique set keyed by the underlying instance identity (for example SOPInstanceUID
or another stable instance key) before summing.
In `@packages/metadata/src/displayset/defaultDisplaySetSplitRules.ts`:
- Around line 95-96: The mixedDimensionalityBValue split rule is now matching
too broadly and making both subgroups stack-preferred, which regresses
defined-b-value multi-slice DWI behavior. Update the rule in
defaultDisplaySetSplitRules so it only applies to the intended subgroup(s) and
does not override the existing volume/volume3d default for the defined-b-value
split; verify the ordering and matching logic around mixedDimensionalityBValue
and the surrounding split rules.
In `@packages/metadata/src/displayset/groupInstancesBySplitRules.ts`:
- Around line 17-27: The split bucket key is currently using only the rule id
when present, which can merge results from different rules that reuse the same
id. Update the grouping logic in buildSplitKey and its caller so the bucket
namespace always includes ruleIndex alongside any splitRule.id, preserving
separation between rules even when ids collide. Make sure the change is applied
wherever the discriminator is constructed so identical groupBy values from
different rules cannot collapse into one InstanceGroup or inherit the wrong
matchedRule/custom attributes.
In `@packages/metadata/src/displayset/ImageStackDisplaySet.ts`:
- Around line 95-101: The fromImageIds path in ImageStackDisplaySet should
preserve the caller-supplied frame-level imageIds instead of rebuilding them
from NaturalizedInstance.imageId. Keep using getNaturalizedInstance to resolve
metadata, but update fromImageIds/fromInstances so the resulting
displaySet.imageIds stays aligned with the original imageIds array for
multi-frame or 4D callers, rather than collapsing to underlying instance ids.
In `@packages/metadata/src/displayset/isVideoInstance.ts`:
- Around line 15-22: The transfer syntax helper is only checking the first
advertised value, so `getTransferSyntaxUid` can miss a video syntax stored later
in the array. Update the logic in `getTransferSyntaxUid` so it examines all
values from `AvailableTransferSyntaxUID`, `TransferSyntaxUID`, or `00083002` and
returns a matching video transfer syntax if any entry qualifies, instead of
blindly using `tsuid[0]`. Make sure the downstream `createDisplaySetFromGroup`
classification uses the corrected result so multi-valued metadata is handled
properly.
In `@packages/metadata/src/displayset/registerDisplaySetMetadata.ts`:
- Around line 13-18: The registration set is being seeded from arbitrary
imageIds in registerDisplaySetMetadata, which can cause split display sets to
overwrite sibling frames under MetadataModules.DISPLAY_SET. Update
registerDisplaySetMetadata to derive the primary keys from the provided
displaySet itself, and only add any extra aliases explicitly through validation
that they belong to the same group. Make sure the logic in
registerDisplaySetMetadata and any related handling around the displaySet
metadata write path only registers identifiers that are valid for that specific
display set.
In `@packages/metadata/src/types/MetadataModuleTypes.ts`:
- Around line 124-131: The `displaySetModule` type in `MetadataModuleTypes` does
not match the object stored by `registerDisplaySetMetadata`, which wraps the
cached `IDisplaySet` inside a `displaySet` property. Update the
`displaySetModule` declaration and any related type usage so
`getTyped(MetadataModules.DISPLAY_SET, imageId)` is typed as the wrapper shape
rather than a bare `IDisplaySet`, keeping the type aligned with the payload
created in `registerDisplaySetMetadata`.
In `@packages/tools/examples/videoColor/index.ts`:
- Around line 192-196: The viewport binding in the videoColor demo is using the
first instance imageId instead of the display set’s own logical id, which breaks
the contract from createDisplaySets/setDisplaySets. Update the logic around
displaySet handling so the value passed to the viewport binding comes from
displaySet itself (the DisplaySetId returned by createDisplaySets) rather than
displaySet.instances[0].imageId, and apply the same fix anywhere else in this
flow uses the instance imageId for binding.
In `@packages/tools/examples/videoContourSegmentation/index.ts`:
- Around line 253-264: The video demo is currently assuming the first result
from createDisplaySets() is the video display set, which can break when the
series is split in a different order. Update the logic around displaySets so it
explicitly selects the display set whose preferredViewportType is 'video' before
reading instances[0].imageId and before using that set for the mounted viewport
content. Keep the No display set found check, but make it validate the
video-specific display set instead of displaySets[0].
In `@packages/tools/examples/videoGroup/index.ts`:
- Around line 327-339: The demo is currently assuming the first result from
createDisplaySets is the video set, which can break when multiple display sets
are returned. Update the selection logic in the videoGroup example to explicitly
find the display set whose preferredViewportType is 'video' before using
instances[0].imageId, and apply the same change in the other affected block so
the mounted viewport and content always come from the video display set.
In `@packages/tools/examples/videoNavigation/index.ts`:
- Around line 172-182: The demo is currently assuming the first result from
createDisplaySets() is the video series, which can break when the series is
split into multiple display sets. Update the videoNavigation flow to explicitly
choose the display set whose preferredViewportType is video before using its
instances[0].imageId, and apply the same selection logic wherever the series is
mounted or rendered in the later videoNavigation code path so the viewport and
content always use the correct video display set.
In `@packages/tools/examples/videoRange/index.ts`:
- Around line 303-314: The demo currently assumes the first entry from
createDisplaySets() is the video set, which can pick the wrong display set when
multiple are returned. Update the videoRange example to explicitly select the
display set whose preferredViewportType is video before accessing
instances[0].imageId, and apply the same selection logic anywhere else in the
file that mounts or references the video display set. Keep the existing error
handling if no matching video display set is found.
In `@packages/tools/examples/videoSplineROITools/index.ts`:
- Around line 279-298: Select the video display set explicitly instead of
relying on the first result from createDisplaySets(). In
videoSplineROITools/index.ts, update the logic around the displaySets
destructuring so it finds the set whose preferredViewportType is 'video' before
using displaySet.instances[0].imageId and
getViewportTypeForDisplaySet(displaySet). Keep the existing no-display-set
guard, but make sure the chosen display set is the video one so the viewport and
mounted content match the demo intent.
In `@utils/demo/helpers/splitDisplaySetsFromImageIds.ts`:
- Around line 100-116: The frame-level image IDs are being collapsed too early
in getInstanceLevelImageIds, which prevents splitImageIdsBySplitRules from
seeing per-frame tags. Update this helper so it preserves distinct
frame-specific candidates instead of keeping only the first imageId per
SOPInstanceUID, and let the later split logic handle grouping; make sure
collectFrameImageIdsForGroup can still operate on the full candidate set for
multiframe MR mixed-b-value cases.
---
Outside diff comments:
In `@packages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.ts`:
- Around line 508-517: The teardown for PlanarViewport is only removing the
renderImageObjectDataId entry, so viewport-owned overlay display sets added by
addImages remain in genericViewportDisplaySetMetadataProvider and keep
buffers/metadata alive. Update the cleanup path in PlanarViewport’s
removeData/destroy logic to also unregister every display set created from
addImages, using the same dataId or displaySetId bookkeeping used when calling
genericViewportDisplaySetMetadataProvider.add. Make sure the fix covers both
teardown locations referenced by the review so stale overlay metadata cannot
survive viewport destruction or be reused later.
In `@packages/core/src/RenderingEngine/WSIViewport.ts`:
- Around line 547-554: `getGenericViewportWSIDisplaySet` is dropping the stored
`miniNavigationOverlay` when it replays metadata through
`WSIViewport.setDataIds`, so the legacy adapter’s saved overlay setting is lost.
Update the metadata shape and forwarding logic in
`getGenericViewportWSIDisplaySet` and `WSIViewportLegacyAdapter.setDataIds` to
include `options.miniNavigationOverlay` alongside `webClient`, and pass it
through unchanged when calling `setDataIds` so registered display sets preserve
the original overlay preference.
In `@packages/tools/examples/videoSegmentation/index.ts`:
- Around line 193-223: The video segmentation example is picking the first
display set from createDisplaySets, which can be the still-image split instead
of the actual video display set. Update the selection logic in the
createDisplaySets / displaySet handling block to explicitly choose the video
display set before using getViewportTypeForDisplaySet(displaySet),
displaySet.instances[0].imageId, and viewport.setDisplaySets, so the viewport is
configured from the correct video series.
In `@packages/tools/examples/videoTools/index.ts`:
- Around line 172-228: The example currently assumes `displaySets[0]` is the
video series, which can pick the still-image set and break both
`getViewportTypeForDisplaySet(displaySet)` and `videoId`. Update the selection
logic in the `createDisplaySets`/`displaySet` block to explicitly find the video
display set using its identifying metadata or type, then use that chosen set for
the viewport setup and `viewport.setDisplaySets` instead of relying on the first
array item.
---
Nitpick comments:
In `@packages/core/src/RenderingEngine/BaseVolumeViewport.ts`:
- Around line 1536-1541: Attach a rejection handler to the fire-and-forget load
path in BaseVolumeViewport by handling the promise returned from volume.load()
after createAndCacheVolume. Keep the non-awaited streaming behavior, but add a
.catch on volume.load() so failures are routed through viewport-scoped logging
or events instead of becoming unhandled promise rejections. Use the unique
symbols createAndCacheVolume, cache.getVolume, and volume.load() to locate the
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 253763be-49f0-479d-af99-6418c1586430
📒 Files selected for processing (97)
packages/core/examples/displaySets/index.tspackages/core/examples/dynamicVolume/index.tspackages/core/examples/genericEcg/index.tspackages/core/examples/genericMultiVolumeAPI/index.tspackages/core/examples/genericStackAPI/index.tspackages/core/examples/genericStackPosition/index.tspackages/core/examples/genericVideo/index.tspackages/core/examples/genericViewportScale/index.tspackages/core/examples/genericWsi/index.tspackages/core/examples/video/index.tspackages/core/examples/viewportProjection/index.tspackages/core/examples/wsi/index.tspackages/core/src/RenderingEngine/BaseVolumeViewport.tspackages/core/src/RenderingEngine/ECGViewport.tspackages/core/src/RenderingEngine/GenericViewport/ECG/DefaultECGDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/GenericViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/DefaultPlanarDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarLegacyCompatibilityController.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewport.tspackages/core/src/RenderingEngine/GenericViewport/Planar/PlanarViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Video/DefaultVideoDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Video/VideoViewport.tspackages/core/src/RenderingEngine/GenericViewport/ViewportArchitectureTypes.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/DefaultVolume3DDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/VolumeViewport3DLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/Volume3D/viewport3D.tspackages/core/src/RenderingEngine/GenericViewport/WSI/DefaultWSIDataProvider.tspackages/core/src/RenderingEngine/GenericViewport/WSI/WSIViewportLegacyAdapter.tspackages/core/src/RenderingEngine/GenericViewport/genericViewportDisplaySetAccess.tspackages/core/src/RenderingEngine/GenericViewport/index.tspackages/core/src/RenderingEngine/StackViewport.tspackages/core/src/RenderingEngine/VideoViewport.tspackages/core/src/RenderingEngine/Viewport.tspackages/core/src/RenderingEngine/WSIViewport.tspackages/core/src/utilities/genericViewportDataSetMetadataProvider.tspackages/core/src/utilities/genericViewportDisplaySetMetadataProvider.tspackages/core/src/utilities/index.tspackages/docs/docs/concepts/cornerstone-core/generic-viewport/api.mdpackages/docs/docs/concepts/cornerstone-core/generic-viewport/data-bindings-and-loading.mdpackages/docs/docs/concepts/cornerstone-core/generic-viewport/migration.mdpackages/docs/docs/concepts/cornerstone-metadata/display-sets.mdpackages/docs/docs/concepts/cornerstone-metadata/index.mdpackages/docs/docs/migration-guides/5x/2-generic-viewport.mdpackages/docs/sidebars.jspackages/metadata/jest.config.jspackages/metadata/src/displayset/BaseDisplaySet.tspackages/metadata/src/displayset/IDisplaySet.tspackages/metadata/src/displayset/ImageStackDisplaySet.tspackages/metadata/src/displayset/buildSeriesInfo.tspackages/metadata/src/displayset/createDisplaySetFromGroup.tspackages/metadata/src/displayset/defaultDisplaySetSplitRules.tspackages/metadata/src/displayset/displaySetProvider.tspackages/metadata/src/displayset/displayset.test.tspackages/metadata/src/displayset/groupInstancesBySplitRules.tspackages/metadata/src/displayset/index.tspackages/metadata/src/displayset/isEcgInstance.tspackages/metadata/src/displayset/isImageInstance.tspackages/metadata/src/displayset/isVideoInstance.tspackages/metadata/src/displayset/isWsiInstance.tspackages/metadata/src/displayset/registerDisplaySetMetadata.tspackages/metadata/src/displayset/resolveInstances.tspackages/metadata/src/displayset/splitImageIdsBySplitRules.tspackages/metadata/src/displayset/types.tspackages/metadata/src/displayset/viewportTypes.tspackages/metadata/src/enums/MetadataModules.tspackages/metadata/src/index.tspackages/metadata/src/registerDefaultProviders.tspackages/metadata/src/types/MetadataModuleTypes.tspackages/metadata/src/utilities/index.tspackages/tools/examples/dynamicCINETool/index.tspackages/tools/examples/generateImageFromTimeData/index.tspackages/tools/examples/genericLabelmapOverlapPlayground/index.tspackages/tools/examples/genericLabelmapRendering/index.tspackages/tools/examples/genericLabelmapSegmentationTools/index.tspackages/tools/examples/genericLabelmapSliceRendering/index.tspackages/tools/examples/genericLabelmapSliceRenderingTools/index.tspackages/tools/examples/genericStackLabelmapSegmentation/index.tspackages/tools/examples/genericStackManipulationTools/index.tspackages/tools/examples/genericVolumeAnnotationTools/index.tspackages/tools/examples/localAdvanced/index.tspackages/tools/examples/videoColor/index.tspackages/tools/examples/videoContourSegmentation/index.tspackages/tools/examples/videoGroup/index.tspackages/tools/examples/videoNavigation/index.tspackages/tools/examples/videoRange/index.tspackages/tools/examples/videoSegmentation/index.tspackages/tools/examples/videoSplineROITools/index.tspackages/tools/examples/videoTools/index.tspackages/tools/examples/viewportProjectionSynchronizer/index.tspackages/tools/examples/wsiAnnotationTools/index.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan.spec.tspackages/tools/src/tools/displayTools/Labelmap/labelmapRenderPlan/planarGenericVolumeLabelmap.tspackages/tools/src/tools/displayTools/Labelmap/removeLabelmapRepresentationData.tstests/vitest-browser/genericStackApi.browser.test.tsutils/ExampleRunner/example-runner-cli.jsutils/demo/helpers/index.jsutils/demo/helpers/splitDisplaySetsFromImageIds.ts
💤 Files with no reviewable changes (1)
- packages/core/src/utilities/genericViewportDataSetMetadataProvider.ts
Comment on lines
+14
to
+31
| const NumberOfSeriesRelatedInstances = instances.length; | ||
| let numberOfFrames = 0; | ||
| let numberOfNonImageObjects = 0; | ||
| let numberOfSOPInstanceUIDsPerSeries = 0; | ||
|
|
||
| for (const instance of instances) { | ||
| if (instance.NumberOfFrames) { | ||
| numberOfFrames += Number(instance.NumberOfFrames); | ||
| } else if (instance.Rows) { | ||
| numberOfFrames += 1; | ||
| } else { | ||
| numberOfNonImageObjects += 1; | ||
| } | ||
|
|
||
| if (instance.SOPInstanceUID) { | ||
| numberOfSOPInstanceUIDsPerSeries += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deduplicate repeated underlying instances before aggregating series facts.
This pipeline starts from imageIds, so frame-expanded inputs can resolve the same naturalized instance multiple times. Counting raw array entries here inflates NumberOfSeriesRelatedInstances, numberOfFrames, and numberOfSOPInstanceUIDsPerSeries, which can skew any split rule reading seriesInfo.
Suggested fix
export function buildSeriesInfo(instances: NaturalizedInstance[]): SeriesInfo {
- const NumberOfSeriesRelatedInstances = instances.length;
+ const uniqueInstances = [
+ ...new Map(
+ instances.map((instance, index) => [
+ instance.SOPInstanceUID ?? instance.imageId ?? `__instance_${index}`,
+ instance,
+ ])
+ ).values(),
+ ];
+
+ const NumberOfSeriesRelatedInstances = uniqueInstances.length;
let numberOfFrames = 0;
let numberOfNonImageObjects = 0;
let numberOfSOPInstanceUIDsPerSeries = 0;
- for (const instance of instances) {
+ for (const instance of uniqueInstances) {
if (instance.NumberOfFrames) {
numberOfFrames += Number(instance.NumberOfFrames);
} else if (instance.Rows) {
numberOfFrames += 1;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const NumberOfSeriesRelatedInstances = instances.length; | |
| let numberOfFrames = 0; | |
| let numberOfNonImageObjects = 0; | |
| let numberOfSOPInstanceUIDsPerSeries = 0; | |
| for (const instance of instances) { | |
| if (instance.NumberOfFrames) { | |
| numberOfFrames += Number(instance.NumberOfFrames); | |
| } else if (instance.Rows) { | |
| numberOfFrames += 1; | |
| } else { | |
| numberOfNonImageObjects += 1; | |
| } | |
| if (instance.SOPInstanceUID) { | |
| numberOfSOPInstanceUIDsPerSeries += 1; | |
| } | |
| } | |
| const uniqueInstances = [ | |
| ...new Map( | |
| instances.map((instance, index) => [ | |
| instance.SOPInstanceUID ?? instance.imageId ?? `__instance_${index}`, | |
| instance, | |
| ]) | |
| ).values(), | |
| ]; | |
| const NumberOfSeriesRelatedInstances = uniqueInstances.length; | |
| let numberOfFrames = 0; | |
| let numberOfNonImageObjects = 0; | |
| let numberOfSOPInstanceUIDsPerSeries = 0; | |
| for (const instance of uniqueInstances) { | |
| if (instance.NumberOfFrames) { | |
| numberOfFrames += Number(instance.NumberOfFrames); | |
| } else if (instance.Rows) { | |
| numberOfFrames += 1; | |
| } else { | |
| numberOfNonImageObjects += 1; | |
| } | |
| if (instance.SOPInstanceUID) { | |
| numberOfSOPInstanceUIDsPerSeries += 1; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/metadata/src/displayset/buildSeriesInfo.ts` around lines 14 - 31,
Deduplicate the resolved instances before computing series aggregates in
buildSeriesInfo, because imageIds can expand to the same naturalized instance
multiple times and inflate the series facts. Update the logic around
NumberOfSeriesRelatedInstances, numberOfFrames, numberOfNonImageObjects, and
numberOfSOPInstanceUIDsPerSeries to operate on a unique set keyed by the
underlying instance identity (for example SOPInstanceUID or another stable
instance key) before summing.
Comment on lines
+100
to
+116
| function getInstanceLevelImageIds(imageIds: string[]): string[] { | ||
| const bySop = new Map<string, string>(); | ||
|
|
||
| for (const imageId of imageIds) { | ||
| const instance = getNaturalizedInstanceForDisplaySetSplit(imageId); | ||
| if (!instance) { | ||
| continue; | ||
| } | ||
|
|
||
| const sopUid = instance.SOPInstanceUID as string | undefined; | ||
| const key = sopUid ?? imageId; | ||
| if (!bySop.has(key)) { | ||
| bySop.set(key, instance.imageId ?? toBaseImageId(imageId)); | ||
| } | ||
| } | ||
|
|
||
| return [...bySop.values()]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't collapse frame-level candidates to one imageId per SOP before splitting.
This keeps only the first imageId for each SOPInstanceUID, so frame-specific tags never reach splitImageIdsBySplitRules. For multiframe MR, collectFrameImageIdsForGroup then pulls the whole SOP back into one display set, which defeats the new mixed-b-value split.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utils/demo/helpers/splitDisplaySetsFromImageIds.ts` around lines 100 - 116,
The frame-level image IDs are being collapsed too early in
getInstanceLevelImageIds, which prevents splitImageIdsBySplitRules from seeing
per-frame tags. Update this helper so it preserves distinct frame-specific
candidates instead of keeping only the first imageId per SOPInstanceUID, and let
the later split logic handle grouping; make sure collectFrameImageIdsForGroup
can still operate on the full candidate set for multiframe MR mixed-b-value
cases.
Library: - defaultDisplaySetSplitRules: mixed-b-value DWI rule now prefers volume (MPR) over stack, matching normal MR series; it runs before volume3d so stack-first was regressing the defined-b-value subgroup. - isVideoInstance: check all advertised transfer syntaxes, not just the first array entry, so a video syntax in a later slot is not missed. - BaseDisplaySet: clone viewportTypes and instances so a caller mutating them after construction cannot drift the display set / preferredViewportType. - ImageStackDisplaySet.fromImageIds: preserve caller frame-level imageIds instead of collapsing them to underlying-instance ids. - BaseVolumeViewport: attach a rejection handler to the fire-and-forget streaming volume.load(). Examples/docs: - video examples: select the video display set by preferredViewportType instead of assuming displaySets[0], which is fragile once a series splits. - displaySets example: build metadata rows with DOM nodes/textContent instead of innerHTML (avoids injecting DICOM-derived markup). - docs: import metaData in the getNaturalizedInstance snippet; align the mixed-b-value rule example with the volume-first viewport order.
The CodeRabbit nitpick assumed volume.load() returns a promise, but it returns void, so .catch was a type error that broke the ESM build (TS2339). Restore the original fire-and-forget volume.load() call.
Member
Author
|
Closing — opened in error. These commits were pushed to the existing PR #2738 (branch fix/dwi-wrong-wl-split) instead. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Moves the display-set concept out of OHIF into
@cornerstonejs/metadataas a framework-agnostic, data-shaped module, and fixes the original bug: a diffusion MR (DWI) series that mixes 4D b-value frames with trailing no-b-value frames was rendered as one volume, applying the wrong window/level to the non-4D portion.What's here
splitImageIdsBySplitRules→createDisplaySetFromGroup): resolves a series' imageIds into instance groups via ordered split rules (first match wins), then builds anIDisplaySetper group.mixedDimensionalityBValuerule): splits the undefined-b-value frames into their own display set so 4D rendering is not applied to the 3D portion.matches(per-instance predicate) +groupBy(bucket recipe), with an optionalserieshook for facts derived from the whole series.Split-rule shape
Most rules need only
matches+groupBy. A rule reaches forseriesonly when it needs a value computed from the whole series and reused bymatches/groupBy.seriesis pure — it returns that rule's derived facts and does not mutate shared state:Testing
packages/metadatajest suite: 63 passing (covers the DWI mixed/non-mixed split, rule namespacing, deterministic group order, custom attributes, classifiers).concepts/cornerstone-metadata/display-sets.Summary by CodeRabbit
New Features
setDisplaySetssupport across core viewports (including WSI, video, ECG, stack, volumes).Bug Fixes
Documentation
Tests