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 21 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
38 changes: 22 additions & 16 deletions core/src/main/java/com/graphhopper/reader/osm/OSMNodeData.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

package com.graphhopper.reader.osm;

import com.carrotsearch.hppc.LongScatterSet;
import com.carrotsearch.hppc.LongSet;
import com.graphhopper.coll.GHLongIntBTree;
import com.graphhopper.coll.LongIntMap;
import com.graphhopper.reader.ReaderNode;
Expand All @@ -26,15 +28,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 @@ -68,11 +65,12 @@ class OSMNodeData {
private final PointAccess towerNodes;

// this map stores an index for each OSM node we keep the node tags of. a value of -1 means there is no entry
// yet and a value of -2 means there was an entry but it was removed again
// yet.
private final LongIntMap nodeTagIndicesByOsmNodeIds;

// stores node tags
private final List<Map<String, Object>> nodeTags;
private final LongSet nodesToBeSplit;
Copy link
Member Author

@easbar easbar Mar 15, 2023

Choose a reason for hiding this comment

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

In my last commit I added this LongSet to replace the artificial (and possibly confusing?) gh:split_node tag, and so we do not need the remove operation for the tags (in case we like to store the node tags with EdgeKVStorage or similar).

Now we use the following data structures for the node data:

  • idsByOSMNodeIds is a (very large) map that maps the full range of possible (64bit) OSM node IDs to a smaller (32bit) range of indices of nodes we actually use to build the graph (typically those contained in ways using the highway tag).

  • nodeTagIndicesByOSMNodeIds is a similar, but smaller, map that does the same, but only for nodes for which we keep the tags for

  • nodeTags is a list that maps the int indices we get via nodeTagIndicesByOSMNodeIds to the actual tags

  • nodesToBeSplit is a separate hash set of OSM node IDS that shall be split. for this we need a way to remove/unset values.

Quite possibly we could combine some of these to either save memory or speed up the import, or both? For example we could combine idsByOsmNodeIds and nodeTagIndicesByOsmNodeIds, which would definitely be an improvement if we stored tags for all nodes. Or we could use the int index we get via nodeTagIndicesByOsmNodeIds to replace the nodesToBeSplit LongSet with a BitSet (using the int index).

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite possibly we could combine some of these to either save memory or speed up the import, or both?

It looks like whenever we do a lookup into nodeTagIndicesByOSMNodeIds we also do a lookup into idsByOSMNodeIds anyway, which means we could get rid of the lookup into nodeTagIndicesByOSMNodeIds. But since there are less nodes for which we store tags than there are nodes we need to map the (larger) int index we get from idsByOSMNodeIds to a smaller, more compact index. At least as long as we want to keep the node tags list compact. So actually we would then need another int-int lookup, and I'm not sure if overall this will be better, but it might be worth a try.

Copy link
Member Author

@easbar easbar Mar 15, 2023

Choose a reason for hiding this comment

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

Did you check for how many nodes we now store the tags (compared to the total number of nodes we map the coordinates for)?

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit off-topic, but it also might speed up the import if we replaced:

 int curr = idsByOsmNodeIds.get(osmNodeId);
 if (curr == EMPTY_NODE)
       idsByOsmNodeIds.put(osmNodeId, newNodeType);

with something like

  int index = idsByOsmNodeIds.indexOf(osmNodeId);
  int curr = idsByOsmNodeIds.indexGet(index);
  if (curr == EMPTY_NODE)
        idsByOsmNodeIds.indexPut(index, newNodeType);

but not sure if using an index like this is possible with GHLongIntBTreeMap.


private int nextTowerId = 0;
private int nextPillarId = 0;
Expand All @@ -88,6 +86,7 @@ public OSMNodeData(PointAccess nodeAccess, Directory directory) {
pillarNodes = new PillarInfo(towerNodes.is3D(), directory);

nodeTagIndicesByOsmNodeIds = new GHLongIntBTree(200);
nodesToBeSplit = new LongScatterSet();
nodeTags = new ArrayList<>();
}

Expand Down Expand Up @@ -187,7 +186,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 @@ -241,9 +240,7 @@ public void addCoordinatesToPointList(int id, PointList pointList) {

public void setTags(ReaderNode node) {
int tagIndex = nodeTagIndicesByOsmNodeIds.get(node.getId());
if (tagIndex == -2)
throw new IllegalStateException("Cannot add tags after they were removed");
else if (tagIndex == -1) {
if (tagIndex == -1) {
nodeTagIndicesByOsmNodeIds.put(node.getId(), nodeTags.size());
nodeTags.add(node.getTags());
} else {
Expand All @@ -258,11 +255,6 @@ public Map<String, Object> getTags(long osmNodeId) {
return nodeTags.get(tagIndex);
}

public void removeTags(long osmNodeId) {
int prev = nodeTagIndicesByOsmNodeIds.put(osmNodeId, -2);
nodeTags.set(prev, emptyMap());
}

public void release() {
pillarNodes.clear();
}
Expand All @@ -282,4 +274,18 @@ public int pillarNodeToId(int pillarId) {
public int idToPillarNode(int id) {
return id - 3;
}

public boolean setSplitNode(long osmNodeId) {
return nodesToBeSplit.add(osmNodeId);
}

public void unsetSplitNode(long osmNodeId) {
int removed = nodesToBeSplit.removeAll(osmNodeId);
if (removed == 0)
throw new IllegalStateException("Node " + osmNodeId + " was not a split node");
}

public boolean isSplitNode(long osmNodeId) {
return nodesToBeSplit.contains(osmNodeId);
}
}
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;
}
}
53 changes: 34 additions & 19 deletions core/src/main/java/com/graphhopper/reader/osm/WaySegmentParser.java
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,13 @@
import java.io.File;
import java.io.IOException;
import java.text.ParseException;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Map;
import java.util.*;
import java.util.function.Consumer;
import java.util.function.LongToIntFunction;
import java.util.function.Predicate;

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 All @@ -68,6 +64,7 @@
*/
public class WaySegmentParser {
private static final Logger LOGGER = LoggerFactory.getLogger(WaySegmentParser.class);
private static final Set<String> INCLUDE_IF_NODE_TAGS = new HashSet<>(Arrays.asList("barrier", "highway", "railway", "crossing", "ford"));

private final ElevationProvider eleProvider;
private final Predicate<ReaderWay> wayFilter;
Expand Down Expand Up @@ -224,14 +221,27 @@ public void handleNode(ReaderNode node) {

acceptedNodes++;

// we keep node tags for barrier nodes
// remember which 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.setSplitNode(node.getId());
}

// store node tags if at least one important tag is included and make this available for the edge handler
for (Map.Entry<String, Object> e : node.getTags().entrySet()) {
if (INCLUDE_IF_NODE_TAGS.contains(e.getKey())) {
node.removeTag("created_by");
node.removeTag("source");
node.removeTag("note");
node.removeTag("fixme");
node.setTags(new HashMap<>(node.getTags())); // create compact Map
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'd rather create the compact map in OSMNodeData.setTags, so it is kind of up to this 'storage' class how to the tags are stored (and compacted and such).

nodeData.setTags(node);
break;
}
}
}

Expand All @@ -251,7 +261,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 +314,11 @@ 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()) {
if (nodeData.isSplitNode(node.osmNodeId)) {
// 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.unsetSplitNode(node.osmNodeId);

// 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 +330,30 @@ private void splitSegmentAtSplitNodes(List<SegmentNode> parentSegment, ReaderWay
}
if (!segment.isEmpty()) {
segment.add(barrierFrom);
handleSegment(segment, way, emptyMap());
handleSegment(segment, way);
segment = new ArrayList<>();
}

// mark barrier edge
way.setTag("gh:barrier_edge", true);
segment.add(barrierFrom);
segment.add(barrierTo);
handleSegment(segment, way, nodeTags);
handleSegment(segment, way);
way.removeTag("gh:barrier_edge");

segment = new ArrayList<>();
segment.add(barrierTo);

// ignore this barrier node from now. 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);
} 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 +373,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 +562,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 @@ -79,15 +79,13 @@ public ConditionalTagInspector getConditionalTagInspector() {
* Updates the given edge flags based on node tags
*/
protected void handleNodeTags(IntsRef edgeFlags, Map<String, Object> nodeTags) {
if (!nodeTags.isEmpty()) {
// for now we just create a dummy reader node, because our encoders do not make use of the coordinates 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.

Let's rename handleNodeTags? It sounds like something general, but cit is only called for edges tagged as gh:barrier_edge, and the same for the JavaDoc comment.

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);
}
// 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);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +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 static java.util.Collections.emptyMap;
import java.util.*;

public abstract class BikeCommonAccessParser extends AbstractAccessParser implements TagParser {

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

Map<String, Object> nodeTags = way.getTag("node_tags", emptyMap());
handleNodeTags(edgeFlags, nodeTags);
if (way.hasTag("gh:barrier_edge")) {
List<Map<String, Object>> nodeTags = way.getTag("node_tags", Collections.emptyList());
handleNodeTags(edgeFlags, nodeTags.get(0));
}
}

protected void handleAccess(IntsRef edgeFlags, ReaderWay way) {
Expand Down