Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add roadSide to RouteStep #219

Merged
merged 5 commits into from
Dec 4, 2017
Merged

Add roadSide to RouteStep #219

merged 5 commits into from
Dec 4, 2017

Conversation

frederoni
Copy link
Contributor

Fixes #212 - Add roadSide to RouteStep

Unblocks mapbox/mapbox-navigation-ios#909

@bsudekum @1ec5 👀

@frederoni frederoni self-assigned this Dec 4, 2017
bsudekum
bsudekum reviewed Dec 4, 2017
View reviewed changes
MapboxDirections/MBRouteStep.swift Outdated
@objc(MBRoadSide)
public enum RoadSide: Int, CustomStringConvertible {
case left
case right
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this is pretty self explanatory, can you document this class it's properties?

bsudekum
bsudekum reviewed Dec 4, 2017
View reviewed changes
MapboxDirections/MBRouteStep.swift Outdated
/**
Indicates what side of a bidirectional road the driver must be driving on. Also referred to as the rule of the road.
*/
open let roadSide: RoadSide?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is always returned, I think we can make this non-optional. Noting, it is in fact returned for biking/walking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it optional will also allow it to bridge to Objective-C.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant non-optional. It should be non-optional.

bsudekum
bsudekum reviewed Dec 4, 2017
View reviewed changes
MapboxDirections/MBRouteStep.swift Outdated
@@ -557,6 +597,7 @@ open class RouteStep: NSObject, NSSecureCoding {
let maneuverType = ManeuverType(description: maneuver["type"] as! String)
let maneuverDirection = ManeuverDirection(description: maneuver["modifier"] as? String ?? "")
let maneuverLocation = CLLocationCoordinate2D(geoJSON: maneuver["location"] as! [Double])
let roadSide = RoadSide(description: json["driving_side"] as! String) ?? .right
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure how to handle a nil here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible for any production profile to not provide a driving_side property? If so, we would add an unknown enumeration value. If not, we would assert or implicitly unwrap the optional.

@1ec5
Copy link
Contributor

1ec5 commented Dec 4, 2017

Regarding the property’s name, roadSide is potentially a little misleading for the walking and cycling profiles, because it doesn’t indicate the side of the road that the sidewalk or bike lane is on. On the other hand, drivingSide would also be misleading because it does indicate the side of a bike trail to use. I don’t think either choice is a huge problem, but here are some more options:

  • ruleOfRoad
  • handedness
  • sideToKeep (as in “Keep left/right”, as opposed to “overtaking side”)

bsudekum
bsudekum reviewed Dec 4, 2017
View reviewed changes
MapboxDirections/MBRouteStep.swift Outdated
guard let roadSideDescription = decoder.decodeObject(of: NSString.self, forKey: "roadSide") as String?, let roadSide = RoadSide(description: roadSideDescription) else {
return nil
}
self.roadSide = roadSide
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be an issue for old routes that are encoded without a roadSide?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let’s assume it’s right if unspecified. That would match OSRM’s behavior prior to Project-OSRM/osrm-backend#4415.

/cc @danpat

@friedbunny
Copy link
Contributor

Regarding the property’s name, roadSide is potentially a little misleading for the walking and cycling profiles, because it doesn’t indicate the side of the road that the sidewalk or bike lane is on.

sideOfTravel/travelSide?

@bsudekum
Copy link

bsudekum commented Dec 4, 2017 •
edited
Loading

@1ec5 I agree. To riff on this a little more, it's important to note that it might make sense to incorporate the word car or driving into the property name. Your suggestions might be confusing for pedestrians as sideToKeep sounds like pedestrians must keep to this side.

Maybe drivingSideOfTravel?

@1ec5
Copy link
Contributor

1ec5 commented Dec 4, 2017 •
edited
Loading

I think sideOfTravel suffers from the same problem, in that it implies something about the current cycleway’s geometry relative to a road. trafficSide would be less problematic in this regard.

The more I think about this, the more comfortable I am with calling it drivingSide as the API does: the rule of the road for bike paths and bike lanes is generally based on the one for vehicular traffic. When it differs, as with reversed bike lanes, we would need some other field to make that clear anyways.

In the end, I think developers will have to just read the documentation to understand this property, unless we call it something silly like steeringWheelSide (which isn’t necessarily accurate either).

@bsudekum
Copy link

bsudekum commented Dec 4, 2017

👍 to drivingSide.

@bsudekum
Copy link

bsudekum commented Dec 4, 2017

@mapbox/navigation-ios this is ready for review.

@bsudekum bsudekum mentioned this pull request Dec 4, 2017
Merged
1 task
@frederoni
Copy link
Contributor Author

✅ LGTM but I can't approve.

1ec5
1ec5 approved these changes Dec 4, 2017
View reviewed changes
MapboxDirections/MBRouteStep.swift Outdated
@@ -643,10 +643,10 @@ open class RouteStep: NSObject, NSSecureCoding {
maneuverDirection = nil
}

guard let roadSideDescription = decoder.decodeObject(of: NSString.self, forKey: "roadSide") as String?, let roadSide = RoadSide(description: roadSideDescription) else {
guard let drivingSideDescription = decoder.decodeObject(of: NSString.self, forKey: "drivingSide") as String?, let drivingSide = DrivingSide(description: drivingSideDescription) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s default this value to right if it’s unspecified, so that preexisting objects don’t suddenly fail to decode.

@bsudekum bsudekum merged commit 7f1d2d2 into master Dec 4, 2017
@bsudekum bsudekum deleted the fred-road-side branch December 4, 2017 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsudekum bsudekum bsudekum left review comments

@1ec5 1ec5 1ec5 approved these changes

@JThramer JThramer Awaiting requested review from JThramer

Assignees

@frederoni frederoni

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@frederoni @1ec5 @friedbunny @bsudekum