Use Strings to add CH profiles to GraphBuilder

[easbar]

This is a little spin-off from https://github.com/graphhopper/graphhopper/tree/remove_turn_weighting. Whats the issue here? In #1820 I want to move the turn weight calculation into AbstractWeighting instead of (as we do so far) wrapping Weightings into TurnWeighting. This requires passing the turn weight stuff into the weighting constructors (which is also a good thing: the turn cost configuration must be defined at the time the weighting is created as is the case when including the turn weighting in configurable weightings). However, this means to create a Weighting we need the TurnCostStorage already, which on the other hand is not available as long as there is no graph. The ugly part here is that to create a graph we already need the weightings to add CHGraphs, which we can see looking at the current usage of GraphBuilder:

Graph graph = new GraphBuilder(encodingManager)
  .setCHProfile(CHProfile.edge(weighting))
  .create();

With the turn cost stuff this becomes:

Graph graph = new GraphBuilder(encodingManager)
   .build(); // graph not created yet, we can still add CH profiles
Weighting w = new FastestWeighting(encoder, new DefaultTurnCostProvider(encoder, graph.getTurnCostStorage());
graph.addCHGraph(CHProfile.edge(w));
graph.create();

So I thought its useful to use a string representation to add the CH profiles and create the CH profiles in GraphBuilder#build() when the turn cost storage is available:

Graph graph = new GraphBuilder(encodingManager)
  .setCHProfileStrings("car|fastest|edge")
  .create();

Obviously this is limited to some hard-coded weightings in GraphBuilder, then again these are sufficient for the testing purposes (where GraphBuilder is mainly used) and of course one can always use the more verbose and flexible way above.

I don't know its a bit of a workaround because really I think there is no reason we cannot add CHGraphs after GraphHopperStorage has been created, but this seems like a bigger undertaking and not sure if its worth atm, especially since I think the string method above is nice and readable (just not type-safe).

Any thoughts?

[karussell]

The savings seems to be minimal especially as you then often have to call:

weighting = chGraph.getCHProfile().getWeighting();

Or is this already work in-sync with #493?

[karussell]

E.g. here.

[easbar]

Or is this already work in-sync with #493?

Kind of: When we use config.yml etc. we define the ch profiles as strings and then there is GraphHopper#initCHAlgoFactoryDecorator and GraphHopper#createWeighting which use this string information to configure the graph, create the CH profiles etc.. This is quite messy: Its way too hard to replicate what happens there in a testing environment. Here its a bit like this: Use the strings and obtain a graph. The string representation here is more concise than what we plan in #493.

The savings seems to be minimal

Hmm, I think this

.setCHProfileStrings("car|fastest|edge")
Weighting w = chGraph.getCHProfile().getWeighting();

is a lot better and shorter than this:

Weighting w = new FastestWeighting(encoder, new DefaultTurnCostProvider(encoder, graph.getTurnCostStorage());
graph.addCHGraph(CHProfile.edge(w));
graph.create(1000);

but you do not like it because of less type-safety?

[karussell]

I just have no strong opinion here. Feel free to merge it :)

The problem of type safety or refactoring safety is not too big as you already indicated that we mainly use this in tests :)

[easbar]

that we mainly use this in tests :)

Yes this is meant to be used mainly in tests. At some point we should extract the 'create a weighting and corresponding CH profiles etc.' part from GraphHopper so it can be reused and exchanged (without having to subclass GraphHopper).