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

Evaluate restrictions with dow only #3039

Merged
merged 2 commits into from
Apr 29, 2021
Merged

Evaluate restrictions with dow only #3039

merged 2 commits into from
Apr 29, 2021

Conversation

CuriOusQuOkka
Copy link
Contributor

@CuriOusQuOkka CuriOusQuOkka commented Apr 27, 2021 •
edited
Loading

Issue

Noticed, that restrictions with dow only, like Monday-Friday were not applied after routing to the edges. They are not applied to the edges when validating the routing path.

In the example below, if I set conditional restriction no_entry @ Mo-Fr for AB->BC left turn
the restriction is ignored, at any time we receive AB->BC->CF path from A to F instead of AB->BD->DE->EC->CF for workdays.

      D----B----A
      |    |
      |    |
      E----C----F

Tasklist

  • Add tests
  • Add gurka tests with day_on/day_off and no hour info #3029
  • Update the changelog

@CuriOusQuOkka CuriOusQuOkka force-pushed the dow_range_fix branch 2 times, most recently from 8732240 to cb5b1aa Compare April 27, 2021 11:26
@CuriOusQuOkka CuriOusQuOkka marked this pull request as ready for review April 27, 2021 11:27
@CuriOusQuOkka CuriOusQuOkka requested a review from gknisely April 27, 2021 11:28
@gknisely
Copy link
Member

@CuriOusQuOkka I think we will be good after we test with https://github.com/valhalla/valhalla/blob/master/test_requests/timed_access_routes.txt and https://github.com/valhalla/valhalla/blob/master/test_requests/timed_conditional_routes.txt Let me know if there are any diffs.

gknisely
gknisely previously approved these changes Apr 27, 2021
View reviewed changes
@CuriOusQuOkka CuriOusQuOkka requested a review from genadz April 28, 2021 09:50
@CuriOusQuOkka
Copy link
Contributor Author

@CuriOusQuOkka I think we will be good after we test with https://github.com/valhalla/valhalla/blob/master/test_requests/timed_access_routes.txt and https://github.com/valhalla/valhalla/blob/master/test_requests/timed_conditional_routes.txt Let me know if there are any diffs.

the tests passed.

@genadz
Copy link
Contributor

genadz commented Apr 29, 2021

what about gurka test ? I expected to see the example you described in the ticket in gurka tests

@CuriOusQuOkka
Copy link
Contributor Author

CuriOusQuOkka commented Apr 29, 2021 •
edited
Loading

what about gurka test ? I expected to see the example you described in the ticket in gurka tests

@genadz Added.

@CuriOusQuOkka CuriOusQuOkka merged commit 0ed7446 into valhalla:master Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@genadz genadz genadz approved these changes

@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.
3 participants
@CuriOusQuOkka @gknisely @genadz