fix(core): report single-location multi-frame series as non-reconstructable#6138
Conversation
…ctable A cine / multi-time series is many frames acquired at a single spatial location: every frame shares one ImagePositionPatient, so the series has no through-plane extent. processMultiframe never checked the spread of the frame positions, so such a series was reported as reconstructable and any volume layout built a degenerate volume with zero slice spacing, which renders black in every plane. Guard both the multi-frame and single-frame paths on a single spatial location. Frame positions are read from the per-frame functional groups, falling back to the shared functional group broadcast to the frame count (a constant position is commonly stored there rather than repeated per frame). The check fails open when fewer than two positions are known, so a genuine volume is never wrongly blocked.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
✅ Deploy Preview for ohif-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
TFRadicalImaging
temporarily deployed
to
fork-pr-approval
GitHub Actions
Inactive
|
@wayfarer3130 this is the core fix you asked for during the downstream review — reporting a single-location multi-frame series as non-reconstructable, rather than guarding the layout switch in the fork. Could you take a look when you get a chance? cc @jbocce |
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesDisplay-set reconstructability
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
platform/core/src/utils/isDisplaySetReconstructable.js (1)
308-328: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
getPerFramePositionstrusts per-frame data on a single valid entry, bypassing the shared-group fallback.
perFramePositions.some(isValidImagePosition)returns the per-frame array whenever just one frame has a valid position, even if the rest are malformed/missing. In that casehasSingleSpatialLocationsees fewer than 2 valid positions and fails open (treats as reconstructable), even thoughSharedFunctionalGroupsSequencemight carry the correct constant position that would reveal a single-location series. Requiring at least two valid per-frame positions before preferring them over the shared fallback would align this threshold withhasSingleSpatialLocation's own fail-open bar.♻️ Proposed tightening
- if (perFramePositions.some(isValidImagePosition)) { + if (perFramePositions.filter(isValidImagePosition).length >= 2) { return perFramePositions; }🤖 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 `@platform/core/src/utils/isDisplaySetReconstructable.js` around lines 308 - 328, Update getPerFramePositions so it only returns the per-frame positions when at least two entries satisfy isValidImagePosition; otherwise continue to the shared-position fallback. Preserve the existing shared fallback and frame-count behavior for insufficient or invalid per-frame data.platform/core/src/utils/isDisplaySetReconstructable.test.js (1)
27-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage; consider adding the exact 2-frame/2-instance boundary case.
The suite covers the helpers and both processing paths well. The PR's core motivating bug was that the old spacing check only runs for
instances.length > 2, so a 2-frame single-location series previously slipped through undetected. Adding an explicit 2-instance test (mirroring the existing 3-instance case) would directly guard that regression.test('not reconstructable when exactly two instances share one location', () => { const instances = [singleFrameInstance([0, 0, 0]), singleFrameInstance([0, 0, 0])]; expect(isDisplaySetReconstructable(instances).value).toBe(false); });🤖 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 `@platform/core/src/utils/isDisplaySetReconstructable.test.js` around lines 27 - 150, Add an explicit boundary test in the “single-frame instances” suite for exactly two instances with the same spatial position, and assert that isDisplaySetReconstructable(...).value is false. Mirror the existing three-instance constant-location test to guard the two-instance regression.
🤖 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.
Nitpick comments:
In `@platform/core/src/utils/isDisplaySetReconstructable.js`:
- Around line 308-328: Update getPerFramePositions so it only returns the
per-frame positions when at least two entries satisfy isValidImagePosition;
otherwise continue to the shared-position fallback. Preserve the existing shared
fallback and frame-count behavior for insufficient or invalid per-frame data.
In `@platform/core/src/utils/isDisplaySetReconstructable.test.js`:
- Around line 27-150: Add an explicit boundary test in the “single-frame
instances” suite for exactly two instances with the same spatial position, and
assert that isDisplaySetReconstructable(...).value is false. Mirror the existing
three-instance constant-location test to guard the two-instance regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea5dc908-8ca7-4981-a88e-3d3b131f0f8e
📒 Files selected for processing (2)
platform/core/src/utils/isDisplaySetReconstructable.jsplatform/core/src/utils/isDisplaySetReconstructable.test.js
…tent getPerFramePositions returned the per-frame positions as soon as a single frame carried a valid one, shadowing the shared functional group. With fewer than two valid positions hasSingleSpatialLocation cannot decide and fails open, so a series whose constant position lives in the shared group was still reported reconstructable. Require the two positions the check needs before preferring the per-frame groups. Also cover the two-instance boundary: the spacing check only runs for more than two instances, so a co-located pair relies entirely on the single-location guard.
TFRadicalImaging
temporarily deployed
to
fork-pr-approval
GitHub Actions
Inactive
Applied. The per-frame groups are now preferred only once they carry two valid positions, which is the same bar
Added. The spacing check only runs for more than two instances, so a co-located pair rests entirely on the single-location guard; the test fails without it (returns |
1 participant
Context
A multi-frame cine / multi-time series (e.g. a cardiac cine) is many frames acquired at a single spatial location — every frame shares one
ImagePositionPatient, so the series has no through-plane extent (span0mm).processMultiframenever checks the spread of the frame positions — that is the intent of the existing// TODO - check spacing consistency— so such a series is reported asisReconstructable: true. Any volume-based layout (MPR / 3D) then builds a degenerate volume: on a real 30-frame cardiac cine we measureddims [208, 174, 30]withspacing [1.92, 1.92, 0]— a zero Z spacing. Every MPR plane renders black, and because the layout switch replaces the hanging protocol there is no way back short of a page reload.The same hazard exists on the single-frame path: many single-frame instances acquired at one location fall through to the spacing calculation and produce the same degenerate volume.
Changes & Results
platform/core/src/utils/isDisplaySetReconstructable.js:zeroExtentTolerance(0.01mm). Real slice spacing is on the order of millimetres, so frames whose positions all collapse within this distance occupy a single location.hasSingleSpatialLocation(positions)— true when two or more positions are known and all collapse to one location. It fails open: with fewer than two valid positions it returnsfalse, so a genuine volume is never wrongly blocked when the geometry can't be determined confidently.getPerFramePositions(multiFrameInstance)— reads each frame'sImagePositionPatientfromPerFrameFunctionalGroupsSequence, falling back toSharedFunctionalGroupsSequencebroadcast toNumberOfFrames. This fallback matters: a single-location cine has one constant position, which the more common DICOM encoding stores in the shared group rather than repeating it per frame. Without it the per-frame read returns[]and the check never fires on the exact degenerate case it targets.processMultiframe(per-frame positions) andprocessSingleframe(instance positions).Before: single-location cine reports
isReconstructable: true→ MPR/3D builds a zero-Z-spacing volume → all viewports black, unrecoverable without a reload.After: it reports
isReconstructable: false→ volume layouts correctly treat it as non-volumetric (they are gated onisReconstructable), so the series stays in its stack/cine view and nothing goes black. A genuine volume series is unaffected and still reconstructs.Testing
New unit tests:
platform/core/src/utils/isDisplaySetReconstructable.test.js(13 cases, none existed before) — helper-level (collapse within tolerance, real extent, fewer than two positions, malformed positions, per-frame read, shared-group fallback, absent sequences) and end-to-end throughisDisplaySetReconstructablefor both multi-frame and single-frame series.Manually verified against real acquisition data in a downstream deployment, with two series from the same study:
4 CV Cine— 30 frames, all at oneImagePositionPatient, span 0mm → nowisReconstructable: false; selecting MPR is a safe no-op instead of turning every viewport black.AXIAL TRUFI CHEST— 20 frames, span 190mm → stillisReconstructable: true; MPR builds a real three-plane volume that renders normally.Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
This fix is contributed by the University of Calgary, who are donating it back to OHIF.
It was originally implemented as a layout guard in a downstream fork; @wayfarer3130 pointed out during review that the correct fix belongs in core — reporting the series as non-reconstructable, so that every surface derives the right behaviour from the data rather than from a UI-level guard — which is what this PR does.
Summary by CodeRabbit