Route Duration Annotations #2734

avi-c · 2020-11-10T02:15:42Z

Add callout annotations showing route durations for comparing alternate routes during preview.

Will include noting which routes have tolls when that information is available.

1ec5

Nice, it really looks the part! A couple initial thoughts:

1ec5 · 2020-11-11T22:49:48Z

Extensions/MGLMapView+Example.swift

+
+
+extension MGLStyle {
+    func addDebugLineLayer(identifier: String, coordinates: [CLLocationCoordinate2D], color: UIColor = UIColor.purple) {


Ultimately, we’ll need to move these extensions from the example application to MapboxNavigation so that developers don’t have to copy the whole file.

1ec5 · 2020-11-11T22:57:14Z

Extensions/MGLMapView+Example.swift

+
+extension MGLMapView {
+    func coordinateBoundsInset(_ inset: CGSize) -> MGLCoordinateBounds {
+        return convert(bounds.insetBy(dx: inset.width, dy: inset.height), toCoordinateBoundsFrom: nil)


MGLMapView.convert(_:toCoordinateBoundsFrom:) is inaccurate on a tilted map: mapbox/mapbox-gl-native#2259.

More broadly, I think we should investigate positioning the labels based on screen coordinates rather than geographic coordinates. Core GL’s own symbol collision avoidance code is based on projected coordinates, which account for rotation and tilting, so it would make sense to be consistent with core GL labeling on that respect.

Imagine taking the maneuver coordinates for a given route, converting them all to screen points, and stringing them together in a UIBezierPath. Calculating the distance between two points is suddenly just a Pythagorean distance problem.

Removed this extension since I no longer needed it. I am still doing a lot of my calculations in geographic coordinates, but do filter them to only use ones that are onscreen. Also, when coordinates are chosen for the annotations I use the on-screen position to decide where to put the "stem" of the bubble callout.

avi-c · 2020-11-18T04:20:32Z

Made a number of updates to my algorithm that simplify it and reduce the total number of needed additions and extensions. I did add the need for an algorithm to simplify a polyline and chose to use the one from: https://github.com/tomislav/Simplify-Swift. I resorted to compiling in the source file since the library is not available through Carthage but we can figure out another solution, including re-implementing the algorithm.

MaximAlien

Thanks for this PR. I like current look of delay views. In my opinion we should aim for implementation on Navigation SDK side. There are also other changes being made to NavigaitonMapView at this point (route styling, congestion consistency, vanishing route line calculation improvements) and it has become pretty large class. I think it'd make sense to extract certain areas of functionality into separate entities (e.g. at least NavigationMapView extensions).

MaximAlien · 2020-11-19T23:29:22Z

Example/ViewController.swift

@@ -94,6 +330,10 @@ class ViewController: UIViewController {
        if let appDelegate = UIApplication.shared.delegate as? AppDelegate {
            appDelegate.currentAppRootViewController = self
        }
+
+        dateComponentsFormatter.maximumUnitCount = 3


I can see that CPTrip extension already has some additional DateComponentsFormatters. Probably would make sense to create extension on MapboxNavigation side for DateComponentsFormatter which contains all properties.

I'll look at consolidating all of these within MapboxNavigation.

I moved the CPTrip DateComponentsFormatter extensions to a dedicated file within MapboxNavigation and changed to use those instead. Removed the use of a local DateComponentsFormatter within the Route Annotations file.

MaximAlien · 2020-11-19T23:37:35Z

MapboxNavigation/MGLStyle.swift

+
+     Useful for debugging or visualizing data.
+     */
+    public func addDebugCircleLayer(identifier: String, coordinate: CLLocationCoordinate2D, color: UIColor = UIColor.purple) {


As I can see @1ec5 proposed to move these methods to MapboxNavigation. I wonder if we really need these methods here, as they're mostly used for debugging purpose. Probably would make more sense to keep them in Example app, as they're meant to be used mostly by us for troubleshooting?

Yeah, they are just for debugging, so I can move them around, but we can also drop them from being in the project. I find them very useful but could just keep them in my own repo or gist or something. @1ec5 - Do you think there is a place for this type of debugging code in the actual project or just something to keep for ourselves?

They are useful not just for our own debugging, they are a good way to see geo info on the map with just simple coordinates. If @1ec5 thinks it is ok to add debug functions to the SDK then that's fine with me. If not, I would move them to the Example app and leave them there.

I didn’t realize they were debugging methods when I suggested moving them to MapboxNavigation. It makes sense to keep them limited to the example application. Sorry for the confusion.

MaximAlien · 2020-11-19T23:45:09Z

Example/ViewController.swift

+
+    private let annotationLayerIdentifier = "RouteETAAnnotations"
+
+    private func addRouteAnnotationSymbolLayer(features: [MGLPointFeature], style: MGLStyle) {


If we plan to integrate this functionality to MapboxNavigation it'd probably make sense to move layer addition to NavigationMapView.

Should I move this PR to come on top of some of the other work you have in progress? What would you suggest as a way to coordinate this rework/refactor of the NavigationMapView class?

Unfortunately there are multiple PRs which contain changes to NavigationMapView: #2741, #2737 and #2719. Easiest way would be to rebase onto main for now. I can then help with tackling conflicts (if there are any) after mentioned PRs are merged into main.

I agree that this extensive runtime styling code should go in NavigationMapView so developers can take advantage of it instead of having to roll their own annotations.

MaximAlien · 2020-11-19T23:57:12Z

Example/ViewController.swift

+                                            trueExpression: NSExpression(forConstantValue: UIColor.white),
+                     falseExpression: NSExpression(forConstantValue: UIColor.black))
+
+        shapeLayer.textFontNames = NSExpression(forConstantValue: ["DIN Pro Medium"])


As I can see on low zoom levels shape size of MGLSymbolStyleLayer will stay the same. Shall we consider a possibility of changing delay view size depending on zoom level. E.g. I was experimenting with iconScale and iconOpacity in #2634.

There are some good discussions to have on exactly what visual treatment we want. I did make the text size vary by zoom as I saw that being done in some other nav apps.

I'd prefer to investigate the designs in a separate ticket later though since it's bound to take a bit longer to come to agreement on the details.

MaximAlien · 2020-11-20T00:11:49Z

Example/ViewController.swift

+        }
+    }
+
+    private func updateRouteAnnotations(_ routes: [Route]?, style: MGLStyle) {


This method is making pretty heavy computation each time. I'd probably consider outlining only specific actions when we should update delay view location (e.g. it doesn't make sense to show delay view on low zoom levels, since route is not really visible anyway). We can also move certain calculations to background thread.

@1ec5 @MaximAlien - This brings up a question I have. Can we perform map style updates from a background thread?

Actually, I don't think this is actually that heavy in terms of performance. I profiled several recomputations of the route annotation coordinates by repeatedly scrolling the map view (which causes a re-layout) and the time spent in this routine is only about 1% of the CPU time. It also will only happen when the map is moved or otherwise redrawn.

It's also much, much smaller than the delay in retrieving the new route from the Directions API so causes only imperceptible delays in the case when a new route is being fetched.

I'd argue it's better just to leave it as is than to try and add in some concurrency.

Hopefully @1ec5 can weigh in on this.

It may be worth moving the calculations to a background thread just in case, but most likely the reason it hasn’t come up as a performance bottleneck yet is that it only runs once per camera change and not continuously (which is fine).

MaximAlien · 2020-11-20T00:18:02Z

Example/ViewController.swift

+
+    private func updateRouteAnnotations(_ routes: [Route]?, style: MGLStyle) {
+        // remove any existing route annotation
+        removeRouteAnnotationsLayerFromStyle(style)


When route is selected ViewController.response will trigger clearMapView(), which will in turn clear delay view. Since this feature is going to be part of MapboxNavigation it makes sense to move existing logic into NavigationMapView instead of improving implementation in Example app.

It'd be also good to rebase current changes onto main as it now contains at least one fix to example app which can be useful here (previously after pressing on route clear button would disappear).

Completed the rebase.

MaximAlien · 2020-11-20T00:20:34Z

Example/Simplify.swift

+//  OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+//  THE SOFTWARE.
+
+import Foundation


Personally, I'm not really fond of storing 3rdparty sources as is. Not sure it's good idea to have separate dependency either. It'd be great if we can have our own implementation of simplification functionality.

Agreed. It isn't something I am very familiar with though. @1ec5 - Is there a mapbox implementation that we could port over to the Nav SDK so I could drop any form of dependency?

Will move to a simplify algorithm I added to Turf: mapbox/turf-swift#124
Once that PR is approved and merged I will update the Turf version in Nav SDK and use it instead of another dependency.

Turf v1.2.0-beta.1 has been released. MapboxDirections doesn’t require it yet, but you can have the navigation SDK require it by modifying the Cartfile in this repository. The CocoaPods integration test will fail until there’s a final release of Turf v1.2.0, but at least you’ll be able to prepare this PR for the moment when that happens.

diff --git a/Cartfile b/Cartfile index ba3e3e59..1cf9c2ca 100644 --- a/Cartfile +++ b/Cartfile @@ -3,7 +3,7 @@ binary "https://api.mapbox.com/downloads/v2/carthage/mobile-navigation-native/Ma binary "https://api.mapbox.com/downloads/v2/carthage/mobile-maps/mapbox-ios-sdk-dynamic.json" ~> 6.0 binary "https://api.mapbox.com/downloads/v2/carthage/mapbox-common/MapboxCommon-ios.json" == 7.1.2 github "mapbox/mapbox-directions-swift" ~> 1.0 -github "mapbox/turf-swift" ~> 1.0 +github "mapbox/turf-swift" ~> 1.2.0-beta.1 github "mapbox/mapbox-events-ios" ~> 0.10.2 github "ceeK/Solar" ~> 2.1.0 github "mapbox/mapbox-speech-swift" ~> 1.0 diff --git a/Cartfile.resolved b/Cartfile.resolved index 1b02b2c1..6042bedd 100644 --- a/Cartfile.resolved +++ b/Cartfile.resolved @@ -10,8 +10,8 @@ github "ceeK/Solar" "2.1.0" github "linksmt/OHHTTPStubs" "563f48d3fab84ef04639649c770b00f4fa502cca" github "mapbox/MapboxGeocoder.swift" "v0.10.2" github "mapbox/mapbox-directions-swift" "v1.1.0" -github "mapbox/mapbox-events-ios" "v0.10.6" +github "mapbox/mapbox-events-ios" "v0.10.7" github "mapbox/mapbox-speech-swift" "v1.0.0" -github "mapbox/turf-swift" "v1.1.0" +github "mapbox/turf-swift" "v1.2.0-beta.1" github "raphaelmor/Polyline" "v5.0.2" github "uber/ios-snapshot-test-case" "5.0.2"

avi-c · 2020-12-17T22:17:50Z

@MaximAlien @1ec5 - I reworked the algorithm a bit because I wasn't satisfied with how the annotations were landing.

It has a number of steps but should actually be much simpler computation wise since it is mainly things like coordinate comparisons, point in polygon checks, and a few closestCoordinateTo computations.

See what you think. I tried to make sure the comments were giving a clear step-by-step overview of what is happening.

avi-c · 2021-01-11T15:39:22Z

@1ec5 @MaximAlien - I'll fix the merge conflicts but would love to get this merged and out of the way this week. If there are changes you'd like to see please update and I'll address those too.

1ec5 · 2021-01-14T20:49:44Z

Example/ViewController.swift

+
+    private let annotationLayerIdentifier = "RouteETAAnnotations"
+
+    private func addRouteAnnotationSymbolLayer(features: [MGLPointFeature], style: MGLStyle) {


I agree that this extensive runtime styling code should go in NavigationMapView so developers can take advantage of it instead of having to roll their own annotations.

Example/ViewController.swift

1ec5 · 2021-01-14T21:01:13Z

Example/ViewController.swift

+                // Take care to snap that coordinate to one that lays on the original route line.
+                // If the chosen snapped coordinate is not visible onthe screen, then we walk back along the route coordinates looking for one that is.
+                // If none of the earlier points are on screen then we walk forward along the route coordinates until we find one that is.
+                if let distance = continuousLine.distance(), let sampleCoordinate = continuousLine.indexedCoordinateFromStart(distance: distance * CLLocationDistance.random(in: 0.3...0.8))?.coordinate, let routeShape = route.shape, let snappedCoordinate = routeShape.closestCoordinate(to: sampleCoordinate) {


What’s the significance of the start and end ranges of this random distance? Should it depend on the zoom level or the style? Does the range hold up better in some kinds of routes versus others?

I’m still concerned that, by focusing on geographic coordinates rather than screen coordinates as suggested in #2734 (comment), the whole algorithm is vulnerable to a variety of factors that are hard to control for at this high level of the stack.

In particular, LineString.indexedCoordinateFromStart(distance:) can only return one of the route line’s vertices, but there’s no guarantee about the density of vertices along the route line. For example, in some places in OpenStreetMap, a vertex may occur only once every quarter mile at an intersection, while in other places, it may occur every few car lengths because turn lanes and parking restrictions have been mapped in fine detail.

The intent is to pick a coordinate along the line some distance away from the start and end points of the route, but with some variation of the distance. The goal is to not hard code something like halfway through the route.

I went with the indexed coordinate in order to reduce the computation burden of calculating a point along the line somewhere between two vertices. It might be that the overall algorithm is simplified enough that we wouldn't need to be calculating as many points and it would be worth it.

In my testing, the annotations generally work out very well, but do sometimes collide. I suspect that the code required to calculate things in screen space so that they would never collide would be significantly more complicated than just doing the geographic calculations.

My hope is that the logic we have is good enough for most of the time and that any customer who found the collision avoidance to not work to their liking would just figure out what works best for them.

At a glance, probably the biggest risk at the moment would be that the map happens to be rotated or tilted differently than the positioning code was designed for. As long as these degrees of freedom are possible, it’ll remain difficult to completely account for them in geographic coordinate space. But to the extent that this poses a problem, we can treat it as tail work, perhaps awaiting new map SDK functionality to make such calculations easier.

Example/ViewController.swift

avi-c · 2021-01-16T04:28:21Z

Responded to the review feedback, including moving the annotations from the Example project over to NavigationMapView.

MapboxNavigation/DayStyle.swift