-
Notifications
You must be signed in to change notification settings - Fork 314
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
Snap to route #57
Snap to route #57
Conversation
|
||
@interface MGLMapView (MGLNavigationAdditions) | ||
|
||
- (void)locationManager:(CLLocationManager *)manager didUpdateLocations:(NSArray *)locations; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be exposed in mapbox-gl-native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in mapbox/mapbox-gl-native#6867, I think the map SDK should instead expose more specific methods like -didUpdateLocationWithUserTrackingAnimated:
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whichever method we choose to override, I'm okay with doing this logic here for the time being while we experiment.
22e2e82
to
f71ec06
Compare
MapboxNavigationUI/MGLMapView.swift
Outdated
func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? | ||
} | ||
|
||
public class NavigationMapView: MGLMapView { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code should go in a separate NavigationMapView.swift file.
Should be make snapping an optional param? |
I think we should also look into snapping bearing/heading. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some opportunities for better naming below.
I agree with #57 (comment) that this implementation is basically a hack (albeit a well-written one!) until we expose a method upstream as part of mapbox/mapbox-gl-native#6867.
@@ -24,6 +24,12 @@ open class RouteController: NSObject { | |||
|
|||
|
|||
/* | |||
If true, the user puck is snapped to closest location on the route. | |||
*/ | |||
public var snapUserToRoute = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“Snap user to route” sounds like a command, so this sounds like an action method instead of a property that can be set. (In Objective-C, an action method and a property getter call can look identical.) Also, it sounds funny to say that we’re moving the user themselves; we’re actually moving the user location annotation.
Let’s call this property snapsUserLocationAnnotationToRoute
. It’s a mouthful, but the developer won’t have to call it often anyways.
MapboxNavigation/Constants.swift
Outdated
/* | ||
Accepted deviation excluding horizontal accuracy before the user is considered to be off route. | ||
*/ | ||
public var RouteControllerHorizontalAccuracyAnomaly: CLLocationDistance = 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This global variable doesn’t represent an anomaly; it represents the maximum deviation distance that can be considered an anomaly. How about calling this global RouteControllerUserLocationSnappingDistance
, to describe the desired output instead of one specific input (which may not be the only output in the future)?
|
||
extension RouteMapViewController: NavigationMapViewDelegate { | ||
|
||
func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a weird name for a method that returns a CLLocation. The name implies that it returns only a Boolean indicating whether the map view should… update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have to do for now, although it’s going to look really weird in Objective-C.
Eventually, as part of mapbox/mapbox-gl-native#6867, we’ll want the MGLMapViewDelegate method upstream to accept an already positioned MGLUserLocation, rather than the raw CLLocationCoordinate2D, so that the heading can also be manipulated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add @objc(navigationMapView:shouldUpdateToLocation:)
to help with Objective-C.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor change requests below in MGLMapView+MGLNavigationAdditions.h.
I’d prefer that we wait until @frederoni or I get a chance to pick up mapbox/mapbox-gl-native#6867, but if we need to land this feature before the next iOS SDK release, then so be it.
|
||
extension RouteMapViewController: NavigationMapViewDelegate { | ||
|
||
func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will have to do for now, although it’s going to look really weird in Objective-C.
Eventually, as part of mapbox/mapbox-gl-native#6867, we’ll want the MGLMapViewDelegate method upstream to accept an already positioned MGLUserLocation, rather than the raw CLLocationCoordinate2D, so that the heading can also be manipulated.
@@ -0,0 +1,7 @@ | |||
#import <Mapbox/Mapbox.h> | |||
|
|||
@interface MGLMapView (MGLNavigationAdditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than redeclare the method below, let’s have this class extension conform to the CLLocationManagerDelegate protocol.
@@ -0,0 +1,7 @@ | |||
#import <Mapbox/Mapbox.h> | |||
|
|||
@interface MGLMapView (MGLNavigationAdditions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add a FIXME:
comment noting that this class extension will be reworked following mapbox/mapbox-gl-native#6867.
Add map to example application; fix example code
WIP
Outlined how snap to route could work.
@1ec5 👀