Add support for pinch zoom to block rotation (fix accidental rotation on pinch) #1917
Conversation
In Google Maps, and other UI interfaces which support both pinch and rotate gestures, it is common for the pinch gesture to block the rotate if the sole intent of the user was to zoom. We determine this 'intent' by a simple race condition. If the pinch gesture surpasses a certain threshold before the rotate, then the intent is assumed to be zooming. If the rotation surpasses it's threshold first, then both pinch zoom AND rotate are allowed.
| this._rotatingSignificantly = false; | ||
| this._blockRotation = undefined; | ||
|
|
||
| // TODO(jliebrand): Move these to config options on the map? |
There was a problem hiding this comment.
Nah, this class is a good place for _significantScaleThreshold and _significantRotateThreshold. We should hold off on making them user configurable unless an good use case arises.
|
This looks great @liebrand! Thank you for the contribution! Once we've addressed the minor changes suggested in the comments above, this can be squashed and merged. |
|
Codewise this looks great. Let's actually try this on a phone to see how it feels before merging though. |
|
@mourner definitely! I've only managed to test this on my Nexus 5X where it "feels" right, but the more people that can try this out the better! Preferably on a variety of different phones, and then compare it to how Google Maps 'feels' wrt this feature. We can then tweak the 0.1% and 5 degree thresholds if needed |
|
Hey @liebrand this looks great! I played around with it a bunch tonight on my iPhone5... It's easy to hit the 10% scale threshold when your fingers are close together so I found that biasing it a bit more towards rotation makes it feel a little more natural. I adjusted the params to: Try that out and let me know what you think.. Also I'll add some other thoughts in the comments. |
| } | ||
|
|
||
| // Only set the bearing if rotation is allowed AND we are rotating more than our threshold | ||
| var updateBearing = ((this._blockRotation === false) && this._rotatingSignificantly); |
There was a problem hiding this comment.
So, I noticed that other native maps seem to lock in the intent first and then allow the movement. The way you are calling easeTo lets a little zoom movement sneak in before it's clear what the user wants. Anyway, this is what I did - try it and let me know what you think:
var updateBearing = ((this._blockRotation === false) && this._rotatingSignificantly),
param = {duration: 0, around: map.unproject(p)};
if (updateBearing) {
param.bearing = this._startBearing + bearing;
}
if (this._blockRotation !== undefined) {
param.zoom = map.transform.scaleZoom(this._startScale * scale);
}
map.easeTo(param);
|
committed as 4166ec3 |
|
Hey, sorry I've been a bit quiet on my side... this annoying thing called a day job keeps pestering me ;-) I see it's been committed now? Didn't you want to change/tweak the constants slightly? I'm happy either which way |
No problem, we did want to get a release out today so I committed it with the tweaked constants. If you try it out and it's not ok, let me know and we can revisit.. |
|
Oh no that's fine. I very much suspect your tweaked values are better than my original ones. It's just that when I looked at the commit I thought I saw my original values? |
|
Yes I ended up keeping the new values, but the commits got a little crazy because I interactively cherry picked stuff from #1917, #1920, and my own branch. |
In Google Maps, and other UI interfaces which support both pinch and rotate gestures, it is common for the pinch gesture to block the rotate if the sole intent of the user was to zoom.
We determine this 'intent' by a simple race condition. If the pinch gesture surpasses a certain threshold before the rotate, then the intent is assumed to be zooming.
If the rotation surpasses it's threshold first, then both pinch zoom AND rotate are allowed.