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

Removed destination_only when reclassifying ferries #1905

Merged
merged 8 commits into from
Aug 14, 2019
Merged

Conversation

kdiluca
Copy link
Member

@kdiluca kdiluca commented Aug 8, 2019

Issue

This PR fixes #1889

Tasklist

  • Add tests
  • Review - you must request approval to merge any PR to master
  • Add #fixes with the issue number that this PR addresses
  • Generally use squash merge to rebase and clean comments before merging
  • Update the changelog
  • Update relevant documentation

Requirements / Relations

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

if (w.destination_only() ||
(w.use() != baldr::Use::kOther &&
static_cast<int>(w.use()) > static_cast<int>(baldr::Use::kTurnChannel))) {
if (w.use() != baldr::Use::kOther &&
Copy link
Member

Choose a reason for hiding this comment

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

im wondering about this. @dnesbitt61 perhaps the correct fix is to, if we detected that while updating edges we werent able to get back to a high class road, we need to do another pass where we allow the use of destination only edges?

Copy link
Member

Choose a reason for hiding this comment

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

ok talked this over with @dnesbitt61. instead of allowing use of destination_only what we can do here is penalize it with like a 5-10 minute penalty in the cost below. this should allow us to most of the time avoid destination only expect when it is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

note that there is a break in the method once n exceeds 400. This may be too low if there is a penalty involved (may need some adjustment of this)

test/node_search.cc Outdated Show resolved Hide resolved
test/search.cc Outdated Show resolved Hide resolved
@kdiluca
Copy link
Member Author

kdiluca commented Aug 13, 2019

Nynähamn to Gotland:

Valhalla staging (before overriding destination_only flag & before penalty added)
Screenshot from 2019-08-13 16-21-17

Local valhalla (after overriding destination_only flag for ferries & adding penalty to costing)
Screenshot from 2019-08-13 16-23-26

kevinkreiser
kevinkreiser previously approved these changes Aug 14, 2019
@kdiluca kdiluca merged commit 0c82ab1 into master Aug 14, 2019
@purew purew deleted the ferry-fix branch February 13, 2020 19:21
nilsnolde added a commit that referenced this pull request Jun 13, 2023
…only while reclassifying if the connecting edge was dest_only AND the pred was destonly, but we still untag ANY destonly if it showed up in the final upclassified path; the main change is: also remove the connecting edge's destonly, which #1905 was missing; also fix a bug in forming the upclassified path where the initial label index could be wrong
kevinkreiser pushed a commit that referenced this pull request Jun 15, 2023
* implement a fix for destonly edges around ferries, including the ability to not un-tag destonly edges further away from ferries, i.e. where there was a break to destonly continuity

* changelog

* new changelog block..

* revert most of the previous changes: we only don't penalize any dest_only while reclassifying if the connecting edge was dest_only AND the pred was destonly, but we still untag ANY destonly if it showed up in the final upclassified path; the main change is: also remove the connecting edge's destonly, which #1905 was missing; also fix a bug in forming the upclassified path where the initial label index could be wrong
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When Upgrading Ferry Connections Drop Restrictive Tags
4 participants