fix: Having sop instance in a per-frame or shared attribute breaks load #4560
Conversation
✅ Deploy Preview for ohif-platform-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
| // During a resize, the slice index should remain unchanged. This is a temporary fix for | ||
| // a larger issue regarding the definition of slice index with slab thickness. | ||
| // We need to revisit this to make it more robust and understandable. | ||
| delete presentation.viewReference.sliceIndex; | ||
| if (presentation?.viewReference) { |
There was a problem hiding this comment.
This is still an issue entering MPR mode, but at least it doesn't throw
Viewers
|
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
fix/sop-instance-in-per-frame
|
| Run status |
|
| Run duration | 02m 11s |
| Commit |
|
| Committer | sedghi |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
2
|
Skipped
|
0
|
Passing
|
44
|
| View all changes introduced in this branch ↗︎ | |
|
@wayfarer3130 can you upload an anonymized study that does not work right now or bug report issue number |
|
@sedghi - could you check the uploaded study? That study works correctly now, but I don't understand why your MPR issue breaks. I tried looking at that, but if you could help debug that with me sometime I'd appreciate it. |
…t component - Removed unused SpacingBetweenSlices variable from combineFrameInstance function. - Enhanced handling of ImagePositionPatient and ImageOrientationPatient by converting values to numbers. - Updated the way newInstance properties are assigned using Object.defineProperty for better control. - Cleaned up LayoutPreset component by removing unnecessary debug statement.
…roved image handling
| Object.entries(item).forEach(([key, value]) => { | ||
| newInstance[key] = value; | ||
| }); | ||
| // Note: do not assign directly to newInstance.ImagePositionPatient |
There was a problem hiding this comment.
This note doesn't seem to apply - I tried with:
objPar = { a: 3, b:4 }
objChild = Object.create(objPar)
objChild.a = 'ay'
objChild.c = 'c'
console.log("obj=", objChild.a, objChild.b, objChild.c, objPar.a)
And I see ay, 4, 'c', 3
which is what I expect.
There was a problem hiding this comment.
I wonder if the instance is a proxy object and what we are seeing is a proxy assignment?
There was a problem hiding this comment.
I don't know what is happening but it is the issue. The instance.ImagePositionPatient should not change
|
|
||
| // merge the shared first then the per frame to override | ||
| [...shared].forEach(item => { | ||
| if (item.SOPInstanceUID) { |
There was a problem hiding this comment.
Should we simply avoid overriding any values in the child? I know this tests for the SOPInstanceUID, but I would guess other things could get overridden.
…property assignment for ImagePositionPatient
Context
requires cornerstonejs/cornerstone3D#1752
Some PACS systems combine single frame into multi-frame and then record the original SOPInstanceUID in the per frame attribute sequence. This PR simply omits such per frame instances.
Changes & Results
Uses a shared object for the parent series/instance data rather than merging, and avoid over-riding anything containing a SOP instance UID.
This addresses #4551
It also addresses the issue from Jagan Vijayaraghavan in slack, for which he provided the study below.
StudyWithSOPInFrameSequence.zip
The issue is that a shared functional groups sequence contains an original SOP instance UID for another object, and that SOP ends up replacing the correct SOP/Series UIDs, causing the wrong UIDs to be used.
Testing
Load the study with the per-frame data from the cornerstone chat
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment