Offline routing

[frederoni]

Note: this PR targets the offline-routing branch.

Started outlining offline routing.

This PR

• Support for Offline Routing via MapboxOfflineDirections
• Release v0.24.1 of MapboxDirections.swift with support for deserializing routes from external sources (fixes the remaining CI 🔴)
• Release a new version of MapboxNavigationNative

Subsequent PR

• Mimic a URLSessionDataTask for Directions.calculateOffline to allow canceling a request and make it easier to combine and use fallbacks between online and offline directions.
• UI for downloading a region

cc @akitchen @JThramer

[frederoni]

Make a new release of MapboxDirections.swift

[frederoni]

Fix this assertion

[frederoni]

Should we capture these exceptions? If so, I think we would have to capture them in Obj-C.

[akitchen]

If that's the case, do we need the OfflineDirections class to be written in ObjC? Or another intermediary wrapper perhaps? Obviously we can't have these exceptions (which seem like they could be used for control flow or cascading workflow options) to crash the containing app.

[frederoni]

OfflineDirections might be a better name. Perhaps the functionality could be added as an extension on the existing Directions object.

[akitchen]

What if OfflineDirections and Directions both conformed to a DirectionsProtocol protocol (although we'd still need to figure out naming).

[akitchen]

I have to wonder - does this option deserve to exist? when would we prefer client side but still "fall back" to server-generated directions? lack of tiles?

[akitchen]

what's the thinking here - preflighting an HTTP request is discouraged/considered an anti-pattern these days

[1ec5]

Nit: category names should be in camel case just like class names. A conventional category name would be MBCoreNavigationAdditions (since both MapboxNavigationNative and MapboxCoreNavigation use the same class prefix).

[1ec5]

Nit: “set up” here is being used as a verb, so it should be setUpRouter:.

[1ec5]

Does this enumeration bridge to Objective-C? If so, it needs a class prefix in Objective-C (and documentation).

[frederoni]

These errors are casted to NSErrors but I guess it needs a localized description key? Or does Error bridge fine to Obj-C since Swift 4.0? 🤔

[1ec5]

Hm, yeah, the localized description keys would need to be provided somehow, though I’m not sure exactly how that works when relying on bridging from Error in Swift.

[1ec5]

It’s a bit odd that optional arguments come before required arguments. Can tilesPath and translationsPath come first?

[frederoni]

Included OSRMTI as a submodule.

[1ec5]

Nit: remove boilerplate code.

[1ec5]

It feels like we’re trying to run around unexpected exceptions coming out of MapboxNavigationNative. If -configureRouterForTilesPath:translationsPath: has expected failure modes, it should be the responsibility of MapboxNavigationNative to catch an exception and turn it into a C-style return code and NSError out parameter.

By default, throwing an exception in Objective-C causes a leak, even under ARC, because the expectation is that an exception is a catastrophic error that leads to an immediate crash anyways. That’s one reason Objective-C programmers generally avoid @try-@catch, even in cases where C++ programmers might rely on it for control flow. It doesn’t leak in Objective-C++, but whether the @throw happens in Objective-C or Objective-C++ is none of this SDK’s business.

[frederoni]

Returning NSErrors from Obj-C has a really poor bridgeability to Swift’s "throws". Noticed that this try-catch block doesn't suppress the underlying exception so I ended up removing it altogether. It's just a fallback for the kind of resource the offline router is looking for. (1: Uncompress tiles, 2: Compressed tiles, 3: Different structure of compressed tiles). It's not a fatal error but one that gets caught if you have "Catch all Exceptions" enabled.

[1ec5]

Returning NSErrors from Obj-C has a really poor bridgeability to Swift’s "throws".

Unless I’m mistaken, the most idiomatic method signature would include an NSError ** out parameter, which would bridge to Swift as a throws, but it wouldn’t return an error.

[frederoni]

the most idiomatic method signature would include an NSError ** out parameter

this is correct but there are some limitations, a boolean is the only supported primitive return type:

- (void)voidNoThrow:(int)arg error:(NSError **)error;      // Bridges to NSErrorPointer
- (NSUInteger)intNoThrow:(int)arg error:(NSError **)error; // Bridges to NSErrorPointer
- (BOOL)boolThrow:(int)arg error:(NSError **)error;        // Throws idiomatically

However, there seems to be a consensus about using -fno-exceptions eventually, partially due to bloated binary size but the inevitable leak in Obj-C is also a good point. Either way, this try-catch block is already removed.

[1ec5]

Documentation would be great here. It wasn’t obvious to me that the return value is a tile count. From the implementation of this method, I initially got the impression that it was an error code of some sort.

[frederoni]

This comment is not relevant any longer. configureRouter is not wrapped in setUpRouter but instead called directly from an initializer in Swift which is documented.

[frederoni]

Not sure about the name of this class. How about NavigationDirections?

[1ec5]

That’s as good a name as I can think of. Hopefully developers don’t end up thinking they can only use NavigationRouteOptions with this class.

[1ec5]

Rename tilesPath and translationsPath to tilesURL and instructionsURL, respectively, since they’re no longer path strings.

[1ec5]

Are any changes to documentation needed?

• README
• CHANGELOG
• jazzy table of contents
• jazzy cover page

[frederoni]

Are any changes to documentation needed?

Yep, we do need some additions to the documentation, but I figured, since we're targeting a feature branch (offline-routing) and api-routing-tiles is not yet deployed to production, this is subject to change. Updates to the README and a guide on how to use the offline tile downloader is slated for a subsequent PR.

[annathomasQB]

Hi @frederoni ,

I'm trying to implement offline routing with the examples provided in the documents. But it doesn't seem to work. I'm trying to figure out where I might have gone wrong. I'm getting the tiles downloaded (is the max limit set to 10?) for Faroe Islands, unpacked at URL - all that's happening... I'm guessing the configureRouter too is getting set properly. Is there anything else left to be done ? how do I assign this to the navigationController? I've raised my issue here : Downloaded Tiles not shown for Offline Routing