search

mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.38k stars 1.32k forks source link

Crash rotating padded map — latitude must not be NaN #15165

Open 1ec5 opened 5 years ago

1ec5 commented 5 years ago

A map that has a top and bottom content inset crashes when the user rotates it with two fingers.

Steps to reproduce

Add the following code to any view controller with a full-screen map:

- (void)mapView:(MGLMapView *)mapView didFinishLoadingStyle:(MGLStyle *)style {
    self.automaticallyAdjustsScrollViewInsets = NO;
    self.mapView.contentInset = UIEdgeInsetsMake(CGRectGetHeight(self.mapView.bounds) * 0.8, CGRectGetWidth(self.mapView.bounds) * 0, CGRectGetHeight(self.mapView.bounds) * 0.2, 0);
    MGLMapCamera *camera = [MGLMapCamera cameraLookingAtCenterCoordinate:CLLocationCoordinate2DMake(39.101944,-84.509842) altitude:1000 pitch:60 heading:249];
    [self.mapView setCamera:camera withDuration:0 animationTimingFunction:nil edgePadding:UIEdgeInsetsZero completionHandler:nil];
}

For example, iosapp:

diff --git a/platform/ios/app/MBXViewController.m b/platform/ios/app/MBXViewController.m
index 2fb95e1b17..702df27eab 100644
--- a/platform/ios/app/MBXViewController.m
+++ b/platform/ios/app/MBXViewController.m
@@ -2253,6 +2253,11 @@ CLLocationCoordinate2D randomWorldCoordinate() {
     // that a device with an English-language locale is already effectively
     // using locale-based country labels.
     _localizingLabels = [[self bestLanguageForUser] isEqualToString:@"en"];
+    
+    self.automaticallyAdjustsScrollViewInsets = NO;
+    self.mapView.contentInset = UIEdgeInsetsMake(CGRectGetHeight(self.mapView.bounds) * 0.8, CGRectGetWidth(self.mapView.bounds) * 0, CGRectGetHeight(self.mapView.bounds) * 0.2, 0);
+    MGLMapCamera *camera = [MGLMapCamera cameraLookingAtCenterCoordinate:CLLocationCoordinate2DMake(39.101944,-84.509842) altitude:1000 pitch:60 heading:249];
+    [self.mapView setCamera:camera withDuration:0 animationTimingFunction:nil edgePadding:UIEdgeInsetsZero completionHandler:nil];
 }

 - (BOOL)mapView:(MGLMapView *)mapView shouldChangeFromCamera:(MGLMapCamera *)oldCamera toCamera:(MGLMapCamera *)newCamera {

Wait for the map to fully load, then perform a two-finger rotation gesture in either direction.

Logs

The crash occurs here:

https://github.com/mapbox/mapbox-gl-native/blob/4a93f39700d455ee37f1b212adf7d4648cde137c/include/mbgl/util/geo.hpp#L34-L36

#1  0x0000000107676abd in mbgl::LatLng::LatLng(double, double, mbgl::LatLng::WrapMode) at /path/to/mapbox-gl-native/include/mbgl/util/geo.hpp:35
#2  0x0000000107676979 in mbgl::LatLng::LatLng(double, double, mbgl::LatLng::WrapMode) at /path/to/mapbox-gl-native/include/mbgl/util/geo.hpp:33
#3  0x00000001079cca23 in mbgl::Projection::unproject(mapbox::geometry::point<double> const&, double, mbgl::LatLng::WrapMode) at /path/to/mapbox-gl-native/include/mbgl/util/projection.hpp:88
#4  0x00000001079d5039 in mbgl::TransformState::screenCoordinateToLatLng(mapbox::geometry::point<double> const&, mbgl::LatLng::WrapMode) const at /path/to/mapbox-gl-native/src/mbgl/map/transform_state.cpp:328
#5  0x00000001079c9870 in mbgl::Transform::screenCoordinateToLatLng(mapbox::geometry::point<double> const&, mbgl::LatLng::WrapMode) const at /path/to/mapbox-gl-native/src/mbgl/map/transform.cpp:576
#6  0x00000001079be911 in mbgl::cameraForLatLngs(std::__1::vector<mbgl::LatLng, std::__1::allocator<mbgl::LatLng> > const&, mbgl::Transform const&, mbgl::EdgeInsets const&) at /path/to/mapbox-gl-native/src/mbgl/map/map.cpp:229
#7  0x00000001079be0ca in mbgl::Map::cameraForLatLngs(std::__1::vector<mbgl::LatLng, std::__1::allocator<mbgl::LatLng> > const&, mbgl::EdgeInsets const&, std::experimental::optional<double>, std::experimental::optional<double>) const at /path/to/mapbox-gl-native/src/mbgl/map/map.cpp:235
#8  0x00000001079bdfac in mbgl::Map::cameraForLatLngBounds(mbgl::LatLngBounds const&, mbgl::EdgeInsets const&, std::experimental::optional<double>, std::experimental::optional<double>) const at /path/to/mapbox-gl-native/src/mbgl/map/map.cpp:169
#9  0x000000010772ce7e in ::-[MGLMapView cameraThatFitsCoordinateBounds:edgePadding:](MGLCoordinateBounds, UIEdgeInsets) at /path/to/mapbox-gl-native/platform/ios/src/MGLMapView.mm:3662
#10 0x000000010772cbc0 in ::-[MGLMapView cameraThatFitsCoordinateBounds:](MGLCoordinateBounds) at /path/to/mapbox-gl-native/platform/ios/src/MGLMapView.mm:3650
#11 0x0000000107711a15 in ::-[MGLMapView cameraByZoomingToZoomLevel:aroundAnchorPoint:](double, CGPoint) at /path/to/mapbox-gl-native/platform/ios/src/MGLMapView.mm:2081
#12 0x000000010770c9e5 in ::-[MGLMapView handlePinchGesture:](UIPinchGestureRecognizer *) at /path/to/mapbox-gl-native/platform/ios/src/MGLMapView.mm:1686

Here’s some of the console spew leading up to the crash:

-[MGLAnnotationView setCenter:] - 122: Setting center: {181, 575.33333333333337}
2019-07-18 16:01:17.997956-0700 Mapbox GL[24780:16398126] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.101944, lon: -84.509842) altitude: 1000m pitch: 60.000000° heading: 249.000000°
2019-07-18 16:01:17.998246-0700 Mapbox GL[24780:16398126] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.101944, lon: -84.509842) altitude: 1000m pitch: 60.000000° heading: 249.000000°
2019-07-18 16:01:17.998523-0700 Mapbox GL[24780:16398126] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.101944, lon: -84.509842) altitude: 1000m pitch: 60.000000° heading: 249.000000°
2019-07-18 16:01:19.835907-0700 Mapbox GL[24780:16398126] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.101944, lon: -84.509842) altitude: 1000m pitch: 60.000000° heading: 249.000000°
2019-07-18 16:01:19.836505-0700 Mapbox GL[24780:16398126] [ERROR] {}[General]: Unable to calculate appropriate zoom level for bounds. Vertical or horizontal padding is greater than map's height or width.
libc++abi.dylib: terminating with uncaught exception of type std::domain_error
2019-07-18 16:04:55.397865-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:55.397993-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:57.188128-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:57.188431-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:57.189068-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:57.189240-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:58.485414-0700 Mapbox GL[24807:16404346] [DEBUG] -[MGLMapCamera initWithCenterCoordinate:altitude:pitch:heading:] - 95: Initializing withCenterCoordinate: (lat: 39.094539, lon: -84.510441) altitude: 1000m pitch: 60.000000° heading: 298.296944°
2019-07-18 16:04:58.485905-0700 Mapbox GL[24807:16404346] [ERROR] {}[General]: Unable to calculate appropriate zoom level for bounds. Vertical or horizontal padding is greater than map's height or width.
libc++abi.dylib: terminating with uncaught exception of type std::domain_error

