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

Makes construction=minor ways routable again, see #4258 #4306

Merged
merged 3 commits into from
Jul 19, 2017

Conversation

daniel-j-h
Copy link
Member

In #4258 we removed construction tags except for construction=widening and construction=no.

Turns out we missed construction=minor in the list of 3804 unique construction values.

This affects 1194 ways on the planet.

@daniel-j-h daniel-j-h added this to the 5.9.0 milestone Jul 19, 2017
@daniel-j-h daniel-j-h requested a review from TheMarex July 19, 2017 11:18
@@ -521,7 +521,7 @@ function WayHandlers.blocked_ways(profile,way,result,data)
local construction = way:get_value_by_key('construction')

-- Of course there are negative tags to handle, too
if construction and construction ~= 'no' and construction ~= 'widening' then
if construction and construction ~= 'no' and construction ~= 'widening' and construction ~= 'minor' then
Copy link

Choose a reason for hiding this comment

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

how about doing an explicit check for the tags we wan't to block from routing? Otherwise we might just end up excluding another strange tag we might not yet even know about/that is introduced in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general this is a good idea in practice osm allows for

The value of the construction=* key should be one of those defined bellow and indicates the type of feature being built (you can use any value from from highway=*, railway=* and building=*).

so unless we want to hard-code all of those values I think we're good for now.

Copy link

Choose a reason for hiding this comment

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

Also from the OSM wiki as a side-note.

Unclassified road under construction (alternatively tag construction=minor)

Copy link

@MoKob MoKob Jul 19, 2017

Choose a reason for hiding this comment

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

We could check for https://github.com/Project-OSRM/osrm-backend/blob/master/profiles/car.lua#L143-L156 or some similar list, right? That would avoid hardcoding more strings there.

Copy link

Choose a reason for hiding this comment

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

Capturing from chat: I was under the misconception that this tagging scheme would try to parse highway=construction and construction=highway_type. I am fine with this change for the special cases to handle deprecated tags.

Copy link
Member

Choose a reason for hiding this comment

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

What would be cool however is a whitelist in the profile table. Then you could do:

if construction and not profile.construction_whitelist[construction] then
  ...
end

@daniel-j-h
Copy link
Member Author

Updated - please have a look again.

@@ -57,6 +57,8 @@ function setup()

restricted_highway_whitelist = Set { },

construction_whitelist = Set {},
Copy link
Contributor

@emiltin emiltin Jul 19, 2017

Choose a reason for hiding this comment

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

probably not needed, since avoid construction is not activated in the foot profile?

Copy link
Member

@TheMarex TheMarex left a comment

Choose a reason for hiding this comment

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

Looks good to me. Please cherry-pick to the 5.9 branch after Travis gives 💚 .

@TheMarex TheMarex merged commit 64e4b7e into master Jul 19, 2017
@daniel-j-h
Copy link
Member Author

Ah, thanks for cherry picking and running this out 🙇

@daniel-j-h daniel-j-h deleted the construction-minor branch July 20, 2017 09:06
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.

4 participants