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

Honor access mode while matching OSMRestriction with graph #2849

Merged
merged 3 commits into from
Feb 17, 2021
Merged

Honor access mode while matching OSMRestriction with graph #2849

merged 3 commits into from
Feb 17, 2021

Conversation

merkispavel
Copy link
Contributor

@merkispavel merkispavel commented Feb 9, 2021 •
edited
Loading

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@merkispavel merkispavel changed the base branch from master to get-graph-ids February 9, 2021 20:52
@merkispavel merkispavel marked this pull request as draft February 9, 2021 20:53
@merkispavel merkispavel force-pushed the look-at-restriction-modes branch 2 times, most recently from e243bc1 to 935e4b8 Compare February 9, 2021 21:00
merkispavel
merkispavel commented Feb 9, 2021
View reviewed changes
test/gurka/test_route.cc
{gurka::way_member, "EF", "from"},
{gurka::way_member, "FB", "via"},
{gurka::way_member, "BA", "to"},
},
Copy link
Contributor Author

@merkispavel merkispavel Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

EF and FG edges have the same way_id. So GetGraphIds return GFBA instead of EFBA. We must look at the access mode in my opinion. Let me know if the suggested solution is correct or not: it's the first that came to my mind

@merkispavel
Copy link
Contributor Author

I compared the number of restrictions between master and this branch: the PR "removes" some of the restrictions(5-10 restrictions of ~2700). So need more time to investigate it

@merkispavel merkispavel mentioned this pull request Feb 9, 2021
Merged
4 tasks
@gknisely gknisely self-requested a review February 10, 2021 03:42
@gknisely
Copy link
Member

see comment here

@merkispavel merkispavel force-pushed the look-at-restriction-modes branch 2 times, most recently from 0b29351 to bf5ee6e Compare February 12, 2021 11:53
@merkispavel
Copy link
Contributor Author

Checked master vs pr diff in restrictions in the Florida. Looks good

List of restrictions that are removed due to access flags:
https://www.openstreetmap.org/relation/10618528
https://www.openstreetmap.org/relation/10618529
https://www.openstreetmap.org/relation/9768860
https://www.openstreetmap.org/relation/9063828
https://www.openstreetmap.org/relation/10368658
https://www.openstreetmap.org/relation/10368657
https://www.openstreetmap.org/relation/9949592
https://www.openstreetmap.org/relation/7509315
In most cases there's at least one oneway way(heh) among the restriction

The last two restrictions are actually buggy: the via way is incorrect
https://www.openstreetmap.org/relation/9949592
https://www.openstreetmap.org/relation/7509315

gknisely
gknisely previously approved these changes Feb 15, 2021
View reviewed changes
@merkispavel merkispavel marked this pull request as ready for review February 16, 2021 18:07
@merkispavel
Copy link
Contributor Author

Example of the fixed case: osm origin restriction, valhalla before, valhalla after

Screen Shot 2021-02-15 at 21 44 57

@merkispavel merkispavel changed the base branch from get-graph-ids to master February 16, 2021 21:26
@merkispavel merkispavel dismissed gknisely’s stale review February 16, 2021 21:26

The base branch was changed.

@gknisely gknisely self-requested a review February 17, 2021 03:50
@merkispavel merkispavel merged commit 9c3f237 into master Feb 17, 2021
@merkispavel merkispavel deleted the look-at-restriction-modes branch March 9, 2021 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gknisely gknisely gknisely approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@merkispavel @gknisely