Snap to route #57

frederoni · 2017-03-04T01:25:08Z

WIP
Outlined how snap to route could work.

frederoni · 2017-03-04T01:26:02Z

MapboxNavigationUI/MGLMapView+MGLNavigationAdditions.h

+
+@interface MGLMapView (MGLNavigationAdditions)
+
+- (void)locationManager:(CLLocationManager *)manager didUpdateLocations:(NSArray *)locations;


This should be exposed in mapbox-gl-native

As I mentioned in mapbox/mapbox-gl-native#6867, I think the map SDK should instead expose more specific methods like -didUpdateLocationWithUserTrackingAnimated:.

Whichever method we choose to override, I'm okay with doing this logic here for the time being while we experiment.

1ec5 · 2017-03-10T20:40:09Z

MapboxNavigationUI/MGLMapView.swift

+    func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation?
+}
+
+public class NavigationMapView: MGLMapView {


This code should go in a separate NavigationMapView.swift file.

bsudekum · 2017-03-10T22:04:55Z

Should be make snapping an optional param?

bsudekum · 2017-03-22T00:35:35Z

I think we should also look into snapping bearing/heading.

bsudekum · 2017-03-22T01:10:42Z

@1ec5 this is ready for review again. Made snapping optional with 15da62d

1ec5

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.

1ec5 · 2017-03-22T07:31:54Z

MapboxNavigation/RouteController.swift

@@ -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


“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.

1ec5 · 2017-03-22T07:40:35Z

MapboxNavigation/Constants.swift

+/*
+ Accepted deviation excluding horizontal accuracy before the user is considered to be off route.
+ */
+public var RouteControllerHorizontalAccuracyAnomaly: CLLocationDistance = 10


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)?

1ec5 · 2017-03-22T18:36:20Z

MapboxNavigationUI/RouteMapViewController.swift

+
+extension RouteMapViewController: NavigationMapViewDelegate {
+
+    func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? {


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.

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.

Maybe add @objc(navigationMapView:shouldUpdateToLocation:) to help with Objective-C.

1ec5

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.

1ec5 · 2017-03-22T18:47:04Z

MapboxNavigationUI/RouteMapViewController.swift

+
+extension RouteMapViewController: NavigationMapViewDelegate {
+
+    func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? {


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.

1ec5 · 2017-03-22T18:48:37Z

MapboxNavigationUI/MGLMapView+MGLNavigationAdditions.h

@@ -0,0 +1,7 @@
+#import <Mapbox/Mapbox.h>
+
+@interface MGLMapView (MGLNavigationAdditions)


Rather than redeclare the method below, let’s have this class extension conform to the CLLocationManagerDelegate protocol.

1ec5 · 2017-03-22T18:49:23Z

MapboxNavigationUI/MGLMapView+MGLNavigationAdditions.h

@@ -0,0 +1,7 @@
+#import <Mapbox/Mapbox.h>
+
+@interface MGLMapView (MGLNavigationAdditions)


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

frederoni commented Mar 4, 2017

View reviewed changes

frederoni added 3 commits March 9, 2017 14:40

Snap to route

14016fc

Tweak userIsOnRoute

beedcac

Avoid infinite rerouting

f71ec06

frederoni force-pushed the fred-snap-to-route branch from 22e2e82 to f71ec06 Compare March 9, 2017 22:40

Merge branch 'master' into fred-snap-to-route

17b0f27

1ec5 reviewed Mar 10, 2017

View reviewed changes

bsudekum mentioned this pull request Mar 11, 2017

Deal with GPS noise #71

Closed

Merge branch 'master' into fred-snap-to-route

f80800c

bsudekum added 2 commits March 21, 2017 17:44

move NavigationMapView to own file

accc5f3

Make snapping optional

15da62d

1ec5 reviewed Mar 22, 2017

View reviewed changes

improve var names

123763c

1ec5 reviewed Mar 22, 2017

View reviewed changes

1ec5 approved these changes Mar 22, 2017

View reviewed changes

improvements

7394d33

bsudekum merged commit 8329ed9 into master Mar 22, 2017

bsudekum deleted the fred-snap-to-route branch March 22, 2017 19:49

This was referenced Mar 23, 2017

Make routeController public while using MapboxNavigationUI #85

Merged

Improve off route calculation #58

Closed

frederoni mentioned this pull request Mar 27, 2017

Don't continual reroute user if standing still, allow user time to get to route start #84

Merged

1ec5 mentioned this pull request Apr 25, 2017

MapboxNavigation podspec fails validation #172

Closed

1ec5 mentioned this pull request May 16, 2017

MGLMapView location/course tracking should be overrideable mapbox/mapbox-gl-native#6867

Closed

1ec5 mentioned this pull request Jul 1, 2017

Allow overriding the location puck's location #324

Closed

wishtrip-dev pushed a commit to wishtrip-dev/mapbox-navigation-ios that referenced this pull request Feb 28, 2018

Merge pull request mapbox#57 from mapbox/1ec5-route-map-56

3763f2e

Add map to example application; fix example code

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snap to route #57

Snap to route #57

frederoni commented Mar 4, 2017

frederoni Mar 4, 2017

1ec5 Mar 4, 2017

bsudekum Mar 4, 2017

1ec5 Mar 10, 2017

bsudekum commented Mar 10, 2017

bsudekum commented Mar 22, 2017

bsudekum commented Mar 22, 2017

1ec5 left a comment

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 left a comment

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017

1ec5 Mar 22, 2017


		@interface MGLMapView (MGLNavigationAdditions)

		- (void)locationManager:(CLLocationManager )manager didUpdateLocations:(NSArray )locations;


		extension RouteMapViewController: NavigationMapViewDelegate {

		func navigationMapView(_ mapView: NavigationMapView, shouldUpdateTo location: CLLocation) -> CLLocation? {

		@@ -0,0 +1,7 @@
		#import <Mapbox/Mapbox.h>

		@interface MGLMapView (MGLNavigationAdditions)

Snap to route #57

Snap to route #57

Conversation

frederoni commented Mar 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Mar 10, 2017

bsudekum commented Mar 22, 2017

bsudekum commented Mar 22, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment