Skip to content

Commit

Permalink
instruction bug fix (#3000)
Browse files Browse the repository at this point in the history 
* instruction bug fix

* ensure that the turn instruction is only skipped if really necessary

* comment

* comment
  • Loading branch information
@karussell
karussell committed May 4, 2024
1 parent 959ccbe commit b9a3fc1
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 30 deletions.
28 changes: 10 additions & 18 deletions core/src/main/java/com/graphhopper/routing/InstructionsFromEdges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ public void next(EdgeIteratorState edge, int index, int prevEdgeId) {
&& (Math.abs(sign) == Instruction.TURN_SLIGHT_RIGHT || Math.abs(sign) == Instruction.TURN_RIGHT || Math.abs(sign) == Instruction.TURN_SHARP_RIGHT)
&& (Math.abs(prevInstruction.getSign()) == Instruction.TURN_SLIGHT_RIGHT || Math.abs(prevInstruction.getSign()) == Instruction.TURN_RIGHT || Math.abs(prevInstruction.getSign()) == Instruction.TURN_SHARP_RIGHT)
&& Double.isFinite(weighting.calcEdgeWeight(edge, false)) != Double.isFinite(weighting.calcEdgeWeight(edge, true))
&& InstructionsHelper.isNameSimilar(prevInstructionName, name)) {
&& InstructionsHelper.isSameName(prevInstructionName, name)) {
// Chances are good that this is a u-turn, we only need to check if the orientation matches
GHPoint point = InstructionsHelper.getPointForOrientationCalculation(edge, nodeAccess);
double lat = point.getLat();
Expand Down Expand Up @@ -352,7 +352,7 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN

// there is no other turn possible
if (nrOfPossibleTurns <= 1) {
if (Math.abs(sign) > 1 && outgoingEdges.getVisibleTurns() > 1) {
if (Math.abs(sign) > 1 && outgoingEdges.getVisibleTurns() > 1 && !outgoingEdges.mergedOrSplitWay(lanesEnc)) {
// This is an actual turn because |sign| > 1
// There could be some confusion, if we would not create a turn instruction, even though it is the only
// possible turn, also see #1048
Expand All @@ -366,8 +366,8 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN
if (Math.abs(sign) > 1) {
// Don't show an instruction if the user is following a street, even though the street is
// bending. We should only do this, if following the street is the obvious choice.
if (InstructionsHelper.isNameSimilar(name, prevName)
&& (outgoingEdges.outgoingEdgesAreSlowerByFactor(2) || isDirectionSeparatelyTagged(edge, prevEdge))) {
if (InstructionsHelper.isSameName(name, prevName) && outgoingEdges.outgoingEdgesAreSlowerByFactor(2)
|| outgoingEdges.mergedOrSplitWay(lanesEnc)) {
return Instruction.IGNORE;
}

Expand Down Expand Up @@ -400,9 +400,9 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN
// For _links, comparing flags works quite good, as links usually have different speeds => different flags
if (otherContinue != null) {
// We are at a fork
if (!InstructionsHelper.isNameSimilar(name, prevName)
|| !InstructionsHelper.isNameSimilar(destinationAndRef, prevDestinationAndRef)
|| InstructionsHelper.isNameSimilar(otherContinue.getName(), prevName)
if (!InstructionsHelper.isSameName(name, prevName)
|| !InstructionsHelper.isSameName(destinationAndRef, prevDestinationAndRef)
|| InstructionsHelper.isSameName(otherContinue.getName(), prevName)
|| !outgoingEdgesAreSlower) {

final RoadClass roadClass = edge.get(roadClassEnc);
Expand All @@ -423,7 +423,7 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN
double otherDelta = InstructionsHelper.calculateOrientationDelta(prevLat, prevLon, tmpPoint.getLat(), tmpPoint.getLon(), prevOrientation);

// This is required to avoid keep left/right on the motorway at off-ramps/motorway_links
if (Math.abs(delta) < .1 && Math.abs(otherDelta) > .15 && InstructionsHelper.isNameSimilar(name, prevName)) {
if (Math.abs(delta) < .1 && Math.abs(otherDelta) > .15 && InstructionsHelper.isSameName(name, prevName)) {
return Instruction.CONTINUE_ON_STREET;
}

Expand All @@ -435,7 +435,8 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN
}
}

if (!outgoingEdgesAreSlower && !isDirectionSeparatelyTagged(edge, prevEdge)
if (!outgoingEdgesAreSlower
&& !outgoingEdges.mergedOrSplitWay(lanesEnc)
&& (Math.abs(delta) > .6 || outgoingEdges.isLeavingCurrentStreet(prevName, name))) {
// Leave the current road -> create instruction
return sign;
Expand All @@ -444,15 +445,6 @@ private int getTurn(EdgeIteratorState edge, int baseNode, int prevNode, int adjN
return Instruction.IGNORE;
}

private boolean isDirectionSeparatelyTagged(EdgeIteratorState edge, EdgeIteratorState prevEdge) {
if (lanesEnc == null) return false;
// for cases like in #2946 we should not create instructions as they are only "tagging artifacts"
int lanes = edge.get(lanesEnc);
int prevLanes = prevEdge.get(lanesEnc);
// Usually it is a 2+2 split and then the equal sign applies. In case of a "3+2 split" we need ">=".
return lanes * 2 >= prevLanes || lanes <= 2 * prevLanes;
}

private void updatePointsAndInstruction(EdgeIteratorState edge, PointList pl) {
// skip adjNode
int len = pl.size() - 1;
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/com/graphhopper/routing/InstructionsHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ static int calculateSign(double prevLatitude, double prevLongitude, double latit
return Instruction.TURN_SHARP_RIGHT;
}

static boolean isNameSimilar(String name1, String name2) {
static boolean isSameName(String name1, String name2) {
// We don't want two empty names to be similar (they usually don't have names if they are random tracks)
if (name1 == null || name2 == null || name1.isEmpty() || name2.isEmpty())
return false;
Expand Down
69 changes: 60 additions & 9 deletions core/src/main/java/com/graphhopper/routing/InstructionsOutgoingEdges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,7 @@
*/
package com.graphhopper.routing;

import com.graphhopper.routing.ev.BooleanEncodedValue;
import com.graphhopper.routing.ev.DecimalEncodedValue;
import com.graphhopper.routing.ev.EnumEncodedValue;
import com.graphhopper.routing.ev.RoadClass;
import com.graphhopper.routing.ev.*;
import com.graphhopper.routing.weighting.Weighting;
import com.graphhopper.storage.NodeAccess;
import com.graphhopper.util.EdgeExplorer;
Expand Down Expand Up @@ -58,15 +55,17 @@ class InstructionsOutgoingEdges {

private final EdgeIteratorState prevEdge;
private final EdgeIteratorState currentEdge;
// Outgoing edges that we would be allowed to turn on
// edges that one can turn onto
private final List<EdgeIteratorState> allowedAlternativeTurns;
// All outgoing edges, including oneways in the wrong direction
// edges, including oneways in the wrong direction
private final List<EdgeIteratorState> visibleAlternativeTurns;
private final DecimalEncodedValue maxSpeedEnc;
private final EnumEncodedValue<RoadClass> roadClassEnc;
private final BooleanEncodedValue roadClassLinkEnc;
private final NodeAccess nodeAccess;
private final Weighting weighting;
private final int baseNode;
private final EdgeExplorer allExplorer;

public InstructionsOutgoingEdges(EdgeIteratorState prevEdge,
EdgeIteratorState currentEdge,
Expand All @@ -86,6 +85,8 @@ public InstructionsOutgoingEdges(EdgeIteratorState prevEdge,
this.roadClassEnc = roadClassEnc;
this.roadClassLinkEnc = roadClassLinkEnc;
this.nodeAccess = nodeAccess;
this.baseNode = baseNode;
this.allExplorer = allExplorer;

visibleAlternativeTurns = new ArrayList<>();
allowedAlternativeTurns = new ArrayList<>();
Expand Down Expand Up @@ -179,19 +180,19 @@ public EdgeIteratorState getOtherContinue(double prevLat, double prevLon, double
* If either of these properties is true, we can be quite certain that a turn instruction should be provided.
*/
public boolean isLeavingCurrentStreet(String prevName, String name) {
if (InstructionsHelper.isNameSimilar(name, prevName)) {
if (InstructionsHelper.isSameName(name, prevName)) {
return false;
}

boolean roadClassOrLinkChange = !isTheSameRoadClassAndLink(prevEdge, currentEdge);
for (EdgeIteratorState edge : allowedAlternativeTurns) {
String edgeName = edge.getName();
// leave the current street
if (InstructionsHelper.isNameSimilar(prevName, edgeName) || (roadClassOrLinkChange && isTheSameRoadClassAndLink(prevEdge, edge))) {
if (InstructionsHelper.isSameName(prevName, edgeName) || (roadClassOrLinkChange && isTheSameRoadClassAndLink(prevEdge, edge))) {
return true;
}
// enter a different street
if (InstructionsHelper.isNameSimilar(name, edgeName) || (roadClassOrLinkChange && isTheSameRoadClassAndLink(currentEdge, edge))) {
if (InstructionsHelper.isSameName(name, edgeName) || (roadClassOrLinkChange && isTheSameRoadClassAndLink(currentEdge, edge))) {
return true;
}
}
Expand All @@ -202,4 +203,54 @@ private boolean isTheSameRoadClassAndLink(EdgeIteratorState edge1, EdgeIteratorS
return edge1.get(roadClassEnc) == edge2.get(roadClassEnc) && edge1.get(roadClassLinkEnc) == edge2.get(roadClassLinkEnc);
}

// for cases like in #2946 we should not create instructions as they are only "tagging artifacts"
public boolean mergedOrSplitWay(IntEncodedValue lanesEnc) {
if (lanesEnc == null) return false;

String name = currentEdge.getName();
RoadClass roadClass = currentEdge.get(roadClassEnc);
if (!InstructionsHelper.isSameName(name, prevEdge.getName()) || roadClass != prevEdge.get(roadClassEnc))
return false;

EdgeIterator edgeIter = allExplorer.setBaseNode(baseNode);
EdgeIteratorState otherEdge = null;
while (edgeIter.next()) {
if (currentEdge.getEdge() != edgeIter.getEdge()
&& prevEdge.getEdge() != edgeIter.getEdge()
&& roadClass == edgeIter.get(roadClassEnc)
&& InstructionsHelper.isSameName(name, edgeIter.getName())
&& (Double.isFinite(weighting.calcEdgeWeight(edgeIter, false))
|| Double.isFinite(weighting.calcEdgeWeight(edgeIter, true)))) {
if (otherEdge != null) return false; // too many possible other edges
otherEdge = edgeIter.detach(false);
}
}
if (otherEdge == null) return false;

if (Double.isFinite(weighting.calcEdgeWeight(currentEdge, true))) {
// assume two ways are merged into one way
// -> prev ->
// <- edge ->
// -> other ->
if (Double.isFinite(weighting.calcEdgeWeight(prevEdge, true))) return false;
// otherEdge has direction from junction outwards
if (!Double.isFinite(weighting.calcEdgeWeight(otherEdge, false))) return false;
if (Double.isFinite(weighting.calcEdgeWeight(otherEdge, true))) return false;

int delta = Math.abs(prevEdge.get(lanesEnc) + otherEdge.get(lanesEnc) - currentEdge.get(lanesEnc));
return delta <= 1;
}

// assume one way is split into two ways
// -> edge ->
// <- prev ->
// -> other ->
if (!Double.isFinite(weighting.calcEdgeWeight(prevEdge, true))) return false;
// otherEdge has direction from junction outwards
if (Double.isFinite(weighting.calcEdgeWeight(otherEdge, false))) return false;
if (!Double.isFinite(weighting.calcEdgeWeight(otherEdge, true))) return false;

int delta = prevEdge.get(lanesEnc) - (currentEdge.get(lanesEnc) + otherEdge.get(lanesEnc));
return delta <= 1;
}
}
46 changes: 44 additions & 2 deletions core/src/test/java/com/graphhopper/util/InstructionListTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -545,9 +545,9 @@ public void testSplitWays() {
PointList list = new PointList();
list.add(43.62549, -79.714292);
g.edge(1, 2).setKeyValues(createKV(STREET_NAME, "main")).setWayGeometry(list).
setDistance(110).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 2);
setDistance(110).set(roadsSpeedEnc, 50, 0).set(lanesEnc, 2);
g.edge(2, 3).setKeyValues(createKV(STREET_NAME, "main")).
setDistance(110).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 3);
setDistance(110).set(roadsSpeedEnc, 50, 0).set(lanesEnc, 3);
g.edge(2, 4).setKeyValues(createKV(STREET_NAME, "main")).
setDistance(80).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 5);

Expand All @@ -556,6 +556,48 @@ public void testSplitWays() {
InstructionList wayList = InstructionsFromEdges.calcInstructions(p, g, weighting, tmpEM, usTR);
List<String> tmpList = getTurnDescriptions(wayList);
assertEquals(Arrays.asList("continue onto main", "arrive at destination"), tmpList);

// Other roads should not influence instructions. Example: https://www.openstreetmap.org/node/392106581
na.setNode(5, 43.625666,-79.714048);
g.edge(2, 5).setDistance(80).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 5);

p = new Dijkstra(g, weighting, tMode).calcPath(1, 4);
wayList = InstructionsFromEdges.calcInstructions(p, g, weighting, tmpEM, usTR);
tmpList = getTurnDescriptions(wayList);
assertEquals(Arrays.asList("continue onto main", "arrive at destination"), tmpList);
}

@Test
public void testNotSplitWays() {
DecimalEncodedValue roadsSpeedEnc = new DecimalEncodedValueImpl("speed", 7, 2, true);
EncodingManager tmpEM = EncodingManager.start().add(roadsSpeedEnc).
add(RoadClass.create()).add(Roundabout.create()).add(RoadClassLink.create()).
add(MaxSpeed.create()).add(Lanes.create()).build();
IntEncodedValue lanesEnc = tmpEM.getIntEncodedValue(Lanes.KEY);
BaseGraph g = new BaseGraph.Builder(tmpEM).create();
// real world example: https://graphhopper.com/maps/?point=51.425484%2C14.223298&point=51.42523%2C14.222864&profile=car
// 3
// |
// 1-2-4

NodeAccess na = g.getNodeAccess();
na.setNode(1, 51.42523, 14.222864);
na.setNode(2, 51.425256, 14.22325);
na.setNode(3, 51.425397, 14.223266);
na.setNode(4, 51.425273, 14.223427);

g.edge(1, 2).setKeyValues(createKV(STREET_NAME, "dresdener")).
setDistance(110).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 2);
g.edge(2, 3).setKeyValues(createKV(STREET_NAME, "dresdener")).
setDistance(110).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 3);
g.edge(2, 4).setKeyValues(createKV(STREET_NAME, "main")).
setDistance(80).set(roadsSpeedEnc, 50, 50).set(lanesEnc, 5);

Weighting weighting = new SpeedWeighting(roadsSpeedEnc);
Path p = new Dijkstra(g, weighting, tMode).calcPath(3, 1);
InstructionList wayList = InstructionsFromEdges.calcInstructions(p, g, weighting, tmpEM, usTR);
List<String> tmpList = getTurnDescriptions(wayList);
assertEquals(Arrays.asList("continue onto dresdener", "turn right onto dresdener", "arrive at destination"), tmpList);
}

private void compare(List<List<Double>> expected, List<List<Double>> actual) {
Expand Down

0 comments on commit b9a3fc1

Please sign in to comment.