Diagnosis

The passed-in padding is zero on all sides, despite the warning about vertical or horizontal padding. However, it does seem incorrect that -[MGLMapView cameraByZoomingToZoomLevel:aroundAnchorPoint:] calls -cameraThatFitsCoordinateBounds: rather than -cameraThatFitsCoordinateBounds:edgePadding:.

https://github.com/mapbox/mapbox-gl-native/blob/4a93f39700d455ee37f1b212adf7d4648cde137c/platform/ios/src/MGLMapView.mm#L2081

Configuration

Mapbox SDK versions: 4a93f39700d455ee37f1b212adf7d4648cde137c iOS/macOS versions: iOS 12.2 Device/simulator models: iPhone X Xcode version: 10.2.1

/cc @mapbox/maps-ios @astojilj

1ec5 commented 5 years ago

The passed-in padding is zero on all sides, despite the warning about vertical or horizontal padding.

As of #14813, the content inset is added to this padding. The code snippet above sets the top inset to 80% of the view’s height and the bottom inset to 20% of the view’s height. Together, the insets add up to the full height of the view. While it may seem rather contrived to pad the entire view, this is in fact how various implementations affix the user puck at just the right spot on the screen.

astojilj commented 5 years ago

this is in fact how various implementations affix the user puck at just the right spot on the screen.

