Revamp waypoint types #388
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import Foundation | ||
|
|
||
| class MatchResponse: Codable { | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. At the time this PR was originally written, MatchResponse interpreted Map Matching API responses as Map Matching API responses (MatchOptions→Match), whereas MapMatchingResponse interpreted Map Matching API responses as Directions API responses (MatchOptions→Route). It was confusing, so #406 eliminated MapMatchingResponse and renamed MatchResponse to MapMatchingResponse. |
||
| var code: String | ||
| var message: String? | ||
| var matches : [Match]? | ||
| var tracepoints: [Match.Tracepoint?]? | ||
|
|
||
| private enum CodingKeys: String, CodingKey { | ||
| case code | ||
| case message | ||
| case matches = "matchings" | ||
| case tracepoints | ||
| } | ||
|
|
||
| public required init(from decoder: Decoder) throws { | ||
| let container = try decoder.container(keyedBy: CodingKeys.self) | ||
| code = try container.decode(String.self, forKey: .code) | ||
| message = try container.decodeIfPresent(String.self, forKey: .message) | ||
| tracepoints = try container.decodeIfPresent([Match.Tracepoint?].self, forKey: .tracepoints) | ||
| matches = try container.decodeIfPresent([Match].self, forKey: .matches) | ||
|
|
||
| if let tracepoints = self.tracepoints, let matches = matches { | ||
| for match in matches { | ||
| // TODO: Filter on matchings_index. | ||
| // TODO: Push tracepoints down to individual legs to reflect waypoint_index. | ||
|
There was a problem hiding this comment. Can someone point me to some kind of a doc or explanation about There was a problem hiding this comment. Here’s the Map Matching API documentation, which is more relevant than before, now that this PR hews more closely to the Map Matching API response format. It isn’t necessary to fix #386 as part of this PR, but I think understanding that issue helped me understand these properties better. There was a problem hiding this comment. According to that doc, our There was a problem hiding this comment. The Map Matching API response format avoids repetition to minimize data transfer, and it’s optimized for a file format (JSON) that has no concept of pointer references. As a result, the response is rife with parallel structures and indices. At the same time, the Directions API response is influenced by the OSRM Router service’s heavily nested response format (maneuvers inside steps inside legs inside routes). As part of developing the “bring your own route” workflow, the Map Matching API adopted most of the Directions API’s format so that the client could duck-type matches as routes. So Map Matching API responses end up with a mix of indexable parallel structures in some places and repetitive, nested structures in other places. Originally, this library only supported the Directions API, so it adopted that API’s highly nested structure. We also avoided index-based structures, even where the Directions API had them, because of potential out-of-bounds issues and other misuse. Pointer references are more convenient to work with than indices and don’t result in excessive memory consumption. It’s also much more convenient for objects to be self-contained: if the application only cares about the match, it can pass around or archive an individual Match object instead of the entire MatchResponse plus a match index. In the long term, I think we will want to go all-in on indexed parallel structures, once a future version of the Directions API adopts Valhalla’s more consistently indexed-based structure. When we do that, things like the But in the short term, we should keep pushing things like This is a good example of how I think we want to be deliberate about where this library’s types resemble or differ from the API response. The API response format has a lot of warts, having bent over backwards to preserve backwards compatibility, so we only want to copy what make sense. We prioritized refactoring the waypoint/tracepoint types because they were too different from the corresponding API response objects, leading to some questionable patterns like treating There was a problem hiding this comment. OK, so due to some reasonable matters like API structure and JSON format we may not follow the doc strictly but instead "adjust" info we provide to users to have some adequate form. |
||
| match.tracepoints = tracepoints | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,40 +1,47 @@ | ||
| import Foundation | ||
| import CoreLocation | ||
|
|
||
| public extension Match { | ||
| /** | ||
| A tracepoint represents a location matched to the road network. | ||
| */ | ||
| struct Tracepoint: Matchpoint, Equatable { | ||
| // MARK: Positioning the Waypoint | ||
|
|
||
| /** | ||
| The geographic coordinate of the waypoint, snapped to the road network. | ||
| */ | ||
| public var coordinate: CLLocationCoordinate2D | ||
|
|
||
| /** | ||
| The straight-line distance from this waypoint to the corresponding waypoint in the `RouteOptions` or `MatchOptions` object. | ||
|
|
||
| The requested waypoint is snapped to the road network. This property contains the straight-line distance from the original requested waypoint’s `DirectionsOptions.Waypoint.coordinate` property to the `coordinate` property. | ||
| */ | ||
| public var correction: CLLocationDistance | ||
|
|
||
| // MARK: Determining the Degree of Confidence | ||
|
|
||
| /** | ||
| Number of probable alternative matchings for this tracepoint. A value of zero indicates that this point was matched unambiguously. | ||
| */ | ||
| public var countOfAlternatives: Int | ||
|
|
||
| public var name: String? // ??? matching response otherwise won't be able to decode waypoints names. | ||
|
There was a problem hiding this comment. If we don't supply a There was a problem hiding this comment. A waypoint (or tracepoint) name coming from the API isn’t actually that meaningful. Normally it’s just the name of the street that the waypoint got snapped to. If the application supplied a waypoint name as part of the request, that name is worth preserving. In the past, we’d override the API response’s waypoint names with the request’s waypoint names, where available. Now it’s up to the application to hang onto the request and associate the request’s waypoints with the response’s waypoints in some cases. There was a problem hiding this comment. Sorry, it's a bit misleading: "that name is worth preserving" and "it’s up to the application to hang onto the request and associate the request’s waypoints with the response’s waypoints" seems to be the opposite statements. |
||
| } | ||
| } | ||
|
|
||
| extension Match.Tracepoint: Codable { | ||
| private enum CodingKeys: String, CodingKey { | ||
| case coordinate = "location" | ||
| case correction = "distance" | ||
| case countOfAlternatives = "alternatives_count" | ||
| case name | ||
| } | ||
| } | ||
|
|
||
| extension Match.Tracepoint: CustomStringConvertible { | ||
| public var description: String { | ||
| return "<latitude: \(coordinate.latitude); longitude: \(coordinate.longitude)>" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ public struct RouteResponse { | |
|
|
||
| public let identifier: String? | ||
| public var routes: [Route]? | ||
| public let waypoints: [Route.Waypoint]? | ||
|
|
||
| public let options: ResponseOptions | ||
| public let credentials: DirectionsCredentials | ||
|
|
@@ -27,15 +27,15 @@ public struct RouteResponse { | |
|
|
||
| extension RouteResponse: Codable { | ||
| enum CodingKeys: String, CodingKey { | ||
| case code // << do we need these? | ||
| case message // << | ||
| case error // << | ||
|
There was a problem hiding this comment. These values are now decoded using There was a problem hiding this comment. I think you’re right, because #418 turns the three fields into an Error in the Result. |
||
| case identifier = "uuid" | ||
| case routes | ||
| case waypoints | ||
| } | ||
|
|
||
| public init(httpResponse: HTTPURLResponse?, identifier: String? = nil, routes: [Route]? = nil, waypoints: [Route.Waypoint]? = nil, options: ResponseOptions, credentials: DirectionsCredentials) { | ||
| self.httpResponse = httpResponse | ||
| self.identifier = identifier | ||
| self.routes = routes | ||
|
|
@@ -58,12 +58,12 @@ extension RouteResponse: Codable { | |
| routes = try decoder.decode([Route].self, from: matchesData) | ||
| } | ||
|
|
||
| var waypoints: [Route.Waypoint]? | ||
|
|
||
| if let tracepoints = response.tracepoints { | ||
| let filtered = tracepoints.compactMap { $0 } | ||
| let tracepointsData = try encoder.encode(filtered) | ||
| waypoints = try decoder.decode([Route.Waypoint].self, from: tracepointsData) | ||
| } | ||
|
|
||
| self.init(httpResponse: response.httpResponse, identifier: nil, routes: routes, waypoints: waypoints, options: .match(options), credentials: credentials) | ||
|
|
@@ -91,37 +91,63 @@ extension RouteResponse: Codable { | |
| self.identifier = try container.decodeIfPresent(String.self, forKey: .identifier) | ||
|
|
||
| // Decode waypoints from the response and update their names according to the waypoints from DirectionsOptions.waypoints. | ||
| let decodedWaypoints = try container.decodeIfPresent([Route.Waypoint?].self, forKey: .waypoints)?.compactMap{ $0 } | ||
| var optionsWaypoints: [Route.Waypoint] = [] | ||
|
|
||
| switch options { | ||
| case let .match(options: matchOpts): | ||
| optionsWaypoints = matchOpts.waypoints.map { | ||
| Route.Waypoint(coordinate: $0.coordinate, | ||
| correction: 0, | ||
| name: $0.name) | ||
| } | ||
| case let .route(options: routeOpts): | ||
| optionsWaypoints = routeOpts.waypoints.map { | ||
| Route.Waypoint(coordinate: $0.coordinate, | ||
| correction: 0, | ||
| name: $0.name) | ||
| } | ||
| } | ||
|
|
||
| if let decodedWaypoints = decodedWaypoints { | ||
| // The response lists the same number of tracepoints as the waypoints in the request, whether or not a given waypoint is leg-separating. | ||
| waypoints = zip(decodedWaypoints, optionsWaypoints).map { (pair) -> Route.Waypoint in | ||
| var (decodedWaypoint, waypointInOptions) = pair | ||
| if let name = waypointInOptions.name?.nonEmptyString { | ||
| decodedWaypoint.name = name | ||
| } | ||
| return decodedWaypoint | ||
| } | ||
| } else { | ||
| waypoints = decodedWaypoints | ||
| } | ||
|
|
||
| if let routes = try container.decodeIfPresent([Route].self, forKey: .routes) { | ||
| // Postprocess each route. | ||
| var legSeparators = waypoints?.compactMap { $0 } | ||
| if let options = decoder.userInfo[.options] as? DirectionsOptions, let waypoints = waypoints { | ||
| // Select first, last and all other waypoints which separate legs. | ||
| var optionsLegSeparators = options.legSeparators | ||
| legSeparators = zip(waypoints, options.waypoints).compactMap { (pair) -> Route.Waypoint? in | ||
| let (decodedWaypoint, waypointInOptions) = pair | ||
| if decodedWaypoint == waypoints.last! || | ||
| decodedWaypoint == waypoints.first! { | ||
| return decodedWaypoint | ||
| } | ||
|
|
||
| guard let index = optionsLegSeparators.firstIndex(of: waypointInOptions) else { | ||
| return nil | ||
| } | ||
| optionsLegSeparators.remove(at: index) | ||
| return decodedWaypoint | ||
|
Comment on lines
+129
to
+139
There was a problem hiding this comment. I hope I understood correctly how filtering should be applied. I decided to stick to |
||
| } | ||
| } | ||
|
|
||
| for route in routes { | ||
| route.routeIdentifier = identifier | ||
| // Imbue each route’s legs with the waypoints refined above. | ||
| if let legSeparators = legSeparators { | ||
| route.legSeparators = legSeparators | ||
| } | ||
| } | ||
| self.routes = routes | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finish this sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some context would be helpful to do it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I had been going through the PR looking for breaking changes to call out but got distracted. If you see anything else that has changed in the public API besides the first item, feel free to add it here; otherwise, we can remove this item.