Revamp waypoint types #388

1ec5 · 2019-11-14T00:00:09Z

Replaced the Waypoint and Tracepoint classes with separate classes for requests and responses to better match the design of the Directions API and Map Matching API and avoid the messy situation of classes with multiple mutually exclusive properties.

Now you create a RouteOptions or MatchOptions using a series of DirectionsOptions.Waypoint instances (also known as RouteOptions.Waypoint or MatchOptions.Waypoint). The RouteCompletionHandler and MatchCompletionHandler closures and the response types represent waypoints as DirectionsResult.Waypoint (also known as RouteOptions.Waypoint) and tracepoints as Match.Tracepoint.

Also renamed the Tracepoint.alternateCount property to Tracepoint.countOfAlternatives and added the snapped distance to the DirectionsResult.Waypoint type.

Fixes #333 and fixes #564. Depends on #382.

/cc @mapbox/navigation-ios

1ec5 · 2019-11-14T00:00:15Z

Some tests are failing because the test fixtures need to updated. They were generated back before the Directions and Map Matching APIs started returning distance as part of each snapped waypoint.

1ec5 · 2019-11-14T00:02:32Z

Sources/MapboxDirections/DirectionsResult.swift

@@ -29,7 +29,10 @@ open class DirectionsResult: Codable {
        distance = try container.decode(CLLocationDistance.self, forKey: .distance)
        expectedTravelTime = try container.decode(TimeInterval.self, forKey: .expectedTravelTime)

-        _directionsOptions = decoder.userInfo[.options] as! DirectionsOptions
+        guard let directionsOptions = decoder.userInfo[.options] as? DirectionsOptions else {
+            throw DirectionsCodingError.missingOptions


This PR continues to assume that options are passed in as a precondition for decoding, but it throws errors instead of force-unwrapping dictionary entries. Ideally, #382 would incorporate these changes to fix #228.

1ec5 · 2019-11-23T20:15:51Z

A major problem with converting Waypoint to a struct is that a given waypoint appears in multiple places in the response. The developer would potentially expect that changing the name of one leg’s destination would affect the name of the next leg’s source. The solution would be to move RouteLeg.source to Route and, once #391 is implemented, make Response.waypoints a computed property based on Route.source and RouteLeg.destination.

Replaced the Waypoint and Tracepoint classes with separate classes for requests and responses.

1ec5 · 2020-01-31T02:02:18Z

CHANGELOG.md

+* Replaced the `Waypoint` and `Tracepoint` classes with separate classes for requests and responses. Now you create a `RouteOptions` or `MatchOptions` using a series of `DirectionsOptions.Waypoint` instances (also known as `RouteOptions.Waypoint` or `MatchOptions.Waypoint`). The `RouteCompletionHandler` and `MatchCompletionHandler` closures and the response types represent waypoints as `DirectionsResult.Waypoint` (also known as `RouteOptions.Waypoint`) and tracepoints as `Match.Tracepoint`. ([#382](https://github.com/mapbox/MapboxDirections.swift/pull/382))
+* Replaced the `Route.coordinates` property with `Route.shape` and the `RouteStep.coordinates` property with `RouteStep.shape`. The `Route.coordinateCount` and `RouteStep.coordinateCount` properties have been removed, but you can use the `LineString.coordinates` property to get the array of `CLLocationCoordinate2D`s. ([#382](https://github.com/mapbox/MapboxDirections.swift/pull/382))
+* `RouteLeg.source` and `RouteLeg.destination` are now optional. They can be `nil` when the `RouteLeg` object is decoded individually from JSON. ([#382](https://github.com/mapbox/MapboxDirections.swift/pull/382))
+* The `


Finish this sentence.

Some context would be helpful to do it :)

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.

Sources/MapboxDirections/MapMatching/MapMatchingResponse.swift

Udumft · 2020-04-03T13:54:57Z

Sources/MapboxDirections/MapMatching/MatchResponse.swift

+                // TODO: Filter on matchings_index.
+                // TODO: Push tracepoints down to individual legs to reflect waypoint_index.


Can someone point me to some kind of a doc or explanation about tracepoints in the response and matchings_index, waypoint_index too?

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.

According to that doc, our Match and Tracepoint objects do not reflect the structure described. Is there any special reason why? Or that's what I can safely take care of?

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 RouteLeg.segmentRangesByStep property added in #250 will be important for avoiding out-of-bounds issues.

But in the short term, we should keep pushing things like tracepoints down into the individual matches so that the matches are more self-contained.

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 RouteOptions.waypoints like an inout parameter. But I tried to avoid churning other classes were we differed in more minor or more deliberate ways.

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.
At the same time, to correctly "keep pushing things like tracepoints down into the individual matches" we need to follow the doc and decode Tracepoint's matchings_index and waypoint_index... I see that previous implementation just copied all tracepoints to all matches, which seem to work only while there is a single Match provided (unless it was removed here).
We may just restore this behavior, but why not improve it to make it "more safe".

# Conflicts: # CHANGELOG.md # Sources/MapboxDirections/Directions.swift # Sources/MapboxDirections/DirectionsResult.swift # Sources/MapboxDirections/MapMatching/MapMatchingResponse.swift # Sources/MapboxDirections/MapMatching/MatchResponse.swift # Sources/MapboxDirections/RouteResponse.swift # Sources/MapboxDirections/Waypoint.swift # Tests/MapboxDirectionsTests/DirectionsTests.swift # Tests/MapboxDirectionsTests/MatchTests.swift # Tests/MapboxDirectionsTests/RoutableMatchTests.swift # Tests/MapboxDirectionsTests/WaypointTests.swift

Udumft · 2020-04-07T12:40:31Z

Sources/MapboxDirections/MapMatching/Tracepoint.swift

+         */
+        public var countOfAlternatives: Int
+
+        public var name: String? // ??? matching response otherwise won't be able to decode waypoints names.


If we don't supply a name in a Tracepoint, Map Matching response decoding won't pass this data to RouteResponse and in it's turn resulting Waypoints won't have name too.

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.

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.
If we are to preserve waypoint name returned by the API (and apparently Route.Waypoint is), Tracepoint should have a name. Otherwise, Waypoints won't be decoded correctly as they depend on Tracepoint data: https://github.com/mapbox/mapbox-directions-swift/pull/388/files#diff-a2198eb412da79c9d8351c4e811b7dc6R63-R66

Udumft · 2020-04-07T12:42:24Z

Sources/MapboxDirections/RouteResponse.swift

+        case code // << do we need these?
+        case message // <<
+        case error // <<


These values are now decoded using ResponseDisposition, but It is only used internally while parsing the response and is not passed to the user. I assume this is what we want, and these keys inside RouteResponse are not needed?

Comment still actual

I think you’re right, because #418 turns the three fields into an Error in the Result.

Udumft · 2020-04-07T12:44:01Z

Sources/MapboxDirections/RouteResponse.swift

+                    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


I hope I understood correctly how filtering should be applied. I decided to stick to options.legSeparators and filter out decoded waypoints that correspond it.

Udumft · 2020-04-07T12:46:20Z

Sources/MapboxDirections/MapMatching/MatchResponse.swift

@@ -0,0 +1,31 @@
+import Foundation
+
+class MatchResponse: Codable {


MatchResponse seems removed in some other PR. I assume it means that that tracepoint filtering should occur in MapMatchingResponse instead?

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.

… matching response decoding to respect matchings index

Udumft · 2020-04-08T09:01:57Z

Sources/MapboxDirections/MapMatching/Tracepoint.swift

+        public var name: String?
+
+        /***
+         The index of the match object in matchings that the sub-trace was matched to.
+         */
+        public var matchingIndex: Int = 0 // TODO: not sure about default value
+
+        /***
+         The index of the waypoint inside the matched route.
+         */
+        public var waypointIndex: Int = 0 // TODO: not sure about default value


There were not originally in the doc, but looks like it is required to have.

These indices are parallel state that can get out of sync with the array of matches or waypoints. This starts to get into #386 territory. I think the filtering suggested in that issue could make these properties unnecessary.

Udumft · 2020-04-08T09:07:05Z

Sources/MapboxDirections/RouteResponse.swift

+            if let routes = routes {
+                let sorted = zip(filtered, waypoints!).sorted {
+                    $0.0.waypointIndex < $1.0.waypointIndex
+                }
+                for (index, route) in routes.enumerated() {
+                    let legSeparators = sorted.filter {
+                        return $0.0.matchingIndex == index
+                    }.map { $0.1 }
+
+                    assert(legSeparators.count == route.legs.count + 1)
+
+                    route.legSeparators = legSeparators
+                }
+            }


This part is how I understand Map Matching Doc. The idea is to set leg separators explicitly via matchings_index and waypoint_index. Not sure if it is required though or will have any effect at all since routes and waypoints are passed to the user as public properties and waypoints are not modified in this code.
A help/review would be required here.

Udumft · 2020-04-08T09:07:44Z

Sources/MapboxDirections/RouteResponse.swift

+                        return $0.0.matchingIndex == index
+                    }.map { $0.1 }
+
+                    assert(legSeparators.count == route.legs.count + 1)


I assume this is something we can expect to be guaranteed?

There will be fewer leg separators if any of the waypoints in the request had separatesLegs equal to false. A waypoint that doesn’t separate legs is also known as a “silent waypoint” or “via point”: it influences the path of the route but doesn’t get called out specifically as a destination.

…d Match relation to Tracepoints and it's decoding in MapMatchingResponse. Fixed tests

Udumft · 2020-04-09T14:04:57Z

distance property added to Waypoint, Waypoint and Tracepoint classes replaced with structures. Did I miss anything else should be done here?

1ec5 added backwards incompatible changes that break backwards compatibility of public API op-ex Refactoring, Tech Debt or any other operational excellence work. labels Nov 14, 2019

1ec5 added this to the v1.0 milestone Nov 14, 2019

1ec5 self-assigned this Nov 14, 2019

1ec5 commented Nov 14, 2019

View reviewed changes

1ec5 mentioned this pull request Nov 14, 2019

Remove Objective-C compatibility; adopt Swift language features #382

Merged

1 task

This was referenced Nov 21, 2019

Please refactor your unwrapping optionals #228

Closed

Encapsulate responses in coherent public types #391

Closed

1ec5 force-pushed the 1ec5-waypoint-struct branch from ae081ab to acc3b21 Compare November 23, 2019 20:20

1ec5 force-pushed the jerrad/objc-delenda-est branch from c5e733d to 9f88ac6 Compare November 23, 2019 20:23

1ec5 force-pushed the 1ec5-waypoint-struct branch from acc3b21 to be7816b Compare November 23, 2019 20:23

1ec5 mentioned this pull request Nov 26, 2019

Upgrade to post-Objective-C MapboxDirections.swift mapbox/mapbox-navigation-ios#2275

Merged

4 tasks

This was referenced Dec 7, 2019

Eliminate abstract classes #392

Open

Discontinue Objective-C compatibility #381

Closed

Revamped waypoint types

b04d6a5

Replaced the Waypoint and Tracepoint classes with separate classes for requests and responses.

1ec5 force-pushed the 1ec5-waypoint-struct branch from be7816b to b04d6a5 Compare December 16, 2019 22:11

1ec5 changed the base branch from jerrad/objc-delenda-est to master December 16, 2019 23:28

1ec5 commented Jan 31, 2020

View reviewed changes

1ec5 mentioned this pull request Feb 14, 2020

Suggestion: Consider using Result instead of optional response and error #414

Closed

JThramer assigned Udumft Apr 1, 2020

Udumft reviewed Apr 3, 2020

View reviewed changes

Sources/MapboxDirections/MapMatching/MapMatchingResponse.swift Outdated Show resolved Hide resolved

Udumft reviewed Apr 3, 2020

View reviewed changes

Udumft mentioned this pull request Apr 4, 2020

Public Response Types #406

Merged

9 tasks

Victor Kononov added 3 commits April 4, 2020 16:12

waypoint-struct: fixed existing tests

aef354b

waypoint-struct: response filtering props

dd04c10

Udumft reviewed Apr 7, 2020

View reviewed changes

waypoint-struct: updated Tracepoint struct to match the doc, fixed(?)…

59db4e4

… matching response decoding to respect matchings index

Udumft reviewed Apr 8, 2020

View reviewed changes

Victor Kononov added 3 commits April 8, 2020 14:04

waypoint-struct: added few tests for Route.Waypoint

e187091

waypoint-rewamp: removed RouteResponse excessive coding keys, restore…

b47fab9

…d Match relation to Tracepoints and it's decoding in MapMatchingResponse. Fixed tests

waypoint-struct: updated CHANGELOG. CI fix.

072b149

Udumft marked this pull request as ready for review April 10, 2020 08:24

Udumft assigned 1ec5 and unassigned 1ec5 Apr 10, 2020

1ec5 modified the milestones: v1.0.0, v0.31.0 (v0.40) Apr 14, 2020

1ec5 modified the milestones: v0.31.0, v1.0.0 Apr 24, 2020

1ec5 mentioned this pull request Aug 25, 2020

Improve route response decoding. #446

Merged

1ec5 modified the milestones: v1.0.0, v2.0.0 Sep 2, 2020

1ec5 changed the base branch from master to main October 6, 2020 17:20

1ec5 added the needs estimate label Jan 20, 2021

truburt modified the milestones: v2.0.0, v2.0.0 (GA) Mar 5, 2021

1ec5 mentioned this pull request Jul 20, 2021

RouteOptions and Waypoint should be structs #564

Open

truburt removed the needs estimate label Oct 4, 2021

1ec5 modified the milestones: v2.0, v3.0 Oct 19, 2021

S2Ler marked this pull request as draft January 17, 2023 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp waypoint types #388

Revamp waypoint types #388

1ec5 commented Nov 14, 2019 •

edited

Loading

1ec5 commented Nov 14, 2019

1ec5 Nov 14, 2019

1ec5 commented Nov 23, 2019 •

edited

Loading

1ec5 Jan 31, 2020

Udumft Apr 4, 2020

1ec5 Apr 6, 2020

Udumft Apr 3, 2020

1ec5 Apr 6, 2020

Udumft Apr 7, 2020

1ec5 Apr 9, 2020

Udumft Apr 9, 2020

Udumft Apr 7, 2020 •

edited

Loading

1ec5 Apr 9, 2020

Udumft Apr 9, 2020

Udumft Apr 7, 2020

Udumft Apr 8, 2020 •

edited

Loading

1ec5 Apr 9, 2020

Udumft Apr 7, 2020

Udumft Apr 7, 2020

1ec5 Apr 9, 2020

Udumft Apr 8, 2020

1ec5 Apr 9, 2020

Udumft Apr 8, 2020

Udumft Apr 8, 2020

1ec5 Apr 9, 2020 •

edited

Loading

Udumft commented Apr 9, 2020

		// TODO: Filter on matchings_index.
		// TODO: Push tracepoints down to individual legs to reflect waypoint_index.

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

		class MatchResponse: Codable {

Revamp waypoint types #388

Are you sure you want to change the base?

Revamp waypoint types #388

Conversation

1ec5 commented Nov 14, 2019 • edited Loading

1ec5 commented Nov 14, 2019

Choose a reason for hiding this comment

1ec5 commented Nov 23, 2019 • 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

Udumft Apr 7, 2020 • 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

Udumft Apr 8, 2020 • 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

1ec5 Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Udumft commented Apr 9, 2020

1ec5 commented Nov 14, 2019 •

edited

Loading

1ec5 commented Nov 23, 2019 •

edited

Loading

Udumft Apr 7, 2020 •

edited

Loading

Udumft Apr 8, 2020 •

edited

Loading

1ec5 Apr 9, 2020 •

edited

Loading