Intersections and lanes #80

bsudekum · 2016-10-17T22:44:45Z

This starts work for adding a lanes class to each step. Currently, this adds an Intersections class where the lanes class will live.

todo:

document
add lanes class

/cc @1ec5

bsudekum · 2016-10-18T01:04:42Z

@1ec5 Added lanes, want to take a look?

1ec5 · 2016-10-18T01:36:59Z

MapboxDirections/MBIntersection.swift

@@ -0,0 +1,56 @@
+import Foundation
+
+public class Intersection: NSObject, NSSecureCoding {


Use @objc to pretend the MB class prefix for Objective-C usage.

1ec5 · 2016-10-18T01:38:28Z

MapboxDirections/MBIntersection.swift

+import Foundation
+
+public class Intersection: NSObject, NSSecureCoding {
+    public var inIndex: Int?


This is an awkward place for a preposition. How about entranceIndex and exitIndex?

Also, does [Bool].Index work as the type for this property?

Actually, to avoid confusion with the existing exitIndex property on RouteStep, how about approachIndex and outletIndex, or ingressIndex and egressIndex?

Does this property need to be mutable? If not, use let.

1ec5 · 2016-10-18T01:44:42Z

MapboxDirections/MBIntersection.swift

+public class Intersection: NSObject, NSSecureCoding {
+    public var inIndex: Int?
+    public var outIndex: Int?
+    public var entry: [Bool]


Instead of an array of Booleans, this property should be an IndexSet called outlets that contains the indices of the roads through which the user can exit the intersection.

1ec5 · 2016-10-18T01:50:15Z

MapboxDirections/MBLane.swift

+import Foundation
+
+public enum LaneIndicationType: Int, CustomStringConvertible {
+    case Left


In Swift 3, enum values start with lowercase letters.

I forgot this is Swift 2.3. Just keep in mind that we'll need to lowercase the first letters when porting to Swift 3.

1ec5 · 2016-10-18T01:51:00Z

MapboxDirections/MBLane.swift

+
+    case SlightRight
+
+    case Straight


straightAhead for consistency with ManeuverDirection.

1ec5 · 2016-10-18T01:51:32Z

MapboxDirections/MBLane.swift

+    }
+}
+
+public class Lane: NSObject {


@objc(MBLane)

1ec5 · 2016-10-18T01:56:41Z

MapboxDirections/MBLane.swift

+}
+
+public class Lane: NSObject {
+    public var validTurn: Bool


Whether a lane can be used for a particular maneuver is not an intrinsic property of a lane. Remove this property and add to Intersection a usableLanes property of type IndexSet or Set<Lane>, your pick.

I'm unclear why this property would live on intersection. It is a specific value for a specific lane in the lanes array. ref docs

I know that’s how it’s exposed in the API. But the API doesn’t use classes at all, so that’s comparing apples and oranges.

When you create a class, you make it possible for the developer to have an instance of that class all by itself. If the developer has a Lane object but not the Intersection or RouteStep object that contains it, validTurn is meaningless or misleading. It’s saying that a physical lane on a road can always or never be used for all routes that may contain the lane, which is unrealistic.

Semantically, the information you’re trying to store must live in a parallel data structure. Specifically, Intersection should have a usableLanes property that contains lanes or lane indices, and this validTurn property should be removed.

1ec5 · 2016-10-18T01:58:34Z

MapboxDirections/MBLane.swift

@@ -0,0 +1,83 @@
+import Foundation
+
+public enum LaneIndicationType: Int, CustomStringConvertible {


Just LaneIndication is fine. But remember to use @objc here too.

1ec5 · 2016-10-18T01:59:48Z

MapboxDirections/MBLane.swift

+
+public class Lane: NSObject {
+    public var validTurn: Bool
+    public var indications: [LaneIndicationType]


An array of enums won't bridge to Objective-C. We'll need a helper property that converts it to an array of NSValues containing these enums.

1ec5 · 2016-10-18T02:01:27Z

MapboxDirections/MBRouteStep.swift

@@ -513,6 +513,11 @@ public class RouteStep: NSObject, NSSecureCoding {
     */
    public let destinations: String?

+    /**
+     Array of interersections


Typo. Also, explain that these are all the intersections along the step, starting with the one at the maneuver and ending with the one just before the next step's maneuver.

And mention the relationship between the size of this array and the next step's exitIndex property.

1ec5 · 2016-10-18T18:16:36Z

MapboxDirections/MBIntersection.swift

+    /**
+     Index into bearings/entry array.
+
+     Used to calculate the bearing just before the turn. Namely, the clockwise angle from true north to the direction of travel immediately before the maneuver/passing the intersection. Bearings are given relative to the intersection. To get the bearing in the direction of driving, the bearing has to be rotated by a value of 180. The value is not supplied for depart maneuvers.


Is it always 180°?

I believe so

1ec5 · 2016-10-18T18:17:46Z

MapboxDirections/MBLane.swift

+
+
+    /**
+     Objectice-c helper function for indication


indications won’t even appear to an Objective-C developer. You’ll have to document this property as if indications doesn’t even exist.

1ec5 · 2016-10-18T18:18:24Z

MapboxDirections/MBRouteStep.swift

@@ -514,7 +514,9 @@ public class RouteStep: NSObject, NSSecureCoding {
    public let destinations: String?

    /**
-     Array of interersections
+     An intersection gives a full representation of any cross-way the path passes bay.


You mean “of any road crossing the route”?

bsudekum · 2016-10-18T21:51:53Z

@1ec5 updated with usableLanes

1ec5 · 2016-10-19T01:12:06Z

MapboxDirections/MBIntersection.swift

+
+     Used to calculate the bearing just before the turn. Namely, the clockwise angle from true north to the direction of travel immediately before the maneuver/passing the intersection. Bearings are given relative to the intersection. To get the bearing in the direction of driving, the bearing has to be rotated by a value of 180. The value is not supplied for depart maneuvers.
+    */
+    public let approachIndex: Int?


Unfortunately, Int? won’t bridge to Objective-C, so this will have to be Int instead. Set it to -1 by default (for the v4 case). Unfortunate, but this is one of the tradeoffs of supporting Objective-C.

1ec5 · 2016-10-19T01:13:35Z

MapboxDirections/MBIntersection.swift

+
+     A value of true indicates that the respective road could be entered on a valid route. false indicates that the turn onto the respective road would violate a restriction.
+     */
+    public var entry: [Bool]


Does this property need to be mutable?

1ec5 · 2016-10-19T01:14:28Z

MapboxDirections/MBIntersection.swift

+
+     A value of true indicates that the respective road could be entered on a valid route. false indicates that the turn onto the respective road would violate a restriction.
+     */
+    public var entry: [Bool]


#80 (comment)

Instead of an array of Booleans, this property should be an IndexSet called outlets that contains the indices of the roads through which the user can exit the intersection. These would be indices into the headings array. The index set would be equal in size to or smaller than the headings array.

1ec5 · 2016-10-19T01:18:27Z

MapboxDirections/MBIntersection.swift

+    public var lanes: [Lane]?
+
+    /**
+     Set of Lane objects that have a valid turn. 


Also mention that this is a subset of the Lane objects in the lanes property.

1ec5 · 2016-10-19T01:18:45Z

MapboxDirections/MBIntersection.swift

+    public var headings: [CLLocationDirection]
+
+    /**
+     Array of Lane objects that denote the available turn lanes at the intersection.


This isn’t just for turn lanes.

1ec5 · 2016-10-19T01:37:36Z

MapboxDirections/MBLane.swift

@@ -0,0 +1,121 @@
+import Foundation
+
+@objc(MBLaneIndication)


This type needs a documentation comment.

1ec5 · 2016-10-19T01:37:52Z

MapboxDirections/MBLane.swift

+public enum LaneIndication: Int, CustomStringConvertible {
+
+    /**
+     An indication indicating a turn to the left.


Indicates a turn to the left.

1ec5 · 2016-10-19T01:40:55Z

MapboxDirections/MBLane.swift

+@objc(MBLane)
+public class Lane: NSObject {
+
+    public let indications: [LaneIndication]


It occurs to me that we need to treat this as an option set rather than an array of enum values. After all, a lane can’t be [.Uturn, .Uturn]. Option sets are really nice in Objective-C and really nice in Swift, but unfortunately Swift option sets don’t bridge to Objective-C. So you’ll have to implement LaneIndication in Objective-C. Use MapboxGeocoder.swift’s MBPlacemarkScope option set and PlacemarkScope extension as a model.

@1ec5 are you saying use a set because the type cannot be repeated?

Not a set, but an option set (NS_OPTIONS in Objective-C, also known as a bitmask or bitfield). Not only because a type can’t be repeated, but also because using an indicationValues property (of type [NSValue]) in Objective-C would look like this:

BOOL isStraightAhead; for (NSValue *indicationValue in lane.indicationValues) { MBLaneIndication laneIndication; [indicationValues.firstObject getValue:&laneIndication objCType:@encode(MBLaneIndication)]; if (laneIndication == MBLaneIndicationStraightAhead) { isStraightAhead = YES; break; } } if (isStraightAhead) …

and there’s no clean way to get all the indications.

If we implement MBLaneIndication using NS_OPTIONS in Objective-C, then the Objective-C code becomes a lot simpler:

if (lane.indications & MBLaneIndicationStraightAhead) …

and the Swift code stays the same:

if lane.indications.contains(.StraightAhead) …

1ec5 · 2016-10-19T01:45:22Z

MapboxDirections/MBRouteStep.swift

@@ -513,6 +513,13 @@ public class RouteStep: NSObject, NSSecureCoding {
     */
    public let destinations: String?

+    /**
+     An intersection gives a full representation of any cross-way the path passes bay.


An array of intersections along the step.

Each item in the array corresponds to a cross street, starting with the intersection at the maneuver location indicated by the coordinates property and continuing with each cross street along the step.

1ec5 · 2016-10-19T01:46:09Z

MapboxDirectionsTests/V5Tests.swift

@@ -86,6 +86,18 @@ class V5Tests: XCTestCase {
        XCTAssertEqual(round(coordinate.longitude), -122)

        XCTAssertEqual(leg.steps[18].name, "Sycamore Valley Road West")
+
+        let intersection = step.intersections![0]


You can also say step.intersections.first!. I find that easier to type because of code completion.

1ec5

Along with some feedback from previous reviews that hasn’t been addressed yet.

1ec5 · 2016-10-19T18:53:51Z

MapboxDirections/MBIntersection.swift

    */
-    public let approachIndex: Int?
+    public var approachIndex: Int = -1


We probably don’t need to allow clients to modify these values. So you can either change this back to public let and set it to -1 in the initializer (as you seem to do anyways), or you can say public private(set) var.

1ec5 · 2016-10-19T18:54:48Z

MapboxDirections/MBIntersection.swift


-     If no lane information is available for an intersection, the lanes property will not be present.
+     If no lane information is available for an intersection, the lanes property will not be present. The first lane represents the left most lane, while the last represents the right most lane.


👍

Is this ordering the same for countries that drive on the left?

They should line up with what's printed on the road if you were standing on it looking at the arrows

1ec5 · 2016-10-19T18:55:22Z

MapboxDirections/MBIntersection.swift

-        let locationArray = json["location"] as! [Double]
-        let location = CLLocationCoordinate2D(latitude: locationArray[0], longitude: locationArray[1])
+        let coords = json["location"] as! [Double]
+        let location = CLLocationCoordinate2D.init(geoJSON: coords)


Nit: drop .init.

1ec5 · 2016-10-19T18:57:48Z

MapboxDirections/MBIntersection.swift

-        approachIndex = decoder.decodeObjectForKey("approachIndex") as? Int
-        outletIndex = decoder.decodeObjectForKey("outletIndex") as? Int
+        approachIndex = decoder.decodeObjectForKey("approachIndex") as! Int
+        outletIndex = decoder.decodeObjectForKey("outletIndex") as! Int


#81 correctly points out that we need to use decodeObjectOfClass(_:forKey:) to be secure. If #81 lands first, we should update the added classes in this PR to use decodeObjectOfClass(_:forKey:) too.

1ec5 · 2016-10-19T21:46:46Z

MapboxDirections/MBIntersection.swift

@@ -11,14 +11,14 @@ public class Intersection: NSObject, NSSecureCoding {

     The index of the item in the headings array that corresponds to the road that the containing route step uses to approach the intersection.


Mention the default value here, since Objective-C clients won’t be able to see the initializer’s implementation.

1ec5 · 2016-10-19T21:47:26Z

MapboxDirections/MBIntersection.swift

@@ -45,13 +45,13 @@ public class Intersection: NSObject, NSSecureCoding {
    public var lanes: [Lane]?

    /**
-     Set of Lane objects that have a valid turn.
+     Set of Lane objects that have a valid turn. This is a subset of the lanes object.


Nit: put lanes in backticks. When we generate jazzy documentation, jazzy will be able to autolink it to the lanes documentation.

1ec5 · 2016-10-19T21:49:01Z

MapboxDirections/MBIntersection.swift

@@ -45,13 +45,13 @@ public class Intersection: NSObject, NSSecureCoding {
    public var lanes: [Lane]?

    /**
-     Set of Lane objects that have a valid turn.
+     Set of Lane objects that have a valid turn. This is a subset of the lanes object.


What makes a turn “valid”?

A subset of the objects in the lanes property representing the lanes that can be used to travel from the road indicated by approachIndex to the road indicated by outletIndex.

1ec5 · 2016-10-19T21:50:05Z

MapboxDirections/MBLaneIndication.h

+    // Indicates no turn.
+    StraightAhead = (1 << 7),
+    // Indicates no turn
+    Uturn = (1 << 8),


Rename to UTurn to match ManeuverDirection.UTurn.

1ec5 · 2016-10-19T21:51:07Z

MapboxDirections/MBLaneIndication.h

+
+typedef NS_OPTIONS(NSUInteger, MBLaneIndication) {
+    // Indicates a turn to the left.
+    Left = (1 << 1),


Use the same order as ManeuverDirection: SharpRight, Right, SlightRight, StraightAhead, SlightLeft, Left, SharpLeft, UTurn.

1ec5 · 2016-10-19T21:52:54Z

MapboxDirections/MBLane.swift

-            type = .None
-        default:
-            return nil
+    public init?(descriptions: [String]) {


This public initializer needs a documentation comment.

1ec5 · 2016-10-19T21:53:23Z

MapboxDirections/MBLane.swift

-        case .None:
-            return "None"
+        var descriptions: [String] = []
+        if contains(LaneIndication.Left) {


Nit: you should be able to remove LaneIndication for brevity.

1ec5 · 2016-10-19T21:55:12Z

MapboxDirections/MBLaneIndication.h

+    SlightRight = (1 << 6),
+    // Indicates no turn.
+    StraightAhead = (1 << 7),
+    // Indicates no turn


Indicates a U-turn.

1ec5 · 2016-10-19T21:56:16Z

MapboxDirections/MBLaneIndication.h

+    StraightAhead = (1 << 7),
+    // Indicates no turn
+    Uturn = (1 << 8),
+    // Indicates a uturn.


No explicit turn indication.

1ec5 · 2016-10-19T21:57:24Z

MapboxDirections/MBLane.swift

+        if contains(LaneIndication.Uturn) {
+            descriptions.append("uturn")
+        }
+        if contains(LaneIndication.None) {


Is there a difference between [] and ["none"] in the API response? Is it possible for the API to output either value depending on whether a way is tagged ||right or none|none|right in OSM? (The two are semantically equivalent.) If ["none"] is output regardless, then let’s ignore the "none" string and use an empty LaneIndication to represent that case. We can remove the None value from the LaneIndication enumeration.

/cc @daniel-j-h @aaronlidman

These are the only values we return:

https://github.com/Project-OSRM/osrm-backend/blob/master/docs/http.md#properties-5

||right and none|none|right are the same for OSRM and will be converted to the fixed set of return types in the spec above.

The other question was whether none;right for a particular lane would turn into ["none", "right"] or ["straight", "right"] or just ["right"]. This is a tagging practice that led to some discussion in mapbox/mapping#193 and on the talk-us and tagging mailing lists.

I’m inclined to remove the None value and use [] to represent ["none"]. If ["none", right"] ever comes out of the API, we can ticket out turning that into [.StraightAhead, .Right].

They should come out as none (cc @MoKob and flagging Project-OSRM/osrm-backend#2645).

Have a look at this and similar scenarios:

https://github.com/Project-OSRM/osrm-backend/blob/ab1a9271c8c163a1bd514e17489cd35c6db22dc9/features/guidance/anticipate-lanes.feature#L109-L132

1ec5 · 2016-10-19T22:09:28Z

MapboxDirections/MBIntersection.swift

+
+     A value of true indicates that the respective road could be entered on a valid route. false indicates that the turn onto the respective road would violate a restriction.
+     */
+    public let entry: [Bool]


Still waiting for this method to be renamed to outlets and changed to be an IndexSet.

1ec5 · 2016-10-19T22:52:26Z

MapboxDirections/MBLaneIndication.h

+
+typedef NS_OPTIONS(NSUInteger, MBLaneIndication) {
+    // Indicates a sharp turn to the right.
+    SharpRight = (1 << 4),


Prefix each of these values with MBLaneIndication. In Swift, MBLaneIndication will be removed automatically.

1ec5 · 2016-10-19T22:53:17Z

MapboxDirections/MBLane.swift

+            descriptions.append("slight left")
+        }
+        if contains(LaneIndication.Left) {
+            descriptions.append("Left")


s/Left/left/

Added documentation for all new public symbols. Renamed and retyped Intersection properties. Corrected coding methods. Split out LaneIndication extension into a separate Swift file. Removed the None value for LaneIndication. Updated tests.

1ec5 · 2016-11-07T23:49:36Z

I’ve addressed all the feedback above and will merge this PR once most of the builds pass. Then I’ll update the swift3 branch. I’m going to let the watchOS build (the only one that actually tries to archive the framework) go red on master while we await #71.

1ec5 · 2016-11-07T23:51:13Z

Note that this PR does not add support for “use lane” instructions. That’s tracked in #79.

bsudekum · 2016-11-08T00:01:08Z

🎉

1ec5 · 2016-11-08T10:03:02Z

Merged to the swift3 branch in 67b8a95.

bsudekum added 2 commits October 17, 2016 15:37

Add intersections class

7b99e6a

Add lanes

e5e0459

Add another test

feb9389

1ec5 suggested changes Oct 18, 2016

View reviewed changes

bsudekum added 2 commits October 18, 2016 10:24

Document, change var names

343c4bf

Fix tests

852c66e

1ec5 reviewed Oct 18, 2016

View reviewed changes

Add usable lanes

4db2040

1ec5 suggested changes Oct 19, 2016

View reviewed changes

bsudekum added 3 commits October 19, 2016 08:45

Update docs

a722e26

make optional

35097e7

Add more docs

83a1690

1ec5 suggested changes Oct 19, 2016

View reviewed changes

bsudekum added 4 commits October 19, 2016 14:31

Better objc support

daaefe7

remove init

53fd75c

better description

9762618

doc

337814c

1ec5 suggested changes Oct 19, 2016

View reviewed changes

bsudekum added 2 commits October 19, 2016 15:29

Fix order, add more docs

6bd2459

fix right

0d556c2

1ec5 suggested changes Oct 19, 2016

View reviewed changes

prefix

4caf946

1ec5 self-assigned this Oct 28, 2016

Merge branch 'master' into lanes

e158c4c

1ec5 changed the title ~~Add lanes [WIP]~~ Intersections and lanes Nov 7, 2016

1ec5 merged commit 9223b88 into master Nov 7, 2016

1ec5 deleted the lanes branch November 7, 2016 23:59

1ec5 mentioned this pull request Nov 8, 2016

Serialize intersections field #59

Closed

		@@ -0,0 +1,56 @@
		import Foundation

		public class Intersection: NSObject, NSSecureCoding {

		@@ -0,0 +1,83 @@
		import Foundation

		public enum LaneIndicationType: Int, CustomStringConvertible {


		If no lane information is available for an intersection, the lanes property will not be present.
		If no lane information is available for an intersection, the lanes property will not be present. The first lane represents the left most lane, while the last represents the right most lane.

		@@ -11,14 +11,14 @@ public class Intersection: NSObject, NSSecureCoding {

		The index of the item in the headings array that corresponds to the road that the containing route step uses to approach the intersection.

Intersections and lanes #80

Intersections and lanes #80

Conversation

bsudekum commented Oct 17, 2016 • edited Loading

bsudekum commented Oct 18, 2016

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

Choose a reason for hiding this comment

bsudekum Oct 18, 2016 • edited Loading

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

Choose a reason for hiding this comment

bsudekum commented Oct 18, 2016

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

1ec5 Oct 19, 2016 • edited Loading

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

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

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

1ec5 Oct 19, 2016 • edited Loading

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 Nov 7, 2016 • edited Loading

1ec5 commented Nov 7, 2016

bsudekum commented Nov 8, 2016

1ec5 commented Nov 8, 2016

bsudekum commented Oct 17, 2016 •

edited

Loading

bsudekum Oct 18, 2016 •

edited

Loading

1ec5 Oct 19, 2016 •

edited

Loading

1ec5 Oct 19, 2016 •

edited

Loading

1ec5 commented Nov 7, 2016 •

edited

Loading