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

Compatibility with Mapbox Optimization API #534

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Bigood
Copy link

@Bigood Bigood commented Jun 6, 2019

Original issue

Passing alternative=(true|false) to the Mapbox Optimization API resulted as a bad request, I needed to have the option to remove the query parameter. While looking at the code, I changed a few things to be able to remove steps and pass the roundtrip parameter to the API.

Reference : https://docs.mapbox.com/api/navigation/#optimization

Usecase

  router: L.Routing.mapbox(process.env.REACT_APP_MAPBOX_API_KEY, {
      serviceUrl: 'https://api.mapbox.com/optimized-trips/v1', //Overrides the serviceUrl
      profile : 'mapbox/cycling',
      routingOptions: {
        roundtrip:true, //Wasn't used on the url construction
        steps: false,  //Will be considered as an option
        alternatives: "default" //Will remove the appended option on the URL
      }
    }),

Changes

Mainly on osrm-v1.js, to be compatible Mapbox Optimization API, see attached commits.

Using the argument routingOptions. A "default" key will result as the parameter not being added at all on the query string, otherwise, if a boolean is provided, it will be appended with the corresponding key.
Mapbox optimization route returns trips instead of routes
@Bigood Bigood changed the title Usability for d Compatibility with Mapbox Optimization API Jun 6, 2019
@novaknole
Copy link

Hi. Any updates on this? have the same mistake and don't want to change source code.

What do I do?

@dcrobertson01
Copy link

Yeah - same here. I'd like to get some cycle routes onto the maps ....

@Bigood
Copy link
Author

Bigood commented Apr 15, 2020

@perliedman It's been a year since I've PR this, any plan to merge it?

route = this._convertRoute(response.routes[i]);
//Mapbox optimization route returns trips instead of routes
let routes = response.routes || response.trips;
for (i = 0; i < routes.length; i++) {
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like some indentation mistake here, perhaps spaces/tabs getting mixed up?

var locs = [],
hints = [],
wp,
latLng,
computeInstructions,
computeAlternative = true;
computeInstructions = options.steps && options.steps !== "default" ? true : false,
Copy link
Owner

Choose a reason for hiding this comment

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

What does this new "default" mean? How is it intended to be used?

@perliedman
Copy link
Owner

@Bigood yeah sorry about that, I don't use LRM for anything myself these days, so it's hard to find the time/energy to work on it, and easy to miss issues and PRs.

This looks good as far as I can tell, with some minor comments I made to the code. Haven't actually tested the code, but I assume you have tested it still works with "normal" routing as well as with the optimization API?

@Bigood
Copy link
Author

Bigood commented Apr 15, 2020

Thanks for taking the time to review this! I'll test it again with the most recent API, double check my code, and probably correct some bits.

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.

4 participants