Implemented forcing routes at start, via and end points into a certain direction

[jansoe]

still to do

• handle case of query graph request at tower nodes
• tests

[karussell]

Thanks!

The U-turn instruction should be made also for via points, but we should move this out of this issue: #289

Will comment inline...

[karussell]

the comment regarding the unit of preferredDirection should go here

[devemux86]

👍
Is this related to #225 ?

[jansoe]

Yes (but will not work with CH')

[karussell]

we need to find a better name (also preferred) or maybe setInferiorEdge? And the sout statements should be removed. And do we really need to make this low level method public?

[jansoe]

this method is required to disprefer the incoming edge at via points.
one could achieve this also by calculating the incoming azimuth, and then use the above method. But in my feeling this would be a quite indirect.
The other way would to directly modify the edge. But here the method assures that both incoming and outgoing virtual Edge are affected.

[jansoe]

regarding the wording: i liked the dichotomy of preferredDirection vs. dispreferredEdge, so shall we do now superiorDirectionand inferiorEdge ? Or what do you think of favoredDirection and unfavoredEdge?

[karussell]

this is good: 'favoredDirection and unfavoredEdge'

[karussell]

setDispreferedEdge(int edgeId, int adjNodeId)
this method is requiered to disprefer the incoming edge at via points.
one could achieve this also by calculating the incoming azimuth, and then use the above method.
But in my feeling this would be a quite indirect.

Hmmh, not sure what you mean here. I would prefer to have just two simple methods in QueryGraph: enforceDirection and dropDirectionEnforcement without any ifs before but probably with more required parameters. Even if we pass the endEdge if it is not necessary. BTW: why -2 in paths.get(placeIndex - 2).getFinalEdge()?

Also for me QueryGraph.setDispreferedEdge is misleading as it suggests one can set any edge but then throws exception for none-virtual edges.

Include instructions for UTurn at start

Why only at start? E.g. if costs are low u-turn can happen also elsewhere. But again: we should move this out of this issue and into: #289

[jansoe]

not sure what you mean here. I would prefer to have just two simple methods in QueryGraph: enforceDirection and dropDirectionEnforcement without any ifs before but probably with more required parameters

I mean, the two methods are applied in two different scenarios. In the one scenario I know only the direction I want to prefer, in the other scenario I have no clue which direction I want to prefer but already know the edge to disprefer. One could use the information in the second scenario, extract from the edge an direction, and then use this direction to feed it into the first scenario. But I dislike this, as it is unnecessary computational overhead.
Regarding your suggestion to use one method for both, its sounds like putting two things together which do not belong together (I exaggerate here a bit, but in the end it has to look similar like this)

favorDirectionOrUnfavorVirtualEdge(double favoredDirection, QueryResult queryPoint, boolean incoming, EdgeIteratorState unfavoredEdge)
{ if (unfavoredEdge == EdgeIteratorState.NO_EDGE)
{
favor direction
}
 else
{
unvafor edge
} 

To be more constructive, (1) in the GraphHopper Class I can remove a lot of ifs by doing all the validity checking within the QueryGraph and (2) a rename of setDispreferedEdge in setUnfavoredVirtualEdge should make the usage much more clear

This are my two cents, but if I still could not convince you, I will implement it like this, since you have the bigger picture in mind :)

[jansoe]

BTW: why -2 in paths.get(placeIndex - 2).getFinalEdge()

placeIndex is initialised by 1, that is for placeIndex==1 the first path is calculated and then stored in the list pathes at the first position (i.e. at 0 with a zero based index). This path is not affected by the requirement for straight via routing.
Then for the next path, i.e. placeIndex==2, we have to get the final edge of the previous path (which is the first path and stored at the 0th position in the pathes list). Therefore it is at position 2-2=0.

[jansoe]

Include instructions for UTurn at start

Why only at start?

Here my reasoning was, that u-turns at via points are old behaviour. But with this code u-turns also can occur at start, which did not existed before. Therefore I regarded it as part of this issue, whereas the other u-turns seemed not directly related to this code.

But now I removed also the start u-turns to issue #289

[jansoe]

open questions:

• _default in EdgeIteratorState.getBoolean()? -> kept!
• location of K_UNFAVORED_EDGE? -> kept in EdgeItaretorState!
• 2 methods queryGraph.favorDirection and queryGraph.unfavorEdge? -> similar method enforceDirection and enforceDirectionByEdgeId

[jansoe]

• fix bug: direction parameters do not do not influence result for route? endpoint
  bug was in test :)

[jansoe]

renamed direction parameter to heading

[karussell]

Here I slightly prefer number of over #xy :)

[karussell]

Looks now good to me!

[jansoe]

On my behalf, I'm done

[karussell]

Would you mind to squash once again :) and move MyTest into the normal (j)unit testing?

[jansoe]

and move MyTest into the normal (j)unit testing

uups, this should not have been comited , will remove it

[devemux86]

I'm wondering if it's worth it to have also elsewhere the angle in human friendly degrees (0, 360) e.g. like here

[karussell]

Hmmh, yes this should also be changed but probably in another issue: #424

@jansoe the problem is now that turn_angle is in radian and already included in the json :(

[karussell]

I would prefer consistent angles, and we can't revert turn_angle using degree without breaking several clients OR introducing two versions of the API for the switchover (see new #437). And as we already have an 'inconsistent state' (GPX export is using degree for gh:azimuth) I vote to keep using degree here and fix turn_angle via #437 later.

Please vote for or against this :) !

[devemux86]

I generally vote for using SI units in engines, though degrees (0-360) is the common way for angles.
Users can always convert the returned values later to their liking.

Of course you are right about existing implementation, an established API is hard to change.

[karussell]

thanks. and yes, I would not have a problem with breaking the Java API (maybe we should do this even earlier) because this is relative easy to detect and fix for consuming developers, but the JSON API can't be changed as easily (this is what we now feel ourself with running the GH Directions API ;))

[jansoe]

I vote for using degrees in this PR, and sometime, with other major changes, also swap radians in roundabouts.

[karussell]

Thanks a lot @jansoe ! Merged!

[karussell]

BTW: should we throw an UnsupportedException if someone wants to use it with CH?

[jansoe]

I'm undecided. On the one hand it will also work with CH in many cases, on the other hand, you sometimes get this artefacts ....

[devemux86]

Great work @jansoe for a valuable feature!

[karussell]

Yeah, me too. Still the problem is that CH is the default and people will report this as bug although this is 'known'. Maybe we throw an exception but allow via parameter in the GraphHopper class to make it still working?