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

Change camera zoom to not pick positions in the middle of a zoom. #4488

Merged
merged 5 commits into from
Oct 24, 2016

Conversation

emackey
Copy link
Contributor

@emackey emackey commented Oct 20, 2016

This fixes an issue where if you start zooming in from some point off the globe, when you're close enough that the limb of the globe touches your starting point, the globe suddenly twists.

This should also fix #4177 and #4440, but I could use some help testing.

Can someone test if this fixes the illusive zoom-crash? @amc723 @smills2929 @willemvdg @lucasvw

/cc @bagnell

@jbo023
Copy link
Contributor

jbo023 commented Oct 21, 2016

We checked this branch against the bug, we filed some time ago: #4407.
We could not reproduce the old Bug.

So it looks nice 👍

@jbo023
Copy link
Contributor

jbo023 commented Oct 21, 2016

During testing i also noted another Bug with touch pinch gestures.

If you start with a pinch gesture the origin of the zoom/rotate gesture is in the upperleft corner of the screen. (Only if this is the first touch/mouse event)

If you then do a short one finger movement the next pinch gesture will use the that movement position as a new origin.

Maybe you can also check this out.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

@emackey is it possible to write a reasonable unit test for this?

Also update CHANGES.md.

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 21, 2016

CC @pmconne

@mramato
Copy link
Contributor

mramato commented Oct 21, 2016

@emackey I can confirm that this fixes the "upper left pixel zoom" crash you mentioned previously as well as the "pinch first" bug. Awesome! I agree it would be nice to add a test for this if we can, if perhaps indirectly test it at least if it was an initialization issue. Even a simple test without any expects that crashed in master but now doesn't crash now would be good.

If tests can't be done cleanly, I'm okay with merging this as-is.

@emackey
Copy link
Contributor Author

emackey commented Oct 21, 2016

Thanks everyone for helping to test!

Added a unit test for this.

@emackey
Copy link
Contributor Author

emackey commented Oct 21, 2016

@jbo023 I think I addressed your additional bug in 52e1a90. A start position was not being computed for the pinch, now it is. Thanks!

@mramato
Copy link
Contributor

mramato commented Oct 21, 2016

I can confirm the new test fails in master and passes in this branch and everything looks good to me.

@bagnell or someone with more camera knowledge should probably merge this.

@emackey emackey mentioned this pull request Oct 21, 2016
3 tasks
@mmccartn
Copy link

Tested and can confirm this fixes #4177. Thanks

@jbo023
Copy link
Contributor

jbo023 commented Oct 24, 2016

Tested again, origin on pinch gestures is now correct. Thanks

@bagnell bagnell merged commit 3728c44 into master Oct 24, 2016
@bagnell bagnell deleted the zoom-center-fix branch October 24, 2016 19:28
@bagnell
Copy link
Contributor

bagnell commented Oct 24, 2016

I updated CHANGES.md in master 39169f7

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 25, 2016

@lilleyse can you please merge master to 3d-tiles so @pmconne has this fix?

@lilleyse
Copy link
Contributor

Done.

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.

Zoom Crash from Touch Input on Viewer Initialization
7 participants