Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Round tap-zoom gestures to nearest integer #8027

Merged
merged 4 commits into from
Feb 13, 2017
Merged

Conversation

friedbunny
Copy link
Contributor

@friedbunny friedbunny commented Feb 11, 2017

iOS changes

Rounds double-tap and two-finger tap zoom gestures to the nearest integer zoom level. This has benefits for raster tiles, as well as styles with zoom-based functions that transition on integers.

This results in a wider possible zoom range per gesticulation — 1.0 → ~0.5-1.5 levels. Here’s a comparison of double-tapping to zoom in with the changes in this PR:

Starting Zoom Old Result New Result
4.0 5.0 (+1.0) 5.0 (+1.0)
4.4 5.4 (+1.0) 5.0 (+0.6)
4.6 5.6 (+1.0) 6.0 (+1.4)

Fixes #2276.

Core changes

Exposes an already-existing setZoom variation that takes an anchor parameter — I did this to avoid calculating scale levels in the iOS zoom code. This makes Map::setZoom(zoom, {}, duration) ambiguous, so it’s now necessary to specify a type for the middle parameter.

@friedbunny friedbunny added Core The cross-platform C++ core, aka mbgl Google Maps parity For feature parity with the Google Maps SDK for Android or iOS iOS Mapbox Maps SDK for iOS MapKit parity For feature parity with MapKit on iOS or macOS labels Feb 11, 2017
@friedbunny friedbunny self-assigned this Feb 11, 2017
@mention-bot
Copy link

@friedbunny, thanks for your PR! By analyzing this pull request, we identified @incanus, @1ec5 and @jfirebaugh to be potential reviewers.

@@ -1556,18 +1556,20 @@ - (void)handleDoubleTapGesture:(UITapGestureRecognizer *)doubleTap
if (doubleTap.state == UIGestureRecognizerStateEnded)
{
MGLMapCamera *oldCamera = self.camera;


double newZoom = round(self.zoomLevel) + 1.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the corresponding changes to the Zoom In and Zoom Out buttons in the Android and macOS SDKs?

if (zoomIn) {
mapView.scaleBy(2.0, x, y, MapboxConstants.ANIMATION_DURATION);
} else {
mapView.scaleBy(0.5, x, y, MapboxConstants.ANIMATION_DURATION);
}

[self setZoomLevel:self.zoomLevel + zoomDelta animated:animated];

/cc @tobrun

Copy link
Contributor Author

@friedbunny friedbunny Feb 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went a bit further on macOS in 4ed80f9 and rounded all of the non-freeform zoom methods gestures and commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll leave Android to @tobrun, in light of #7630.

@friedbunny
Copy link
Contributor Author

friedbunny commented Feb 11, 2017

As an extreme example, here’s Mapbox Streets at z10.99 vs z11.00:

simulator screen shot feb 11 2017 2 59 11 pm copy

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, and thanks also for the changes on the macOS side. I’ll leave it to you to decide whether the Android SDK should get this fix here or in a separate PR.

@@ -39,6 +39,7 @@
* Fixed flickering that occurred when panning past the antimeridian. ([#7574](https://github.com/mapbox/mapbox-gl-native/pull/7574))
* Added a method to MGLMapViewDelegate, `-mapView:shouldChangeFromCamera:toCamera:`, that you can implement to restrict which parts the user can navigate to using gestures. ([#5584](https://github.com/mapbox/mapbox-gl-native/pull/5584))
* Added a `MGLDistanceFormatter` class for formatting geographic distances. ([#7888](https://github.com/mapbox/mapbox-gl-native/pull/7888))
* Double-tap, two-finger tap, zoom buttons, and demo app menu items and shortcut keys now zoom to the nearest integer zoom level. ([#8027](https://github.com/mapbox/mapbox-gl-native/pull/8027))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, the ⌥↑ and ⌥↓ shortcuts are built into MGLMapView. The menu items (which have their own shortcuts) are defined in MapDocument but could conceivably be moved into MGLMapView as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, did not know that — I’ll update the text here.

@1ec5
Copy link
Contributor

1ec5 commented Feb 11, 2017

It looks like #7630 would’ve implemented a similar fix on the Android side. Not sure why @tobrun closed it.

Round double-tap and two-finger tap zoom gestures to the nearest integer zoom level. This has the benefits for raster tiles, as well as styles with zoom-based functions.

This results in a wider possible zoom range — ~0.5-1.5:

Old: z4.6 → z5.6 (+1.0), z4.4 → z5.4 (+1.0)
New: z4.6 → z6.0 (+1.4), z4.4 → z5.0 (+0.6)
Affects:
 - Double-tap gestures
 - Two-finger tap gestures
 - +/- button pushes
 - Shortcut keys
 - Menu items and shortcut keys (in macapp)
@friedbunny friedbunny added the macOS Mapbox Maps SDK for macOS label Feb 11, 2017
@tobrun
Copy link
Member

tobrun commented Feb 13, 2017

@1ec5 @friedbunny I initially created that PR to showcase the possibility of such a feature but this didn't make it to master at that time as I didn't want to change the "default" behaviour. We could definitely look into reopening it for consistency between the bindings.

cc @mapbox/android

@zugaldia
Copy link
Member

We could definitely look into reopening it for consistency between the bindings.

Yeah, let's do that for 5.0. It doesn't look like a big lift.

@1ec5
Copy link
Contributor

1ec5 commented Feb 13, 2017

The Android side is happening once again in #7630. 🚢

@friedbunny friedbunny merged commit d12ac71 into master Feb 13, 2017
@friedbunny friedbunny deleted the fb-integer-zooming branch February 13, 2017 23:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl Google Maps parity For feature parity with the Google Maps SDK for Android or iOS iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS MapKit parity For feature parity with MapKit on iOS or macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants