Favor turn channels more

[merkispavel]

Issue

At the moment we boost turn channel speeds by some factor

valhalla/src/mjolnir/speed_assigner.h

Line 267 in 5ec6f65

speed = static_cast<uint32_t>((speed * kTurnChannelFactor) + 0.5f);

But we don't do anything with its speeds/costs when there are historic speeds.

PR changes:

• reduce kTurnChannel edge cost for both has-historic-speeds and doesn't-have-historic-speeds cases
• increase turn channel factor 1.25 -> 1.66. (Note: 1.66 is basically 1 / 0.6)

Screenshots

Before	After

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
• If you made changes to the lua files, update the taginfo too.

Requirements / Relations

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

[merkispavel]

Should be noticed that durations will be increased after removing this line so I'm hesitating if it's the right move..
Alternative safe solution might be

• leave speed_assigner code as it is
• handle only if (edge.has_historic_speeds) case in the AutoCost::EdgeCost

cons:

• Different turn channel factor in 2 places. It would be better to have everything in one place

[kevinkreiser]

thats a very good question you raise... here's my opinion:

in general, our speed assignments are always too optimistic. also turn channels are by definition short. i think its maybe its not a big deal to get 20% slower considering these two facts..

[merkispavel]

also turn channels are by definition short. i think its maybe its not a big deal to get 20% slower considering these two facts

agreed

[kevinkreiser]

looks like you need to update the isochrone tests since they change just slightly (i guess they get slightly smaller)

[merkispavel]

looks like you need to update the isochrone tests since they change just slightly (i guess they get slightly smaller)

thanks! didn't notice that ci had failed

yeah, it's just 3 points were changed a little bit( +- 1-e4). Fixed 👍

[kevinkreiser]

@merkispavel @genadz
one thing i wanted to ask about here was the inverse problem. assuming we are in a right-hand driving country, is it possible that we take a right hand turn channel to make a left turn:

here the route is red. i havent checked but i think this is possible. i would, like uturns, hope that we penalize sharp turns off of turn channels so this only happens if its the only possibility for whatever reason. anyway i was just wondering if you saw anything like that in RAD

[merkispavel]

@kevinkreiser
I didn't see such cases in RAD. The reasons might be

• there are not such cases in the RAD tests(like in your screenshot). So no one of them tests uturns
• Turn channel end road sometimes is one way like in the screenshot below.Even if it's not one way - there's a chance that uturn restriction is already added. Plus, as you said, maybe we already assign increased turn weight for such sharp turns.

I'll double check manually that this PR doesn't add such bugs but I bet I won't find them. Non-oneway end road + no uturn restriction .. the chances are pretty low

)

[kevinkreiser]

yep i agree the chances are low, i just wanted to point it out

[dnesbitt61]

the stop_impact is raised when making a sharp turn off of a turn channel - that likely will prevent this.