[GraphHopper] Understanding GraphHopper Instructions
Peter
graphhopper at gmx.de
Sun Jun 29 13:22:13 UTC 2014
Hey Matthias,
yes, the instructions could be better as outlined in #185 and #94.
Before digging in your code: what change do you propose? Creating an
instruction after every edge?
Regards,
Peter.
> I total I have adjusted 3 classes in the core - before I am going to
> start a "pull" request I would ask for your opinion - if you would
> consider to add this modification - OR if there are good reasons why
> you implemented GraphHopper in the way you just did ?!
>
>
> Normally I would use a static constant for the prefix '@@@_' - but
> since I am not familiar
> with the code yet, I did not really knew where would be an appropriate
> place.
>
>
> 1) Ensure that each edge will have a unique name [when will be parsed
> from the OSM-Data]
> the 'genFakeUUID()' is just as example - it's very simple and you
> might not like that
> kind of code in the graphhopper sources - but I am lazy too :-)
>
> file com/graphhopper/routing/uti/EncodingManager
>
> ORIGINAL:
> public void applyWayTags( OSMWay way, EdgeIteratorState edge )
> {
> // storing the road name does not yet depend on the
> flagEncoder so manage it directly
> if (enableInstructions)
> {
> // String wayInfo = carFlagEncoder.getWayInfo(way);
> // http://wiki.openstreetmap.org/wiki/Key:name
> String name = fixWayName(way.getTag("name"));
> // http://wiki.openstreetmap.org/wiki/Key:ref
> String refName = fixWayName(way.getTag("ref"));
> if (!Helper.isEmpty(refName))
> {
> if (Helper.isEmpty(name))
> name = refName;
> else
> name += ", " + refName;
> }
> edge.setName(name);
> }
>
> for (AbstractFlagEncoder encoder : edgeEncoders)
> {
> encoder.applyWayTags(way, edge);
> }
> }
>
> ADJUSTMENT:
> public void applyWayTags( OSMWay way, EdgeIteratorState edge )
> {
> // storing the road name does not yet depend on the
> flagEncoder so manage it directly
> if (enableInstructions)
> {
> // String wayInfo = carFlagEncoder.getWayInfo(way);
> // http://wiki.openstreetmap.org/wiki/Key:name
> String name = fixWayName(way.getTag("name"));
> // http://wiki.openstreetmap.org/wiki/Key:ref
> String refName = fixWayName(way.getTag("ref"));
> if (!Helper.isEmpty(refName))
> {
> if (Helper.isEmpty(name))
> name = refName;
> else
> name += ", " + refName;
> }
> if(Helper.isEmpty(name)){
> name = "@@@_"+genFakeUUID();
> }
> edge.setName(name);
> }
>
> for (AbstractFlagEncoder encoder : edgeEncoders)
> {
> encoder.applyWayTags(way, edge);
> }
> }
>
> private String lastUsedId = "";
> private String genFakeUUID(){
> // for sure this UUID generator might be "overkill" cause of
> // performance issues
> // return java.util.UUID.randomUUID().toString();
>
> // this is for sure not ThreadSafe!
> String ret = Long.toString(System.currentTimeMillis());
> if(ret.equals(lastUsedId)){
> ret = lastUsedId+"X";
> }
> lastUsedId = ret;
> return ret;
> }
>
>
> 2) Simple adjustment in order to ensure, that our "fake names" will be
> never displayed
> in the instruction list
>
> file: com/graphhopper/util/Instruction.java
>
> ORIGINAL:
> public Instruction( int sign, String name, InstructionAnnotation
> ia, PointList pl )
> {
> this.sign = sign;
> this.name <http://this.name> = name;
> this.points = pl;
> this.annotation = ia;
> }
>
> ADJUSTMENT:
> public Instruction( int sign, String name, InstructionAnnotation
> ia, PointList pl )
> {
> this.sign = sign;
> if(!name.startsWith("@@@_")){
> this.name <http://this.name> = name;
> }else{
> this.name <http://this.name> = "";
> }
> this.points = pl;
> this.annotation = ia;
> }
>
>
>
> 3) the final adjustment in the Instruction generation process - since
> the data is already
> adjusted, that each edge will have a name, the instructions are more
> or less fine. The
> only reason why I found it necessary to modify the list generation is,
> that I wanted to
> get rid of the 'Straight ahead' instructions when traveling from one
> 'fakedName' section
> to another... -> as long as it's straight ahead, there is IMHO no need
> for an instruction.
> Please note, that I have moved the creation of the points object into the
> if(!(name.startsWith("@@@_") ... section
>
> file: com/graphhopper/routing/Path.java
>
> ORIGINAL:
> public InstructionList calcInstructions( final Translation tr )
> {
> ...
> forEveryEdge(new EdgeVisitor()
> {
> ...
> @Override
> public void next( EdgeIteratorState edge, int index )
> {
> ...
> double orientation = ac.calcOrientation(prevLat,
> prevLon, latitude, longitude);
> if (name == null)
> {
> ...
> } else
> {
> double tmpOrientation =
> ac.alignOrientation(prevOrientation, orientation);
> String tmpName = edge.getName();
> InstructionAnnotation tmpAnnotation =
> encoder.getAnnotation(flags, tr);
> if ((!name.equals(tmpName))
> || (!annotation.equals(tmpAnnotation)))
> {
> points = new PointList(10, nodeAccess.is3D());
> name = tmpName;
> annotation = tmpAnnotation;
>
> ...
>
> prevInstruction = new Instruction(sign,
> name, annotation, points);
> cachedWays.add(prevInstruction);
> }
>
> updatePointsAndInstruction(edge, wayGeo);
> }
> ...
> }
> ...
> });
>
> return cachedWays;
> }
>
>
> ADJUSTMENT:
> public InstructionList calcInstructions( final Translation tr )
> {
> ...
> forEveryEdge(new EdgeVisitor()
> {
> ...
> @Override
> public void next( EdgeIteratorState edge, int index )
> {
> ...
> double orientation = ac.calcOrientation(prevLat,
> prevLon, latitude, longitude);
> if (name == null)
> {
> ...
> } else
> {
> double tmpOrientation =
> ac.alignOrientation(prevOrientation, orientation);
> String tmpName = edge.getName();
> InstructionAnnotation tmpAnnotation =
> encoder.getAnnotation(flags, tr);
> if ((!name.equals(tmpName))
> || (!annotation.equals(tmpAnnotation)))
> {
> name = tmpName;
> annotation = tmpAnnotation;
>
> ...
>
> // do not add the instruction, if we are
> traveling from one
> // 'no-name' path to another 'no-name' path
> and it's just
> // straight ahead (no turn required - it might
> make sense
> // to make this configurable?!)
> if(!(name.startsWith("@@@_") &&
> sign==Instruction.CONTINUE_ON_STREET) ){
> points = new PointList(10,
> nodeAccess.is3D());
> prevInstruction = new Instruction(sign,
> name, annotation, points);
> cachedWays.add(prevInstruction);
> }
> }
> updatePointsAndInstruction(edge, wayGeo);
> }
> ...
> }
> ...
> });
>
> return cachedWays;
> }
>
>
> 2014-06-28 19:22 GMT+02:00 Matthias Marquardt <marquardt24 at gmail.com
> <mailto:marquardt24 at gmail.com>>:
>
> Peter - would you mind, to review my suggestion for an adjustment
> in the Instruction calculation? - With this changes I was able to
> create the expected TurnByTurn Instructions I am looking forward
> to receive from GraphHopper once I am offroad
>
>
> 2014-06-28 16:20 GMT+02:00 Matthias Marquardt
> <marquardt24 at gmail.com <mailto:marquardt24 at gmail.com>>:
>
> ... I think I have already found it :-)
>
> com.graphhopper.routing.util.EncodingManager.applyWayTags(...)
>
> right?!
>
>
> 2014-06-28 16:04 GMT+02:00 Matthias Marquardt
> <marquardt24 at gmail.com <mailto:marquardt24 at gmail.com>>:
>
> Hello,
>
> my name is Matthias - 43 year old guy from the Bielefeld
> area. I want to make use of your offer 'do not hesitate to
> ask questions' - I am at the begin of a none commercial
> outdoor project - before I am going into details [what I
> plan to do, and why I expect that GraphHopper is just the
> 'right' thing (beside the fact that's pure Java and I am
> lazy too!)] - I am currently focusing on the instruction
> list functionality.
>
> At the beginning I had difficulties to understand why they
> are, like they are - when it comes to short trails like
> this one:
>
> http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274045%2C8.646782&vehicle=bike2&elevation=true
>
> I am missing at least one (IMHO essential) turn in the
> resulting instruction list (in the last section of the
> path (shortly after leaving the woods) - without knowing
> too much details about the used OSD from that specific
> area I am even more confused when I move the endpoint to
> the other path...
>
> http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274421%2C8.648252&vehicle=bike2&elevation=true
>
> Still only two turns - and the (IMHO essential)
> instruction is missing - actually my "expectation" would
> be, that I would get four (or even five) turning
> instructions for this small route.
>
> Lucky enough the GraphHopper sources are available, so I
> made my way though the calls (while having the code
> running in my IDE debugger) and realized the root of my
> "issue" is in the com.graphhopper.routing.Path class - to
> be more precise in the lines 416, 417 and 418 (in the
> 'forEveryEdge' code)
>
> if ((!name.equals(tmpName))
> ||
> (!annotation.equals(tmpAnnotation)))
> {
>
> While the graph in the given example has six edges the
> resulting list of instructions will be so short simply
> cause the edge objects does not have any names AND no
> essential differences in their annotations - this even
> explains instantly, why when switching over from bike to
> foot, that I get zero turning instructions at all, since
> the foot encoder does not not make any differences in the
> annotations...
>
> So with other words (moving away from the java object
> world back to the real world) - the current GraphHopper
> version give you only instructions, IF there is a
> significant change between the edges [like street name
> change, or change from tarmac to gravel] - I do not doubt,
> that the chosen implementation is the right way by default
> - a quick test (removing the name & annotation check)
> showed me that with my hacked code, the instruction list
> result is finally matching "my expectation".
>
> My question right now is, since I have realized, that the
> extracted data is IMHO the root cause of the issue, in
> which area of the code I have to dig for the portion, that
> is responsible for creating the base data from which later
> the edges are created - since my next attempt would be to
> give every path that will be extracted from the OSM data a
> unique name (if not present) - or would that be a silly
> idea?! [I think I will add a prefix, so that when the name
> later will be used, it can be detected, that this is just
> a "internal name" - and that instructions like 'turn left
> to MyUUIDPrefix_243211' could be avoided']
>
> TIA - Matthias
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/graphhopper/attachments/20140629/f230f079/attachment.html>
More information about the GraphHopper
mailing list