<div dir="ltr">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 ?!<br>
<br><br>Normally I would use a static constant for the prefix '@@@_' - but since I am not familiar<br>with the code yet, I did not really knew where would be an appropriate place.<br><br><br>1) Ensure that each edge will have a unique name [when will be parsed from the OSM-Data]<br>
the 'genFakeUUID()' is just as example - it's very simple and you might not like that<br>kind of code in the graphhopper sources - but I am lazy too :-)<br><br>file com/graphhopper/routing/uti/EncodingManager<br>
<br>ORIGINAL:<br> public void applyWayTags( OSMWay way, EdgeIteratorState edge )<br> {<br> // storing the road name does not yet depend on the flagEncoder so manage it directly<br> if (enableInstructions)<br>
{<br> // String wayInfo = carFlagEncoder.getWayInfo(way);<br> // <a href="http://wiki.openstreetmap.org/wiki/Key:name">http://wiki.openstreetmap.org/wiki/Key:name</a><br> String name = fixWayName(way.getTag("name"));<br>
// <a href="http://wiki.openstreetmap.org/wiki/Key:ref">http://wiki.openstreetmap.org/wiki/Key:ref</a><br> String refName = fixWayName(way.getTag("ref"));<br> if (!Helper.isEmpty(refName))<br>
{<br> if (Helper.isEmpty(name))<br> name = refName;<br> else<br> name += ", " + refName;<br> }<br> edge.setName(name);<br>
}<br><br> for (AbstractFlagEncoder encoder : edgeEncoders)<br> {<br> encoder.applyWayTags(way, edge);<br> }<br> }<br><br>ADJUSTMENT:<br> public void applyWayTags( OSMWay way, EdgeIteratorState edge )<br>
{<br> // storing the road name does not yet depend on the flagEncoder so manage it directly<br> if (enableInstructions)<br> {<br> // String wayInfo = carFlagEncoder.getWayInfo(way);<br>
// <a href="http://wiki.openstreetmap.org/wiki/Key:name">http://wiki.openstreetmap.org/wiki/Key:name</a><br> String name = fixWayName(way.getTag("name"));<br> // <a href="http://wiki.openstreetmap.org/wiki/Key:ref">http://wiki.openstreetmap.org/wiki/Key:ref</a><br>
String refName = fixWayName(way.getTag("ref"));<br> if (!Helper.isEmpty(refName))<br> {<br> if (Helper.isEmpty(name))<br> name = refName;<br> else<br>
name += ", " + refName;<br> }<br> if(Helper.isEmpty(name)){<br> name = "@@@_"+genFakeUUID();<br> }<br> edge.setName(name);<br>
}<br><br> for (AbstractFlagEncoder encoder : edgeEncoders)<br> {<br> encoder.applyWayTags(way, edge);<br> }<br> }<br> <br> private String lastUsedId = ""; <br> private String genFakeUUID(){<br>
// for sure this UUID generator might be "overkill" cause of<br> // performance issues<br> // return java.util.UUID.randomUUID().toString();<br> <br> // this is for sure not ThreadSafe!<br>
String ret = Long.toString(System.currentTimeMillis());<br> if(ret.equals(lastUsedId)){<br> ret = lastUsedId+"X";<br> }<br> lastUsedId = ret;<br> return ret;<br> }<br>
<br><br>2) Simple adjustment in order to ensure, that our "fake names" will be never displayed<br>in the instruction list<br><br>file: com/graphhopper/util/Instruction.java<br><br>ORIGINAL:<br> public Instruction( int sign, String name, InstructionAnnotation ia, PointList pl )<br>
{<br> this.sign = sign;<br> <a href="http://this.name">this.name</a> = name;<br> this.points = pl;<br> this.annotation = ia;<br> }<br><br>ADJUSTMENT:<br> public Instruction( int sign, String name, InstructionAnnotation ia, PointList pl )<br>
{<br> this.sign = sign;<br> if(!name.startsWith("@@@_")){<br> <a href="http://this.name">this.name</a> = name;<br> }else{<br> <a href="http://this.name">this.name</a> = "";<br>
}<br> this.points = pl;<br> this.annotation = ia;<br> }<br><br><br><br>3) the final adjustment in the Instruction generation process - since the data is already<br>adjusted, that each edge will have a name, the instructions are more or less fine. The<br>
only reason why I found it necessary to modify the list generation is, that I wanted to<br>get rid of the 'Straight ahead' instructions when traveling from one 'fakedName' section<br>to another... -> as long as it's straight ahead, there is IMHO no need for an instruction.<br>
Please note, that I have moved the creation of the points object into the <br>if(!(name.startsWith("@@@_") ... section<br><br>file: com/graphhopper/routing/Path.java<br><br>ORIGINAL:<br> public InstructionList calcInstructions( final Translation tr )<br>
{<br> ...<br> forEveryEdge(new EdgeVisitor()<br> {<br> ...<br> @Override<br> public void next( EdgeIteratorState edge, int index )<br> {<br> ...<br>
double orientation = ac.calcOrientation(prevLat, prevLon, latitude, longitude);<br> if (name == null)<br> {<br> ...<br> } else<br> {<br>
double tmpOrientation = ac.alignOrientation(prevOrientation, orientation);<br> String tmpName = edge.getName();<br> InstructionAnnotation tmpAnnotation = encoder.getAnnotation(flags, tr);<br>
if ((!name.equals(tmpName))<br> || (!annotation.equals(tmpAnnotation)))<br> {<br> points = new PointList(10, nodeAccess.is3D());<br>
name = tmpName;<br> annotation = tmpAnnotation;<br> <br> ...<br> <br> prevInstruction = new Instruction(sign, name, annotation, points);<br>
cachedWays.add(prevInstruction);<br> }<br><br> updatePointsAndInstruction(edge, wayGeo);<br> }<br> ...<br> }<br> ...<br>
});<br><br> return cachedWays;<br> }<br><br><br>ADJUSTMENT:<br> public InstructionList calcInstructions( final Translation tr )<br> {<br> ... <br> forEveryEdge(new EdgeVisitor()<br>
{<br> ...<br> @Override<br> public void next( EdgeIteratorState edge, int index )<br> {<br> ...<br> double orientation = ac.calcOrientation(prevLat, prevLon, latitude, longitude);<br>
if (name == null)<br> {<br> ...<br> } else<br> {<br> double tmpOrientation = ac.alignOrientation(prevOrientation, orientation);<br>
String tmpName = edge.getName();<br> InstructionAnnotation tmpAnnotation = encoder.getAnnotation(flags, tr);<br> if ((!name.equals(tmpName))<br> || (!annotation.equals(tmpAnnotation)))<br>
{<br> name = tmpName;<br> annotation = tmpAnnotation;<br> <br> ...<br> <br> // do not add the instruction, if we are traveling from one <br>
// 'no-name' path to another 'no-name' path and it's just<br> // straight ahead (no turn required - it might make sense<br> // to make this configurable?!)<br>
if(!(name.startsWith("@@@_") && sign==Instruction.CONTINUE_ON_STREET) ){<br> points = new PointList(10, nodeAccess.is3D()); <br>
prevInstruction = new Instruction(sign, name, annotation, points);<br> cachedWays.add(prevInstruction);<br> }<br> }<br> updatePointsAndInstruction(edge, wayGeo);<br>
}<br> ...<br> }<br> ...<br> });<br><br> return cachedWays;<br> }<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-28 19:22 GMT+02:00 Matthias Marquardt <span dir="ltr"><<a href="mailto:marquardt24@gmail.com" target="_blank">marquardt24@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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 <br>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-28 16:20 GMT+02:00 Matthias Marquardt <span dir="ltr"><<a href="mailto:marquardt24@gmail.com" target="_blank">marquardt24@gmail.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>... I think I have already found it :-)<br><br>com.graphhopper.routing.util.EncodingManager.applyWayTags(...)<br>
<br></div>right?!<br></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-28 16:04 GMT+02:00 Matthias Marquardt <span dir="ltr"><<a href="mailto:marquardt24@gmail.com" target="_blank">marquardt24@gmail.com</a>></span>:<div>
<div><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Hello,<br><br>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.<br>
<br>At the beginning I had difficulties to understand why they are, like they are - when it comes to short trails like this one:<br>
<br><a href="http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274045%2C8.646782&vehicle=bike2&elevation=true" target="_blank">http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274045%2C8.646782&vehicle=bike2&elevation=true</a><br>
<br>
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...<br>
<br><a href="http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274421%2C8.648252&vehicle=bike2&elevation=true" target="_blank">http://graphhopper.com/maps/?point=51.271743%2C8.648402&point=51.274421%2C8.648252&vehicle=bike2&elevation=true</a><br>
<br>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.<br>
<br>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)<br>
<br> if ((!name.equals(tmpName))<br> || (!annotation.equals(tmpAnnotation)))<br> {<br><br>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...<br>
<br></div><div>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".<br>
<br></div><div>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']<br>
</div><div><br>TIA - Matthias<br></div></div>
</blockquote></div></div></div><span><font color="#888888"><br><br clear="all"><br>-- <br>Matthias Marquardt<br><span><a href="http://about.me/matthiasmarquardt" target="_blank">http://about.me/matthiasmarquardt</a></span><br>
<br>[FileScout | iMazing | Iconify | TOMPlayer | LittleBrother | GPSLogger II | GPSiesConnect]<br>
<a href="http://www.emacberry.com/" target="_blank">http://www.emacberry.com</a><br>
<span><a href="http://about.me/matthiasmarquardt" target="_blank"></a></span>
</font></span></div>
</blockquote></div></div></div><div><div class="h5"><br><br clear="all"><br>-- <br>Matthias Marquardt<br><span><a href="http://about.me/matthiasmarquardt" target="_blank">http://about.me/matthiasmarquardt</a></span><br><br>
[FileScout | iMazing | Iconify | TOMPlayer | LittleBrother | GPSLogger II | GPSiesConnect]<br>
<a href="http://www.emacberry.com/" target="_blank">http://www.emacberry.com</a><br>
<span><a href="http://about.me/matthiasmarquardt" target="_blank"></a></span>
</div></div></div>
</blockquote></div><br><br clear="all"><br>-- <br>Matthias Marquardt<br><span><a href="http://about.me/matthiasmarquardt" target="_blank">http://about.me/matthiasmarquardt</a></span><br><br>[FileScout | iMazing | Iconify | TOMPlayer | LittleBrother | GPSLogger II | GPSiesConnect]<br>
<a href="http://www.emacberry.com/" target="_blank">http://www.emacberry.com</a><br>
<span><a href="http://about.me/matthiasmarquardt" target="_blank"></a></span>
</div>