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

Use new OSRM API #1261

Closed
wants to merge 6 commits into from
Closed

Use new OSRM API #1261

wants to merge 6 commits into from

Conversation

TheMarex
Copy link
Contributor

@TheMarex TheMarex commented Aug 4, 2016

This addresses issue #1161

Currently this is not working yet and mostly the result of an long-ish train ride.

  • Fix query code
  • Add sprites icons for new instructions (almost done)
  • Make sure there are translations for each new instruction type

This adds support for the new API in OSRM 5.x.
Since this also introduced new instruction types new icon sprites are included.
@TheMarex TheMarex changed the title [WIP] Use new OSRM API Use new OSRM API Aug 5, 2016
@TheMarex
Copy link
Contributor Author

TheMarex commented Aug 5, 2016

Rebased this against master and adapted other routing plugins to also use the new icons.

@tomhughes would be much obliged if you could review this so we can get this up to production, since we want to shut down the OSRM 4.9 server soon. 🙇

@tomhughes
Copy link
Member

Well it would be a lot easier if you hadn't gratuitously changed all the icons instead of just adding the new ones... I'm a little concerned to that the new ones appear to be somewhat less bold, using thinner lines and things that will effect the visual appearance.

I don't have much time this weekend and I'm away all next week but I'll see what I can do.

@tomhughes
Copy link
Member

Presumably I need to change the endpoint we are hitting for this to work? Currently I just get an error for any route I try...

@tomhughes
Copy link
Member

So some specific issues with the new icons:

  • Icon 16 has bad the direction of the arrows reversed - admittedly I don't think it's actually used but we should either remove it or keep it the same.
  • Icons 8 (start) and 14 (end) are the same, which made sense when it was just a blob but doesn't now you've added an arrow, though you seem to be using 9 for start with OSRM anyway.

In general terms as I said I'm not a great fan of the thinner lines, shorter arrows and generally more anguiar forms compared to the old icons.

Some of the new ones are also very complicated though I understand it's hard to visualise such things in such a small space. Icons 20 and 21 in particular are hard to understand, though I'm not really sure where they would be used anyway - there are very few places where roads "merge" other than on ramps and those have other icons anyway (well actually they don't, they use slight-right and slight-left!).

@tomhughes
Copy link
Member

Oh, and there are jshint warnings being reported by travis.

@tomhughes
Copy link
Member

Hmm you have got mapzen using 20 for on ramps, but OSRM is using 2 and 6 which just seems wrong - that isn't even slight left and right, it's full 90 degree left and right.

@TheMarex
Copy link
Contributor Author

TheMarex commented Aug 5, 2016

I need to change the endpoint we are hitting for this to work?

I'm not sure how to configure this. I modified config/application.yml on my local test setup. The new URL is //router.project-osrm.org/route/v1/driving/. Maybe you can point me to the location where I can fix this.

Icon 16 has bad the direction of the arrows reversed

Good catch.

Icons 8 (start) and 14 (end) are the same

Agreed, fixed.

you have got mapzen using 20 for on ramps, but OSRM is using 2 and 6

I don't see that. Github let's you comment on the exact lines in the changes of a PR, maybe that helps.

You will note that the new icons come with a vector source unlike the old ones. I'm sure some more experienced designers can improve them.

@tomhughes
Copy link
Member

You should update the URL in config/example.application.yml which is the example config.

@tomhughes
Copy link
Member

Not sure if this is a bug in OSRM or in the javascript, but http://dev.osm.compton.nu/directions?engine=osrm_car&route=50.9580%2C-2.7590%3B50.9465%2C-2.7696#map=15/50.9523/-2.7660 is telling me to a right turn when it's clear a left at the bottom of the slip road!

@TheMarex
Copy link
Contributor Author

TheMarex commented Aug 5, 2016

telling me to a right turn when it's clear a left at the bottom of the slip road!

Good catch, the maneuver simplification had a small bug.

@tomhughes
Copy link
Member

tomhughes commented Aug 5, 2016

Merged as 311acad with a few fixes for icon mapping in the other engines as 078dc1a.

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.

None yet

2 participants