Fix inconsistent route progress and index after rerouting

[1ec5]

After rerouting or proactive rerouting, keep the array of routes in the same order as in the Directions API response. Instead of reshuffling the array to put the most similar route first, update the route index to point to the most similar route (except when proactively rerouting to the most optimal route). This change makes the post-rerouting state more consistent with the pre-rerouting state as of #3182.

RouteController.routeProgress and LegacyRouteController.routeProgress are no longer writable. Synchronize RouteController.indexedRouteResponse after rerouting to ensure consistency with routeProgress.

Consolidated calls to NavigationServiceDelegate.navigationService(_:willRerouteFrom:) and NavigationServiceDelegate.navigationService(_:didRerouteAlong:at:proactive:) and the posting of Notification.Name.routeControllerWillReroute and Notification.Name.routeControllerDidReroute to ensure they occur consistently around reroutes. When posting these notifications, pass in a proactive rerouting flag directly instead of relying on a parallel didFindFasterRoute flag that was difficult to reason about.

Fixes #3343 and potentially other bugs.

/cc @mapbox/navigation-ios

[S2Ler]

One of the tests are failing.

[azarovalex]

lgtm, but if we add changes in the latest moment before the RC, we need to test them at least manually

[1ec5]

The route index and step index default to 0, so I removed the explicit call to change the step index to 0.

[1ec5]

This was the one other place where the did-reroute notification needed to get inlined. It’s weird that an application explicitly setting the route will get a notification back telling it what it just did, but that’s the only way that various parts of MapboxNavigation will be able to respond to the new route being set. As a case in point, the addition of this line fixes the integration test failure.

[1ec5]

While inlining setRoute(_:proactive:routeOptions:) here, I restored an old comment from before #1986 that explained why we try to persist the spoken instruction index in the new route progress. Apparently we consider it confusing to hear the “Head northwest on Main Street” instruction out of the blue without the rerouting chime.

This bit of gymnastics dates back to when we originally implemented proactive rerouting in #346. At the time, there was no concept of an instruction index, only an “alert level” that increased as the user neared the maneuver point. Maintaining the alert level made sense as a way to suppress that initial instruction, since the alert level would’ve been guaranteed to be nonzero at any time beyond the beginning of the old step.

We migrated to server-generated instructions in #614, eliminating the concept of alert levels in favor of instruction indices. 903e020 migrated this code to persist the instruction index, but that introduced a potential problem: unlike alert levels, there’s theoretically an arbitrary number of instruction indices that could vary from one step to the next, or between two steps belonging to two different routes.

I don’t know for sure that this persistence is causing any problems, so I’ve retained the behavior for now, but it needs a closer look. There’s got to be a better way to suppress the initial departure instruction, if we still think that’s necessary.

[1ec5]

lgtm, but if we add changes in the latest moment before the RC, we need to test them at least manually

Since we’re literally on the cusp of releasing v2.0.0-rc.1, I’ve cleaned up #3344 so we can land just that targeted fix for #3343 and the related bugs. We’ll land this PR right after v2.0.0-rc.2 to give us some additional time to test the more comprehensive fixes before releasing v2.0.0-rc.2.

[ShanMa1991]

When I test this pr on Example-CarPlay with multiple route legs under simulation mode and repeated several times. There's a failure occurring as following:

mapbox-navigation-ios/Sources/MapboxCoreNavigation/RouteController.swift

