Enable simulated navigation for tunnel segments. #1218

vincethecoder · 2018-03-12T21:13:04Z

This will enable the simulated navigation whenever the commuter enters a tunnel until they exit the tunnel.

TODO:

Accompanied Issue Ticket
Tests
Documentation
Class dedicated to Simulated Tunnel Navigation

…nce.

# Conflicts: # MapboxCoreNavigation/Constants.swift

# Conflicts: # MapboxCoreNavigation/RouteController.swift # MapboxNavigation/NavigationViewController.swift # MapboxNavigation/StyleManager.swift

…to start/stop animation.

bsudekum · 2018-03-12T23:19:38Z

MapboxCoreNavigation/RouteController.swift

+            if let currentIntersection = routeProgress.currentLegProgress.currentStepProgress.currentIntersection,
+                let classes = currentIntersection.outletRoadClasses {
+                if classes.contains(.tunnel) {
+                    startTunnelAnimation(for: manager, routeProgress: routeProgress, distanceTraveled: distanceTraveled)


Why is distanceTraveled being passed in here?

When location simulation is needed in special cases such as dead reckoning, the distanceTraveled on the given route determines where the SimulatedLocationManager needs to begin the simulation.

… SimulatedLocationManager. Additional logic to stop simulated location updates when user halts navigation.

frederoni · 2018-03-13T16:41:38Z

MapboxCoreNavigation/SimulatedLocationManager.swift

+     - parameter distanceTraveled: The distance traveled on the current route.
+     - returns: A `SimulatedLocationManager`
+     */
+    @objc public init(route: Route, distanceTraveled: CLLocationDistance) {


Would it be more versatile if we would pass a from:CLLocationCoordinate2D,to:CLLocationCoordinate2D?
As it stands now, we don't stop simulation after simulating through a tunnel if you would get stuck or slow down in the middle, right? Slicing the route could possibly fix that.

@frederoni a couple of things.

Yes, we stop simulation after simulating through a tunnel

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Line 584 in 544c5d1

stopTunnelAnimation(for: manager)

We don't need to slice, as the SimulatedLocationManager provides us with a robust data based on the currentDistance (a.k.a. distanceTraveled). Given the currentDistance, the manager can determine the current location on the given route - which is used to determine factors such as expectedSegmentTravelTimes, closestCoordinateOnRoute, nextCoordinateOnRoute and time.

mapbox-navigation-ios/MapboxCoreNavigation/SimulatedLocationManager.swift

Line 137 in 544c5d1

guard let newCoordinate = polyline.coordinateFromStart(distance: currentDistance) else {

👍
I overlooked this part:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Lines 583 to 585 in 544c5d1

} else {

stopTunnelAnimation(for: manager)

}

and thought we would just continue simulating to the destination.

frederoni · 2018-03-13T16:42:31Z

MapboxCoreNavigation/RouteController.swift

+        simulatedLocationManager?.delegate = self
+        simulatedLocationManager?.routeProgress = routeProgress
+
+        let dispatchGroup = DispatchGroup()


Can you explain the reasoning behind using this dispatch group?

@frederoni Sure.

This ensures we don't encounter any concurrency (dispatch) race between the local locationManager and the simulatedLocationManager. Basically, when we dispatch using a group, we are assured that our local locationManager will stop stopUpdatingLocation before the simulatedLocationManager starts startUpdatingLocation.

This facilitates a smooth handshake between using our RouteController.locationManager and RouteController.simulatedLocationManager where we will not have both running at the same time or vice versa.

frederoni · 2018-03-13T16:46:45Z

refactor to use a delegate approach in NavigationLocationManager. This will allow for ReplayLocationManager and SimulatedLocationManager to utilize this feature.

Would replaying using ReplayLocationManager also make use of SimulatedLocationManager?

vincethecoder · 2018-03-13T20:27:31Z

@frederoni Yes, I've considered using a delegate approach in NavigationLocationManager but it should be tracked in another PR. The delegate approach will make this PR bloated. NavigationLocationManager is used by several difference sub classes. It makes more sense to just simulate navigation using this approach and get some field tests going to address all possible tunnel simulation conditions.

Also, ReplayLocationManager and SimulatedLocationManager use 2 different approaches in simulating navigation as well. We can find some common ground, however I don't think it should be tracked in this PR. A follow up PR would be ideal.

mapbox-navigation-ios/MapboxCoreNavigation/SimulatedLocationManager.swift

Line 132 in 544c5d1

@objc fileprivate func tick() {

mapbox-navigation-ios/MapboxCoreNavigation/ReplayLocationManager.swift

Line 55 in 544c5d1

@objc fileprivate func tick() {

cc @bsudekum @1ec5

frederoni · 2018-03-13T20:33:10Z

MapboxCoreNavigation/RouteController.swift

@@ -232,7 +232,9 @@ open class RouteController: NSObject {
    }

    var userSnapToStepDistanceFromManeuver: CLLocationDistance?
-
+
+    var simulatedLocationManager: SimulatedLocationManager?


Can we rename this to tunnelLocationManager or similar to avoid confusion?

@frederoni this simulatedLocationManager is intentional. It can be used for other context of dead reckoning (such as highways, motorways, tolls etc.) not just tunnel navigation. I had followed up with @brsbl regarding this simulated navigation effort for dead reckoning.

Ok, how about animatedLocationManager then? simulator should be reserved for the actual simulator in the sdk I think.

🤔 maybe we should have a MapboxLocationManager to manage all of these real/faux CLLocationManager objects? Something to combine the (three now?) native managers (simulated, native, and tunnel) into one concern/interface. There would also be some advantages here in testability, dynamically enabling/disabling route sim, and handoff between one location manager and another, say when entering/exiting a tunnel.

It doesn't have to happen now (or ever), just an idea.
/cc @akitchen

@JThramer I concur. We should have a LocationManager to handle the 3 variations of location manager. @frederoni, @1ec5 and I had a chat and we concluded there is definitely candidacy for ReplayLocationManager, SimulatedLocationManager to be tied to the location manager, MapboxLocationManager. I intend to track this in a new issue ticket for an upcoming PR.

/cc @bsudekum

I still disagree about this. Names are cheap, and this variable name tips off the developer that this SimulatedLocationManager is used for simulation. While we are simulating, this is particular kind of simulation that we should distinguish from others.

Even if this variable will be used for other types of animation in the future, we can still come up with a better name now that does not lead to confusion.

bsudekum · 2018-03-13T23:00:20Z

MapboxCoreNavigation/RouteController.swift

@@ -353,6 +355,11 @@ open class RouteController: NSObject {
    @objc public func suspendLocationUpdates() {
        locationManager.stopUpdatingLocation()
        locationManager.stopUpdatingHeading()
+
+        // In case simulated navigation is abruptly stopped before completion,


I think this sentence is missing a few the.

@frederoni Haha yeah - facepalm (an old habit of eliminating a few prepositions to keep the comments concise 😸) Good looks. Thanks 👍

… for qualified coordinates.

…de clean up. Fixed a few bugs for simulation.

…dant check.

…ring dead reckoning.

…TunnelAnimation(for:routeController:routeProgress:callback:)

…st suite.

# Conflicts: # MapboxNavigation.xcodeproj/project.pbxproj

bsudekum · 2018-05-03T16:50:20Z

MapboxCoreNavigation/RouteController.swift

+
+    var tunnelIntersectionManagerCompletionHandler: RouteControllerSimulationCompletionBlock?
+
+    public var tunnelSimulationFeatureEnabled: Bool = false


I'd say either make these private if they can be or document them.

Also, rename this to tunnelSimulationEnabled.

@bsudekum we need to make it public. The developer needs a way to enable the tunnel simulation feature.

bsudekum · 2018-05-03T16:56:06Z

MapboxCoreNavigation/SimulatedLocationManager.swift

-            currentDistance = 0
-            currentSpeed = 30
+            currentSpeed = 0
+            currentDistance = distance + (currentSpeed * speedMultiplier)


I think we should add a new function here for these changes. Also, the units here do not match. We're adding a distance (meters) to a speed (meters per second).

@bsudekum this is an already existing logic. Remember we had a prior discussion: #1218 (comment)

Should we probably leave some comment in the code for it, just to prevent a confusion in the future?

bsudekum · 2018-05-03T17:01:47Z

MapboxCoreNavigation/RouteController.swift

        }

        updateDistanceToIntersection(from: location)
        updateRouteStepProgress(for: location)
        updateRouteLegProgress(for: location)

-        guard userIsOnRoute(location) || !(delegate?.routeController?(self, shouldRerouteFrom: location) ?? true) else {
+        guard tunnelAnimationEnabled || userIsOnRoute(location) || !(delegate?.routeController?(self, shouldRerouteFrom: location) ?? true) else {


I'm slightly concerned with this. If the tunnel animation taking place, and the user is obviously outside of the tunnel, someplace else, we won't reroute them.

@bsudekum We will reroute. Basically, whenever we get a valid location update we suspend tunnel animation demonstrated here: https://github.com/mapbox/mapbox-navigation-ios/pull/1218/files#diff-987c4bbd828e654bd63d59215f890022R129
Hence, the default navigation location manager will kick in and reroute if needed.

bsudekum · 2018-05-03T17:03:48Z

MapboxCoreNavigation/TunnelIntersectionManager.swift

+
+        guard isAnimationEnabled else { return }
+
+        // Disable the tunnel animation after at least 3 bad location updates.


It think it might make more sense to disable tunnel animation after 3 good location updates. If we get 3 bad location updates in a row, we should not do anything IMO.

@bsudekum just for the record, this might delay style switches and delay in puck movements in case the driver exits the tunnel very quickly. (PS: I will still proceed with the suggested changes.)

bsudekum · 2018-05-03T17:08:27Z

MapboxCoreNavigation/RouteController.swift

+        tunnelIntersectionManager?.delegate = self
+        tunnelIntersectionManagerCompletionHandler = { enabled, _ in
+            self.tunnelIntersectionManager?.isAnimationEnabled = enabled
+        }


It might be cleaner to wrap this in an initTunnelManager function.

@bsudekum Done ✅ 👉 setupTunnelIntersectionManager()

bsudekum · 2018-05-03T17:10:10Z

MapboxCoreNavigation/RouteController.swift

                // Allow the user puck to advance. A stationary puck is not great.
                self.rawLocation = lastLocation
+
+                // Check for a tunnel intersection at the current step we found the bad location update.
+                checkForTunnelIntersection(at: lastLocation, for: manager)


I'd move this this out of this if statement and put it below. This function should use location.

@bsudekum shouldn't the simulation move along with the puck as indicated in the comment above

Allow the user puck to advance. A stationary puck is not great.

@bsudekum also when you say "I'd move this out of this if statement and put it below."... do you mean, moving this out of the conditional statement:
if let lastLocation = locations.last, delegate?.routeController?(self, shouldDiscard: lastLocation) ?? true ?

Move to at least here:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Line 589 in f00ae83

self.rawLocation = location

@bsudekum It already resides here:

mapbox-navigation-ios/MapboxCoreNavigation/RouteController.swift

Line 615 in f00ae83

checkForTunnelIntersection(at: location, for: manager)

(Thus where step’s remaining distance has changed.)

The reason the check for tunnel lives here is to ensure whenever:

filteredLocations does not contain good locations and we have found at least one good location previously.

and... to follow suit as we advance the puck as indicated here:
https://github.com/mapbox/mapbox-navigation-ios/pull/1218/files#diff-1a2149692b3d7c58b6a0392120d6011aL549

... unless I'm missing something, here.

Per discussion via slack, we decided to keep this function here.
/cc @bsudekum

bsudekum · 2018-05-03T17:11:06Z

MapboxCoreNavigation/RouteController.swift

+                                                    callback: callback)
+    }
+
+    public func tunnelIntersectionManager(_ manager: CLLocationManager,


These functions need documentation since they are public.

@bsudekum Hey, it's already documented in the TunnelIntersectionManagerDelegate class: https://github.com/mapbox/mapbox-navigation-ios/pull/1218/files#diff-987c4bbd828e654bd63d59215f890022R20

…ded a documentation to the tunnel simulation enabled property.

…suite.

…ction Manager class responsible for enabling simulated navigation through tunnels.

…tion through tunnels feature.

…g. Added a function determine the current distance. Updated custom initializer for simulated location manager.

…ure.

vincethecoder added 8 commits March 2, 2018 15:26

Merge branch 'master' into tunnel/simulateNavigation

750f195

Merge branch 'tunnel/dark_mode_style' into tunnel/simulate_navigation

4d3256b

Preliminary work for tunnel animation along a route and a given dista…

9668384

…nce.

Merge branch 'master' into tunnel/simulate_navigation

beb8ad9

# Conflicts: # MapboxCoreNavigation/Constants.swift

Merge branch 'tunnel/dark_mode_style' into tunnel/simulate_navigation

eedfbe2

# Conflicts: # MapboxCoreNavigation/RouteController.swift # MapboxNavigation/NavigationViewController.swift # MapboxNavigation/StyleManager.swift

Removed extra space padding.

760f674

Added extra space for readability

7908992

Check if manager is not a SimulatedLocationManager before we proceed …

8483bea

…to start/stop animation.

vincethecoder added wip labels Mar 12, 2018

vincethecoder added this to the v0.16.x milestone Mar 12, 2018

bsudekum modified the milestone: v0.16.x Mar 12, 2018

bsudekum reviewed Mar 12, 2018

View reviewed changes

vincethecoder added 2 commits March 13, 2018 11:08

Updates with comments for params required on creating an instances of…

2807988

… SimulatedLocationManager. Additional logic to stop simulated location updates when user halts navigation.

Formatted function for begin tunnel animation

544c5d1

frederoni reviewed Mar 13, 2018

View reviewed changes

bsudekum reviewed Mar 13, 2018

View reviewed changes

vincethecoder added 5 commits March 14, 2018 10:39

Minor texts updates.

a75dbd2

Updated carthage. Minor change in reset signature.

2eb9deb

Changed variable name to animated location manager. Additional checks…

2b698ec

… for qualified coordinates.

Modified CLLocation isWithin method to be an inclusive comparator. Co…

6ea99cd

…de clean up. Fixed a few bugs for simulation.

Added documentation for the animatedLocationManager and removed redun…

f29b77c

…dant check.

vincethecoder added the feature New feature request. label Mar 15, 2018

bsudekum changed the base branch from tunnel/dark_mode_style to master March 19, 2018 21:45

Merge branch 'master' into tunnel/simulate_navigation

732ff8b

vincethecoder added 2 commits April 30, 2018 16:19

Modified function signature for detecting tunnel intersection.

60e11f0

Updated to set current speed to zero to prevent puck sudden bounce du…

3f84588

…ring dead reckoning.

mapbox deleted a comment from vincethecoder Apr 30, 2018

vincethecoder added 8 commits April 30, 2018 16:59

Merge branch 'master' into tunnel/simulate_navigation

76cdf56

Added a tunnel simulation feature flag.

7915ea8

Merge branch 'master' into tunnel/simulate_navigation

cf184a4

Minor refactor to make tunnelAnimationEnabled a private property.

4bbd659

Merge branch 'master' into tunnel/simulate_navigation

556735e

Deleted unneeded method parameter in tunnelIntersectionManager.enable…

684628d

…TunnelAnimation(for:routeController:routeProgress:callback:)

Enabled simulation feature flag in the tunnel intersection manager te…

7bb2c5b

…st suite.

Merge branch 'master' into tunnel/simulate_navigation

f00ae83

# Conflicts: # MapboxNavigation.xcodeproj/project.pbxproj

bsudekum reviewed May 3, 2018

View reviewed changes

vincethecoder added 9 commits May 3, 2018 14:48

Disable simulation only after 3 valid location updates.

1d8b7a3

Updated condition check for at least 3 valid location updates.

6bb59aa

Renamed tunnelSimulationFeatureEnabled to tunnelSimulationEnabled. Ad…

fe0f9e8

…ded a documentation to the tunnel simulation enabled property.

Deleted the expectations within the tunnel intersection manager test …

fbe993e

…suite.

Added a record into the change log to announce the new Tunnel Interse…

13777ca

…ction Manager class responsible for enabling simulated navigation through tunnels.

Updated the change log to indicate how to enable the simulated naviga…

3a6f37e

…tion through tunnels feature.

Deleted the tunnel animation enabled flag from reroute prevention fla…

b3ba334

…g. Added a function determine the current distance. Updated custom initializer for simulated location manager.

Merge branch 'master' into tunnel/simulate_navigation

086d811

Modified the initializeSimulatedLocationManager(for:) function signat…

239fea3

…ure.

bsudekum approved these changes May 3, 2018

View reviewed changes

vincethecoder merged commit e753711 into master May 4, 2018

vincethecoder deleted the tunnel/simulate_navigation branch May 4, 2018 14:43

1ec5 mentioned this pull request May 8, 2018

TunnelIntersectionManager is undocumented #1403

Closed


		var tunnelIntersectionManagerCompletionHandler: RouteControllerSimulationCompletionBlock?

		public var tunnelSimulationFeatureEnabled: Bool = false


		guard isAnimationEnabled else { return }

		// Disable the tunnel animation after at least 3 bad location updates.

Enable simulated navigation for tunnel segments. #1218

Enable simulated navigation for tunnel segments. #1218

Conversation

vincethecoder commented Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederoni Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

frederoni commented Mar 13, 2018

vincethecoder commented Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JThramer Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder Mar 14, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder May 3, 2018 • edited Loading

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

vincethecoder May 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder May 3, 2018 • edited Loading

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

vincethecoder commented Mar 12, 2018 •

edited

Loading

frederoni Mar 13, 2018 •

edited

Loading

vincethecoder Mar 13, 2018 •

edited

Loading

vincethecoder Mar 13, 2018 •

edited

Loading

vincethecoder commented Mar 13, 2018 •

edited

Loading

vincethecoder Mar 13, 2018 •

edited

Loading

JThramer Mar 13, 2018 •

edited

Loading

vincethecoder Mar 14, 2018 •

edited

Loading

vincethecoder May 3, 2018 •

edited

Loading

vincethecoder May 3, 2018 •

edited

Loading

vincethecoder May 3, 2018 •

edited

Loading

vincethecoder May 3, 2018 •

edited

Loading