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.

Sign up for GitHub

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

Jump to bottom

Penalize turns from non-local access only edges to local access only edges #3721

Merged
merged 1 commit into from
Feb 20, 2017
Merged

Penalize turns from non-local access only edges to local access only edges #3721

merged 1 commit into from
Feb 20, 2017

Conversation

karenzshea
Copy link
Contributor

@karenzshea karenzshea commented Feb 16, 2017 •
edited by TheMarex
Loading

Issue

Closes #156

Instead of completely removing ways tagged with local access only or routing over them with impunity, this pr adds a heavy penalty on turn maneuvers from non-local access only edges to local access only edges. This allows routing onto a local access only, but only if the destination of the route ends on the way.

"Local access only" is defined in the local_access_tag_list set in the car lua profile. The default profile includes ways tagged like

  • delivery
  • private
  • destination
  • hov

Tasklist

  • add regression / cucumber cases (see docs/testing.md)
  • review
  • adjust for comments

@TheMarex TheMarex added this to the 5.6.0 milestone Feb 16, 2017
@danpat
Copy link
Member

danpat commented Feb 16, 2017

There's one scenario that I can think of that's very similar, but not covered by this:

Nodes: http://wiki.openstreetmap.org/wiki/Tag:barrier=toll_booth
Ways: http://wiki.openstreetmap.org/wiki/Key:toll

I think this PR could handle the toll=yes tagging on ways that should be added, and hooked into the avoid mechanism in the profile so we can turn it on/off easily (and it should default to not avoiding tolls).

Some highways only have barrier=toll_booth though - the ways are missing the toll tags. It would be nice to try to handle these too, but that's a harder problem because it's a node tag, and maybe belongs in a separate PR.

daniel-j-h
daniel-j-h reviewed Feb 16, 2017
View reviewed changes
Copy link
Member

@daniel-j-h daniel-j-h left a comment

Choose a reason for hiding this comment

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

Nice 👍 Should we engage the OSM community - there might be more tags out there we might be missing.

profiles/car.lua
'agricultural',
'forestry',
'emergency',
'psv',
'delivery'
'psv'
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for having the blacklist at all now? If we never route onto the local access ways we're fine in the sense of "blacklisting" them from routing. But if users somehow end up on those ways we still want to be able to route them off of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point. As someone who has definitely driven onto roads I wasn't supposed to be on, routing out of these is probably a better way to handle them, rather than pretending they don't exist in the road network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll ticket this and do it in a separate PR because it will involve changing all tests that test access of any kind.

profiles/car.lua Outdated
@@ -358,6 +358,7 @@ function way_function(way, result)
Handlers.run(handlers,way,result,data,profile)
end

-- Called during edge-based-graph creation
Copy link
Member

Choose a reason for hiding this comment

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

Hm maybe remove impl. detail comment here, I think this might just confuse users who have no idea about the C++ side of things.

profiles/car.lua Outdated
if not turn.source_local_access_only and turn.target_local_access_only then
turn.weight = turn.weight + 3000
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Can we symbolize both routability as well as 3000. Also why 3000?

unit_tests/library/tile.cpp Outdated
@@ -205,7 +205,7 @@ BOOST_AUTO_TEST_CASE(test_tile)
}

BOOST_CHECK_EQUAL(number_of_turn_keys, 3);
BOOST_CHECK_EQUAL(number_of_turns_found, 732);
BOOST_CHECK_EQUAL(number_of_turns_found, 763);
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove exact checks here - every other pull request has to adapt these numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is annoying

@daniel-j-h
Copy link
Member

Btw SIGABRT in some Cucumber tests on Travis is something to look into.

@danpat danpat mentioned this pull request Feb 16, 2017
Closed
@karenzshea karenzshea force-pushed the feature/locals-only branch 2 times, most recently from bbeb938 to 8bba0f0 Compare February 17, 2017 11:00
@karenzshea karenzshea mentioned this pull request Feb 17, 2017
Open
TheMarex
TheMarex reviewed Feb 17, 2017
View reviewed changes
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, but maybe we can think about a better name for the property exposed via profile API.

include/extractor/extraction_way.hpp Outdated
@@ -106,6 +108,8 @@ struct ExtractionWay
bool roundabout;
bool circular;
bool is_startpoint;
bool backward_local_access_only;
Copy link
Member

Choose a reason for hiding this comment

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

Using the term restricted instead of local_access_only might be more in line with our usual vocabulary for this. My suggestion would be {backward,forward}_restricted

Copy link
Member

Choose a reason for hiding this comment

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

As per slack, renamed.

profiles/car.lua Outdated
@@ -34,6 +34,9 @@ local profile = {
speed_reduction = 0.8,
traffic_light_penalty = 2,
u_turn_penalty = 20,
local_access_penalty = 3000,
Copy link
Member

Choose a reason for hiding this comment

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

Possibly rename to restriced_penalty

@karenzshea karenzshea mentioned this pull request Feb 17, 2017
Closed
@TheMarex TheMarex merged commit c2727f2 into master Feb 20, 2017
@TheMarex TheMarex deleted the feature/locals-only branch February 20, 2017 12:40
@daniel-j-h daniel-j-h mentioned this pull request Feb 23, 2017
Closed
@danpat danpat mentioned this pull request Apr 24, 2017
Closed
@karenzshea karenzshea mentioned this pull request Jan 9, 2018
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-j-h daniel-j-h daniel-j-h left review comments

@TheMarex TheMarex TheMarex left review comments

@danpat danpat Awaiting requested review from danpat

Assignees
No one assigned
Labels
Ready To Merge
Projects
None yet
Milestone
5.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@karenzshea @danpat @daniel-j-h @TheMarex