Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: multiframe image prefetch logic #3097

Open
wants to merge 1 commit into
base: v2-legacy
Choose a base branch
from

Conversation

slak44
Copy link
Contributor

@slak44 slak44 commented Jan 5, 2023

When the StudyPrefetcher is enabled, it prefetches the images of the current stack (and a prop is passed to react-cornerstone-viewport to stop it from prefetching).

Unfortunately, the code in StudyPrefetcher that computed which imageIds should be fetched was broken for multiframe images. As a result, nothing except the current image got loaded (triggering a load on every stack scroll, which is not great).

The first issue was that StudyPrefetcher used a non existent property numFrames, which made it think all images have a single frame (undefined > 1 is false).
The second issue was that the getImageId standalone function delegates to InstanceMetadata#getImageId, but without passing along its arguments (the frame argument, in particular).
The third issue was that InstanceMetadata#getImageId cached its return value, but that's not ok to do when the frame argument can vary.

@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Merging #3097 (1a86208) into master (fe57c00) will decrease coverage by 0.73%.
The diff coverage is 2.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3097      +/-   ##
==========================================
- Coverage   12.68%   11.94%   -0.74%     
==========================================
  Files         306      306              
  Lines        8233     8437     +204     
  Branches     1593     1679      +86     
==========================================
- Hits         1044     1008      -36     
- Misses       5797     5942     +145     
- Partials     1392     1487      +95     
Impacted Files Coverage Δ
...latform/core/src/DICOMSR/SCOORD3D/parseSCOORD3D.js 2.66% <0.00%> (-0.73%) ⬇️
.../core/src/DICOMSR/SCOORD3D/utils/addMeasurement.js 5.40% <ø> (ø)
...re/src/DICOMSR/SCOORD3D/utils/getRenderableData.js 1.72% <0.00%> (-0.07%) ⬇️
...orm/core/src/DICOMSR/parseDicomStructuredReport.js 7.50% <0.00%> (-1.60%) ⬇️
platform/core/src/classes/HotkeysManager.js 76.66% <0.00%> (ø)
platform/core/src/classes/StudyLoadingListener.js 1.03% <0.00%> (+0.01%) ⬆️
platform/core/src/classes/StudyPrefetcher.js 0.69% <0.00%> (ø)
.../core/src/classes/metadata/OHIFInstanceMetadata.js 0.00% <0.00%> (ø)
...latform/core/src/classes/metadata/StudyMetadata.js 1.25% <0.00%> (-0.04%) ⬇️
.../core/src/measurements/tools/dicomSRDisplayTool.js 50.00% <0.00%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfa697a...1a86208. Read the comment docs.

@sedghi sedghi changed the base branch from master to v2-legacy June 19, 2023 13:12
@sedghi
Copy link
Member

sedghi commented Jun 19, 2023

base has changed, read more here #3477

@sedghi sedghi added the v2-legacy OHIF v2 label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2-legacy OHIF v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants