<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hey Matthias,<br>
<br>
yes, the instructions could be better as outlined in #185 and #94.<br>
Before digging in your code: what change do you propose? Creating
an instruction after every edge?<br>
<br>
Regards,<br>
Peter.<br>
<br>
</div>
<blockquote
cite="mid:CAFUrcRAnHbrHvANEroMUM_Hb+LsfMnpNHJxJw-TGBrPMJFU3AQ@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true" 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 moz-do-not-send="true" href="http://this.name">this.name</a>
= name;<br>
}else{<br>
<a moz-do-not-send="true" 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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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 moz-do-not-send="true"
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 moz-do-not-send="true"
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></span><br>
</div>
</blockquote>
</div>
</div>
</div>
</div>
</blockquote>
</div>
</div>
</blockquote>
<br>
</body>
</html>