[core] Tighten LatLng and other geo.hpp class invariants #5286
Conversation
jfirebaugh
force-pushed
the
lat-lng-null
branch
2 times, most recently
from
ae645e0
to
6f21133
Compare
|
@jfirebaugh want to apply this before #8601? |
|
I rebased this on master, but I think it needs attention at the SDK binding point for LatLng -- we want the SDK to enforce the same invariants, but report errors using a platform-appropriate exception mechanism (not |
* Remove LatLng::null and enforce invariants * Remove unnecessary operator bool()
| if (std::isnan(coordinate.longitude)) { | ||
| [NSException raise:NSInvalidArgumentException format:@"longitude must not be NaN"]; | ||
| } | ||
| if (std::abs(coordinate.latitude) > 90.0) { |
There was a problem hiding this comment.
I think we’ll want to push these checks further up. In many places where we call this function, the analogous MapKit API fails silently when given out-of-bounds coordinates; an exception would be heavy-handed. In other cases, a debug assertion is all that’s needed.
There was a problem hiding this comment.
Can you be more specific about what "further up" means in this context, or what specific changes you'd recommend?
There was a problem hiding this comment.
My suggestion was to move these checks to individual call sites, or to add a “checked” version of this function that wraps MGLLatLngFromLocationCoordinate2D(), adding these checks.
I reviewed the existing call sites to MGLLatLngFromLocationCoordinate2D() and tested similar functionality in MapKit using invalid coordinates. Based on MapKit behavior, having the exceptions where they are is OK, although we’ll want to add identical preflight checks in methods like -[MGLMapView convertCoordinate:toPointToView:] that return things like CGPointMake(NAN, NAN).
| Mapbox call site | MapKit equivalent | MapKit behavior | Mapbox expected behavior |
|---|---|---|---|
-[MGLMapView handlePinchGesture:] |
— | Constrained | NSAssert (but NSException is fine) |
-[MGLMapView _setCenterCoordinate:edgePadding:zoomLevel:direction:duration:animationTimingFunction:completionHandler:] |
-[MKMapView setRegion:animated:] |
NSException |
NSException |
-[MGLMapView cameraOptionsObjectForAnimatingToCamera:edgePadding:] |
-[MKMapView setCamera:animated:] |
NSException |
NSException |
-[MGLMapView convertCoordinate:toPointToView:] |
-[MKMapView convertCoordinate:toPointToView:] |
CGPoint(NaN, NaN) |
CGPoint(NaN, NaN) |
-[MGLMapView showAnnotations:edgePadding:animated:] |
-[MKMapView showAnnotations:animated:] |
Moves to invalid part of map, in ocean | NSException is OK |
-[MGLMapView directionByFollowingWithCourse] |
— | — | NSException |
+[MGLMapCamera cameraLookingAtCenterCoordinate:fromEyeCoordinate:eyeAltitude:] |
+[MKMapCamera cameraLookingAtCenterCoordinate:fromEyeCoordinate:eyeAltitude:] |
Camera with invalid coordinates | Camera with invalid coordinates |
-[MGLMultiPoint overlayBounds] |
-[MKMultiPoint boundingMapRect] |
Empty MKMapRect with invalid origin | ? |
-[MGLPointCollection overlayBounds] |
— | — | ? |
There was a problem hiding this comment.
ea20dd5 adds preflight checks to some of the call sites above that avoid the exception and instead return special values indicating invalidity. If you try to convert an invalid coordinate to a point, you get an invalid point. You can create a camera with an invalid coordinate, but you’ll get an exception when you try to move the map to that camera. You can create a polyline, polygon, or point collection with an invalid coordinate, and the overlay will be invalid, but you’ll get an exception when you try to add that shape to the map.
| state.size.height - padding.top - padding.bottom); | ||
| double w0 = padding ? std::max(state.size.width - padding->left - padding->right, | ||
| state.size.height - padding->top - padding->bottom) | ||
| : std::max(state.size.width, state.size.height); |
There was a problem hiding this comment.
Wow, great catch! I wonder if this makes #8300 more difficult to hit. Hard to say just from inspecting the code, since r() is a bit convoluted.
include/mbgl/util/geo.hpp
Outdated
| @@ -21,17 +22,29 @@ using ScreenBox = mapbox::geometry::box<double>; | |||
|
|
|||
| class LatLng { | |||
| public: | |||
| struct null {}; | |||
|
|
|||
| double latitude; | |||
There was a problem hiding this comment.
Can we mark latitude and longitude as const here? We validate their values when initializing LatLng, but these are free to be modified after without checks.
Another option is to move these checks to a setter, and having these private.
Replaced preflight checks in MGLLatLngFromLocationCoordinate2D() with a try-catch block that relays exceptions raised by mbgl. De-inlined the function because it’s no longer trivial.
Invalid coordinates no longer cause an exception to be raised immediately when used in conversion methods and model objects. Instead, the appropriate invalid values are used, consistent with MapKit. Exceptions are still raised when invalid model objects are used with the map.
This has caught two bugs so far:
Transform::flyToFixes #3802. Reroll of #4355.