Lines 310 to 318 in f462297

	func updateIndexes(status: NavigationStatus, progress: RouteProgress) {
	let newLegIndex = Int(status.legIndex)
	let newStepIndex = Int(status.stepIndex)
	let newIntersectionIndex = Int(status.intersectionIndex)
	let oldLegIndex = progress.legIndex
	if newLegIndex != progress.legIndex {
	progress.legIndex = newLegIndex
	}

It happened in a two legs route which would advance from the first leg to the next leg. The routeProgress.legIndex as 1.

But the route in the routeProgress only has one leg , and the oldLegIndex = 0. Then it triggers the assertion failure of legIndex < route.legs.endIndex after the progress.legIndex = newLegIndex.

So my concern is that why the routeProgress contains the route only has 1 leg which in fact should have 2 on this branch. Is it related with the indexedRouteResponse?

[1ec5]

When I test this pr on Example-CarPlay with multiple route legs under simulation mode and repeated several times. There's a failure occurring as following

I’m unable to reproduce this issue when simulating a route with two legs in Example-CarPlay on this branch.

Did you use the navigation SDK’s built-in route simulation feature or the iOS or Xcode route simulation feature? Can you share the coordinates of the waypoints you used?

[ShanMa1991]

Did you use the navigation SDK’s built-in route simulation feature or the iOS or Xcode route simulation feature? Can you share the coordinates of the waypoints you used?

Yes, I'm using the navigation SDK’s built-in route simulation feature with the waypoints inside the SF downtown area. This issue could be reproduced with multiple times of 2-leg routes navigation star and cancel, along with the issue #3276 .

[ShanMa1991]

The inconsistent route progress and index issue in multi-leg rout navigation under simulation mode will be tracked in #3276 . Thanks for the testing !

[1ec5]

After squashing and merging, this build failed. Rerunning the build succeeded.

Test Case '-[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed]' started.
Warning: Using NavigationSettings.initialize(directions:tileStoreConfiguration:) after corresponding variables was initialized. Possible reasons: Initialize called more than once, or the following properties was accessed before initialization: `tileStoreConfiguration`, `directions`. This might result in an undefined behaviour. 
2021-09-16 23:49:38.511471+0000 xctest[1677:62393] [Billing] Trip session isn't started. Please check that you have the correct Mapboox Access Token
2021-09-16 23:49:38.521475+0000 xctest[1677:62689] <nav-native> No PersistentConfig found at /Users/distiller/Library/Developer/CoreSimulator/Devices/DED5E213-8043-44B3-9F26-6D4CB4FED885/data/Library/Application Support/.mapbox/tile_store/navigation/config.json
2021-09-16 23:49:38.521783+0000 xctest[1677:62689] <nav-native> Unable to find the latest version of routing tiles!
2021-09-16 23:49:38.522037+0000 xctest[1677:62689] <nav-native> Async config init - Unable to determine the routing tiles version
/Users/distiller/project/Tests/MapboxCoreNavigationTests/BillingHandlerTests.swift:159: error: -[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed] : XCTAssertEqual failed: ("stopped") is not equal to ("running")
Test Case '-[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed]' failed (0.276 seconds).

@S2Ler, should we increase the timeout for this test too, or is there a deeper issue?

mapbox-navigation-ios/Tests/MapboxCoreNavigationTests/BillingHandlerTests.swift

Line 160 in 1e35987

waitForExpectations(timeout: 1, handler: nil)

[S2Ler]

After squashing and merging, this build failed. Rerunning the build succeeded.

Test Case '-[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed]' started.
Warning: Using NavigationSettings.initialize(directions:tileStoreConfiguration:) after corresponding variables was initialized. Possible reasons: Initialize called more than once, or the following properties was accessed before initialization: `tileStoreConfiguration`, `directions`. This might result in an undefined behaviour. 
2021-09-16 23:49:38.511471+0000 xctest[1677:62393] [Billing] Trip session isn't started. Please check that you have the correct Mapboox Access Token
2021-09-16 23:49:38.521475+0000 xctest[1677:62689] <nav-native> No PersistentConfig found at /Users/distiller/Library/Developer/CoreSimulator/Devices/DED5E213-8043-44B3-9F26-6D4CB4FED885/data/Library/Application Support/.mapbox/tile_store/navigation/config.json
2021-09-16 23:49:38.521783+0000 xctest[1677:62689] <nav-native> Unable to find the latest version of routing tiles!
2021-09-16 23:49:38.522037+0000 xctest[1677:62689] <nav-native> Async config init - Unable to determine the routing tiles version
/Users/distiller/project/Tests/MapboxCoreNavigationTests/BillingHandlerTests.swift:159: error: -[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed] : XCTAssertEqual failed: ("stopped") is not equal to ("running")
Test Case '-[MapboxCoreNavigationTests.BillingHandlerUnitTests testSessionBeginFailed]' failed (0.276 seconds).

@S2Ler, should we increase the timeout for this test too, or is there a deeper issue?

mapbox-navigation-ios/Tests/MapboxCoreNavigationTests/BillingHandlerTests.swift

Line 160 in 1e35987

waitForExpectations(timeout: 1, handler: nil)

yes, increasing to 3 or 5 should fix it.