-
Notifications
You must be signed in to change notification settings - Fork 76
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
imviz matched zoom: use center & radius instead of corners #3215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks better! I will test on Rampviz later, just wanted to log that this implementation looks good to me.
Looks like there is still jittering in Rampviz with 3 viewers (but not with 2 viewers or Imviz with 3 viewers)... will continue to investigate and see if I can find a more general solution 🤞 |
56e9a43
to
74e9ce9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah! This looks a lot better now. There's still >1 zoom reset happening in the matched viewers, but it's happening really fast for me now.
Thank you!
Oh I forgot I still need to update tests 🤦♂️ I'll put in draft and work on that - glad to hear it seems to have fixed the use-case though! |
I also confirmed that it fixed the behavior I was seeing in Rampviz, I'll do a more thorough review once you update the tests. |
c08dab4
to
3104c03
Compare
Requires #3222 to pass remaining failing tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test revisions make sense. Thanks!
de20cac
to
492a0dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, looks fine.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3215 +/- ##
==========================================
+ Coverage 88.56% 88.62% +0.05%
==========================================
Files 125 125
Lines 18758 18775 +17
==========================================
+ Hits 16613 16639 +26
+ Misses 2145 2136 -9 ☔ View full report in Codecov by Sentry. |
492a0dd
to
cca2396
Compare
…ius instead of corners
… corners (#3226) Co-authored-by: Kyle Conroy <[email protected]>
Description
This pull request fixes matched image box zoom when the matched viewers have different aspect ratios.'
TODO: this breaks the old tests that expect the same limits to be returnedBefore:
Screen.Recording.2024-10-08.at.1.10.41.PM.mov
After:
Screen.Recording.2024-10-08.at.1.10.02.PM.mov
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.