Hybrid Router #3261

Udumft · 2021-08-16T12:34:03Z

Description

This PR introduces "Hybrid ~~Theory~~ Router" usage. This feature adds API to allow making explicitly online/offline/hybrid route requests, selecting between Directions or onboard router engine.

Implementation

Removed previous Directions extension with similar functionality. Tried to make feature similar to Android analog. Added NavigationRouter (naming suggestions are welcome) to be responsible for making requests. User is free to instantiate a router for a single request or keep the object alive to make several requests.
Current implementation has not finished parsing refreshing response (TODO).

Sources/MapboxNavigation/CarPlayManager.swift

Sources/MapboxCoreNavigation/NavigationRouter.swift

S2Ler

Adding some tests are very much welcomed.

The API is easy to use, thumbs up!

CHANGELOG.md

Sources/MapboxCoreNavigation/CacheHandlerFactory.swift

Sources/MapboxCoreNavigation/NavigationSettings.swift

Sources/TestHelper/TestCase.swift

Sources/MapboxCoreNavigation/NavigationRouter.swift

Udumft · 2021-09-17T13:52:29Z

Sources/MapboxCoreNavigation/NavigationRouter.swift

+}
+
+extension URL {
+    func removingSKU() -> URL {


Removing the SKU token, as NN automatically appends it on their side, which may result in conflict.

Do you have an integration test for this? I'm worried that we break encapsulation here by using magic constant "sku". Can we move this logic to Directions library?

I don't think it should be a publicly available Directions feature, as it is a part of internal logic communicating w/ NN. Agreed on magic constants usage, but don't know how to avoid it in this case as ideally it should be shared with Directions too.

Nave you concidered using SPI? https://github.com/apple/swift/pull/29810

Can it be shared through the environment, the same way we used to share the SKU token with MapboxDirections?

I think we could. It would require internal changes on Directions side too, so maybe we could do it as a tailwork?

mapbox-github-ci-issues-4 · 2021-11-23T13:14:15Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-4 · 2021-11-23T13:14:18Z

No breaking changes detected in MapboxNavigation

Sources/MapboxCoreNavigation/LegacyRouteController.swift

Sources/MapboxCoreNavigation/MapboxRoutingProvider.swift

S2Ler · 2021-11-24T12:32:00Z

Sources/MapboxCoreNavigation/MapboxRoutingProvider.swift

+        /**
+         Use online data only
+
+         In order for such `MapboxRoutingProvider` to function properly, proper navigation data should be available onboard. `.offline` router will not be able to refresh routes.


Not really clear what "onboard" means. Can we think of a better word that is more widely used?

.offline router will not be able to refresh routes.

Is future tense needed here? >> Offline routing provider can't refresh routes. <Link to what "refresh route" means.

Code comment corrected. I don't quite understand the issue with tenses. Is it not grammatical?

It is correct, but maybe not that good to use it here.

Sources/MapboxCoreNavigation/MapboxRoutingProvider.swift

S2Ler · 2021-11-24T12:54:53Z

Sources/MapboxCoreNavigation/MapboxRoutingProvider.swift

+    fileprivate func parseResponse<ResponseType: Codable>(requestId: RequestId, userInfo: [CodingUserInfoKey : Any], result: Expected<AnyObject, AnyObject>, completion: @escaping (Result<ResponseType, DirectionsError>) -> Void) {
+        do {
+            let json = result.value as? String
+            guard let data = json?.data(using: .utf8) else {


nit: Data(json.utf8) isn't nullable type, so we can guard on emtpy json and proceed.

Sorry, I didn't get your suggestion

guard let json = result.value as? String else {...} let data = Data(json.utf8) ...

Why is it preferable?

Data(json.utf8) isn't optional.
data(using: .utf8) is optional.

It is more clear that this code can fail because result value is of incorrect type. In your variant it is hidden, but the one who knows that data method returns nullable, it is not.

Just noticed, that you could use Result+Expected.swift for parsing NavNative's Expected type.

Sources/MapboxCoreNavigation/Router.swift

Sources/MapboxCoreNavigation/RoutingProvider.swift

mapbox-github-ci-issues-2 · 2021-11-25T09:47:01Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-2 · 2021-11-25T09:47:05Z

No breaking changes detected in MapboxNavigation

S2Ler · 2021-11-25T11:01:11Z

Sources/MapboxCoreNavigation/MapboxRoutingProvider.swift

+                                            credentials: settings.directions.credentials)
+        self.router = RouterFactory.build(for: source.nativeSource,
+                                          cache: factory.cacheHandle,
+                                          historyRecorder: factory.historyRecorder)


I found the following issues with this approach:

NativeHandlersFactory is created without the potentially adjusted history URL. In the example app, it leads to two alive CacheHandles.

If the above is fixed, there will still be an issue with the Router using a different instance of the history record handler.

I don't think we are supposed to have more than one history recorder unless it is an artificial use case.

So we shall reuse the history recorder from Navigator and, in the future, decouple HistoryRecorder from Navigator/PassiveLocationProvider/RouteController.

We assume that settings are configured before any call to SDK entities is done. Due to CacheHandlerFactory CacheHandle instance should be reused between MapboxRoutingProvider and Navigator. But history recorder will be created twice, that's true. Not sure how critical it is, and not sure what's the best way to mitigate this. Referring to Navigator here is not an option since it will trigger MBNNNavigator initialization which is not desired.

We assume that settings are configured before any call to SDK entities is done. Due to CacheHandlerFactory CacheHandle instance should be reused between MapboxRoutingProvider and Navigator.

Please try Example app, it doesn't seem to work as you describe.

Referring to Navigator here is not an option since it will trigger MBNNNavigator initialization which is not desired.

Well, then we need similar approach to CacheHandlerFactory. Or change it completely to not only cache cachehandle, but other relevant instances as well.

Indeed, there was a difference in cache key form Routing Provider and Navigator, I think I fixed it by adding more parameters to CacheHandlerFactory initialization. At least it should work if Navigator's parameters are not modified before launch.
I also added same caching mechanism for HistoryRecorder. I would prefer to solve this issue in a different way, but I don't know how, and it is outside of this task's scope. Currently, only CacheHandle and HistoryRecorder instances are used in multiple places, so I've added caching only for these 2. With HandlerFactory it should be trivial to add caching to any other required entity though.

Let's resolve this thread, although it is not clear what will happen during switch to offline/online for Navigator.

mapbox-github-ci-issues-2 · 2021-11-25T16:16:42Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-2 · 2021-11-25T16:16:45Z

No breaking changes detected in MapboxNavigation

mapbox-github-ci-issues-4 · 2021-11-26T07:43:22Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-4 · 2021-11-26T07:43:25Z

No breaking changes detected in MapboxNavigation

…r functionality for requesting and refreshing routes; removed Directions extension and Navigator.routerInterface; added CacheHandlerFactory to cache latest CacheHandle created.

…tingProvider uses the same cache and history recorder handles; refactored handlers caching class to be more generic (+9 squashed commits) Squashed commits: [99e73e7] vk-1024-hybrid-router: improved code comments; added sanity check for route refreshing; re-organized MapboxRoutingProvider.swift [cd1a117] vk-1024-hybrid-router: CHANGELOG updated. [26467b8] vk-1024-hybrid-router: reverted breaking changes; update code to be backwards compatible [6a7833e] vk-1024-hybrid-router: improved code readability; fixed Directions RoutingProvider callbacks calling thread [683b7cd] vk-1024-hybrid-router: restored file references [a77db21] vk-1024-hybrid-router: fixed unit tests; updated deprecated directions symbols usage; NN imports updated [f3dd481] vk-1024-hybrid-router: renamed new entities and it's members; added assertation messages, added code doc for CacheHandlerFactory. [838cd74] vk-1024-hybrid-router: file references added. [2e1ff03] vk-1024-hybrid-router: unit tests updated. CHANGELOG updated. (+5 squashed commits) Squashed commits: [1b2285a] vk-1024-hybrid-router: introduced NavigationProvider protocol to allow usage Directions or NavigationRouter for routing tasks. Corresponding conformances added. [30c02f5] vk-1024-hybrid-router: eased conditions for reporting an error on offline route refreshing attempt [54e287d] vk-1024-hybrid-router: RouteController update route fixed [8e3ef17] vk-1024-hybrid-router: restored NavigationService changes. Tests fixed. [c8937ff] vk-1024-hybrid-router: CHANGELOG updated.

mapbox-github-ci-issues-3 · 2021-11-29T09:37:39Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-3 · 2021-11-29T09:37:43Z

No breaking changes detected in MapboxNavigation

mapbox-github-ci-issues-5 · 2021-11-29T10:15:20Z

No breaking changes detected in MapboxCoreNavigation

mapbox-github-ci-issues-1 · 2021-11-29T10:15:23Z

No breaking changes detected in MapboxNavigation

Udumft added feature New feature request. wip backwards incompatible changes that break backwards compatibility of public API Core Work related to core navigation and integrations. labels Aug 16, 2021

Udumft requested a review from a team August 16, 2021 12:34

Udumft self-assigned this Aug 16, 2021

S2Ler reviewed Aug 16, 2021

View reviewed changes

Sources/MapboxNavigation/CarPlayManager.swift Outdated Show resolved Hide resolved

S2Ler reviewed Aug 16, 2021

View reviewed changes

Sources/MapboxCoreNavigation/NavigationRouter.swift Outdated Show resolved Hide resolved

S2Ler force-pushed the s2ler/tileset-descriptors branch from bd611ce to 8a9a26e Compare August 18, 2021 09:17

Base automatically changed from s2ler/tileset-descriptors to main August 18, 2021 09:26

Udumft force-pushed the vk/1024-hybrid-router branch from f7d1583 to 994a1de Compare August 19, 2021 14:34

Udumft requested a review from S2Ler August 19, 2021 16:05

S2Ler reviewed Aug 23, 2021

View reviewed changes

Udumft force-pushed the vk/1024-hybrid-router branch 2 times, most recently from afcefff to 7e4a4c0 Compare August 24, 2021 12:57

Udumft force-pushed the vk/1024-hybrid-router branch from f258de9 to 369205f Compare September 7, 2021 07:04

Udumft requested a review from a team September 7, 2021 15:00

Udumft force-pushed the vk/1024-hybrid-router branch from 235eb2c to 68a07a1 Compare September 14, 2021 11:43

Udumft marked this pull request as ready for review September 15, 2021 11:24

Udumft commented Sep 17, 2021

View reviewed changes

Udumft force-pushed the vk/1024-hybrid-router branch 2 times, most recently from 5449667 to b775cdb Compare September 22, 2021 14:31

Udumft force-pushed the vk/1024-hybrid-router branch from b775cdb to b06af4d Compare September 29, 2021 14:40

truburt removed the wip label Sep 29, 2021

Udumft mentioned this pull request Oct 7, 2021

Native Router mapbox/mapbox-navigation-android#4791

Merged

Udumft force-pushed the vk/1024-hybrid-router branch from b06af4d to 8e3ef17 Compare November 2, 2021 07:25

This comment has been minimized.

Sign in to view

Udumft requested a review from S2Ler November 23, 2021 15:35

S2Ler reviewed Nov 24, 2021

View reviewed changes

S2Ler reviewed Nov 25, 2021

View reviewed changes

Udumft force-pushed the vk/1024-hybrid-router branch from 99e73e7 to c46cc50 Compare November 25, 2021 16:07

Udumft requested a review from a team November 26, 2021 08:25

S2Ler approved these changes Nov 26, 2021

View reviewed changes

Udumft added 3 commits November 29, 2021 12:26

vk-1024-hybrid-router: added NavigationRouter to provide hybrid route…

262db1a

…r functionality for requesting and refreshing routes; removed Directions extension and Navigator.routerInterface; added CacheHandlerFactory to cache latest CacheHandle created.

vk-1024-hybrid-router: podfile.lock updated

6af4602

Udumft force-pushed the vk/1024-hybrid-router branch from c51f3b4 to 6af4602 Compare November 29, 2021 09:27

vk-1024-hybrid-router: CHANGELOG updated.

72e9146

Udumft merged commit 06f790b into main Nov 29, 2021

Udumft deleted the vk/1024-hybrid-router branch November 29, 2021 10:26

ShanMa1991 mentioned this pull request Nov 29, 2021

[Bug]: Route line blinks when refresh and reroute #3621

Closed

1ec5 added this to the v2.1 milestone Dec 15, 2021

This was referenced Dec 15, 2021

Error building MapboxCoreNavigation using CocoaPods and Xcode 12.4 — cannot use module MapboxNavigationNative_Private #3661

Closed

v2.1.1 mapbox/mapbox-navigation-ios-examples#160

Merged

1ec5 mentioned this pull request Jan 27, 2022

[Issue-3707] Support Offline Navigation Rerouting #3708

Closed

Hybrid Router #3261

Hybrid Router #3261

Conversation

Udumft commented Aug 16, 2021

Description

Implementation

S2Ler 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

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mapbox-github-ci-issues-4 bot commented Nov 23, 2021

mapbox-github-ci-issues-4 bot commented Nov 23, 2021

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

mapbox-github-ci-issues-2 bot commented Nov 25, 2021

mapbox-github-ci-issues-2 bot commented Nov 25, 2021

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

mapbox-github-ci-issues-2 bot commented Nov 25, 2021

mapbox-github-ci-issues-2 bot commented Nov 25, 2021

mapbox-github-ci-issues-4 bot commented Nov 26, 2021

mapbox-github-ci-issues-4 bot commented Nov 26, 2021

mapbox-github-ci-issues-3 bot commented Nov 29, 2021

mapbox-github-ci-issues-3 bot commented Nov 29, 2021

mapbox-github-ci-issues-5 bot commented Nov 29, 2021

mapbox-github-ci-issues-1 bot commented Nov 29, 2021