From 881e510d8b902269e04bc9bb15e24b694e370636 Mon Sep 17 00:00:00 2001 From: Pieter Buzing Date: Tue, 24 May 2016 17:07:17 +0200 Subject: [PATCH 1/4] Give Simple Street linker an iterative search This improves the performance of the linker (#2170) considerably. Instead of examining all edges within a 1 km radius we now do this in a number of iterations, each time doubling the radius. Usually the first iteration suffices in a dense area. --- .../linking/SimpleStreetSplitter.java | 82 +++++++++++-------- 1 file changed, 46 insertions(+), 36 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java b/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java index 864063be9f6..6a951c13d48 100644 --- a/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java +++ b/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java @@ -39,6 +39,7 @@ import java.util.Collections; import java.util.Comparator; import java.util.List; +import java.util.stream.Collectors; /** * This class links transit stops to streets by splitting the streets (unless the stop is extremely close to the street @@ -98,44 +99,64 @@ else if (v instanceof BikeRentalStationVertex) else if (v instanceof BikeParkVertex) LOG.warn(graph.addBuilderAnnotation(new BikeParkUnlinked((BikeParkVertex) v))); }; + } } } - /** Link this vertex into the graph */ - public boolean link (Vertex vertex) { + /** + * Iteratively search the area around a location and find all the close by edges that can be traversed. + * This method tries to collect at least 10 candidates per iteration in order to prevent the loss of + * edges that are approximately at the same distance from the center. This number is arbitrary, though. + *

+ * Note that the returned list is unsorted. + * + * @param vertex the center vertex + * @return a (possibly empty) list of edges close to the vertex + */ + private List getCandidateEdges(Vertex vertex) { + List edges; + double maxRadiusDeg = SphericalDistanceLibrary.metersToDegrees(MAX_SEARCH_RADIUS_METERS); + int iteration = 7; // We double the radius at most n times + + final TraverseModeSet traverseModeSet = new TraverseModeSet(TraverseMode.WALK); + final Envelope env = new Envelope(vertex.getCoordinate()); + double xscale = Math.cos(vertex.getLat() * Math.PI / 180); + double radius = maxRadiusDeg / (1 << (iteration - 1)); + env.expandBy(radius / xscale, radius); + + do { + //Do the envelope query and filter edges that can be traversed and that are still in the + //graph + edges = idx.query(env).stream().parallel().filter(edge -> + edge.canTraverse(traverseModeSet) && + edge.getToVertex().getIncoming().contains(edge) + ).collect(Collectors.toList()); + + iteration--; + radius = maxRadiusDeg / (1 << iteration); + env.expandBy(radius / xscale, radius); + } while (edges.size() < 10 && iteration > 0); + + return edges; + } + + /** + * Link this vertex into the graph + */ + public boolean link(Vertex vertex) { // find nearby street edges - // TODO: we used to use an expanding-envelope search, which is more efficient in - // dense areas. but first let's see how inefficient this is. I suspect it's not too - // bad and the gains in simplicity are considerable. final double radiusDeg = SphericalDistanceLibrary.metersToDegrees(MAX_SEARCH_RADIUS_METERS); - Envelope env = new Envelope(vertex.getCoordinate()); - // local equirectangular projection final double xscale = Math.cos(vertex.getLat() * Math.PI / 180); - env.expandBy(radiusDeg / xscale, radiusDeg); - double duplicateDeg = SphericalDistanceLibrary.metersToDegrees(DUPLICATE_WAY_EPSILON_METERS); // We sort the list of candidate edges by distance to the stop // This should remove any issues with things coming out of the spatial index in different orders // Then we link to everything that is within DUPLICATE_WAY_EPSILON_METERS of of the best distance // so that we capture back edges and duplicate ways. - // TODO all the code below looks like a good candidate for Java 8 streams and lambdas - List candidateEdges = new ArrayList( - Collections2.filter(idx.query(env), new Predicate() { - - @Override - public boolean apply(StreetEdge edge) { - // note: not filtering by radius here as distance calculation is expensive - // we do that below. - return edge.canTraverse(new TraverseModeSet(TraverseMode.WALK)) && - // only link to edges still in the graph. - edge.getToVertex().getIncoming().contains(edge); - } - }) - ); + List candidateEdges = getCandidateEdges(vertex); // make a map of distances final TIntDoubleMap distances = new TIntDoubleHashMap(); @@ -145,19 +166,8 @@ public boolean apply(StreetEdge edge) { } // sort the list - Collections.sort(candidateEdges, new Comparator () { - - @Override - public int compare(StreetEdge o1, StreetEdge o2) { - double diff = distances.get(o1.getId()) - distances.get(o2.getId()); - if (diff < 0) - return -1; - if (diff > 0) - return 1; - return 0; - } - - }); + Collections.sort(candidateEdges, (o1, o2) -> + Double.compare(distances.get(o1.getId()), distances.get(o2.getId()))); // find the closest candidate edges if (candidateEdges.isEmpty() || distances.get(candidateEdges.get(0).getId()) > radiusDeg) From cd46228f79d1a22cc7f5f01095ec2a17845b8060 Mon Sep 17 00:00:00 2001 From: Pieter Buzing Date: Tue, 24 May 2016 17:19:34 +0200 Subject: [PATCH 2/4] Style and indentation fixes --- .../linking/SimpleStreetSplitter.java | 71 ++++++++++++------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java b/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java index 6a951c13d48..dc43d9f8d8c 100644 --- a/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java +++ b/src/main/java/org/opentripplanner/graph_builder/linking/SimpleStreetSplitter.java @@ -44,15 +44,15 @@ /** * This class links transit stops to streets by splitting the streets (unless the stop is extremely close to the street * intersection). - * + *

* It is intended to eventually completely replace the existing stop linking code, which had been through so many * revisions and adaptations to different street and turn representations that it was very glitchy. This new code is * also intended to be deterministic in linking to streets, independent of the order in which the JVM decides to * iterate over Maps and even in the presence of points that are exactly halfway between multiple candidate linking * points. - * + *

* It would be wise to keep this new incarnation of the linking code relatively simple, considering what happened before. - * + *

* See discussion in pull request #1922, follow up issue #1934, and the original issue calling for replacement of * the stop linker, #1305. */ @@ -62,7 +62,9 @@ public class SimpleStreetSplitter { public static final int MAX_SEARCH_RADIUS_METERS = 1000; - /** if there are two ways and the distances to them differ by less than this value, we link to both of them */ + /** + * if there are two ways and the distances to them differ by less than this value, we link to both of them + */ public static final double DUPLICATE_WAY_EPSILON_METERS = 0.001; private Graph graph; @@ -74,9 +76,10 @@ public class SimpleStreetSplitter { /** * Construct a new SimpleStreetSplitter. Be aware that only one SimpleStreetSplitter should be * active on a graph at any given time. + * * @param graph */ - public SimpleStreetSplitter (Graph graph) { + public SimpleStreetSplitter(Graph graph) { this.graph = graph; // build a nice private spatial index, since we're adding and removing edges @@ -87,10 +90,12 @@ public SimpleStreetSplitter (Graph graph) { } } - /** Link all relevant vertices to the street network */ - public void link () { + /** + * Link all relevant vertices to the street network + */ + public void link() { for (Vertex v : graph.getVertices()) { - if (v instanceof TransitStop || v instanceof BikeRentalStationVertex || v instanceof BikeParkVertex) + if (v instanceof TransitStop || v instanceof BikeRentalStationVertex || v instanceof BikeParkVertex) { if (!link(v)) { if (v instanceof TransitStop) LOG.warn(graph.addBuilderAnnotation(new StopUnlinked((TransitStop) v))); @@ -98,7 +103,7 @@ else if (v instanceof BikeRentalStationVertex) LOG.warn(graph.addBuilderAnnotation(new BikeRentalStationUnlinked((BikeRentalStationVertex) v))); else if (v instanceof BikeParkVertex) LOG.warn(graph.addBuilderAnnotation(new BikeParkUnlinked((BikeParkVertex) v))); - }; + } } } } @@ -193,8 +198,10 @@ public boolean link(Vertex vertex) { return true; } - /** split the edge and link in the transit stop */ - private void link (Vertex tstop, StreetEdge edge, double xscale) { + /** + * split the edge and link in the transit stop + */ + private void link(Vertex tstop, StreetEdge edge, double xscale) { // TODO: we've already built this line string, we should save it LineString orig = edge.getGeometry(); LineString transformed = equirectangularProject(orig, xscale); @@ -217,17 +224,17 @@ else if (ll.getSegmentIndex() == orig.getNumPoints() - 1) { // nPoints - 2: -1 to correct for index vs count, -1 to account for fencepost problem else if (ll.getSegmentIndex() == orig.getNumPoints() - 2 && ll.getSegmentFraction() > 1 - 1e-8) { makeLinkEdges(tstop, (StreetVertex) edge.getToVertex()); - } - - else { + } else { // split the edge, get the split vertex SplitterVertex v0 = split(edge, ll); makeLinkEdges(tstop, v0); } } - /** Split the street edge at the given fraction */ - private SplitterVertex split (StreetEdge edge, LinearLocation ll) { + /** + * Split the street edge at the given fraction + */ + private SplitterVertex split(StreetEdge edge, LinearLocation ll) { LineString geometry = edge.getGeometry(); // create the geometries @@ -254,9 +261,11 @@ private SplitterVertex split (StreetEdge edge, LinearLocation ll) { return v; } - /** Make the appropriate type of link edges from a vertex */ - private void makeLinkEdges (Vertex from, StreetVertex to) { - if (from instanceof TransitStop) + /** + * Make the appropriate type of link edges from a vertex + */ + private void makeLinkEdges(Vertex from, StreetVertex to) { + if (from instanceof TransitStop) makeTransitLinkEdges((TransitStop) from, to); else if (from instanceof BikeRentalStationVertex) makeBikeRentalLinkEdges((BikeRentalStationVertex) from, to); @@ -264,7 +273,9 @@ else if (from instanceof BikeParkVertex) makeBikeParkEdges((BikeParkVertex) from, to); } - /** Make bike park edges */ + /** + * Make bike park edges + */ private void makeBikeParkEdges(BikeParkVertex from, StreetVertex to) { for (StreetBikeParkLink sbpl : Iterables.filter(from.getOutgoing(), StreetBikeParkLink.class)) { if (sbpl.getToVertex() == to) @@ -275,10 +286,10 @@ private void makeBikeParkEdges(BikeParkVertex from, StreetVertex to) { new StreetBikeParkLink(to, from); } - /** + /** * Make street transit link edges, unless they already exist. */ - private void makeTransitLinkEdges (TransitStop tstop, StreetVertex v) { + private void makeTransitLinkEdges(TransitStop tstop, StreetVertex v) { // ensure that the requisite edges do not already exist // this can happen if we link to duplicate ways that have the same start/end vertices. for (StreetTransitLink e : Iterables.filter(tstop.getOutgoing(), StreetTransitLink.class)) { @@ -290,8 +301,10 @@ private void makeTransitLinkEdges (TransitStop tstop, StreetVertex v) { new StreetTransitLink(v, tstop, tstop.hasWheelchairEntrance()); } - /** Make link edges for bike rental */ - private void makeBikeRentalLinkEdges (BikeRentalStationVertex from, StreetVertex to) { + /** + * Make link edges for bike rental + */ + private void makeBikeRentalLinkEdges(BikeRentalStationVertex from, StreetVertex to) { for (StreetBikeRentalLink sbrl : Iterables.filter(from.getOutgoing(), StreetBikeRentalLink.class)) { if (sbrl.getToVertex() == to) return; @@ -301,14 +314,18 @@ private void makeBikeRentalLinkEdges (BikeRentalStationVertex from, StreetVertex new StreetBikeRentalLink(to, from); } - /** projected distance from stop to edge, in latitude degrees */ - private static double distance (Vertex tstop, StreetEdge edge, double xscale) { + /** + * projected distance from stop to edge, in latitude degrees + */ + private static double distance(Vertex tstop, StreetEdge edge, double xscale) { // use JTS internal tools wherever possible LineString transformed = equirectangularProject(edge.getGeometry(), xscale); return transformed.distance(geometryFactory.createPoint(new Coordinate(tstop.getLon() * xscale, tstop.getLat()))); } - /** project this linestring to an equirectangular projection */ + /** + * project this linestring to an equirectangular projection + */ private static LineString equirectangularProject(LineString geometry, double xscale) { Coordinate[] coords = new Coordinate[geometry.getNumPoints()]; From cf7de0646b73865c0044114b4c6c5a3da8405367 Mon Sep 17 00:00:00 2001 From: Pieter Buzing Date: Tue, 24 May 2016 17:21:42 +0200 Subject: [PATCH 3/4] Improve efficiency of linker by storing lowercase Working on #2170 I found that removing these String.toLowerCase() calls gives a 10% speedup. --- .../graph_builder/module/osm/OSMSpecifier.java | 6 +++--- .../opentripplanner/openstreetmap/model/OSMWithTags.java | 7 +++---- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java index 060512c9365..287312bcced 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java @@ -41,7 +41,7 @@ public OSMSpecifier(String spec) { } public void setKvpairs(String spec) { - String[] pairs = spec.split(";"); + String[] pairs = spec.toLowerCase().split(";"); for (String pair : pairs) { String[] kv = pair.split("="); kvpairs.add(new P2(kv[0], kv[1])); @@ -64,8 +64,8 @@ public P2 matchScores(OSMWithTags match) { for (P2 pair : kvpairs) { // TODO why are we repeatedly converting these to lower case every time they are used? // Probably because it used to be possible to set them from Spring XML. - String tag = pair.first.toLowerCase(); - String value = pair.second.toLowerCase(); + String tag = pair.first; + String value = pair.second; String leftMatchValue = match.getTag(tag + ":left"); String rightMatchValue = match.getTag(tag + ":right"); String matchValue = match.getTag(tag); diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index da431f43a5e..48a934e5e34 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -57,7 +57,7 @@ public void addTag(OSMTag tag) { if (_tags == null) _tags = new HashMap(); - _tags.put(tag.getK().toLowerCase(), tag.getV()); + _tags.put(tag.getK().toLowerCase(), tag.getV().toLowerCase()); } /** @@ -70,7 +70,7 @@ public void addTag(String key, String value) { if (_tags == null) _tags = new HashMap(); - _tags.put(key.toLowerCase(), value); + _tags.put(key.toLowerCase(), value.toLowerCase()); } /** @@ -125,8 +125,7 @@ public boolean doesTagAllowAccess(String tag) { /** @return a tag's value, converted to lower case. */ public String getTag(String tag) { - tag = tag.toLowerCase(); - if (_tags != null && _tags.containsKey(tag)) { + if (_tags != null) { return _tags.get(tag); } return null; From 91692a1ff5f714f95337fa27abaa39c2f07deec5 Mon Sep 17 00:00:00 2001 From: Pieter Buzing Date: Wed, 25 May 2016 14:55:16 +0200 Subject: [PATCH 4/4] Revert "Improve efficiency of linker by storing lowercase" This reverts commit cf7de0646b73865c0044114b4c6c5a3da8405367. --- .../graph_builder/module/osm/OSMSpecifier.java | 6 +++--- .../opentripplanner/openstreetmap/model/OSMWithTags.java | 7 ++++--- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java index 287312bcced..060512c9365 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OSMSpecifier.java @@ -41,7 +41,7 @@ public OSMSpecifier(String spec) { } public void setKvpairs(String spec) { - String[] pairs = spec.toLowerCase().split(";"); + String[] pairs = spec.split(";"); for (String pair : pairs) { String[] kv = pair.split("="); kvpairs.add(new P2(kv[0], kv[1])); @@ -64,8 +64,8 @@ public P2 matchScores(OSMWithTags match) { for (P2 pair : kvpairs) { // TODO why are we repeatedly converting these to lower case every time they are used? // Probably because it used to be possible to set them from Spring XML. - String tag = pair.first; - String value = pair.second; + String tag = pair.first.toLowerCase(); + String value = pair.second.toLowerCase(); String leftMatchValue = match.getTag(tag + ":left"); String rightMatchValue = match.getTag(tag + ":right"); String matchValue = match.getTag(tag); diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 48a934e5e34..da431f43a5e 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -57,7 +57,7 @@ public void addTag(OSMTag tag) { if (_tags == null) _tags = new HashMap(); - _tags.put(tag.getK().toLowerCase(), tag.getV().toLowerCase()); + _tags.put(tag.getK().toLowerCase(), tag.getV()); } /** @@ -70,7 +70,7 @@ public void addTag(String key, String value) { if (_tags == null) _tags = new HashMap(); - _tags.put(key.toLowerCase(), value.toLowerCase()); + _tags.put(key.toLowerCase(), value); } /** @@ -125,7 +125,8 @@ public boolean doesTagAllowAccess(String tag) { /** @return a tag's value, converted to lower case. */ public String getTag(String tag) { - if (_tags != null) { + tag = tag.toLowerCase(); + if (_tags != null && _tags.containsKey(tag)) { return _tags.get(tag); } return null;