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

Enable alleyPriority for automobile routing #416

Merged
merged 1 commit into from
Mar 12, 2020
Merged

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Feb 20, 2020

The RouteOptions.alleyPriority property now works with DirectionsProfileIdentifier.automobile in addition to DirectionsProfileIdentifier.walking.

/cc @mapbox/navigation-ios @avi-c

@1ec5 1ec5 added improvement Improvement for an existing feature. platform parity labels Feb 20, 2020
@1ec5 1ec5 self-assigned this Feb 20, 2020
@1ec5 1ec5 requested a review from avi-c February 20, 2020 01:22
@1ec5
Copy link
Contributor Author

1ec5 commented Feb 20, 2020

Until this PR lands, or in v0.38, you can work around the lack of alley priority support while driving by subclassing RouteOptions and overriding urlQueryItems to add an alley_bias query item.

@1ec5
Copy link
Contributor Author

1ec5 commented Feb 20, 2020

DirectionsTests.testGETRequest() and DirectionsTests.testPOSTRequest() are failing in several ways. The cascading failures start with:

/Users/distiller/project/Tests/MapboxDirectionsTests/DirectionsTests.swift:54: error: -[MapboxDirectionsTests.DirectionsTests testGETRequest] : XCTAssertLessThanOrEqual failed: ("8200") is greater than ("8192") - maximumCoordinateCount is too high

Now that the alley_bias query parameter is included in the request URL by default for the driving profile, slightly fewer coordinate pairs can fit in the URL for a GET request.

@avi-c
Copy link
Contributor

avi-c commented Feb 20, 2020

These changes look good to me aside from fixing the tests.

@1ec5 1ec5 added this to the v1.0.0 milestone Mar 10, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Mar 10, 2020

I’ve updated the tests.

@1ec5 1ec5 requested review from JThramer and Udumft March 10, 2020 21:00
@@ -110,7 +110,7 @@ open class RouteOptions: DirectionsOptions {
/**
A number that influences whether the route should prefer or avoid alleys or narrow service roads between buildings.

This property has no effect unless the profile identifier is set to `DirectionsProfileIdentifier.walking`.
This property has no effect unless the profile identifier is set to `DirectionsProfileIdentifier.automobile` or `DirectionsProfileIdentifier.walking`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe with this change it now make sense to set alleyPriority to .low by default if current profile is .automobile?
It alters current behavior, but maybe that is what we wanted with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the application or maybe the navigation SDK should be responsible for making that decision. As much as possible, MapboxDirections should align with the Directions API defaults. But the navigation SDK does vend a NavigationRouteOptions subclass that could set the alleyPriority to .low for .automobile.

@1ec5 1ec5 merged commit 7c1ced4 into master Mar 12, 2020
@1ec5 1ec5 deleted the 1ec5-driving-alley branch March 12, 2020 16:12
@1ec5 1ec5 modified the milestones: v1.0.0, v0.31.0 (v0.40) Apr 14, 2020
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.

3 participants