Adding TaxiCost

[AurelienLP]

Hello all !

Greeting from Navitia ! :)

We'd like to add a taxicost in Valhalla. To do so, we'll create a new class TaxiCost in sif/autocost.h and make it inherate from autocost. We'll follow the same model used for the bus and the HOV like this commit.

https://github.com/valhalla/valhalla/commit/7b73d59ccb7619b39ac07e2e99fcd97742936bca

What do you think about it ?

Thank you very much !

Aurelien

[kevinkreiser]

It seems reasonable to me! I'm assuming you'd actually like to make changes to this to differentiate it from HOVCost over time?

[AurelienLP]

Yes, the idea is to have a "taxi behaviour" as close as we could have from reality using the kTaxiAccess attribute.

[dnesbitt61]

One thing to watch out for is whether taxi access is properly set. I do not see any places in mjolnir directededgebuilder.cc where the taxi access is set. This could be an easy addition if this is the only place the setting is missed. @gknisely may have some thoughts here as well.

[gknisely]

Yes. Should be able to search for kHOVAccess and follow the patterns. Let me know if you have questions @AurelienLP

[AurelienLP]

Thank you very much guys! You are the best 😉

[AurelienLP]

Hello @gknisely !

I am editing graph.lua and especially filter_tags_generic now. To do so, I am following the same logic as the bus, since they (bus and taxi) are both psvs. From what I understand I have to create three keys, taxi_forward, taxi_backward and taxi_tag and set them according the value of access, psv, motor_vehicle and so on. Am I right ?

Thank you for your answer.

[gknisely]

Yes that is correct. Be careful with the oneway logic. https://github.com/valhalla/valhalla/blob/master/lua/graph.lua#L1039 Of course I will review the logic. Thanks for working on this task.

[AurelienLP]

I have pushed my branch here https://github.com/AurelienLP/valhalla/tree/add_taxi_cost After multiple iterations, the coutryaccess test fails with the message "[FAIL: Enhanced: Reverse access is not correct for way 139156014.]" I am guessing my kTaxiAccess is not properly set somewhere... Hoping you could help me @gknisely

Thank you !

[gknisely]

Basically the issue is that Taxi access needs to be added to kCountryAccess. https://github.com/valhalla/valhalla/blob/master/valhalla/mjolnir/adminconstants.h#L32 For each road in the vector, https://github.com/valhalla/valhalla/blob/master/valhalla/mjolnir/adminconstants.h#L15, we set what the country override access is for that particular country.

// Based on logic at http://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access-Restrictions // vector = trunk, trunk_link, track, footway, pedestrian, bridleway, cycleway, path, and motorroad //-1 indicates a null value will be set which means no change from default access. // 0 indicates no access.

So basically we need to look at http://wiki.openstreetmap.org/wiki/OSM_tags_for_routing/Access-Restrictions and determine where taxi access should be added. Then, we have to rebuild the Admin database as it contains these overrides.

If you want to give it a shot, please go for it. Otherwise, I will do it as soon as I can.

[AurelienLP]

I have indeed added kTaxiAccess in adminconstants.h everywhere kBusAccess was set (since they are both psvs) and run make check. The test fails after GraphEnhancer::Enhance but not after GraphBuilder::Build.

[gknisely]

@AurelienLP did you push up these changes? I am not seeing the test failures.

[AurelienLP]

Just created the PR. Need to reformat first !

[gknisely]

@AurelienLP to get the tests working, please do the following.

cd into build/test/data run spatialite netherlands_admin.sqlite run update admin_access set trunk = 1257, trunk_link = 1257;

This will update the test admin db for the tests. Let me know how it goes.

[AurelienLP]

Looks like it worked ! Thanks @gknisely ! :)

[AurelienLP]

Hello @gknisely !

My PR is updated and ready to be reviewed whenever you can. Seems like I can't remove the label WIP and request reviewers.

Thank you for your help !

Aurelien