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

Walking parameters #370

Merged
merged 6 commits into from
May 16, 2019
Merged

Walking parameters #370

merged 6 commits into from
May 16, 2019

Conversation

frederoni
Copy link
Contributor

@frederoni frederoni commented May 10, 2019

This PR adds walking parameters (alleyPriority, walkwayPriority, and speed) to RouteOptions.

  • Audit Obj-C bridging

Fixes #371.

cc @1ec5 @kevinkreiser 👀

/**
A bias which determines whether the route should prefer or avoid the use of alleys. The
allowed range of values is from -1.0 to 1.0, where -1 indicates preference to avoid
alleys, 1 indicates preference to favor alleys, and 0 indicates no preference.

Choose a reason for hiding this comment

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

🙏 thank you for grabbing the correct docs!

@frederoni frederoni changed the title Walking options Walking parameters May 10, 2019
@frederoni frederoni requested a review from 1ec5 May 10, 2019 13:15
Copy link

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

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

this looks good to me! i defer to @1ec5

@frederoni
Copy link
Contributor Author

Initially in 68dfa3f, I started with a clear separation between route options and walking options by subclassing directions options because walking parameters and driving parameters are mutually exclusive but not yet enforced on the server-side. However, currently, as this PR stands, the walking parameters are folded into RouteOptions but the relevant query items are parsed for the corresponding profile identifier.

case .cycling:
break
default:
break
}

if !roadClassesToAvoid.isEmpty {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"exclude" (roadClassesToAvoid) should only be appended if profile identifier is any of [.automobile, .automobileAvoidingTraffic, .cycling] for now.

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.

We’re putting these properties in RouteOptions for now because they aren’t supported in the Map Matching API yet, but we’ll need to move them from RouteOptions to DirectionsOptions as soon as the Map Matching API adds support for them.

Remember to add a blurb to the changelog about the new options.

queryItems.append(URLQueryItem(name: "roundabout_exits", value: String(includesExitRoundaboutManeuver)))

switch profileIdentifier {
case .automobile, .automobileAvoidingTraffic:
Copy link
Contributor

Choose a reason for hiding this comment

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

These switch statements might become more invasive over time. Maybe we can extend the MBDirectionsProfileIdentifier enumeration so that a given profile knows which parameters it supports?

Defaults to 1.42
This property has no effect unless profile identifier is set to `MBDirectionsProfileIdentifier.walking`.
*/
@objc open var walkingSpeed: CLLocationSpeed = 1.42

This comment was marked as resolved.

@objc open var walkwayBias: Double = 0

/**
Walking speed in meters per second. Must be between 0.14 and 6.94 meters per second.

This comment was marked as resolved.

@1ec5 1ec5 self-assigned this May 15, 2019
@1ec5 1ec5 added the improvement Improvement for an existing feature. label May 15, 2019
switch profileIdentifier {
case .automobile, .automobileAvoidingTraffic:
if includesExitRoundaboutManeuver {
queryItems.append(URLQueryItem(name: "roundabout_exits", value: String(includesExitRoundaboutManeuver)))
Copy link
Contributor

Choose a reason for hiding this comment

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

All profiles support this parameter, and roundabouts do occasionally occur along bike/hike trails. Better that this library pass through the parameter so that the navigation SDK can assume its presence.

1ec5 added 2 commits May 15, 2019 15:49
A more generic property name for speed allows the same property to be reused for cycling options in the future. “Priority” sounds more positive than “bias” and aligns with UILayoutPriority and similar types. An extensible enumeration gives us a way to symbolically refer to meaningful priorities.
Copy link
Contributor

@JThramer JThramer left a comment

Choose a reason for hiding this comment

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

:shipit:

@1ec5
Copy link
Contributor

1ec5 commented May 16, 2019

d57c1e0 renamed alleyBias, walkwayBias, and walkingSpeed to alleyPriority, walkwayPriority, and speed, respectively. alleyPriority and walkwayPriority now make use of a new MBDirectionsPriority extensible enumeration, which allows us to define preset values without proliferating global constants or hard-coding magic number literals.

@1ec5 1ec5 merged commit e23c776 into master May 16, 2019
@1ec5 1ec5 deleted the walking-options branch May 16, 2019 07:16
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. platform parity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add walking bias options
4 participants