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 the wrong aspect ratio load #3534

Merged
merged 2 commits into from
Jul 19, 2023
Merged

Conversation

wayfarer3130
Copy link
Contributor

@wayfarer3130 wayfarer3130 commented Jul 12, 2023

Context

The resize listener in OHIFCornerstoneViewport wasn't firing the resize most of the time on DICOM SR load because the wrong parameter was being applied.
Also added fixes to OHIFCornerstoneViewport to provide ready events for image rendered so that the tests can proceed properly. That allows testing the actual expansion/collapse of the viewport rendering properly because it is possible to know when it has been updated.
Finally, a tweak to the scrollbar size to reflect the number of images rather than being a constant size.

Changes & Results

Testing

This test only fails if one has added automatically opening of the measurement panel. I'm actually not sure how to reproduce this inside vanilla OHIF.

Find a study that has a non square image (SIIM Sally dataset with XC works) in fact any dataset with an SR saved is sufficient
Save a DICOM SR to the study
Double click the DICOM SR to load into 1x1 mode
Click the load button, watching the position of measurements. Often, previously, they wouldn't move, AND the aspect ratio of non-square images would change as actually viewed. This is not 100%, but was 20-50%

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • [] My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] OS:
  • [] Node version:
  • [] Browser:

@wayfarer3130 wayfarer3130 requested a review from sedghi July 12, 2023 20:38
@netlify
Copy link

netlify bot commented Jul 12, 2023

Deploy Preview for ohif-platform-docs ready!

Name Link
🔨 Latest commit df9e8de
🔍 Latest deploy log https://app.netlify.com/sites/ohif-platform-docs/deploys/64b6ed6e3bea930008466357
😎 Deploy Preview https://deploy-preview-3534--ohif-platform-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@netlify
Copy link

netlify bot commented Jul 12, 2023

Deploy Preview for ohif-dev ready!

Name Link
🔨 Latest commit df9e8de
🔍 Latest deploy log https://app.netlify.com/sites/ohif-dev/deploys/64b6ed6edbf6310008bc214a
😎 Deploy Preview https://deploy-preview-3534--ohif-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Jul 12, 2023

Codecov Report

Merging #3534 (df9e8de) into master (18156bb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3534   +/-   ##
=======================================
  Coverage   42.75%   42.75%           
=======================================
  Files          82       82           
  Lines        1450     1450           
  Branches      338      338           
=======================================
  Hits          620      620           
  Misses        667      667           
  Partials      163      163           

Continue to review full report in Codecov by Sentry.

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

@cypress
Copy link

cypress bot commented Jul 12, 2023

Passing run #3376 ↗︎

0 36 0 0 Flakiness 0

Details:

Merge branch 'master' into fix/wrong-aspect-ratio-load-sr
Project: Viewers Commit: df9e8def6c
Status: Passed Duration: 03:51 💡
Started: Jul 18, 2023 7:58 PM Ended: Jul 18, 2023 8:02 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@sedghi
Copy link
Member

sedghi commented Jul 13, 2023

I remember this weird effect when I used throttle for resize, that is why I made it debounce, though throttle seem much more smooth, but it has these werid flickerings

OHIF-Viewer.1.webm

@wayfarer3130 wayfarer3130 force-pushed the fix/wrong-aspect-ratio-load-sr branch from c4794e0 to 805612f Compare July 15, 2023 12:55
@wayfarer3130 wayfarer3130 changed the title Fix the wrong aspect ratio load. Also some integration test fixes Fix the wrong aspect ratio load Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants