Detailed Road Classification #507
Conversation
…reetClass enum. Added property and its coding to Intersection
Udumft
requested review from
1ec5,
MaximAlien,
chezzdev,
dersim-davaod and
avi-c
| /// Smaller variation of a roundabout with no center island or obstacle | ||
| case MiniRoundabout = "mini_roundabout" |
There was a problem hiding this comment.
By definition, mini roundabouts are never mapped as ways in OpenStreetMap, so they wouldn’t appear in the routing graph as an edge to assign a Mapbox Streets road class to. It’s worth confirming that this is also true of other data sources, but I’d be hard-pressed to think of a situation where it would make sense to represent a mini roundabout as a linear feature (as opposed to a full-size roundabout).
| /// (point Widened section at the end of a cul-de-sac for turning around a vehicle | ||
| case TurningCircle = "turning_circle" | ||
| /// (point Similar to a turning circle but with an island or other obstruction at the centerpoint | ||
| case TurningLoop = "turning_loop" | ||
| /// (point Lights or other signal controlling traffic flow at an intersection | ||
| case TrafficSignals = "traffic_signals" | ||
| /// (point A point indication for road junctions | ||
| case Junction = "junction" | ||
| /// (point Indicating the class and type of roads meeting at an intersection. Intersections are only available in Japan | ||
| case Intersection = "intersection" |
There was a problem hiding this comment.
I think we should omit these values. As these road classifications are currently used, they would only occur on linear features, not point features. They appear in the road layer of the Mapbox Streets source because that layer also contains point features. For example, some styles use the road layer’s traffic_signals-classed point features to label traffic lights with 🚦.
There was a problem hiding this comment.
Directions team suggests to still have these values even though not all of them can really be present... So maybe we should leave it just to be safe?
There was a problem hiding this comment.
I’m inclined to say YAGNI, since we could always add more cases to this non-frozen enumeration without breaking backwards compatibility, and it’s difficult to imagine a situation where the Directions API would start adding these values to edges without breaking backwards compatibility in other ways.
If we really want to be on the safe side, I guess we could leave it in but keep it undocumented. But even then, the downside is that developers still have to deal with these dead code paths whenever they deal with Streets road classes. Swift will complain about these cases being missing from any switch statement unless the developer adds a default @unknown clause. So I’m inclined to omit these values; if on the off chance they occur, it would be the same as an unexpected new value that the Directions API adds after the fact.
There was a problem hiding this comment.
According to the source code, there are more cases which will never make it to the real world navigation: pedestrian (this one is surprising, why not?) , construction, track, path, "major_rail", minor_rail, service_rail, aerialway, golf.
Maybe we should also not add them for now?
| /** | ||
| Street class according to Mapbox Streets V8 classification. | ||
|
|
||
| This value is set to `nil` of such info is not available. | ||
| */ |
There was a problem hiding this comment.
| /** | |
| Street class according to Mapbox Streets V8 classification. | |
| This value is set to `nil` of such info is not available. | |
| */ | |
| /** | |
| The road classes of the road that the containing step uses to leave the intersection, according to the [Mapbox Streets source](https://docs.mapbox.com/vector-tiles/reference/mapbox-streets-v8/#road), version 8. | |
| If detailed road class information is unavailable, this property is set to `nil`. This property only indicates the road classification; for other aspects of the road, use the `outletRoadClasses` property. | |
| */ |
And in the outletRoadClasses documentation, add a cross-reference like this:
For more detailed road classifications, use the
outletMapboxStreetsRoadClassproperty.
…perties and MapboxStreetsRoadClass
| /// Railways, including mainline, commuter rail, and rapid transit. | ||
| case majorRail = "major_rail" | ||
| /// Includes light rail & tram lines. | ||
| case minorRail = "minor_rail" | ||
| /// Yard and service railways. | ||
| case serviceRail = "service_rail" | ||
| /// Ski lifts, gondolas, and other types of aerialway. | ||
| case aerialway = "aerialway" | ||
| /// The approximate centerline of a golf course hole | ||
| case golf = "golf" |
There was a problem hiding this comment.
In case you’re wondering how these classes would ever come up: railroads are sometimes used as driveways in rural areas (but would have to be tagged with the appropriate access tags to be included in a route), and theoretically a ski lift could be part of a pedestrian route.
…r MapboxSteetsRoadClassCodable
|
I've added a small change that will allow to safely ignore unrecognized road classes. This will allow backwards compatibility with potential future additions to the road classes list without crashing the app. |
| if let classString = try container.decodeIfPresent(String.self, forKey: .streetClass) { | ||
| streetClass = MapboxStreetsRoadClass(rawValue: classString) | ||
| } else { | ||
| streetClass = nil |
There was a problem hiding this comment.
I think this is fine because adding new road classes is a rare event and the new value wouldn’t be immediately useful in the navigation SDK anyways. However, if we ever learn of a plan to add new road classes in Streets source v8.x, we should proactively implement the other case with an associated value, similar to what’s proposed in #506 (comment). Not necessary just yet though.
Adds support for Mapbox Streets V8 road classes.