Make sure node-based algos are not running with turn costs

[easbar]

Fixes #1965

[easbar]

Since disabling the turn costs is actually something we need to do sometimes I made this an actual parameter

[karussell]

Couldn't we use turn_costs=false in hints? I would like to avoid further parameters here.

[easbar]

But why not? We are creating weightings here and sometimes we need them without turn costs.. But if you really want to avoid the extra parameter, we can use a hint as before, I would just not like to call it turn_costs, because that had a different meaning before. So I would call it __disable_turn_costs, like it was called at the very beginning :)

[karussell]

But why not?

I see createWeighting as a more generic method and it is unclear why disabling turn costs should be so special.

Shouldn't the caller of createWeighting be able to create a Weighting and only use edge weights?

I would just not like to call it turn_costs, because that had a different meaning before

Isn't the meaning very similar in the sense that we ask the profile to ignore turn costs? Like a none-CH profile with turn_costs=true configured but turn_costs=false in the request hints?

[karussell]

Ok, one argument for the additional boolean is a minor higher type safety for users overriding the createWeighting method. The would not need to explicitly figure out what is wrong in the source code.

[easbar]

I see createWeighting as a more generic method and it is unclear why disabling turn costs should be so special.

Basically there are two ways to disable turn costs: The first one is via the profile, there are profiles with and without turn costs and depending on this the weighting will include turn costs or not. But then there are cases where we have to disable the turn costs although they are enabled in the profile, for example for Isochrones (because they are only implemented for node-based graph traversal so far) and for LM preparation (also not implemented edge-based and the preparation is even more useful when turn costs are excluded (can be used for queries with and without turn costs)). So we need a second flag to disable turn costs also for profiles that include turn costs. This second flag is what makes disabling the turn costs special. In principle it could also go into the hints parameter, but it is slightly different than the other hints in that it is only set internally vs. something the user can change via the request.

Shouldn't the caller of createWeighting be able to create a Weighting and only use edge weights?

Not sure if I understand the question, but yes thats totally possible with the disableTurnCosts flag.

Isn't the meaning very similar in the sense that we ask the profile to ignore turn costs? Like a none-CH profile with turn_costs=true configured but turn_costs=false in the request hints?

Yes its similar to that, but turn_costs existed as a request parameter before and now its an internal thing, so it seems a bit confusing to use the same name.

[karussell]

Shouldn't the caller of createWeighting be able to create a Weighting and only use edge weights?

Not sure if I understand the question, but yes thats totally possible with the disableTurnCosts flag.

I meant: createWeighting could be made independent of the use case. And then in the algorithm the node based traversal leads to skip calling calcTurnWeight without the need for this disableTurnCosts flag. I.e. turn cost supports needs then two things for the algorithm: a "proper created" Weighting and edge-based traversal.

[karussell]

I.e. turn cost supports needs then two things for the algorithm: a "proper created" Weighting and edge-based traversal.

I do not say this is wrong :) ... just that it might be slightly cleaner but likely also more complicated to do it at the algorithm level

[easbar]

No its not complicated at the algorithm level and I suggested this a few times (here and here) :) . The reason why I eventually decided against this was that this way when we pass a weighting that includes turn costs into an algorithm with node-based graph traversal the turn costs will be silently ignored. The way we are doing this now is more strict in this sense: If we choose node-based we also have to make sure the weighting does not include turn costs, otherwise we will be notified that what we are trying to do is not possible.

createWeighting could be made independent of the use case.

The createWeighting method does not really know what it is used for, it just has a switch that allows creating a weighting without turn costs even for profiles that have turn costs enabled

[karussell]

I eventually decided against this was that this way when we pass a weighting that includes turn costs into an algorithm with node-based graph traversal the turn costs will be silently ignored. The way we are doing this now is more strict in this sense:

Ah, thanks for the explanation.

[devemux86]

Does that mean that flexible mode now cannot use turn restrictions?

[easbar]

Does that mean that flexible mode now cannot use turn restrictions?

No, it just means that when e.g. using Dijkstra the traversal mode has to be edge-based to use turn restrictions. But that has not changed, its been the same before.

// Dijkstra running node-based without turn costs
new Dijkstra(graph, new FastestWeighting(encoder), NODE_BASED).calcPath(0, 3);
// Dijkstra running edge-based without turn costs
new Dijkstra(graph, new FastestWeighting(encoder), EDGE_BASED).calcPath(0, 3);
// Dijkstra running edge-based with turn costs
new Dijkstra(graph, new FastestWeighting(encoder, new DefaultTurnCostProvider(...)), EDGE_BASED).calcPath(0, 3);
// Dijkstra running node-based with turn costs -> this yields wrong results without this PR and gives an error with this PR
new Dijkstra(graph, new FastestWeighting(encoder, new DefaultTurnCostProvider(...)), NODE_BASED).calcPath(0, 3);