-
Notifications
You must be signed in to change notification settings - Fork 658
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
shortcuts behave different than base edges in transition costing #4672
Comments
I support this. I am not sure OSRMCarTurnDuration is "better" than how Valhalla computes transition costs. Do we have users who depend on this? We use Valhalla transition cost everywhere else - so why would we not use it when generating shortcuts? |
sounds like a mapbox thing. so I'd say no. @kevinkreiser suggested offline to fix our implementation and have only one, i.e. remove OSRMCarTurnDuration again. seems like that's better (less optimistic) at estimating true transition cost from turns/traffic light. |
yeah the reason it was added was because our turn cost implementation was too optimistic which leads to unrealistic ETAs. we certainly care about having realistic ETAs. it would be best if we could amend our turn costs to be similarly pessimistic. doign so properly will affect routes not just ETAs. this hack currently decouples the two so we can keep our routes sane but get worse ETAs. |
since #2651, we use
OSRMCarTurnDuration
to add transition cost for traffic light and turns. but we only add this to theCost::secs
in e.g.AutoCost::TransitionCost
while we add our "own" logic withstopimpact
toCost::cost
.the problem with that is that when we create shortcuts we can only take into account duration penalties for turn costs, but not the
stopimpact
logic. that is because we can't set an edge's cost directly, we do it via its speed and attributes.eventually this leads to different paths depending whether the expansion settled the base edges or the shortcut. I realized that during #4671 which had a lot of geometry differences where it turned out it was because a* expands less and hence uses less shortcuts compared to dijkstra.
it's just something to be aware of. the only thing we could do is remove
OSRMCarTurnDuration
again and use thestopimpact
logic for everything. it could be done to shortcuts then too and likely we'd see more consistent path finding across algorithms.The text was updated successfully, but these errors were encountered: