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

Fix signature to re-enable transition cost for motorcycle #2837

Merged
merged 4 commits into from
Feb 4, 2021

Conversation

SvetlanaBayarovich
Copy link
Contributor

@SvetlanaBayarovich SvetlanaBayarovich commented Feb 4, 2021

Issue

Method signature got inconsistent with the one of the base class. As a result, zero transition cost was applied for motorcycle profile.

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?

@SvetlanaBayarovich SvetlanaBayarovich self-assigned this Feb 4, 2021
danpat
danpat previously approved these changes Feb 4, 2021
@gknisely gknisely self-requested a review February 4, 2021 18:00
gknisely
gknisely previously approved these changes Feb 4, 2021
mandeepsandhu
mandeepsandhu previously approved these changes Feb 4, 2021
Copy link
Contributor

@mandeepsandhu mandeepsandhu left a comment

Choose a reason for hiding this comment

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

1 minor comment. Rest looks good 👍

@@ -242,8 +241,7 @@ class MotorcycleCost : public DynamicCost {
virtual Cost TransitionCostReverse(const uint32_t idx,
const baldr::NodeInfo* node,
const baldr::DirectedEdge* pred,
const baldr::DirectedEdge* edge,
const bool has_traffic) const;
const baldr::DirectedEdge* edge) const override;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove comment for has_traffic from both function's doc comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! 👍

@@ -10,6 +10,7 @@
* FIXED: Mark the restriction start and end as via ways to fix IsBridgingEdge function in Bidirectional Astar [#2796](https://github.com/valhalla/valhalla/pull/2796)
* FIXED: Dont add predictive traffic to the tile if it's empty [#2826](https://github.com/valhalla/valhalla/pull/2826)
* FIXED: Fix logic bidirectional astar to avoid double u-turns and extra detours [#2802](https://github.com/valhalla/valhalla/pull/2802)
* FIXED: Re-enable transition cost for motorcycle profile [#2837](https://github.com/valhalla/valhalla/pull/2837)
Copy link
Member

@kevinkreiser kevinkreiser Feb 4, 2021

Choose a reason for hiding this comment

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

s/profile/costing 😄

@SvetlanaBayarovich SvetlanaBayarovich merged commit 64c7c75 into master Feb 4, 2021
@SvetlanaBayarovich SvetlanaBayarovich deleted the bsv-fix-motorcycle-cost branch February 4, 2021 21:52
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.

5 participants