fix(multiframe): metadata handling of NM studies and loading order #4540
Conversation
…taProvider: Improve multiframe handling and error checking - Corrected a typo in a comment from "necessarliy" to "necessarily" in `DicomJSONDataSource`. - Enhanced multiframe handling in `DicomLocalDataSource` and `DicomWebDataSource` by creating a separate `imageId` for each frame, ensuring proper metadata mapping for multiframe instances. - Removed usage of `instance.imageId` for multiframe instances to prevent invalid `imageId` assignments. - Added error handling in `MetadataProvider` to throw an error if `imageId` is empty during metadata operations. - Removed unnecessary comments and redundant error checks for empty `imageId` in `MetadataProvider`.
- Import `vec3` from 'gl-matrix' for vector operations - Refactor shared and perFrame sequences to improve readability and maintainability - Add support for calculating `ImagePositionPatient` using `ImageOrientationPatient` and `SpacingBetweenSlices` - Ensure accurate frame position calculation for NM multiframe datasets - Safeguard against undefined `ImagePositionPatient` by using calculated or default values
- Removed conditional rendering for text with colons and simplified to always use a single span with font-medium class - Adjusted indentation and alignment of the Number Box for better readability and consistency - Enhanced the Details Section to include both primary and secondary details, if available - Added a muted-foreground style for secondary details to visually distinguish them from primary details
…t labels - Refactored the null check in `interleaveTopToBottom.ts` to use optional chaining for cleaner and safer access to `requests[0].imageId` - Updated the label for `meanValue` in `preclinical-4d` mode to include units "(ml)" - Updated the label for `volume` in `tmtv` mode to include units "(ml)"
- Ensure that the slice index remains unchanged during viewport resize by deleting `sliceIndex` from the presentation view reference. - This change addresses a temporary fix for a larger issue regarding the definition of slice index with slab thickness. - Plan to revisit this area to make the implementation more robust and understandable. - Adjust the `setPresentations` method to accommodate the updated position
✅ Deploy Preview for ohif-prod ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
sedghi
changed the title
Viewers
|
||||||||||||||||||||||||||||
| Project |
Viewers
|
| Branch Review |
fix/3p9-recovery-1
|
| Run status |
|
| Run duration | 02m 31s |
| 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 ↗︎ | |
|
@salimkanoun I think this PR closes all your recent issues for 3.9, can you please double check? |
|
Yes sure will do this weekend, thank you so much Alireza ! |
|
Here some feedbacks :
|
- Updated import path for `useToggleOneUpViewportGridStore` to use `@ohif/extension-default` - Enabled interleaved image retrieval stages for volume in `index.tsx` - Registered image load strategies with metadata handling in `init.tsx` - Introduced `createMetadataWrappedStrategy` for safe metadata configuration - Added `useAppConfig` in `PanelStudyBrowserTracking` to use `studyBrowserMode` from app configuration - Updated webpack configuration to ignore certain node_modules during watch - Removed `maximumFileSizeToCacheInBytes` from PWA webpack configuration - Added `studyBrowserMode` to app configuration and documentation to set initial study browser tab
…eight override - Increased the default height for 'collapsed' mode in SegmentationSegments component from 600px to 900px for better user experience. - Removed the global CSS rule that set `min-height: 0 !important;` from multiple Tailwind CSS files across the platform. - The removal of the global min-height override is intended to prevent unexpected layout issues and ensure more consistent styling across components.
|
@salimkanoun I believe it addresses most of your comments. I can't reproduce the stage change issue in the demo. |
|
Yeah you can merge it will open new issue If I can identify reproducible scénarios |
| hangingProtocolService.registerImageLoadStrategy('interleaveTopToBottom', interleaveTopToBottom); | ||
| hangingProtocolService.registerImageLoadStrategy('nth', nthLoader); | ||
| // Register strategies using the wrapper | ||
| const imageLoadStrategies = { |
There was a problem hiding this comment.
Can we indicate that these are deprecated in favour of setting a image load strategy metadata? The three image loaders are not as robust as the new CS3D image loader because it doesn't work effectively with the priority fetching mechanism. Ideally, just setting one of those strategies in the hanging protocol and applying that into the metadata would allow new strategies to be defined directly in the hanging protocol as JSON data.
There was a problem hiding this comment.
I'm not sure if they are the same. The hanging protocol loading strategy and the cs3d image loaders work per series, so you can't implement an interleave CT, PT (one CT, one PT) with cs3d, as far as I know, right?
There was a problem hiding this comment.
It uses the priority to do the interleaved strategy, and allows for more complex interleaving, even if the image viewing starts at slightly different times.
There was a problem hiding this comment.
I like that, do we have an example of that somewhere in cornerstone3D? Let's discuss tomorrow
extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts
Show resolved
Hide resolved
| @@ -438,19 +438,37 @@ function createDicomWebApi(dicomWebConfig, servicesManager) { | |||
| instance, | |||
| }); | |||
|
|
|||
| const isMultiframe = instance.NumberOfFrames > 1; | |||
There was a problem hiding this comment.
Can you extract a common parent class rather than repeating the code? That would help new implementers with adding new data source types.
There was a problem hiding this comment.
I can't find a logical place for it because in dicomlocal, it occurs in series.metadata, while in dicomweb, it happens in store. I'll leave it as is for now.
...ions/measurement-tracking/src/panels/PanelStudyBrowserTracking/PanelStudyBrowserTracking.tsx
Outdated
Show resolved
Hide resolved
| @@ -120,7 +120,7 @@ function modeFactory({ modeConfiguration }) { | |||
| minValue: 'Minimum Value', | |||
| maxValue: 'Maximum Value', | |||
| meanValue: 'Mean Value', | |||
| volume: 'Volume', | |||
| volume: 'Volume (ml)', | |||
There was a problem hiding this comment.
This is supposed to display as part of the value. Properly it would be a fix in CS3D as to how it generates the values, in order to allow other volume units such as ul for microscopy.
There was a problem hiding this comment.
Yeah i think we should revisit this in next version
| @@ -280,7 +280,7 @@ export default class CustomizationService extends PubSubService { | |||
| return customization; | |||
| } | |||
| const parent = this.getCustomization(customizationType); | |||
| const result = parent ? Object.assign(Object.create(parent), customization) : customization; | |||
| const result = parent ? Object.assign({}, parent, customization) : customization; | |||
There was a problem hiding this comment.
This isn't the same value here - is it fixing a bug? If not, it means one can't assign a typed value as a default value, or have an object with hidden/non iterable values, which is useful sometimes.
There was a problem hiding this comment.
Yes, it's a bug fix. I'm not sure what the original intention was.
| .filter(it => !!it) | ||
| .map(it => it[0]) | ||
| .filter(it => it !== undefined && typeof it === 'object'); | ||
| const shared = SharedFunctionalGroupsSequence |
There was a problem hiding this comment.
This is ok here, but I'd like to discuss with you the option of making it use the javascript objects with inheritance - we can start with an object containing all the instance data, which we have, then use
const shared = Object.create(instance, ... shared functional groups)
SharedFunctionalGroupsSequence. = shared
Then for the functional groups, use a hidden value per frame which is
Object.create(shared, ...per frame values)
That is going to be far more memory efficient, and will avoid recreating the objects all the time.
There was a problem hiding this comment.
Yeah i have opened an issue to implement it #4551
- Simplified frame processing logic in DicomLocalDataSource and DicomWebDataSource to handle both single and multiframe instances uniformly. - Removed redundant checks for multiframe instances, streamlining the addition of image IDs to metadata. - Enhanced the combineFrameInstance utility by improving filtering logic and optimizing vector calculations for image positioning. This update improves code readability and maintainability while ensuring consistent behavior across different DICOM data sources.
- Updated webpack.base.js to set NODE_ENV to 'production' if not already defined, improving environment handling. - Refactored writePluginImportsFile.js for better readability by formatting return statements. - Modified build-for-production.md to clarify directory navigation and removed outdated caution note regarding platform naming. These changes improve the build process and documentation clarity.
- Removed automatic setting of NODE_ENV to 'production' in webpack.base.js, allowing for more explicit environment management. - Updated babel-loader configuration to conditionally include 'react-refresh/babel' plugin based on the build mode. - Cleaned up writePluginImportsFile.js by removing unnecessary console warnings for better clarity. - Enhanced documentation in custom-url-access.md to emphasize the need for updating the public/serve.json file for new routerBasename. These changes streamline the build process and enhance documentation clarity.
Fixes #4505
Fixes #4519
Fixes #4485
Fixes #4531
Fixes #4352
Fixes #4530
fixes potentially #4529
This pull request includes various changes across multiple files to improve functionality and fix issues in the project. The most important changes include fixes to image handling, improvements to metadata management, and updates to UI components.
Image Handling Improvements:
extensions/cornerstone/src/services/ViewportService/CornerstoneViewportService.ts: During a resize, the slice index is now removed to maintain the current position presentations. This is a temporary fix for a larger issue regarding the definition of slice index with slab thickness.extensions/cornerstone/src/utils/interleaveTopToBottom.ts: Simplified the image load request check using optional chaining.Metadata Management Enhancements:
extensions/default/src/DicomLocalDataSource/index.js: Added logic to handle multiframe instances by breaking each frame into a separate instance and creating new imageIds for each frame. [1] [2]platform/core/src/classes/MetadataProvider.ts: Added error handling for empty imageIds in several methods to ensure robustness. [1] [2] [3]UI Component Updates:
platform/ui-next/src/components/DataRow/DataRow.tsx: Simplified the rendering logic for data rows and updated the details section to handle both primary and secondary details more effectively. [1] [2]Miscellaneous Fixes:
extensions/default/src/DicomJSONDataSource/index.js: Corrected a typo in a comment regarding WADO-URI.modes/preclinical-4d/src/index.tsxandmodes/tmtv/src/index.ts: Updated volume labels to include units (ml). [1] [2]These changes collectively enhance the functionality, reliability, and maintainability of the codebase.