dont build shortcuts across access_restrictions

[kevinkreiser]

fixes #4401
fixes #4257

this is a quick check to see if we can fix #4025 based on a hypothesis that we are building shortcuts across truck restrictions and then the bidirectional algorithm is getting screwed up with one search tree using the shortcut and one tree not

[kevinkreiser]

seems to have a runtime error during CI so looks like my code is borked. i'll check it when i have time

[nilsnolde]

In case you won't find the time for it in the next weeks, I'd take this over, after fixing some more matrix stuff. @radekvermirovsky and their company are actually a client. They've been funding a lot of my recent efforts and this is big no-go for compliant truck routing.

I'll try to come up with a strategy along your idea to add mode-specific shortcuts @kevinkreiser.

[kevinkreiser]

i was going to do it yesterday but wanted to finish my chore #4392 first. i will at least get this compiling in the next couple days

[kevinkreiser]

I'll try to come up with a strategy along your idea to add mode-specific shortcuts

that sounds good. i was hoping to finish this simple idea in this pr and then yeah let you do the more complicated shortcut building that accounts for different modes as i suggested (as that will be quite a bit more time consuming) in a separate pr. is that what you are thinking?

[nilsnolde]

Jep let’s do that.

[nilsnolde]

Just to give a public update on our thoughts: the planet is currently building with this PR and we'll see how the performance is impacted. If it's not much (1-2%?), we might be fine leaving it as-is.

Personally I'd like to see that we don't break up shortcuts on most access restrictions and rather assign the most restrictive value of all base edges to the entire shortcut. It wouldn't be a lot of code changes, basically tracking those restrictions similar to total edge length etc, and it would restore those 1-2% or whatever almost fully for anything else than truck, and even truck would be less impacted.

[nilsnolde]

Did some benchmarking on this on the 2 largest test files we have, with a running server and each file 4 complete runs for each branch, both server and client with 8 threads each:

test_requests/de_benchmark_routes.txt:
master: 132 secs
branch: 131 secs

test_requests/random_routes.txt:
master: 658 secs
branch: 659 secs

So it's fair to say, performance doesn't seem to be impacted at all 🎉

[kevinkreiser]

really have to fix that tar index thing on osx

[kevinkreiser]

I'm good to merge this if you are @nilsnolde

[nilsnolde]

really have to fix that tar index thing on osx

it's on arm too. not ever seen it on x86 linux though!

[nilsnolde]

amended the test so it'll at least once get to the "return false" to prove we're breaking here too

[nilsnolde]

also that we're building shortcuts over consecutive edges with the same restriction. could've done many more permutations, but this should be ok

[nilsnolde]

do another graph without those restrictions and test that we're only creating 1 shortcut now

[nilsnolde]

this is done now @kevinkreiser . I added a bit more testing, let me know if it all makes sense to you. I started another planet build and do one more perf test.

[kevinkreiser]

yep its what i expected, good touch with the checking that matching access restrictions allow shortcuts to be built

[kevinkreiser]

feel free to put a sh ip on it again so we can merge once the testing shows basically no difference 😄

[nilsnolde]

Naa, thinking again, the last perf test was done on code which should produce more expansion where only the value was checked. That must've broken up shortcuts more than now. So if anything it got even less impacted. I'll merge :)

[nilsnolde]

Why is codecov doing that? It's a 100% tested and still it reports the whole coverage is decreased.. sometimes I can understand if patch coverage is close to current project coverage and we remove quite a bit of code which might've been 100% tested, the overall coverage decreases, makes sense. But in this case it's clearly bullshit..