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/prefetch beyond bounds #1437

Merged

Conversation

wayfarer3130
Copy link
Contributor

Bug Fix for prefetching an undefined imageId
Current behaviour:
On prefetching a stack, when the LAST image is displayed first, the prefetch tries to fetch an undefined imageId.
This is caused by the nearestIndex behaviour, which returns -1 for nearest when the requested item is beyond the bounds.

New Behaviour:
The response of nearestIndex was changed to always return valid indices, in specific
low: index size-1
high: 0 for the nearest item, so that the bounds are valid.
Also, the implementation performance is quite a bit better as it doesn't involve creating a list of size n just to determine the bounds location, making the new implementation O(n) rather than O(n log n) as it was previously (the log n factor was caused by inserting into the array).

The PR currently has the code for the overlay fix in it, which should be merged before this one is merged.

@wayfarer3130
Copy link
Contributor Author

@igoroctaviano - could you take a look?

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #1437 (d09a979) into master (9d569ba) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1437      +/-   ##
==========================================
- Coverage   19.87%   19.85%   -0.02%     
==========================================
  Files         285      285              
  Lines       10040    10036       -4     
  Branches     2045     2045              
==========================================
- Hits         1995     1993       -2     
+ Misses       6845     6843       -2     
  Partials     1200     1200              
Impacted Files Coverage Δ
src/stackTools/stackPrefetch.js 4.96% <0.00%> (-1.10%) ⬇️
src/util/uuidv4.js 0.00% <0.00%> (ø)

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 9d569ba...d09a979. Read the comment docs.

@wayfarer3130 wayfarer3130 force-pushed the fix/prefetchBeyondBounds branch from 217bf6a to fb91918 Compare October 13, 2021 14:31
@wayfarer3130 wayfarer3130 force-pushed the fix/prefetchBeyondBounds branch from fb91918 to 45bc894 Compare October 22, 2021 16:01
@wayfarer3130
Copy link
Contributor Author

@igoroctaviano - could you take a look. The defect is that the stack prefetch generated an undefined URL, which was then attempted to be fetched. The fix is in stackPrefetch (the rest of the changes are cumulative), and just sets it up to avoid hitting an undefined URL.

@igoroctaviano igoroctaviano self-requested a review October 22, 2021 18:15
@igoroctaviano igoroctaviano merged commit d884d4d into cornerstonejs:master Oct 22, 2021
@dannyrb
Copy link
Member

dannyrb commented Oct 22, 2021

🎉 This PR is included in version 6.0.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants