Pair route with route index #2591

1ec5 · 2020-09-02T16:30:05Z

This PR updates #2366 to work with the latest changes in mapbox/mapbox-directions-swift#420 that decapsulate route refreshing a bit. I’ve opened a separate PR because the changes are massive and gangly, and I’m open to reconsidering the approach to make it less disruptive. See the changes to CHANGELOG.md for details.

With this PR, classes that previously kept track of the route now also keep track of the route’s index in the overall route response and use it when refreshing the route. This entailed adding a required route index argument to a litany of public and private methods and initializers, though it was possible to hard-code 0 in many places due to the prior use of routes.first. With these changes, it becomes clear how we’d transition the public API to treating the route as merely an indexed item in the overall route response: #2589. We could bite that off here too; it all comes down to whether the (route, route index) tuple would be easier or harder to transition to.

Additionally and critically, the code to merge the skeletal refreshed route into the original route has moved from mapbox/mapbox-directions-swift#420 into this PR.

/cc @mapbox/navigation-ios

1ec5 · 2020-09-02T20:40:25Z

It’s expected that the Xcode_11.4.1_iOS_12.2_CP_update and Xcode_11.4.1_iOS_12.2_CP_install build bots would fail, because this PR depends on mapbox/mapbox-directions-swift#420, which hasn’t been released yet.

1ec5 · 2020-09-02T21:12:08Z

CHANGELOG.md

+* The `NavigationViewController.route` and `NavigationService.route` properties are now read-only. To change the route that the user is traveling along, set the `NavigationViewController.indexedRoute` or `NavigationService.indexedRoute` property instead, pairing the route with the index of the route in the original `RouteResponse` object. ([#2366](https://github.com/mapbox/mapbox-navigation-ios/pull/2366))
+* The following methods now require a route index to be passed in as an argument ([#2366](https://github.com/mapbox/mapbox-navigation-ios/pull/2366)):
+   * `NavigationViewController(for:routeIndex:routeOptions:navigationOptions:)`
+   * `NavigationViewController(route:routeIndex:routeOptions:navigationService:)`
+   * `CarPlayManagerDelegate.carPlayManager(_:navigationServiceAlong:routeIndex:routeOptions:desiredSimulationMode:)`
+   * `MapboxNavigationService(route:routeIndex:routeOptions:)`
+   * `MapboxNavigationService(route:routeIndex:routeOptions:directions:locationSource:eventsManagerType:simulating:routerType:)`
+   * `RouteProgress(route:routeIndex:options:legIndex:spokenInstructionIndex:)`
+   * `RouteProgress(route:routeIndex:options:legIndex:spokenInstructionIndex:)`
+   * `Router(along:routeIndex:options:directions:dataSource:)`
+   * `RouteController(along:routeIndex:options:directions:dataSource:)`


This blurb nicely encapsulates why I’m nervous about this PR. Adding these extra arguments is pretty safe from a runtime stability standpoint, other than in some of the CarPlay code that relies on casting Any into tuples, but it’s always a big deal to add complexity to the basic setup workflow in NavigationViewController particularly.

If I'm not mistaken in all public API methods which were modified to contain routeIndex RouteOptions argument is also present. Would it work if we add routeIndex as part of options?

A single RouteOptions may be associated with more than one route, so I don’t think that would work. Unless you mean NavigationOptions?

Cartfile

1ec5 · 2020-09-02T21:15:26Z

Example/ViewController.swift

        guard let response = response, let route = response.routes?.first, case let .route(routeOptions) = response.options else { return }

-        let service = navigationService(route: route, options: routeOptions)
+        let service = navigationService(route: route, routeIndex: 0, options: routeOptions)


In many cases, the developer will be able to hard-code 0 as the route index after getting the first route out of the route array. But I made the argument required because I didn’t want any code that deals with multiple routes to go unnoticed during an upgrade.

1ec5 · 2020-09-02T21:16:51Z

MapboxCoreNavigation/CoreConstants.swift

+     The user info dictionary contains the key `RouteController.NotificationUserInfoKey.routeProgressKey`.
+     */
+    static let routeControllerDidRefreshRoute: Notification.Name = .init(rawValue: "RouteControllerDidRefreshRoute")


Is it weird that a notification about refreshing a route comes with a route progress object as user info? The idea is that the route progress object contains the route as well as other members that are affected by refreshing, so it’s handy to have immediate access to the whole route progress object.

I think it's not really obvious, I'd expect route Route object to be there. We also have routeControllerProgressDidChange, but it doesn't contain only RouteProgress updates.

1ec5 · 2020-09-02T21:17:53Z

MapboxCoreNavigation/NavigationServiceDelegate.swift

@@ -30,7 +30,7 @@ public protocol NavigationServiceDelegate: class, UnimplementedLogging {
    /**
     Called immediately before the navigation service calculates a new route.

-     This method is called after `navigationService(_:shouldRerouteFrom:)` is called, simultaneously with the `NavigationServiceWillReroute` notification being posted, and before `navigationService(_:didRerouteAlong:)` is called.
+     This method is called after `navigationService(_:shouldRerouteFrom:)` is called, simultaneously with the `Notification.Name.routeControllerWillReroute` notification being posted, and before `navigationService(_:didRerouteAlong:)` is called.


A bunch of these references were always inaccurate. I don’t think we’ve ever prefixed the notification names with NavigationService.

1ec5 · 2020-09-02T21:20:49Z

MapboxCoreNavigation/Router.swift

+/**
+ A route and its index in a `RouteResponse` that sorts routes from most optimal to least optimal.
+ */
+public typealias IndexedRoute = (Route, Int)


This is the most generic place I could find for a type that ends up getting used all over the place. The “indexed” pattern is inspired by Turf for Swift. The idea is to force code that sets the route to a new value to also set the route index to a potentially new value at the same time, avoiding stale state or race conditions.

1ec5 · 2020-09-02T21:29:41Z

MapboxNavigation/CPTrip.swift

+            let info: (Route, Int, RouteOptions) = (route: route, routeIndex: routeIndex, options: routeOptions)
            routeChoice.userInfo = info


It’s frustrating that the only way to associate arbitrary data with UI elements that CarPlay controls is to stuff it in an untyped property and cast it later. As tail work, it might be better to define a set of dictionary keys and set this property to a dictionary, similar to when posting a Notification.

1ec5 · 2020-09-02T21:31:43Z

MapboxNavigationTests/CarPlayManagerTests.swift

@@ -199,7 +199,7 @@ class CarPlayManagerTests: XCTestCase {
            CLLocationCoordinate2D(latitude: 37.764793, longitude: -122.463161),
            CLLocationCoordinate2D(latitude: 34.054081, longitude: -118.243412),
        ])
-        choice.userInfo = (Fixture.route(from: "route-with-banner-instructions", options: options), options)
+        choice.userInfo = (Fixture.route(from: "route-with-banner-instructions", options: options), 0, options)


The route index is almost always 0 in the unit tests and integration tests, because Fixture.route(from:options:) always returns the first route, and few of the fixtures come with multiple routes anyways.

chezzdev

The changes look good to me.

MapboxNavigation/NavigationViewController.swift

MaximAlien · 2020-09-03T18:24:20Z

MapboxNavigation/NavigationViewController.swift

@@ -525,6 +535,14 @@ extension NavigationViewController: NavigationServiceDelegate {
        delegate?.navigationViewController(self, didFailToRerouteWith: error)
    }

+    public func navigationService(_ service: NavigationService, didRefresh routeProgress: RouteProgress) {


I think in this place when should redraw all congestion segments (if they were updated) by calling mapView?.updateRoute(routeProgress). At the same time current implementation of congestion segments calculation re-uses already existing lineGradient stops which means that route in most recent RouteProgress will not be used. Previously I was aiming to prevent re-calculation because for routes with many congestion segments such logic might cause performance drawbacks, on the other hand in current situation there seems no other way.

Do you mean in RouteMapViewController?

https://github.com/mapbox/mapbox-navigation-ios/blob/a303150d3a3762799048914d58de793b6ffbd9f4/MapboxNavigation/RouteMapViewController.swift#L522-L524

Would it be a good idea to call updateRoute(_:) right after showing the route? We only attempt to fetch a refreshed route every 2 minutes (as determined by RouteControllerProactiveReroutingInterval, which is publicly configurable), which is the same interval used for proactively finding a faster route. So I’m not terribly concerned about runtime performance unless calculating the segments is very poor.

I think most recent update in RouteMapViewController should work (removing all route layers, showing them for traveled distance = 0.0, updating them for traveled distance from RouteProgress). On the other hand it defats the idea of reusing layers (I think the only data we need from RouteProgress in such case is shape). We can leave current implementation, but revise it in future.

Yeah, let’s deal with any performance optimizations as tail work.

Example/ViewController.swift

Keep track of the route’s index in the overall route response and use it when refreshing the route. Added a route index argument to a litany of public and private methods and initializers. Explicitly merge the skeletal refreshed route into the original route.

Post a notification and inform the navigation service and navigation view controller delegates whenever the route is refreshed. On refresh, update the route line to reflect the new traffic congestion data.

Cherry picked from c945bcf.

1ec5 added op-ex Refactoring, Tech Debt or any other operational excellence work. topic: location backwards incompatible changes that break backwards compatibility of public API labels Sep 2, 2020

1ec5 added this to the v1.0.0 milestone Sep 2, 2020

1ec5 requested a review from chezzdev September 2, 2020 16:30

1ec5 self-assigned this Sep 2, 2020

1ec5 mentioned this pull request Sep 2, 2020

Route Refresh #2366

Merged

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch from 9a09b0f to 5193625 Compare September 2, 2020 20:31

1ec5 force-pushed the 2284-route-refresh branch from 893f57f to e0df11d Compare September 2, 2020 20:48

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch from 9357cdb to a303150 Compare September 2, 2020 20:51

1ec5 commented Sep 2, 2020

View reviewed changes

chezzdev approved these changes Sep 3, 2020

View reviewed changes

MaximAlien reviewed Sep 3, 2020

View reviewed changes

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch from a303150 to fc7cf89 Compare September 3, 2020 18:43

1ec5 commented Sep 3, 2020

View reviewed changes

Example/ViewController.swift Outdated Show resolved Hide resolved

Example/ViewController.swift Outdated Show resolved Hide resolved

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch 3 times, most recently from 1bd8497 to ac01728 Compare September 3, 2020 22:41

1ec5 force-pushed the 2284-route-refresh branch from e0df11d to 6103287 Compare September 3, 2020 22:51

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch from ac01728 to d29ee08 Compare September 3, 2020 22:52

1ec5 and others added 5 commits September 3, 2020 16:39

MapboxDirections v1.0.0-rc.1

45ae385

Refresh route line based on refreshed route

c7de3e5

Post a notification and inform the navigation service and navigation view controller delegates whenever the route is refreshed. On refresh, update the route line to reflect the new traffic congestion data.

Enable route refreshing on NavigationRouteOptions

d5245c2

Enable route refresh in example app.

e979932

1ec5 force-pushed the 1ec5-route-refresh-index-2366 branch from d29ee08 to e979932 Compare September 3, 2020 23:39

1ec5 merged commit e979932 into 2284-route-refresh Sep 3, 2020

1ec5 deleted the 1ec5-route-refresh-index-2366 branch September 3, 2020 23:55

MaximAlien pushed a commit that referenced this pull request Sep 4, 2020

Pair route with route index #2591.

16b49be

Cherry picked from c945bcf.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pair route with route index #2591

Pair route with route index #2591

1ec5 commented Sep 2, 2020 •

edited

Loading

1ec5 commented Sep 2, 2020

1ec5 Sep 2, 2020

MaximAlien Sep 3, 2020

1ec5 Sep 3, 2020

1ec5 Sep 2, 2020

1ec5 Sep 2, 2020

MaximAlien Sep 3, 2020

1ec5 Sep 2, 2020

1ec5 Sep 2, 2020

1ec5 Sep 2, 2020

1ec5 Sep 2, 2020

chezzdev left a comment

MaximAlien Sep 3, 2020

1ec5 Sep 3, 2020 •

edited

Loading

MaximAlien Sep 3, 2020

1ec5 Sep 3, 2020

		let info: (Route, Int, RouteOptions) = (route: route, routeIndex: routeIndex, options: routeOptions)
		routeChoice.userInfo = info

Pair route with route index #2591

Pair route with route index #2591

Conversation

1ec5 commented Sep 2, 2020 • edited Loading

1ec5 commented Sep 2, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chezzdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Sep 2, 2020 •

edited

Loading

1ec5 Sep 3, 2020 •

edited

Loading