Show current way name #155

bsudekum · 2017-04-17T23:33:29Z

Closes: #142

I'm still not convinced this any better than using the way name returned in the route name. Perhaps in cases where we cannot find a feature, we can fallback to the way name in the route?

/c @frederoni @1ec5

1ec5 · 2017-04-18T05:53:49Z

I'm still not convinced this any better than using the way name returned in the route name.

What do you think of the consideration in #142 (comment)? OSRM is suppressing name changes that are unworthy of a voice notification but worthy of an UI update. That tradeoff still makes some sense to me, but it does complicate this feature.

1ec5 · 2017-04-18T05:59:27Z

MapboxNavigation/RouteMapViewController.swift

+            // Somehow check the current style has mapbox streets
+            //
+            //let streets = MGLSource(identifier: "mapbox://mapbox-streets-v7")
+            //guard mapView.style!.sources.contains(streets) else {


Here’s an example of checking whether a source is Mapbox Streets. You’d want to check whether any source in the style is Mapbox Streets.

1ec5 · 2017-04-18T06:01:17Z

MapboxNavigation/RouteMapViewController.swift

+            if let userPuck = userPuck {
+                let features = mapView.visibleFeatures(in: userPuck.frame)
+
+                for layer in features {


visibleFeatures(in:) returns an array of features, not an array of layers. The iOS SDK currently lacks a way to tell what style layer each visible feature belongs to: mapbox/mapbox-gl-native#7162. As a workaround, you can call visibleFeatures(in:styleLayerIdentifiers:) multiple times, once per relevant layer.

1ec5 · 2017-04-18T06:13:47Z

MapboxNavigation/RouteMapViewController.swift

+                for layer in features {
+
+                    // TODO: check for acceptable classes
+                    // if let feature = layer as? MGLPolylineFeature,


MGLPolylineFeature is a kind of feature; a feature is part of a shape or vector source. But note for example that a single vector source might include features of many types at the same time.

You want to find out which style layers would display roads, which are linear. A style layer that renders linear shapes is an MGLLineStyleLayer. After you filter the style’s list of layers to include only MGLLineStyleLayers, you can further filter the list to layers whose source is Mapbox Streets and whose source layer identifier is road.

You could also filter the layers by their predicates, but note that predicates can be arbitrarily complex, so this might be more trouble than it’s worth. For example, if you wanted to know whether a layer’s predicate includes only motorways, you’d have to account for predicates such as (class IN {'motorway_link', 'trunk_link'} AND class != 'motorway' AND oneway = FALSE) OR layer = -2 etc.

1ec5 · 2017-04-18T06:17:47Z

MapboxNavigation/RouteMapViewController.swift

+                    // if let feature = layer as? MGLPolylineFeature,
+
+                    // TODO: Localize
+                    if let name = layer.attribute(forKey: "name") as? String {


Should this match the map’s language or the UI language? Should we automatically localize the map to match the UI language (mapbox/ios-sdk-examples#36)?

bsudekum · 2017-04-18T21:52:47Z

Updated the logic a bit in 06fac74 but now I'm not getting any features

1ec5 · 2017-04-18T22:37:23Z

MapboxNavigation/RouteMapViewController.swift

@@ -268,6 +270,56 @@ extension RouteMapViewController: NavigationMapViewDelegate {
            return defaultReturn
        }

+        if let userLocation = mapView.userLocation,
+            let style = mapView.style,
+            routeController.showCurrentWayNameLabel == true {


If showCurrentWayNameLabel is a Boolean, is it necessary to compare it to true?

1ec5 · 2017-04-18T22:40:37Z

MapboxNavigation/RouteMapViewController.swift

+                        return false
+                    }
+                } else {
+                    return false


This filter call is more complex than it needs to be:

let streetsSourceIdentifiers = style.sources.flatMap { $0 as? MGLVectorSource }.filter { $0.configurationURL?.absoluteString.range(of: "mapbox.mapbox-streets-v7") != nil }.map { $0.identifier }

1ec5 · 2017-04-18T22:46:06Z

MapboxNavigation/RouteMapViewController.swift

+                        return false
+                    }
+                } else {
+                    return false


I hope we never release a mapbox.mapbox-streets-v7.1. 😉

Implement this extension:

extension MGLVectorSource { var isMapboxStreets: Bool { guard let configurationURL = configurationURL else { return false } return configurationURL.scheme = "mapbox" && configurationURL.host.components(separatedBy: ",").contains("mapbox.mapbox-streets-v7") } }

then filter on it:

let streetsSourceIdentifiers = style.sources.flatMap { $0 as? MGLVectorSource }.filter { $0.isMapboxStreets }.map { $0.identifier }

1ec5 · 2017-04-18T22:46:43Z

MapboxNavigation/RouteMapViewController.swift

+                    $0 as? MGLLineStyleLayer
+                }
+
+                let sourceIdenitifers = validSources.map {


Typo: Idenitifers

1ec5 · 2017-04-18T22:49:07Z

MapboxNavigation/RouteMapViewController.swift

+                    if let identifier = $0.sourceIdentifier {
+                        return sourceIdenitifers.contains(identifier) && $0.sourceLayerIdentifier == "road"
+                    } else {
+                        return false


This filter could be written more succinctly as:

lineLayers.filter { streetsSourceIdentifiers.contains($0.sourceIdentifier ?? "") && $0.sourceLayerIdentifier == "road" }

1ec5 · 2017-04-18T23:04:04Z

Playing with macosapp a bit, I’m discovering that if you perform a query at the location marked by a •:

===•=== Main St ========

you get a feature with only the attributes that mbgl considers for rendering line layers (class, oneway, etc.). Whereas if you query here:

======= Main•St ========

you get the same, plus a feature from the symbol layer that includes the name. In both cases, the returned feature comes from the same feature in the vector tile, but unless I’m missing something, mbgl has optimized out any vector tile properties that are irrelevant to the style layer that returned the feature.

This wouldn’t be a problem with source querying, but source querying doesn’t allow you to specify a bounding box or point at which to query – it returns all loaded features.

Unless I’m missing something, unfortunately this could rule out feature querying as a source for this street name label.

1ec5 · 2017-04-19T03:02:52Z

plus a feature from the symbol layer that includes the name.

Ah, that’s the road_label layer, handled in 98d53dc. 👍

bsudekum · 2017-04-19T19:43:09Z

@1ec5 ready for review again. Now, we're adding the road_label layer as a line instead of as a symbol layer.

bsudekum · 2017-04-19T20:20:27Z

Updated screenshot:

1ec5 · 2017-04-19T22:54:10Z

MapboxCoreNavigation/RouteController.swift

@@ -37,7 +37,7 @@ open class RouteController: NSObject {

     Note: the feature queries the underlying tile data and makes the assumption the current style includes the source `mapbox://mapbox.streets-v7`. All default Mapbox styles include this source. If you are using a custom source, make sure it is included when using this feature. Otherwise, it will be ignored.
     */
-    public var showCurrentWayNameLabel = false
+    public var showCurrentWayNameLabel = true


Does this still need to be an option?

1ec5 · 2017-04-19T22:58:28Z

MapboxNavigation/RouteMapViewController.swift

@@ -42,6 +42,7 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe
    var arrowCurrentStep: RouteStep?

    let streetsLanguages = ["zh", "ru", "fr", "es", "en"]


According to this documentation, the Mapbox Streets v7 source’s road_label layer has a name_ru property. There’s also an undocumented name_zh-Hans property that should be documented.

1ec5 · 2017-04-19T22:59:01Z

MapboxNavigation/RouteMapViewController.swift

@@ -57,6 +58,10 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe
        mapView.tintColor = NavigationUI.shared.tintColor
        recenterButton.tintColor = NavigationUI.shared.tintColor
        recenterButton.applyDefaultCornerRadiusShadow(cornerRadius: 22)
+        wayNameLabel.applyDefaultCornerRadiusShadow()
+        wayNameLabel.layer.masksToBounds = true
+        wayNameLabel.insets = UIEdgeInsetsMake(1, 4, 1, 4)


Use the Swift-style UIEdgeInsets(top:left:bottom:right:) instead of the C-style UIEdgeInsetsMake(_:_:_:_:).

1ec5 · 2017-04-19T22:59:58Z

MapboxNavigation/RouteMapViewController.swift

+            let streetsSourceIdentifiers = streetsSources.map {
+                $0.identifier
+            }
+            assert(!streetsSourceIdentifiers.isEmpty, "The option `showCurrentWayNameLabel` must contain the source `mapbox.mapbox-streets-v7`")


An option can’t contain a source. A style contains a source.

1ec5 · 2017-04-19T23:00:21Z

MapboxNavigation/RouteMapViewController.swift

+            let streetsSourceIdentifiers = streetsSources.map {
+                $0.identifier
+            }
+            assert(!streetsSourceIdentifiers.isEmpty, "The option `showCurrentWayNameLabel` must contain the source `mapbox.mapbox-streets-v7`")


If the style doesn’t contain Mapbox Streets source, we could add it.

1ec5 · 2017-04-19T23:03:36Z

MapboxNavigation/RouteMapViewController.swift

+                for feature in features {
+
+                    var key = "name"
+                    if let language = Locale.preferredLanguages.first!.components(separatedBy: "-").first,


It’d be great to adapt this UI to the user’s preferred language, but are we doing that in the turn banner and step table and on the map? I think it’d be more important to be consistent throughout the UI. We could use the runtime styling API to localize the map, with code very similar to this code. However, the Directions API doesn’t yet return localized way names, so the turn banner and step table are stuck in the local language.

1ec5 · 2017-04-19T23:04:35Z

MapboxNavigation/RouteMapViewController.swift

@@ -40,6 +41,9 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe
    var shieldImageDownloadToken: SDWebImageDownloadToken?
    var arrowCurrentStep: RouteStep?

+    let streetsLanguages = ["zh", "ru", "fr", "es", "en"]
+    let RoadLabelLayerIdentifier = "roadLabelLayer"


This property is only used in one method, and there won’t ever be a need for another class to know about it. Make this a local variable instead.

bsudekum · 2017-04-19T23:17:20Z

Ok, bridges, overpasses, etc, seem to be causing trouble:

1ec5 · 2017-04-20T04:46:21Z

MapboxNavigation/RouteMapViewController.swift

+            let style = mapView.style,
+            recenterButton.isHidden {
+
+            let streetsLanguages = ["zh", "ru", "fr", "es", "en"]


1ec5 · 2017-04-20T04:47:35Z

MapboxNavigation/RouteMapViewController.swift

+            let streetsSourceIdentifiers = streetsSources.map {
+                $0.identifier
+            }
+            assert(!streetsSourceIdentifiers.isEmpty, "Style must contain the source `mapbox.mapbox-streets-v7`")


If there isn’t a Streets-based source in the style, let’s create one instead of asserting.

1ec5 · 2017-04-20T06:09:29Z

MapboxNavigation/RouteMapViewController.swift

+
+                    var key = "name"
+                    if let language = Locale.preferredLanguages.first!.components(separatedBy: "-").first,
+                        streetsLanguages.contains(language) || language == "zh-Hans" {


If the first item in Locale.preferredLanguages is zh-Hans, language is zh, not zh-Hans, and this special case won’t work.

1ec5 · 2017-04-20T06:15:53Z

MapboxNavigation/RouteMapViewController.swift

+                            currentWayNameCounter += 1
+                        }
+                        print(currentWayNameCounter)
+                        if currentWayNameCounter >= 10 {


It takes ten location updates (at least ten seconds) to update the label? What if the user has already turned onto another road? Wouldn’t they get confused when they glance at the label in the meantime and find that it’s still the old road name?

Instead of delaying the update, let’s check whether the feature matches the route line. If more than one consecutive coordinates in the feature’s coordinates are also in the route step’s coordinates, then use the feature for the label. Otherwise, ignore the feature.

Added Levenshtein comparison in 04cba87

1ec5

Instead of comparing names, let’s try to compare geometries. Consider the case where the user is traveling on a highway overpass over a residential surface street and we query at exactly the point where the two roads cross. Both roads intersect the query point, but only one of them matches the route step geometry beyond that point.

Look at the two coordinates along the road on either side of the query point. If either coordinate is contained by the current route step’s coordinates array, that road matches and we should update the label for that road.

If we really want to consider the name’s edit distance, I guess we could use that as a fallback for the couple dozen (?) places in the U.S. where one road runs parallel above another road.

1ec5 · 2017-04-20T20:57:04Z

MapboxNavigation/String.swift

+        return numbers.reduce(numbers[0], {$0 < $1 ? $0 : $1})
+    }
+
+    func distanceFrom(string: String) -> Int{


Let’s rely on an existing package instead of trying to maintain this functionality ourselves. I’m partial to MDCDamerauLevenshtein, which has the benefit of being more efficiently written by virtue of being written mostly in C. (I’m a bit skeptical of relying so heavily on array subscripting in Swift.)

1ec5 · 2017-04-20T20:58:48Z

MapboxNavigation/RouteMapViewController.swift

+                            return name.distanceFrom(string: $0) <= 6
+                        }
+
+                        if !filteredStreetNames.isEmpty {


If I’m reading this code correctly, we’re trying to never update the label unless the user travels along similarly named streets for their entire trip. If the user actually travels from Main Street to Broadway Avenue, wouldn’t the edit distance be high enough that the label will never update to Broadway Avenue?

OK, I misread the code: a “filtered” street name must be w/i 6 of any of the streets along the route. Presumably there would be an edit distance of 0 between Broadway Avenue and the Broadway Avenue in the route data. However, there’s also the problem of false positives: if the user drives along State Route 185 where it passes over State Route 85, the label will happily switch to “State Route 85” because of an edit distance of one.

1ec5 · 2017-04-20T22:29:43Z

MapboxNavigation/RouteMapViewController.swift

+                style.addLayer(streetLabelLayer)
+            }
+
+            if let userPuck = mapView.view(for: userLocation),


All we need for querying is the coordinate (snapped to the route line), not the view itself. Use MGLMapView.convert(_:toCoordinateFrom:) to get the point corresponding to the snapped location.

bsudekum · 2017-04-27T19:12:25Z

Getting there. For some reason, a lot of ways have a distanceBetwenAheadOfUserAndFeature that is often 10m. Oddly enough, this is double the distance we're looking ahead. If I change this value, it is always the sum of tenMetersAheadOfUser and tenMetersAheadofFeature.

bsudekum · 2017-04-27T19:17:37Z

MapboxNavigation/RouteMapViewController.swift

+                    // todo: account for -10
+                    let tenMetersAheadofFeature = coordinate(at: 5, fromStartOf: featureSlice)!
+                    let tenMetersAheadOfUser = coordinate(at: 5, fromStartOf: slicedLine)!
+                    let distanceBetwenAheadOfUserAndFeature = tenMetersAheadofFeature - tenMetersAheadOfUser


@1ec5 I feel like in some cases we're looking at either end of the the query point:

This is 10m <-----5m----*-----5m---->

Instead of

On the money *-----5m----> *-----5m---->

Oddly enough, this is double the distance we're looking ahead. If I change this value, it is always the sum of tenMetersAheadOfUser and tenMetersAheadofFeature.

This is because of:

// todo: account for -10

On a two-way street, the route line may travel in the direction opposite the polyline feature.

bsudekum · 2017-04-27T19:52:53Z

MapboxNavigation/RouteMapViewController.swift

+                    let pointAheadFeature = coordinate(at: lookAheadDistance, fromStartOf: featureSlice)!
+                    let pointAheadUser = coordinate(at: lookAheadDistance, fromStartOf: slicedLine)!
+                    let pointBehindFeature = coordinate(at: -lookAheadDistance, fromStartOf: featureSlice)!
+                    let pointBehindUser = coordinate(at: -lookAheadDistance, fromStartOf: slicedLine)!


@1ec5 how can i clean this up? Even when i guard here, it crashes. I'm not sure if coordinate(at:) supports negative values.

Correct, coordinate(at:fromStartOf:) doesn’t support negative distances. We could either create a coordinate(at:fromEndOf:) method or pass in the reversed() coordinate array.

bsudekum · 2017-04-27T23:20:23Z

Working pretty well now

bsudekum · 2017-04-28T00:20:01Z

@frederoni when you get in, could you take a look at adding style support to this PR? Ideally, the label could be stylized as needed.

frederoni · 2017-04-28T11:40:03Z

Added MBHighlightedLabel that defaults to the tint color. We can add more customizability as the design solidifies.

bsudekum · 2017-04-28T18:21:03Z

@1ec5 @frederoni this is ready for review.

frederoni · 2017-04-28T18:50:23Z

MapboxNavigation/MGLVectorSource.swift

@@ -0,0 +1,19 @@
+//
+//  MGLVectorSource.swift


Nit: Drop default header comments

frederoni · 2017-04-28T18:51:53Z

MapboxNavigation/Resources/Navigation.storyboard

+                                                <nil key="textColor"/>
+                                                <nil key="highlightedColor"/>
+                                                <userDefinedRuntimeAttributes>
+                                                    <userDefinedRuntimeAttribute type="number" keyPath="inspectableTextStyle">


We don't have these inspectableTextStyle anymore. Should be able to just drop the key.

frederoni · 2017-04-28T18:52:33Z

MapboxNavigation/Resources/Navigation.storyboard

+                                                <rect key="frame" x="8" y="2" width="69" height="21"/>
+                                                <color key="backgroundColor" red="1" green="1" blue="1" alpha="1" colorSpace="custom" customColorSpace="sRGB"/>
+                                                <fontDescription key="fontDescription" type="system" pointSize="12"/>
+                                                <nil key="textColor"/>


Hm, is this just an empty key? Does it log any errors? Can we drop the key?

bsudekum · 2017-04-28T21:02:09Z

Going to merge this. Tests failures are out of our control right now.

1ec5

Sorry for the late review; I was off the grid the other day.

1ec5 · 2017-05-02T00:00:29Z

Examples/Swift/ViewController.swift

@@ -145,7 +145,7 @@ class ViewController: UIViewController, MGLMapViewDelegate, NavigationViewContro

        navigationViewController.simulatesLocationUpdates = simulatesLocationUpdates
        navigationViewController.routeController.snapsUserLocationAnnotationToRoute = true
-        navigationViewController.voiceController?.volume = 0.5
+        navigationViewController.voiceController?.volume = 0


Did you mean to check this in?

1ec5 · 2017-05-02T00:03:21Z

MapboxNavigation/RouteMapViewController.swift

+                        smallestLabelDistance = minDistanceBetweenPoints
+
+                        var key = "name"
+                        if let languages = Locale.preferredLanguages.first,


The text in the label should match the map, turn banner, or voice instructions, even if we happen to have a translated name in the vector tiles that is closer to the user’s preferred language. After all, it’s important to be able to match what’s on screen to real-world signage.

Since the Directions API always uses OpenStreetMap’s name tag, we should always use the name attribute for consistency with the turn banner and voice instructions.

bsudekum requested review from frederoni and 1ec5 April 17, 2017 23:33

Show current way name

9cf513b

bsudekum force-pushed the street-label branch from 16c4f5b to 9cf513b Compare April 18, 2017 00:16

1ec5 reviewed Apr 18, 2017

View reviewed changes

update

06fac74

Bobby Sudekum added 2 commits April 18, 2017 14:53

Merge branch 'master' into street-label

6238945

Only filter in roads

f5b84d4

1ec5 reviewed Apr 18, 2017

View reviewed changes

make prettier

c201373

localize, hanlde points

98d53dc

Bobby Sudekum added 2 commits April 19, 2017 12:10

update

8c8784d

fix

0de970a

Bobby Sudekum added 2 commits April 19, 2017 13:13

move

9cdd7f2

default true

4f2dfbe

Hide when recenter is visible

fd5193c

1ec5 reviewed Apr 19, 2017

View reviewed changes

Bobby Sudekum added 2 commits April 19, 2017 17:26

Only change label after its hit a few times

194e474

account for zh-hans

b298170

1ec5 reviewed Apr 20, 2017

View reviewed changes

fallback

04cba87

1ec5 suggested changes Apr 20, 2017

View reviewed changes

1ec5 reviewed Apr 20, 2017

View reviewed changes

Bobby Sudekum added 3 commits April 26, 2017 17:17

Merge branch 'master' into street-label

04614fd

move

310d211

Fix

77d309f

bsudekum commented Apr 27, 2017

View reviewed changes

Bobby Sudekum added 2 commits April 27, 2017 12:19

remove

9dad9fa

account for behind

93974a9

bsudekum commented Apr 27, 2017

View reviewed changes

Merge branch 'master' into street-label

15efc46

Add border

75c67c6

bsudekum force-pushed the street-label branch from 0cfd99c to 75c67c6 Compare April 28, 2017 18:21

frederoni reviewed Apr 28, 2017

View reviewed changes

remove

4d036fa

frederoni approved these changes Apr 28, 2017

View reviewed changes

Bobby Sudekum added 2 commits April 28, 2017 13:44

Add source instead of asserting

dec8456

smaller

a9fffd6

bsudekum merged commit c6f7686 into master Apr 28, 2017

bsudekum deleted the street-label branch April 28, 2017 21:02

1ec5 reviewed May 2, 2017

View reviewed changes

1ec5 mentioned this pull request May 2, 2017

Current road label is translated into user’s preferred language #194

Closed

1ec5 mentioned this pull request May 9, 2017

Turn banner truncates road names #214

Closed

wishtrip-dev pushed a commit to wishtrip-dev/mapbox-navigation-ios that referenced this pull request Feb 28, 2018

Add accessToken used to make request to Route (mapbox#155)

46685ad

1ec5 mentioned this pull request May 27, 2021

Use EHorizon to improve road label during turn-by-turn navigation #3019

Merged

		@@ -42,6 +42,7 @@ class RouteMapViewController: UIViewController, PulleyPrimaryContentControllerDe
		var arrowCurrentStep: RouteStep?

		let streetsLanguages = ["zh", "ru", "fr", "es", "en"]

Show current way name #155

Show current way name #155

Conversation

bsudekum commented Apr 17, 2017

1ec5 commented Apr 18, 2017

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

bsudekum commented Apr 18, 2017

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

1ec5 commented Apr 18, 2017

1ec5 commented Apr 19, 2017

bsudekum commented Apr 19, 2017

bsudekum commented Apr 19, 2017

Choose a reason for hiding this comment

1ec5 Apr 19, 2017 • edited

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

bsudekum commented Apr 19, 2017

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

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 Apr 20, 2017 • edited

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

bsudekum commented Apr 27, 2017

bsudekum Apr 27, 2017 • edited

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

bsudekum commented Apr 27, 2017

bsudekum commented Apr 28, 2017

frederoni commented Apr 28, 2017 • edited

bsudekum commented Apr 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsudekum commented Apr 28, 2017

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Apr 19, 2017 •

edited

1ec5 Apr 20, 2017 •

edited

bsudekum Apr 27, 2017 •

edited

frederoni commented Apr 28, 2017 •

edited