@1ec5 , This seems like a wrong approach to me - to set content size to 0 height. Is it something that customers are using as a warkaround?

astojilj commented 5 years ago

@chloekraw , @1ec5

chloekraw added the release blocker label 16 hours ago

Suggest removing release blocker: this is reproducible (the same exception is thrown throw std::domain_error("latitude must not be NaN");) using iosapp with release-mojito, too. I didn't check release before mojito. I'd like to verify usage and purpose of doing it...

1ec5 commented 5 years ago

This seems like a wrong approach to me - to set content size to 0 height. Is it something that customers are using as a warkaround?

In fact, on a tilted map, setting the edge padding (not the content inset) like this is the only way to ensure that the map’s center coordinate is at the desired screen coordinate – important for positioning the user puck at the correct location. We implemented this approach in mapbox/mapbox-navigation-ios#402 because of the need to synchronize the user puck view with a specific geographic coordinate while keeping the view’s screen coordinate fixed:

https://github.com/mapbox/mapbox-navigation-ios/blob/55dad5ff424e3af8ba43688dc542b84ad2319cfc/MapboxNavigation/NavigationMapView.swift#L340-L343

The map SDK takes a somewhat different approach for its built-in puck:

https://github.com/mapbox/mapbox-gl-native/blob/4a93f39700d455ee37f1b212adf7d4648cde137c/platform/ios/src/MGLMapView.mm#L5655-L5671

which is easier to see if we simplify the formula:

CGRect bounds = self.bounds;
return UIEdgeInsetsMake(correctPoint.y - CGRectGetMidY(bounds),
                        correctPoint.x - CGRectGetMidX(bounds),
                        CGRectGetMidY(bounds) - correctPoint.y,
                        CGRectGetMidX(bounds) - correctPoint.x);

The map SDK’s edge padding results in a screen rectangle as large as the original content frame, shifted according to the desired anchor point. But this approach is inaccurate because, on a tilted map, the scale varies as you go from the bottom of the screen to the top of the screen. It’s even more inaccurate if there’s an additional top content inset.

The navigation SDK’s approach would be unnecessary if there were a way to set the map’s camera with respect to an anchor point, as there is internally for zooming and rotating.

1ec5 commented 5 years ago

I think it would be OK to disallow covering the entire map with content insets, as long as it remains valid to cover the entire map with non-persistent edge padding, as required for user location tracking. If we disallow full-view content insets, I think the exception should happen when setting the content insets, not when the user rotates the map and the map view asks its delegate whether rotating should be allowed.