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

RouteStep parsing fails when directions for cycling require user to dismount bike #413

Closed
SebastianOsinski opened this issue Feb 14, 2020 · 3 comments · Fixed by #417
Closed
Labels

Comments

@SebastianOsinski
Copy link
Contributor

SebastianOsinski commented Feb 14, 2020

Version: 1.0.0-alpha.1

Response for calculate(_: completionHandler:) sometimes fails to decode, when RouteOptions is NavigationRouteOptions with profileIdentifier set to .cycling. If directions require user to dismount bike, then mode field in one of the RouteStep parts of response is set to pushing bike:

 {
                "intersections": [
                     ...
                ],
                "driving_side": "right",
                "geometry": "sonn`Bafmn_@nLoaA|@{G",
                "mode": "pushing bike", // <-- this
                "maneuver": {
                  "bearing_after": 106,
                  "bearing_before": 57,
                  "location": [
                    17.030257,
                    51.109642
                  ],
                  "modifier": "right",
                  "type": "continue",
                  "instruction": "Turn right to stay on Rynek"
                },
                "weight": 87.4,
                "duration": 87.4,
                "name": "Rynek",
                "distance": 88.6,
                "bannerInstructions": [
                     ...
                ]
              },

RouteStep does not support this type of mode. mode is represented by TransportType enum which does not have case for that.

Documentation comment:

This is the usual transport type when the `profileIdentifier` is `DirectionsProfileIdentifier.walking`. For cycling directions, this value indicates that the user is expected to dismount.
suggests that walking should be returned as mode instead of pushing bike

Documentation for Directions API doesn't mention this type of mode: https://docs.mapbox.com/api/navigation/#route-step-object

Screen Shot 2020-02-14 at 10 06 48

@1ec5
Copy link
Contributor

1ec5 commented Feb 22, 2020

Apparently the documentation is incorrect. OSRM returns pushing bike as the mode. The plan at some point may have been for the Directions API to transform it to walking, but that hasn’t happened. When we rewrote RouteStep in #382, we probably saw this table in the documentation and thought pushing bike had gotten replaced by walking in Directions API v5 and the pushing bike support was only present for v4 compatibility.

We can fix the issue by mapping pushing bike to walking in the context of the cycling profile. There’s currently an effort underway to transition the cycling profile from OSRM to Valhalla, which lacks support for the bike-pushing mode, so it probably isn’t worth keeping an explicit bike-pushing mode around. Once the API cuts over to Valhalla, this mapping from pushing bike to walking would become dead code.

/cc @danpaz

@1ec5 1ec5 added the bug label Feb 22, 2020
@SebastianOsinski
Copy link
Contributor Author

SebastianOsinski commented Feb 24, 2020

@1ec5 Thank you. Is there some estimate when the transition to Valhalla will be finished? IMO this is pretty serious bug right now, as it prevents SDK users from getting bicycle directions in many cases. Maybe it would be best for now to implement parsing pushing bike as walking in RouteStep? This should be pretty easy fix - we only need to implement decoding and coding by hand instead of relying on compiler-generated implementation. I can do it if this change is welcome

@1ec5
Copy link
Contributor

1ec5 commented Feb 24, 2020

Yes, a compatibility shim on the client side would be welcome. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants