Bug fix TransitionCostReverse call: pass right internal_turn argument #3271
Conversation
| @@ -311,7 +307,7 @@ inline bool UnidirectionalAStar<expansion_direction, FORWARD>::ExpandInner( | |||
| (pred.not_thru_pruning() || !meta.edge->not_thru()), | |||
| (pred.closure_pruning() || !(costing_->IsClosed(meta.edge, tile))), | |||
| static_cast<bool>(flow_sources & kDefaultFlowMask), | |||
| costing_->TurnType(meta.edge->localedgeidx(), nodeinfo, meta.edge), | |||
| costing_->TurnType(pred.opp_local_idx(), nodeinfo, meta.edge), | |||
There was a problem hiding this comment.
It isn't related to the main purpose of the PR but I found it while writing a unit test
| : costing_->TransitionCostReverse(meta.edge->localedgeidx(), nodeinfo, opp_edge, | ||
| opp_pred_edge, | ||
| static_cast<bool>(flow_sources & kDefaultFlowMask), | ||
| pred.internal_turn()); |
There was a problem hiding this comment.
The key is this line
valhalla/valhalla/sif/dynamiccost.h
Line 614 in c9ddd4b
We need to check whether the previous maneuver is internal turn and the current one is a turn. And the "previousness" is the same for both forward and reverse searches
There was a problem hiding this comment.
haha i looked at this change in the previous pr several times and i wasnt a fan of the temporary internal_turn variable, im glad to see it go
There was a problem hiding this comment.
i'm feeling the same 😄
| forward.trip().routes(0).legs(0).node().Get(node_count - 1).cost().elapsed_cost(); | ||
| auto reverse_cost = | ||
| reverse.trip().routes(0).legs(0).node().Get(node_count - 1).cost().elapsed_cost(); | ||
| // if assert is triggered - check if uturn on internal edges is detected correctly |
There was a problem hiding this comment.
I mean the "expect_near" check below
There was a problem hiding this comment.
i dont suppose these changes fix the TODO below? still not?
There was a problem hiding this comment.
right 😞
I should mention that this delta is pretty small(~ 1e-4..1e-5) so it's not so critical(but annoying). EXPECT_NEAR with a small threshold(e.g 0.1) is the best workaround until it's fixed
There was a problem hiding this comment.
ah interesting. i feel like before it was a larger epsilon. perhaps your fixes are fixing it but since we are recosting in the forward direction, and its single precision float, the chagne in order of operations is enough to have such a small deviation? do you mind enabling them and adding the delta with the comment explaining that they are super close and some of our hypotheses?
There was a problem hiding this comment.
yea, no problem. Done
|
Unwanted divebombs from #3260 are gone 🎉 |
|
@merkispavel you cant argue with those images |
Issue
internal_turnparameter changes of Fix TransitionReverseCost usage #3260Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?