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 support for speed limits via maxspeed annotations. #367

Merged
merged 5 commits into from
Aug 7, 2019

Conversation

avi-c
Copy link
Contributor

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

Include SDK/API support for speed limits from the Directions API via the maxspeed annotations in the directions response.

@avi-c avi-c requested a review from 1ec5 April 16, 2019 19:32
@1ec5
Copy link
Contributor

1ec5 commented Apr 16, 2019

This is a revival of #250.

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! In addition to the feedback below, would you do the honors and add a changelog entry?

MapboxDirections/MBAttribute.h Outdated Show resolved Hide resolved
MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
/**
Indicates the road segment uses kilometers per hour for measuring speed limits.
*/
case kilometersPerHour
Copy link
Contributor

Choose a reason for hiding this comment

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

Argh, it would be great if we could just use Measurement along with UnitSpeed.kilometersPerHour, but alas we still support iOS 9.0 (along with macOS 10.11.0, tvOS 9.0, and watchOS 2.0). I’m chomping at the bit to drop iOS 9 support.

/cc @mapbox/navigation-ios

Copy link
Contributor

@1ec5 1ec5 Apr 23, 2019

Choose a reason for hiding this comment

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

A proposed change to the Directions API would add information about locally relevant units to each step, making this class somewhat redundant. We could store the speed limit annotations as CLLocationSpeeds (converting everything to meters per second). Then client code could losslessly convert back to kilometers per hour or miles per hour as indicated by the step.

MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
MapboxDirections/MBSpeedLimit.swift Outdated Show resolved Hide resolved
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.

A few nits, but this looks good. Note that we probably shouldn’t merge this PR until we’re ready to mention the new annotation type in the official Directions API documentation.

CHANGELOG.md Outdated Show resolved Hide resolved
MapboxDirections/MBRouteLeg.swift Outdated Show resolved Hide resolved
MapboxDirections/MBRouteLeg.swift Outdated Show resolved Hide resolved
@1ec5
Copy link
Contributor

1ec5 commented Aug 7, 2019

I revived #250 and rebased and retargeted this PR atop it, because that PR already had tests and handled Autobahns correctly.

@1ec5 1ec5 force-pushed the maxspeedAnnotations branch from 4682568 to d723ec9 Compare August 7, 2019 02:36
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.

Going to handle #367 (comment) and #367 (comment) as to-do items for landing #250.

@1ec5 1ec5 merged commit 4bcbcf8 into maxspeeds Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement for an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants