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

In Belgium the maximum speed in rural areas is 70 in the region Flanders #5214

Closed
wants to merge 1 commit into from

Conversation

frodrigo
Copy link
Member

In Belgium the maximum speed in rural areas is 70 in the region Flanders
(hint from osm-fr/osmose-backend#334)

@chaupow
Copy link
Member

chaupow commented Sep 27, 2018

Hi @hgcvm and @frodrigo !
thanks a lot for the PR ✨

I just tested this and noticed that BE-VLG:rural is often tagged as source:maxspeed which is not parsed by the current lua profiles at all. (This smells like a bug that is unrelated to this change).
See https://www.openstreetmap.org/way/593881430 as an example.

@frodrigo would you be interested in fixing that? I wil leave notes in your code on how to do it. I can then code review and merge 🙂

Let me know if you are time constrained, then I will take over.

@chaupow
Copy link
Member

chaupow commented Sep 27, 2018

I noticed I can't add proposals to your PR because the affected files weren't changed so here my thoughts:

The relevant line is:
https://github.com/Project-OSRM/osrm-backend/blob/master/profiles/lib/way_handlers.lua#L435

local keys = Sequence { 'maxspeed:advisory', 'maxspeed' }

Per the wiki https://wiki.openstreetmap.org/wiki/Key:source:maxspeed we should add source:maxspeed and also maxspeed:type to the list of keys that we consider. Otherwise the tag will be ignored for maxspeed checks.

The function Tags.get_forward_backward_by_set here will return the first value found, so the order of keys matters.

I think it would be wise to check the tags in this order: maxspeed, maxspeed:type, source:maxspeed, maxspeed:advisory but open to other opinions here.

Let me know if you have questions.

@frodrigo
Copy link
Member Author

Maybe @alain-andre may be interested in implementing this.

@chaupow
Copy link
Member

chaupow commented Sep 28, 2018

@frodrigo @alain-andre 👋

I picked this up here #5217

If you want and have some time, want to take a look at it and either 👍 or 👎 ?

Again: Thanks for fixing this ❤️

@danpat
Copy link
Member

danpat commented Dec 15, 2018

Superseded by #5217, which was merged and will be in 5.21.0

@danpat danpat closed this Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants