Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make use of node tags like 'highway' or 'crossing' #2705

Merged
merged 27 commits into from
Mar 16, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ac52a74
Pass all node tags to edge handlers
easbar Dec 2, 2022
1e70c4a
Remove node tag count (was only used for analysis)
easbar Dec 2, 2022
8a09a17
crossing EV (#2706)
karussell Dec 3, 2022
c7d978b
add tests; fix traffic_signalS; remove zebra as same as uncontrolled;…
karussell Dec 6, 2022
717d1f7
merged master
karussell Mar 2, 2023
971ac6f
Merge branch 'master' into node_tags
karussell Mar 2, 2023
cdbf3ad
fix
karussell Mar 2, 2023
6e6cbe9
fix tests
karussell Mar 2, 2023
9324f5a
minor fix
karussell Mar 2, 2023
b5ec43e
minor typo
easbar Mar 6, 2023
f0efbd5
node tag whitelist; use get not put in removeTag
karussell Mar 6, 2023
b4d9983
Merge branch 'master' into node_tags
karussell Mar 6, 2023
39b8fbf
Merge branch 'master' into node_tags
karussell Mar 10, 2023
e764512
include node tags handling for road_access and fords (road_environment)
karussell Mar 10, 2023
dd896ed
mark only node tags of barrier edge as 'split_node'
karussell Mar 11, 2023
adf3ef9
avoid stream and collect
karussell Mar 11, 2023
48f3634
minor further perf tune
karussell Mar 11, 2023
8f4cbf1
make barrier edge explicit via tag
karussell Mar 14, 2023
9d88f1d
simplify handleNodeTags
karussell Mar 14, 2023
0e4f70f
Remove artificial gh:split_node tag and use long hash set instead
easbar Mar 15, 2023
b785cc0
minor rename
easbar Mar 15, 2023
c0e29fa
use EdgeKVStorage for node tags storage
karussell Mar 15, 2023
5bdce99
rename EdgeKVStorage to KVStorage as also used to temporarily store n…
karussell Mar 15, 2023
95f8358
log more infos about node tags; no compact necessary due to KVStorage…
karussell Mar 15, 2023
7b22889
rename method to handleBarrierEdge
karussell Mar 15, 2023
f02421a
fix taggednodecount method
karussell Mar 15, 2023
8287848
node tags for the barrier edge should be identical, i.e. use the ref …
karussell Mar 16, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
### 7.0 [not yet released]

- access node tags via List instead of Map: List<Map<String, Object>> nodeTags = way.getTag("node_tags", emptyList()), see #2705
- remove StringEncodedValue support from custom model due to insufficient usage/testing
- handle also node_tags in handleWayTags, when extending AbstractAccessParser call handleNodeTags, #2738
- Format of 'areas' in CustomModel changed to 'FeatureCollection'. The old format is deprecated and will be removed in a later version, #2734
Expand Down
13 changes: 4 additions & 9 deletions core/src/main/java/com/graphhopper/reader/osm/OSMNodeData.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,10 @@
import com.graphhopper.util.PointList;
import com.graphhopper.util.shapes.GHPoint3D;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.*;
import java.util.function.DoubleSupplier;
import java.util.function.IntUnaryOperator;

import static java.util.Collections.emptyMap;

/**
* This class stores OSM node data while reading an OSM file in {@link WaySegmentParser}. It is not trivial to do this
* in a memory-efficient way. We use the following approach:
Expand Down Expand Up @@ -187,7 +182,7 @@ SegmentNode addCopyOfNode(SegmentNode node) {
if (idsByOsmNodeIds.put(newOsmId, INTERMEDIATE_NODE) != EMPTY_NODE)
throw new IllegalStateException("Artificial osm node id already exists: " + newOsmId);
int id = addPillarNode(newOsmId, point.getLat(), point.getLon(), point.getEle());
return new SegmentNode(newOsmId, id);
return new SegmentNode(newOsmId, id, new HashMap<>(node.tags));
}

int convertPillarToTowerNode(int id, long osmNodeId) {
Expand Down Expand Up @@ -258,9 +253,9 @@ public Map<String, Object> getTags(long osmNodeId) {
return nodeTags.get(tagIndex);
}

public void removeTags(long osmNodeId) {
public void removeTag(long osmNodeId, String key) {
int prev = nodeTagIndicesByOsmNodeIds.put(osmNodeId, -2);
karussell marked this conversation as resolved.
Show resolved Hide resolved
nodeTags.set(prev, emptyMap());
nodeTags.get(prev).remove(key);
}

public void release() {
Expand Down
8 changes: 5 additions & 3 deletions core/src/main/java/com/graphhopper/reader/osm/OSMReader.java
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ private boolean isFerry(ReaderWay way) {
* This method is called during the second pass of {@link WaySegmentParser} and provides an entry point to enrich
* the given OSM way with additional tags before it is passed on to the tag parsers.
*/
protected void setArtificialWayTags(PointList pointList, ReaderWay way, double distance, Map<String, Object> nodeTags) {
protected void setArtificialWayTags(PointList pointList, ReaderWay way, double distance, List<Map<String, Object>> nodeTags) {
way.setTag("node_tags", nodeTags);
way.setTag("edge_distance", distance);
way.setTag("point_list", pointList);
Expand Down Expand Up @@ -293,14 +293,16 @@ protected void setArtificialWayTags(PointList pointList, ReaderWay way, double d
* @param toIndex a unique integer id for the last node of this segment
* @param pointList coordinates of this segment
* @param way the OSM way this segment was taken from
* @param nodeTags node tags of this segment if it is an artificial edge, empty otherwise
* @param nodeTags node tags of this segment. there is one map of tags for each point.
*/
protected void addEdge(int fromIndex, int toIndex, PointList pointList, ReaderWay way, Map<String, Object> nodeTags) {
protected void addEdge(int fromIndex, int toIndex, PointList pointList, ReaderWay way, List<Map<String, Object>> nodeTags) {
// sanity checks
if (fromIndex < 0 || toIndex < 0)
throw new AssertionError("to or from index is invalid for this edge " + fromIndex + "->" + toIndex + ", points:" + pointList);
if (pointList.getDimension() != nodeAccess.getDimension())
throw new AssertionError("Dimension does not match for pointList vs. nodeAccess " + pointList.getDimension() + " <-> " + nodeAccess.getDimension());
if (pointList.size() != nodeTags.size())
throw new AssertionError("there should be as many maps of node tags as there are points. node tags: " + nodeTags.size() + ", points: " + pointList.size());

// todo: in principle it should be possible to delay elevation calculation so we do not need to store
// elevations during import (saves memory in pillar info during import). also note that we already need to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@

package com.graphhopper.reader.osm;

import java.util.Map;

class SegmentNode {
long osmNodeId;
int id;
Map<String, Object> tags;

public SegmentNode(long osmNodeId, int id) {
public SegmentNode(long osmNodeId, int id, Map<String, Object> tags) {
this.osmNodeId = osmNodeId;
this.id = id;
this.tags = tags;
}
}
35 changes: 21 additions & 14 deletions core/src/main/java/com/graphhopper/reader/osm/WaySegmentParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@

import static com.graphhopper.reader.osm.OSMNodeData.*;
import static com.graphhopper.util.Helper.nf;
import static java.util.Collections.emptyMap;

/**
* This class parses a given OSM file and splits OSM ways into 'segments' at all intersections (or 'junctions').
Expand Down Expand Up @@ -224,15 +223,22 @@ public void handleNode(ReaderNode node) {

acceptedNodes++;

// we keep node tags for barrier nodes
// set a special tag for nodes we want to split
if (splitNodeFilter.test(node)) {
if (nodeType == JUNCTION_NODE) {
LOGGER.debug("OSM node {} at {},{} is a barrier node at a junction. The barrier will be ignored",
node.getId(), Helper.round(node.getLat(), 7), Helper.round(node.getLon(), 7));
ignoredSplitNodes++;
} else
nodeData.setTags(node);
node.setTag("gh:split_node", true);
karussell marked this conversation as resolved.
Show resolved Hide resolved
}

// we important node tags, so they will be available for the edge handler
node.removeTag("created_by");
node.removeTag("source");
node.removeTag("note");
if (node.hasTags())
nodeData.setTags(node);
}

@Override
Expand All @@ -251,7 +257,7 @@ public void handleWay(ReaderWay way) {
return;
List<SegmentNode> segment = new ArrayList<>(way.getNodes().size());
for (LongCursor node : way.getNodes())
segment.add(new SegmentNode(node.value, nodeData.getId(node.value)));
segment.add(new SegmentNode(node.value, nodeData.getId(node.value), nodeData.getTags(node.value)));
wayPreprocessor.preprocessWay(way, osmNodeId -> nodeData.getCoordinates(nodeData.getId(osmNodeId)));
splitWayAtJunctionsAndEmptySections(segment, way);
}
Expand Down Expand Up @@ -304,9 +310,8 @@ private void splitSegmentAtSplitNodes(List<SegmentNode> parentSegment, ReaderWay
List<SegmentNode> segment = new ArrayList<>();
for (int i = 0; i < parentSegment.size(); i++) {
SegmentNode node = parentSegment.get(i);
Map<String, Object> nodeTags = nodeData.getTags(node.osmNodeId);
// so far we only consider node tags of split nodes, so if there are node tags we split the node
if (!nodeTags.isEmpty()) {
Map<String, Object> nodeTags = node.tags;
if ((boolean) nodeTags.getOrDefault("gh:split_node", false)) {
// this node is a barrier. we will copy it and add an extra edge
SegmentNode barrierFrom = node;
SegmentNode barrierTo = nodeData.addCopyOfNode(node);
Expand All @@ -318,28 +323,29 @@ private void splitSegmentAtSplitNodes(List<SegmentNode> parentSegment, ReaderWay
}
if (!segment.isEmpty()) {
segment.add(barrierFrom);
handleSegment(segment, way, emptyMap());
handleSegment(segment, way);
segment = new ArrayList<>();
}
segment.add(barrierFrom);
segment.add(barrierTo);
handleSegment(segment, way, nodeTags);
handleSegment(segment, way);
segment = new ArrayList<>();
segment.add(barrierTo);

// ignore this barrier node from now. for example a barrier can be connecting two ways (appear in both
// do not split this node again. for example a barrier can be connecting two ways (appear in both
// ways) and we only want to add a barrier edge once (but we want to add one).
nodeData.removeTags(node.osmNodeId);
nodeData.removeTag(node.osmNodeId, "gh:split_node");
} else {
segment.add(node);
}
}
if (segment.size() > 1)
handleSegment(segment, way, emptyMap());
handleSegment(segment, way);
}

void handleSegment(List<SegmentNode> segment, ReaderWay way, Map<String, Object> nodeTags) {
void handleSegment(List<SegmentNode> segment, ReaderWay way) {
final PointList pointList = new PointList(segment.size(), nodeData.is3D());
final List<Map<String, Object>> nodeTags = new ArrayList<>(segment.size());
int from = -1;
int to = -1;
for (int i = 0; i < segment.size(); i++) {
Expand All @@ -359,6 +365,7 @@ else if (i == segment.size() - 1)
else if (isTowerNode(id))
throw new IllegalStateException("Tower nodes should only appear at the end of segments, way: " + way.getId());
nodeData.addCoordinatesToPointList(id, pointList);
nodeTags.add(node.tags);
}
if (from < 0 || to < 0)
throw new IllegalStateException("The first and last nodes of a segment must be tower nodes, way: " + way.getId());
Expand Down Expand Up @@ -547,7 +554,7 @@ default void onFinish() {
}

public interface EdgeHandler {
void handleEdge(int from, int to, PointList pointList, ReaderWay way, Map<String, Object> nodeTags);
void handleEdge(int from, int to, PointList pointList, ReaderWay way, List<Map<String, Object>> nodeTags);
}

public interface RelationProcessor {
Expand Down
30 changes: 30 additions & 0 deletions core/src/main/java/com/graphhopper/routing/ev/Crossing.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package com.graphhopper.routing.ev;

import com.graphhopper.util.Helper;

public enum Crossing {
MISSING, // no information
RAILWAY_BARRIER, // railway crossing with barrier
RAILWAY, // railway crossing with road
TRAFFIC_SIGNALS, // with light signals
UNCONTROLLED, // with crosswalk, without traffic lights
MARKED, // with crosswalk, with or without traffic lights
UNMARKED, // without markings or traffic lights
NO; // crossing is impossible or illegal
public static final String KEY = "crossing";

@Override
public String toString() {
return Helper.toLowerCase(name());
}

public static Crossing find(String name) {
if (name == null)
return MISSING;
try {
return Crossing.valueOf(Helper.toUpperCase(name));
} catch (IllegalArgumentException ex) {
return MISSING;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,10 @@ public EncodedValue create(String name, PMap properties) {
return AverageSlope.create();
} else if (Curvature.KEY.equals(name)) {
return Curvature.create();
} else
} else if (Crossing.KEY.equals(name)) {
return new EnumEncodedValue<>(Crossing.KEY, Crossing.class);
} else {
throw new IllegalArgumentException("DefaultEncodedValueFactory cannot find EncodedValue " + name);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,18 @@ public ConditionalTagInspector getConditionalTagInspector() {
/**
* Updates the given edge flags based on node tags
*/
protected void handleNodeTags(IntsRef edgeFlags, Map<String, Object> nodeTags) {
if (!nodeTags.isEmpty()) {
protected void handleNodeTags(IntsRef edgeFlags, List<Map<String, Object>> nodeTags) {
if (nodeTags.isEmpty()) return; // allMatch returns 'true' by default
boolean isBarrier = nodeTags.stream().allMatch(tags -> {
Copy link
Member

@karussell karussell Mar 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We split barrier nodes into separate (short) edges where both nodes have these barrier tags. The adjacent edges only have one node with these barrier tags and so we need allMatch and anyMatch is not sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should do this already one level higher in OSMReader#handleEdge#setArtificialWayTags, something like:

        if (!nodeTags.isEmpty() && (boolean) nodeTags.get(0).getOrDefault("gh:split_node", false))
            way.setTag("gh:split_node_edge", true);
        else
            way.removeTag("gh:split_node_edge");

This way the downstream tag parsers can recognize edges that were created by splitting a node by looking at the gh:split_node_edge tag and can access the tags of the split node by looking at the node tags for either of the two nodes (they are the same anyway).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this even earlier: The moment we create the barrier edge we now add the gh:barrier_edge tag for the reader way.

We could even use the node tags of the split node as value of the gh:barrier_edge tag (instead of simply true)? But we can also obtain these tags in the parsers by looking at the node tags of either of the two nodes of these edges, like we currently do, not sure if one would be more clear than the other. We could even do both :)

// for now we just create a dummy reader node, because our encoders do not make use of the coordinates anyway
ReaderNode readerNode = new ReaderNode(0, 0, 0, nodeTags);
// block access for barriers
if (isBarrier(readerNode)) {
BooleanEncodedValue accessEnc = getAccessEnc();
accessEnc.setBool(false, edgeFlags, false);
accessEnc.setBool(true, edgeFlags, false);
}
ReaderNode readerNode = new ReaderNode(0, 0, 0, tags);
return isBarrier(readerNode);
});
// block access for barriers
if (isBarrier) {
BooleanEncodedValue accessEnc = getAccessEnc();
accessEnc.setBool(false, edgeFlags, false);
accessEnc.setBool(true, edgeFlags, false);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@
import com.graphhopper.routing.util.WayAccess;
import com.graphhopper.storage.IntsRef;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.*;

import static java.util.Collections.emptyMap;

Expand Down Expand Up @@ -133,7 +130,7 @@ public void handleWayTags(IntsRef edgeFlags, ReaderWay way) {
handleAccess(edgeFlags, way);
}

Map<String, Object> nodeTags = way.getTag("node_tags", emptyMap());
List<Map<String, Object>> nodeTags = way.getTag("node_tags", Collections.emptyList());
handleNodeTags(edgeFlags, nodeTags);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@
import com.graphhopper.storage.IntsRef;
import com.graphhopper.util.PMap;

import java.util.Arrays;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.*;

import static java.util.Collections.emptyMap;

Expand Down Expand Up @@ -158,7 +155,7 @@ public void handleWayTags(IntsRef edgeFlags, ReaderWay way) {
accessEnc.setBool(true, edgeFlags, true);
}

Map<String, Object> nodeTags = way.getTag("node_tags", emptyMap());
List<Map<String, Object>> nodeTags = way.getTag("node_tags", Collections.emptyList());
handleNodeTags(edgeFlags, nodeTags);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ else if (name.equals(Footway.KEY))
return new OSMFootwayParser(lookup.getEnumEncodedValue(Footway.KEY, Footway.class));
else if (name.equals(Country.KEY))
return new CountryParser(lookup.getEnumEncodedValue(Country.KEY, Country.class));
else if (name.equals(Crossing.KEY))
return new OSMCrossingParser(lookup.getEnumEncodedValue(Crossing.KEY, Crossing.class));
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,7 @@
import com.graphhopper.storage.IntsRef;
import com.graphhopper.util.PMap;

import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.*;

import static com.graphhopper.routing.ev.RouteNetwork.*;
import static com.graphhopper.routing.util.PriorityCode.UNCHANGED;
Expand Down Expand Up @@ -184,7 +181,7 @@ public void handleWayTags(IntsRef edgeFlags, ReaderWay way) {
accessEnc.setBool(true, edgeFlags, true);
}

Map<String, Object> nodeTags = way.getTag("node_tags", emptyMap());
List<Map<String, Object>> nodeTags = way.getTag("node_tags", Collections.emptyList());
handleNodeTags(edgeFlags, nodeTags);
}
}