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.

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

Add speed limit data returned by MapboxDirections.swift to the RouteProgress object #2112

Closed
wants to merge 6 commits into from

Conversation

avi-c
Copy link
Contributor

@avi-c avi-c commented Apr 22, 2019

Incorporate the new maxspeed annotations on the RouteLeg object from MapboxDirections.swift. Provide this data to Navigation SDK clients via the maximumSpeedLimitsByLeg array on the RouteProgress object.

Copy link
Contributor

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

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

Thanks for putting this PR together! Like mapbox/mapbox-directions-swift#367, this PR won’t be merged until we’re ready to document the new speed limit annotation type as part of the public Directions API.

@@ -1,6 +1,6 @@
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 4.3
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 6.1.1
github "mapbox/MapboxDirections.swift" ~> 0.27.3
github "mapbox/MapboxDirections.swift" "maxspeedAnnotations"
Copy link
Contributor

Choose a reason for hiding this comment

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

This PR depends on mapbox/mapbox-directions-swift#367. Once that PR lands, we’ll need to change this dependency to master. Finally, once we publish a new release of MapboxDirections.swift, we’d change this line and corresponding lines in the podspecs to require the new release.


maneuverCoordinateIndex = 0
if let segmentMaximumSpeedLimits = leg.segmentMaximumSpeedLimits {
for step in leg.steps {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consolidate this loop with the loop above that populates the segmentCongestionLevels and expectedSegmentTravelTimes properties for better performance. We’ll need to refactor a bit of the code, so that the absence of traffic congestion data doesn’t prevent us from processing speed limit data and vice versa.

Returns the SpeedLimit for the current position along the route. Returns SpeedLimit.invalid if the speed limit is unknown or missing.
*/
public var currentSpeedLimit: SpeedLimit {
let coordinatesLeftOnStepCount = Int(floor((Double(currentLegProgress.currentStepProgress.step.coordinateCount)) * currentLegProgress.currentStepProgress.fractionTraveled))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the same approach taken by #1242. Unfortunately, it assumes that each coordinate is evenly spaced along the step, which is almost never the case. For our purposes, we should assume that the segments are of arbitrary length.

With traffic congestion, it made sense to store congestion levels by segment, because we use those segments to create a shape layer all at once. Displaying the right speed limit at the right time is a very similar problem to presenting the right spoken or visual instruction at the right time, as RouteController and LegacyRouteController do. As you loop over the attributes, set each speed limit to the cumulative distance along the step. Then this computed property can become:

public var currentSpeedLimit: SpeedLimit {
    
    let speedLimits = maximumSpeedLimitsByLeg[legIndex][currentLegProgress.stepIndex]
    return speedLimits.last { $0.distance <= distanceTraveled } ?? .invalid
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@1ec5 - Are you suggesting that I add a distance property to the SpeedLimit object or to keep a collection of (SpeedLimit, distance) pairs that is private to RouteProgress, built in the init() and referenced by currentSpeedLimit()?

Copy link
Contributor

Choose a reason for hiding this comment

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

The latter, a collection of (SpeedLimit, distance) pairs. If the additional nesting turns out to be problematic, then the tuples could be grouped by leg and the distances could be measured from the start of the leg, but the tradeoff would be worse performance during the trip.

@1ec5
Copy link
Contributor

1ec5 commented Apr 24, 2019

The saga continues in #2114.

@1ec5 1ec5 closed this Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants