Added option for country specific toll factor

[ADMUKHE]

Associate toll factor with specific country

This PR is targetting #4161. Presently valhalla cannot apply toll factors to specific countries. These changes will allow toll factor to be set in the algorithm depending on the value of a multiple costing option flag which can be set in the request.

The program was tested solely for our own use cases, which might differ otherwise. I acknowledge that these changes have the potential to impact other use cases, and it is essential to exercise caution and thorough testing when merging them to the master. Your expert opinion is requested to help us review, test, and adapt these modifications as needed. I am also looking forward to other implementation ideas and discussions about how we can tackle this issue.

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.

Adrika Mukherjee adrika.mukherjee@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information

Licensed under MIT

[nilsnolde]

Hm, this is honestly a bit far away from being mergeable. Skimming it quickly I don’t even understand how this is supposed to be used. It’d certainly help if you’d outline the expected request parameterization and explain the design choices.

Generally: we don’t modify costing stuff per algorithm. The costing class exists so all algorithms can share the costing logic and it’s one of the major principles of Valhalla.

But before you modify smth, I’d appreciate a more thorough explanation.

[ADMUKHE]

Hi Nils

Thank you for your feedback. I'd like to outline our use case and our efforts to implement it with backward compatibility.

A sample route request would look like:

{
    "locations": [
        {
            "lat": 48.756988213085634,  
            "lon": 9.192367381240384
           
        },
        {
            "lat": 48.755804, 
            "lon": 9.191274
        }
    ],
    "costing": "auto",
    "costing_options":{"auto": {"use_tolls": 1, "use_highways": 0.2, "cost_per_country": [{"use_tolls":0.5, "country_code":["DE", "CH"]}, {"use_tolls":0.2, "country_code":["FR"]}]}},
    "directions_options": {
        "units": "kilometers"
    }
}

'country_code' is introduced in Costing-Options and a new 'cost_per_country' message is added which can take multiple Options. The "costing_options" can parse a request like the following:

 "costing_options":{"auto": {"use_tolls": 1, "use_highways": 0.2, "cost_per_country": [{"use_tolls":0.4, "country_code":["DE", "CH"]}, {"use_tolls":0.2, "country_code":["FR"]}]}}

Inside "cost_per_country" list, individual avoid options can be bound to country_code list. In short, "use_tolls" = 0.4 is applied for DE and CH, "use_tolls" = 0.2 is applied for FR and "use_tolls" = 1 is applied for any other country.

Dynamiccost is modified to introduce 4 new functions, these methods are overriden in the autocost.
In Autocost the multi cost options are parsed, default_toll_factor is set depending on the "use_tolls" value specified and a map is generated with country and toll_factor as key value pair if "cost_per_country" is present.

Finally in the algorithm(bidir astar) the toll_factor_per_country map is pulled and the toll_factor_is set dynamically depending on the admin iso code of the end node before calling the EdgeCost function on the costing_.

We thought of having a country_based costing wrapper but costing_ is set depending on the mode in valhalla and the wrapper class does not fit. Or what do you think?

I understand this might not be a good way to implement the whole thing. It would be helpful if you could give your feedback and outline the drawbacks of the approach. We can improve based on your comments.

[nilsnolde]

(talking purely for myself, not the project)

This is a bigger implementation with quite a (positive) impact. It seems backwards-compatible just fine, however the implementation is itself is, as you suggest, not really inline with our general architecture IMO. To get this into master would take more effort than I'm willing to accept pro-bono. If you need assistance to make this mergeable, I'm available on enquiry@gis-ops.com. I'm not the only maintainer of course, you might have more luck with others.

[FScholPer]

I'm a software developer colleague of @ADMUKHE and @johannes-no . @kevinkreiser can you give us some information how we can change the implementation to get it mergeable and fitting to the general architecture? We would also be interested in a short meeting if possible like you had it with our colleague @PhilippVoigt which left the company.

Thank you and regards

Frank Scholter Peres frank.scholter_peres@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information

[kevinkreiser]

many apologies @ADMUKHE @johannes-no and @FScholPer im not dragging my feet on this on purpose its all just a matter of finding the time on my end. the original issue that is linked pretty much has all the information but i realize that some of the details might be better discussed in a meeting for sake of clarity and time. i dont have very good availability this week however next week should be fine. i'll send you guys an email to set something up

[kevinkreiser]

@ADMUKHE @johannes-no @FScholPer i havent heard back from you, let me know if you would like to meet to discuss this. thanks again!

[kevinkreiser]

i owe you a second pass on the code with more specitic pointers and suggestions. i'll try to get that in the next few days. make a comment here if i dont get to it soon enough sometimes i need a "bump" 😄

[ADMUKHE]

@kevinkreiser Hey, have you had a chance to look into this?

[ADMUKHE]

@kevinkreiser I havent heard back from you. Should we schedule another F2F to discuss the architektural changes?

[kevinkreiser]

yeah i started typing something and never finished it. i have 3 pieces to propose one of those we already discussed. they are

• we should make an admin cache object, similar to time_info_t, which algorithms can use to track the current admin name as the algorithm expands a given search path
  • the idea here would be to pump in a node and its tile to its operator() to get back an admin string
  • internally it would be able to use the tileid and the nodes admin index to see if it already grabbed the admin string
  • otherwise it would have to make a new entry in its cache
  • i can sketch up this caching object for you if you like let me know
• we should update the signature of TransitionCost to take an admin and pred_admin argument and the EdgeCost signature to take a single admin argument
  • this is going to be a huge pain butt the only way to accurately support the things we want to do
  • we could do this in pieces, maybe start with just edge cost since it only takes the current admin
  • maybe we could default the parameters to just "" so that we dont have to update all the algorithms
  • i would kind of advocate for biting the bullet though and just enforce it everywhere
• we should modify the costing factor to allow for specifying admin specific costing in such a way that things are organized in a map with admins as keys and costing options as values
  • the idea here is that we'd like to be able to reuse our costing options parsing as much as possible
  • your api change suggestion is not bad in that it keeps the request as small as possible however the json then doesnt match the protobuf which is frustrating
  • it would be awesome if we could find a way to just have all sets of options bet at the top level like "costing_options":["auto":{},"auto_de":{},"auto_ch":{},"auto_at":{}]
  • or maybe it should be on a separate list for the ease of parsing: "costing_options":["auto":{}],"admin_costing_option_overrides":["auto_de":{},"auto_ch":{},"auto_at":{}]
  • i'm not sure what the answer is here but i think its better if the proto and the json look similar and i like the idea that we parse the regular auto one and then we duplicate it and override just the parts that each country costing option overrides (ie we can pass the copy of auto costing options to the parsing function and it can override just those it finds)
  • the devil will be in the details and im sure im forgetting a lot so its possible you already tried all this and there is some fatally annoying design flaw here so just let me know 😄

[ADMUKHE]

@kevinkreiser It would be very helpful if you can sketch this cache object for us. Especially its tricky as where we should initialize it?