Offline routing #1808

frederoni · 2018-11-01T20:53:05Z

This PR brings offline routing to the Navigation SDK

Offline routing API Offline routing #1768
Download examples Offline Example #1803
Offline tile downloader Offline Tile downloader mapbox-directions-swift#303
~~Automatic failover~~
~~Wrap NavigationDirections.calculate(_:offline:completionHandler:) in a URLSessionDataTask or another operation that supports canceling~~
~~Pause/Resume a download operation~~
Replace OSRMTI submodule by using bundled translation files by default

cc @akitchen @1ec5 @JThramer @vincethecoder

akitchen · 2018-11-02T22:13:20Z

Some thoughts on a few of the remaining checklist items:

Automatic failover

Should this be a gating feature at this point? I'm not entirely convinced but it should be open to debate. Also, if we are going to do this we should have a small group design discussion about the mechanism. I could see this being a candidate for a boolean delegate method or configuration option.

Wrap NavigationDirections.calculate(_:offline:completionHandler:) in a URLSessionDataTask or another operation that supports canceling

As we discussed, if we're going to use an NSURLSessionDataTask-like abstraction for this, perhaps the client-side routes should be loaded via a custom URL scheme filtered by an NSURLProtocol, e.g. mbx-offline:///route?options=values -- there is a bit of complexity there but the abstraction is close to what we need. And a route is technically a resource (which takes time to generate/load) so it feels kind of right.

Pause/Resume a download operation

I have a feeling this will punt to a further iteration. Especially considering that there are other potential changes to tile transport currently under debate which could change the scope of this.

akitchen · 2018-12-04T01:21:06Z

I've given a 👍 - @mapbox/navigation-ios any other feedback?

Example/OfflineViewController.swift

+        Directions.shared.fetchAvailableOfflineVersions { [weak self] (versions, error) in
+
+            let nonEmptyVersions = versions?.filter { !$0.isEmpty }
+            guard let version = nonEmptyVersions?.first else { return }


frederoni · 2018-12-04T13:10:22Z

Example/ResizableView.swift

+        return self / 2
+    }
+
+    func distance(_ to: CGFloat) -> CGFloat {


The calculations in the pan gesture recognizer were getting unreadable but it has been cleaned up since then so it makes sense to remove this simple distance.

1ec5 · 2018-12-04T10:51:17Z

Example/SettingsItems.swift

+
+        let offlineItem = Item(title: "Download arbitrary region", viewControllerType: OfflineViewController.self, payload: nil)
+        let offlineSection = Section(title: "Offline Examples", items: [offlineItem])
+        let versionSection = Section(title: "Downloaded versions", items: versionDirectories())


Nit: use title case consistently for section titles:

Suggested change

let versionSection = Section(title: "Downloaded versions", items: versionDirectories())

let versionSection = Section(title: "Downloaded Versions", items: versionDirectories())

1ec5 · 2018-12-04T10:54:52Z

Example/SettingsViewController.swift

+            let tilesURL = Bundle.mapboxCoreNavigation.suggestedTilePathURL(for: selectedOfflineVersion)
+
+            Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] (numberOfTiles) in
+                let message = "Router configured w/ \(numberOfTiles) tiles."


Suggested change

let message = "Router configured w/ \(numberOfTiles) tiles."

let formattedTileCount = NumberFormatter.localizedString(from: numberOfTiles as NSNumber, number: .decimal)

let message = "Router configured with \(formattedTileCount) tiles."

I was under the impression that our Examples should only be in English while having empty languages added to test all supported languages in the SDK to mitigate translators fatigue. However, it's only a few localizable strings in the examples so far so I added this one as well.

Example/ViewController.swift

+
+    func navigationViewController(_ navigationViewController: NavigationViewController, shouldRerouteFrom location: CLLocation) -> Bool {
+
+        let shouldUseOfflineRouting = Settings.selectedOfflineVersion != nil


MapboxCoreNavigation/OfflineDirections.swift

+        OfflineDirectionsConstants.offlineSerialQueue.async { [weak self] in
+
+            guard let result = self?.navigator.getRouteForDirectionsUri(url.absoluteString) else {
+                let error = OfflineRoutingError.unexpectedRouteResult("Unexpected routing result")


1ec5 · 2018-12-04T11:11:39Z

MapboxCoreNavigationTests/OfflineRoutingTests.swift

+        let calculateRouteExpectation = expectation(description: "Calculate route offline")
+        var route: Route?
+
+        directions.calculate(options, offline: true) { (waypoints, routes, error) in


Can we verify somehow that nothing is fetched from an online source?

MapboxCoreNavigationTests/OfflineRoutingTests.swift

+
+            progressUpdates += 1
+
+            if (progressUpdates >= 2) {


MapboxNavigation.xcodeproj/project.pbxproj

@@ -1182,6 +1202,19 @@
 			name = Resources;
 			sourceTree = "<group>";
 		};
+		3529FCEE21A5C52C00AEA9AA /* Recovered References */ = {


1ec5 · 2018-12-04T11:18:10Z

MapboxCoreNavigation/OfflineDirections.swift

+}
+
+/**
+ Defines additional functionality similar to `Directions` with support for offline routing.


How does this protocol differ from the OfflineDirectionsProtocol defined by MapboxDirections.swift? I think this could become a source of confusion among developers.

I agree that it can cause confusion, however, NavigatorDirectionsProtocol depends on nav-native so it should be defined here, while OfflineDirectionsProtocol is a good fit for MapboxDirections since it's only adding additional directions related API calls. Do you have an idea how to simplify it? The protocol could be moved to MapboxDirections’s OfflineDirectionsProtocol but the implementation must reside in MapboxCoreNavigation.

I think the confusion would stem from how this protocol is described as “similar to Directions with support for offline routing”. So if we need to keep it as a separate protocol, then can we frame it as a kind of NavigationService that has Directions functionality built in?

1ec5

Some comments about user-facing strings and localization fit-and-finish. Also, there needs to be at least one changelog entry about the new stuff. I can take it from here.

MapboxCoreNavigation/OfflineDirections.swift

+        OfflineDirectionsConstants.offlineSerialQueue.async { [weak self] in
+
+            guard let result = self?.navigator.getRouteForDirectionsUri(url.absoluteString) else {
+                let message = NSLocalizedString("OFFLINE_NO_RESULT", bundle: .mapboxCoreNavigation, value: "No result", comment: "No result from offline route request")


Example/SettingsViewController.swift

+
+            Settings.directions.configureRouter(tilesURL: tilesURL!) { [weak self] (numberOfTiles) in
+                let formattedTileCount = NumberFormatter.localizedString(from: numberOfTiles as NSNumber, number: .decimal)
+                let message = String.localizedStringWithFormat(NSLocalizedString("Router configured with %@ tile(s).", comment: "Alert message when a router has been configured"), formattedTileCount)


MapboxCoreNavigation/OfflineDirections.swift

+            }
+
+            guard let data = result.json.data(using: .utf8) else {
+                let message = NSLocalizedString("OFFLINE_CORRUPT_DATA", bundle: .mapboxCoreNavigation, value: "Corrupt route data", comment: "Unable to deserialize the offline route response")


Example/UIViewController.swift

+
+    func presentAlert(_ title: String? = nil, message: String? = nil) {
+        let controller = UIAlertController(title: title, message: message, preferredStyle: .alert)
+        controller.addAction(UIAlertAction(title: "OK", style: .default, handler: { (action) in


Example/OfflineViewController.swift

+
+        view.addSubview(resizableView)
+
+        navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Download", style: .done, target: self, action: #selector(downloadRegion))


Example/OfflineViewController.swift

+
+        view.addSubview(resizableView)
+
+        navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Download", style: .done, target: self, action: #selector(downloadRegion))


Example/OfflineViewController.swift

+                NavigationDirections.unpackTilePack(at: url, outputDirectoryURL: outputDirectoryURL!, progressHandler: { (totalBytes, bytesRemaining) in
+
+                    let progress = (Float(bytesRemaining) / Float(totalBytes)) * 100
+                    self?.updateTitle("Unpacking \(Int(progress))%")


1ec5 · 2018-12-05T02:56:39Z

I’m seeing tons of console spew along these lines, seemingly in a hot loop, while displaying a visual instruction that should include shields in the simulator. The shields never load, but more importantly, I think the level of spew would quickly consume battery and storage.

2018-12-04 18:53:40.639392-0800 Example[17124:9955719] Task <C932B850-0162-4891-94CC-D4135E89115F>.<7533> load failed with error Error Domain=NSURLErrorDomain Code=-1002 "unsupported URL" UserInfo={NSLocalizedDescription=unsupported URL, NSErrorFailingURLStringKey=@2x.png, NSErrorFailingURLKey=@2x.png, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <C932B850-0162-4891-94CC-D4135E89115F>.<7533>"
), _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <C932B850-0162-4891-94CC-D4135E89115F>.<7533>, NSUnderlyingError=0x600003a123a0 {Error Domain=kCFErrorDomainCFNetwork Code=-1002 "(null)"}} [-1002]
2018-12-04 18:53:40.644171-0800 Example[17124:9956578] Task <1DC0B9B7-FAFC-401C-96B1-11BCCE418505>.<7534> finished with error - code: -1002

Copyedited various strings in the example application and gave them all stable IDs. Made some additional strings localizable. Removed a redundant Base.lproj folder from the example project structure.

1ec5

The code looks ready to go, but I’m concerned about the console spew in #1808 (comment).

These methods return file URLs, not path strings. Also added documentation and Objective-C compatibility.

Also fixed some copypasta in documentation comments.

MapboxCoreNavigation/OfflineDirections.swift

+public class NavigationDirections: Directions {
+
+    public typealias UnpackProgressHandler = (_ totalBytes: UInt64, _ remainingBytes: UInt64) -> ()
+    public typealias UnpackCompletionHandler = (_ result: UInt64, _ error: Error?) -> ()


1ec5 · 2018-12-05T05:53:10Z

MapboxCoreNavigation/OfflineDirections.swift

+public class NavigationDirections: Directions {
+
+    public typealias UnpackProgressHandler = (_ totalBytes: UInt64, _ remainingBytes: UInt64) -> ()
+    public typealias UnpackCompletionHandler = (_ result: UInt64, _ error: Error?) -> ()


Does result represent a status code? What codes can be returned, and under what circumstances?

renamed to numberOfTiles since it corresponds to the number of unpacked tiles.

1ec5 · 2018-12-06T02:23:14Z

I’m seeing tons of console spew along these lines, seemingly in a hot loop, while displaying a visual instruction that should include shields in the simulator.

This turned out to be infinite recursion: #1887.

1ec5 · 2018-12-06T02:51:45Z

The infinite recursion will be fixed by #1888. Locally, with that patch applied, I see generic shields as expected, though I don’t see exit number tabs as I do with server-side routing. The exit number is included in spoken instructions, and the exit number tabs do show up when performing server-side routing on this same branch. Unless an obvious cause materializes very shortly, I think it’s fine to treat that issue as tail work.

akitchen requested review from JThramer and 1ec5 November 2, 2018 22:13

frederoni added 27 commits November 7, 2018 17:57

Outline offline routing

94aa6c7

Add test fixtures

772f1b3

Add OfflineRoutingTests and clean up OfflineDirections

a8f6f12

Clean up error handling

4056716

Minify fixtures

d66cc31

Capture expection and return tile count

08ac909

Temporarily disable offline routing tests

3d56ce9

Add documentation fix naming conventions

ebcb07a

Bump MapboxDirections

fae883b

Return number of tiles

952ae0b

Refactor

4e54461

Don't use offline directions by default in the example app

6733b05

Remove unneeded Obj-C category

3f71104

Remove boilerplate code

dca0396

Use URLs

a78c3d9

Replace bundled translations with submodule

3c6a554

Update submodules on CI

0cab7df

Remove unused import

1eae9d6

Fix error bridging and description

929f384

Naming conventions

cea45a4

Remove unused error

e20d854

Naming conventions

2488c25

Outline Offline example

6472fba

Add arbitrary region downloader example

15d24bd

Pixel pushing

c32d501

Add unpacker

65328c8

List downloaded offline versions and clean up examples

4733e9c

1ec5 reviewed Dec 4, 2018

View reviewed changes

frederoni added 7 commits December 4, 2018 13:36

Remove stray refs

dbb5c5e

Title Case Titles

c935fb0

Simplify test

28e0c4d

Localizable error messages

e28190b

Clean up example

d7bbc32

Extract localizable

a0bb041

Remove excessive protocol

26a4eeb

1ec5 reviewed Dec 4, 2018

View reviewed changes

1ec5 assigned 1ec5 and frederoni Dec 4, 2018

1ec5 added 3 commits December 4, 2018 18:59

Convert all strings files, not just Core strings files

9553286

Copyedited offline routing error messages

c76ed23

Made example app offline strings localizable

3a80547

Copyedited various strings in the example application and gave them all stable IDs. Made some additional strings localizable. Removed a redundant Base.lproj folder from the example project structure.

1ec5 approved these changes Dec 5, 2018

View reviewed changes

1ec5 added 3 commits December 4, 2018 21:36

Refined tile URL methods

e3be2ff

These methods return file URLs, not path strings. Also added documentation and Objective-C compatibility.

Continued renaming OfflineDirections to NavigationDirections

d9e8147

Also fixed some copypasta in documentation comments.

Updated changelog

ea4844d

1ec5 reviewed Dec 5, 2018

View reviewed changes

1ec5 and others added 3 commits December 4, 2018 22:40

Added missing files to CarPlay example app target

bdc1da0

Unnest type aliases for Obj-C bridging

ebbc84e

result -> numberOfTiles

14b3c37

1ec5 mentioned this pull request Dec 6, 2018

Hang due to malformed shield image URL #1887

Closed

1ec5 merged commit 7c305e0 into master Dec 6, 2018

1ec5 deleted the offline-routing branch December 6, 2018 07:15

frederoni mentioned this pull request Dec 6, 2018

Expose offline routing functionality to Obj-C #1891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offline routing #1808

Offline routing #1808

frederoni commented Nov 1, 2018 •

edited

Loading

akitchen commented Nov 2, 2018 •

edited

Loading

akitchen commented Dec 4, 2018

This comment was marked as resolved.

This comment was marked as resolved.

frederoni Dec 4, 2018

1ec5 Dec 4, 2018

1ec5 Dec 4, 2018

This comment was marked as resolved.

frederoni Dec 4, 2018

This comment was marked as resolved.

This comment was marked as resolved.

1ec5 Dec 4, 2018

This comment was marked as resolved.

This comment was marked as resolved.

1ec5 Dec 4, 2018

frederoni Dec 4, 2018

1ec5 Dec 4, 2018

1ec5 left a comment •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

1ec5 commented Dec 5, 2018

1ec5 left a comment

This comment was marked as resolved.

1ec5 Dec 5, 2018

frederoni Dec 5, 2018

1ec5 commented Dec 6, 2018

1ec5 commented Dec 6, 2018

	let versionSection = Section(title: "Downloaded versions", items: versionDirectories())
	let versionSection = Section(title: "Downloaded Versions", items: versionDirectories())

	let message = "Router configured w/ \(numberOfTiles) tiles."
	let formattedTileCount = NumberFormatter.localizedString(from: numberOfTiles as NSNumber, number: .decimal)
	let message = "Router configured with \(formattedTileCount) tiles."


		func navigationViewController(_ navigationViewController: NavigationViewController, shouldRerouteFrom location: CLLocation) -> Bool {

		let shouldUseOfflineRouting = Settings.selectedOfflineVersion != nil


		view.addSubview(resizableView)

		navigationItem.rightBarButtonItem = UIBarButtonItem(title: "Download", style: .done, target: self, action: #selector(downloadRegion))

Offline routing #1808

Offline routing #1808

Conversation

frederoni commented Nov 1, 2018 • edited Loading

akitchen commented Nov 2, 2018 • edited Loading

akitchen commented Dec 4, 2018

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

1ec5 commented Dec 5, 2018

1ec5 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Dec 6, 2018

1ec5 commented Dec 6, 2018

frederoni commented Nov 1, 2018 •

edited

Loading

akitchen commented Nov 2, 2018 •

edited

Loading

1ec5 left a comment •

edited

Loading