From 7fe1f3ee7486b1bcdcecc8cfa777d80e6a585f66 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 19 Mar 2024 23:07:42 +0100 Subject: [PATCH 01/30] Make more classes package private --- .../opentripplanner/graph_builder/module/osm/DisjointSet.java | 2 +- .../java/org/opentripplanner/graph_builder/module/osm/Ring.java | 2 +- .../graph_builder/module/osm/WalkableAreaBuilder.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/DisjointSet.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/DisjointSet.java index a5b9179f696..15bfc3605b5 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/DisjointSet.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/DisjointSet.java @@ -12,7 +12,7 @@ import org.opentripplanner.framework.collection.MapUtils; /** Basic union-find data structure with path compression */ -public class DisjointSet { +class DisjointSet { TIntList sets = new TIntArrayList(); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/Ring.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/Ring.java index 9804c2d646f..754bdbc36b2 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/Ring.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/Ring.java @@ -19,7 +19,7 @@ import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.openstreetmap.model.OSMNode; -public class Ring { +class Ring { private final LinearRing shell; private final List holes = new ArrayList<>(); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java index 5562f1df1a3..b83673c239b 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java @@ -68,7 +68,7 @@ * number of edges for an area wouldn't be determined by the nodes. The current approach can lead * to an excessive number of edges, or to no edges at all if maxAreaNodes is surpassed. */ -public class WalkableAreaBuilder { +class WalkableAreaBuilder { private final DataImportIssueStore issueStore; From debb6eb2a9370064b037d19146ee87f0bc07aa2e Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 20 Mar 2024 14:53:23 +0100 Subject: [PATCH 02/30] Use instance method for figuring out bogus name --- .../graph_builder/module/osm/OsmModule.java | 7 ++----- .../module/osm/WalkableAreaBuilder.java | 18 ++++++------------ .../openstreetmap/model/OSMWithTags.java | 7 +++++++ .../openstreetmap/model/OSMWithTagsTest.java | 9 +++++++++ 4 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java index e831d2ada47..0cd53b70fc6 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java @@ -541,11 +541,8 @@ private StreetEdge getEdgeForStreet( .withRoundabout(way.isRoundabout()) .withSlopeOverride(way.getOsmProvider().getWayPropertySet().getSlopeOverride(way)) .withStairs(way.isSteps()) - .withWheelchairAccessible(way.isWheelchairAccessible()); - - if (!way.hasTag("name") && !way.hasTag("ref")) { - seb.withBogusName(true); - } + .withWheelchairAccessible(way.isWheelchairAccessible()) + .withBogusName(way.needsFallbackName()); StreetEdge street = seb.buildAndConnect(); params.edgeNamer().recordEdge(way, street); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java index b83673c239b..5a828f2b689 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java @@ -400,7 +400,7 @@ private void pruneAreaEdges( Set edges, Set edgesToKeep ) { - if (edges.size() == 0) return; + if (edges.isEmpty()) return; StreetMode mode; StreetEdge firstEdge = (StreetEdge) edges.iterator().next(); @@ -496,7 +496,7 @@ private Set createSegments( } // do we need to recurse? if (intersects.size() == 1) { - Area area = intersects.get(0); + Area area = intersects.getFirst(); OSMWithTags areaEntity = area.parent; StreetTraversalPermission areaPermissions = areaEntity.overridePermissions( @@ -531,11 +531,8 @@ private Set createSegments( .withPermission(areaPermissions) .withBack(false) .withArea(edgeList) - .withCarSpeed(carSpeed); - - if (!areaEntity.hasTag("name") && !areaEntity.hasTag("ref")) { - streetEdgeBuilder.withBogusName(true); - } + .withCarSpeed(carSpeed) + .withBogusName(areaEntity.needsFallbackName()); streetEdgeBuilder.withWheelchairAccessible(areaEntity.isWheelchairAccessible()); @@ -559,11 +556,8 @@ private Set createSegments( .withPermission(areaPermissions) .withBack(true) .withArea(edgeList) - .withCarSpeed(carSpeed); - - if (!areaEntity.hasTag("name") && !areaEntity.hasTag("ref")) { - backStreetEdgeBuilder.withBogusName(true); - } + .withCarSpeed(carSpeed) + .withBogusName(areaEntity.needsFallbackName()); backStreetEdgeBuilder.withWheelchairAccessible(areaEntity.isWheelchairAccessible()); diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 37757823362..5128fb49d3f 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -576,6 +576,13 @@ public boolean isWheelchairAccessible() { return !isTagFalse("wheelchair"); } + /** + * Does this entity has a name of its own or if it needs to have a fallback one assigned? + */ + public boolean needsFallbackName() { + return !hasTag("name") && !hasTag("ref"); + } + /** * Returns true if this tag is explicitly access to this entity. */ diff --git a/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java b/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java index ca5db77df12..18d92a5eec8 100644 --- a/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java +++ b/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java @@ -262,4 +262,13 @@ void testGenerateI18NForPattern() { osmTags.generateI18NForPattern("Note: {note}, {wheelchair:description}, {foobar:description}") ); } + + @Test + void fallbackName() { + var nameless = WayTestData.cycleway(); + assertTrue(nameless.needsFallbackName()); + + var namedTunnel = WayTestData.carTunnel(); + assertFalse(namedTunnel.needsFallbackName()); + } } From f2d5e4a5bd7aebdf0a520d5b5fd98228571ff8fc Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 20 Mar 2024 15:37:35 +0100 Subject: [PATCH 03/30] Improve debugging of edge names --- .../vector/edge/EdgePropertyMapper.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java b/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java index fb65d0b5d3b..704bf961dfa 100644 --- a/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java +++ b/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java @@ -3,6 +3,7 @@ import static org.opentripplanner.framework.lang.DoubleUtils.roundTo2Decimals; import static org.opentripplanner.inspector.vector.KeyValue.kv; +import com.google.common.collect.Lists; import java.util.Collection; import java.util.List; import org.opentripplanner.apis.support.mapping.PropertyMapper; @@ -16,16 +17,26 @@ public class EdgePropertyMapper extends PropertyMapper { @Override protected Collection map(Edge input) { - List baseProps = List.of(kv("class", input.getClass().getSimpleName())); + var baseProps = List.of(kv("class", input.getClass().getSimpleName())); List properties = switch (input) { - case StreetEdge e -> List.of( - kv("permission", e.getPermission().toString()), - kv("bicycleSafetyFactor", roundTo2Decimals(e.getBicycleSafetyFactor())) - ); + case StreetEdge e -> mapStreetEdge(e); case EscalatorEdge e -> List.of(kv("distance", e.getDistanceMeters())); default -> List.of(); }; return ListUtils.combine(baseProps, properties); } + + private static List mapStreetEdge(StreetEdge se) { + var props = Lists.newArrayList( + kv("permission", se.getPermission().toString()), + kv("bicycleSafetyFactor", roundTo2Decimals(se.getBicycleSafetyFactor())) + ); + if (se.hasBogusName()) { + props.addFirst(kv("generated name", "%s".formatted(se.getName().toString()))); + } else { + props.addFirst(kv("name", se.getName().toString())); + } + return props; + } } From ea94f83770cf7c6970a0157aec13b0504db2c936 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 20 Mar 2024 15:43:44 +0100 Subject: [PATCH 04/30] Simplify builder chains --- .../module/osm/WalkableAreaBuilder.java | 16 ++++++---------- .../openstreetmap/model/OSMWay.java | 4 ++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java index 5a828f2b689..6244a50321f 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java @@ -532,11 +532,9 @@ private Set createSegments( .withBack(false) .withArea(edgeList) .withCarSpeed(carSpeed) - .withBogusName(areaEntity.needsFallbackName()); - - streetEdgeBuilder.withWheelchairAccessible(areaEntity.isWheelchairAccessible()); - - streetEdgeBuilder.withLink(areaEntity.isLink()); + .withBogusName(areaEntity.needsFallbackName()) + .withWheelchairAccessible(areaEntity.isWheelchairAccessible()) + .withLink(areaEntity.isLink()); label = "way (area) " + @@ -557,11 +555,9 @@ private Set createSegments( .withBack(true) .withArea(edgeList) .withCarSpeed(carSpeed) - .withBogusName(areaEntity.needsFallbackName()); - - backStreetEdgeBuilder.withWheelchairAccessible(areaEntity.isWheelchairAccessible()); - - backStreetEdgeBuilder.withLink(areaEntity.isLink()); + .withBogusName(areaEntity.needsFallbackName()) + .withWheelchairAccessible(areaEntity.isWheelchairAccessible()) + .withLink(areaEntity.isLink()); if (!wayPropertiesCache.containsKey(areaEntity)) { WayProperties wayData = areaEntity diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWay.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWay.java index 1ce6c698f22..c0cd049bd83 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWay.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWay.java @@ -60,6 +60,10 @@ public boolean isSteps() { return isTag("highway", "steps"); } + /** + * Checks the wheelchair-accessibility of this way. Stairs are by default inaccessible but + * can be made accessible if they explicitly set wheelchair=true. + */ public boolean isWheelchairAccessible() { if (isSteps()) { return isTagTrue("wheelchair"); From c25c12ab69da21a7ca45255530e6c412210caf0e Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 21 Mar 2024 17:19:33 +0100 Subject: [PATCH 05/30] Add first draft of edge naming --- .../apis/vectortiles/DebugStyleSpec.java | 27 +++++-- .../apis/vectortiles/model/StyleBuilder.java | 20 +++++ .../apis/vectortiles/model/StyleSpec.java | 5 ++ .../module/osm/naming/SidewalkNamer.java | 77 +++++++++++++++++++ .../graph_builder/services/osm/EdgeNamer.java | 4 +- .../vector/edge/EdgePropertyMapper.java | 2 +- .../openstreetmap/model/OSMWithTags.java | 20 ++++- 7 files changed, 142 insertions(+), 13 deletions(-) create mode 100644 src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java index 8350a10d670..1558346493a 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java @@ -13,6 +13,7 @@ import org.opentripplanner.service.vehiclerental.street.StreetVehicleRentalLink; import org.opentripplanner.street.model.edge.AreaEdge; import org.opentripplanner.street.model.edge.BoardingLocationToStopLink; +import org.opentripplanner.street.model.edge.Edge; import org.opentripplanner.street.model.edge.ElevatorHopEdge; import org.opentripplanner.street.model.edge.EscalatorEdge; import org.opentripplanner.street.model.edge.PathwayEdge; @@ -49,6 +50,13 @@ public class DebugStyleSpec { 1, List.of(new ZoomStop(15, 0.2f), new ZoomStop(MAX_ZOOM, 3)) ); + private static final Class[] ALL_EDGES = new Class[]{StreetEdge.class, + AreaEdge.class, + EscalatorEdge.class, + PathwayEdge.class, + ElevatorHopEdge.class, + TemporaryPartialStreetEdge.class, + TemporaryFreeEdge.class}; static StyleSpec build( VectorSourceLayer regularStops, @@ -74,18 +82,23 @@ static StyleSpec build( .vectorSourceLayer(edges) .lineColor(MAGENTA) .edgeFilter( - StreetEdge.class, - AreaEdge.class, - EscalatorEdge.class, - PathwayEdge.class, - ElevatorHopEdge.class, - TemporaryPartialStreetEdge.class, - TemporaryFreeEdge.class + ALL_EDGES ) .lineWidth(LINE_WIDTH) .minZoom(13) .maxZoom(MAX_ZOOM) .intiallyHidden(), + StyleBuilder + .ofId("edge-name") + .typeSymbol() + .lineText("name") + .vectorSourceLayer(edges) + .edgeFilter( + ALL_EDGES + ) + .minZoom(15) + .maxZoom(MAX_ZOOM) + .intiallyHidden(), StyleBuilder .ofId("link") .typeLine() diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java index 07efe376968..c7f259b0c9b 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java @@ -8,6 +8,7 @@ import java.util.Map; import java.util.Objects; import java.util.stream.Stream; +import org.opentripplanner.apis.vectortiles.model.ZoomDependentNumber.ZoomStop; import org.opentripplanner.framework.collection.ListUtils; import org.opentripplanner.framework.json.ObjectMappers; import org.opentripplanner.street.model.edge.Edge; @@ -41,6 +42,7 @@ public enum LayerType { Line, Raster, Fill, + Symbol } private StyleBuilder(String id) { @@ -94,11 +96,29 @@ public StyleBuilder typeFill() { return this; } + public StyleBuilder typeSymbol() { + type(LayerType.Symbol); + return this; + } + private StyleBuilder type(LayerType type) { props.put(TYPE, type.name().toLowerCase()); return this; } + public StyleBuilder lineText(String name){ + layout.put("symbol-placement", "line"); + layout.put("text-field", "{%s}".formatted(name)); + layout.put("text-font", List.of("KlokanTech Noto Sans Regular")); + layout.put("text-size", new ZoomDependentNumber(14, List.of(new ZoomStop(14, 12), new ZoomStop(20, 14))).toJson()); + layout.put("text-offset", List.of(0, 0.9)); + paint.put("text-color", "#000"); + paint.put("text-halo-color", "#fff"); + paint.put("text-halo-blur", 4); + paint.put("text-halo-width", 3); + return this; + } + public StyleBuilder circleColor(String color) { paint.put("circle-color", validateColor(color)); return this; diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleSpec.java b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleSpec.java index 64f680ed202..71e5df9d335 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleSpec.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleSpec.java @@ -46,4 +46,9 @@ public Map sources() { public List layers() { return layers; } + + @JsonSerialize + public String glyphs() { + return "https://cdn.jsdelivr.net/gh/klokantech/klokantech-gl-fonts@master/{fontstack}/{range}.pbf"; + } } diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java new file mode 100644 index 00000000000..e32df244530 --- /dev/null +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -0,0 +1,77 @@ +package org.opentripplanner.graph_builder.module.osm.naming; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Comparator; +import java.util.List; +import org.locationtech.jts.algorithm.distance.DiscreteHausdorffDistance; +import org.locationtech.jts.index.SpatialIndex; +import org.locationtech.jts.index.quadtree.Quadtree; +import org.opentripplanner.framework.i18n.I18NString; +import org.opentripplanner.framework.logging.ProgressTracker; +import org.opentripplanner.graph_builder.services.osm.EdgeNamer; +import org.opentripplanner.openstreetmap.model.OSMWithTags; +import org.opentripplanner.street.model.edge.StreetEdge; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class SidewalkNamer implements EdgeNamer { + + private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamer.class); + + private SpatialIndex streetEdges = new Quadtree(); + private Collection unnamedSidewalks = new ArrayList<>(); + + @Override + public I18NString name(OSMWithTags way) { + return way.getAssumedName(); + } + + @Override + public void recordEdge(OSMWithTags way, StreetEdge edge) { + if (way.isSidewalk() && way.needsFallbackName()) { + unnamedSidewalks.add(edge); + } + if (way.isNamed()) { + streetEdges.insert(edge.getGeometry().getEnvelopeInternal(), edge); + } + } + + @Override + public void postprocess() { + ProgressTracker progress = ProgressTracker.track( + "Assigning names to sidewalks", + 500, + unnamedSidewalks.size() + ); + unnamedSidewalks + .parallelStream() + .forEach(sidewalk -> { + var envelope = sidewalk.getGeometry().getEnvelopeInternal(); + envelope.expandBy(0.0002); + var candidates = (List) streetEdges.query(envelope); + + candidates + .stream() + .min(lowestHausdorffDistance(sidewalk)) + .ifPresent(named -> { + sidewalk.setName(named.getName()); + }); + + //Keep lambda! A method-ref would cause incorrect class and line number to be logged + //noinspection Convert2MethodRef + progress.step(m -> LOG.info(m)); + }); + LOG.info(progress.completeMessage()); + + // set the indices to null so they can be garbage-collected + streetEdges = null; + unnamedSidewalks = null; + } + + private static Comparator lowestHausdorffDistance(StreetEdge sidewalk) { + return Comparator.comparingDouble(candidate -> + DiscreteHausdorffDistance.distance(sidewalk.getGeometry(), candidate.getGeometry()) + ); + } +} diff --git a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java index d9e52a18465..2733e705c70 100644 --- a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java @@ -5,6 +5,7 @@ import org.opentripplanner.framework.i18n.NonLocalizedString; import org.opentripplanner.graph_builder.module.osm.naming.DefaultNamer; import org.opentripplanner.graph_builder.module.osm.naming.PortlandCustomNamer; +import org.opentripplanner.graph_builder.module.osm.naming.SidewalkNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; import org.opentripplanner.standalone.config.framework.json.NodeAdapter; import org.opentripplanner.standalone.config.framework.json.OtpVersion; @@ -50,7 +51,7 @@ public static EdgeNamer fromConfig(NodeAdapter root, String parameterName) { var osmNaming = root .of(parameterName) .summary("A custom OSM namer to use.") - .since(OtpVersion.V2_0) + .since(OtpVersion.V1_5) .asString(null); return fromConfig(osmNaming); } @@ -65,6 +66,7 @@ public static EdgeNamer fromConfig(String type) { return switch (type) { case "portland" -> new PortlandCustomNamer(); + case "sidewalks" -> new SidewalkNamer(); default -> throw new IllegalArgumentException( String.format("Unknown osmNaming type: '%s'", type) ); diff --git a/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java b/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java index 704bf961dfa..d43a91d384d 100644 --- a/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java +++ b/src/main/java/org/opentripplanner/inspector/vector/edge/EdgePropertyMapper.java @@ -33,7 +33,7 @@ private static List mapStreetEdge(StreetEdge se) { kv("bicycleSafetyFactor", roundTo2Decimals(se.getBicycleSafetyFactor())) ); if (se.hasBogusName()) { - props.addFirst(kv("generated name", "%s".formatted(se.getName().toString()))); + props.addFirst(kv("name", "%s (generated)".formatted(se.getName().toString()))); } else { props.addFirst(kv("name", se.getName().toString())); } diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 5128fb49d3f..a4922400b04 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -30,7 +30,7 @@ public class OSMWithTags { /** * highway=* values that we don't want to even consider when building the graph. */ - public static final Set NON_ROUTABLE_HIGHWAYS = Set.of( + private static final Set NON_ROUTABLE_HIGHWAYS = Set.of( "proposed", "planned", "construction", @@ -46,7 +46,8 @@ public class OSMWithTags { "escape" ); - static final Set LEVEL_TAGS = Set.of("level", "layer"); + private static final Set LEVEL_TAGS = Set.of("level", "layer"); + private static final Set DEFAULT_LEVEL = Set.of("0"); /* To save memory this is only created when an entity actually has tags. */ private Map tags; @@ -163,6 +164,10 @@ public boolean isBicycleDismountForced() { return isTag("bicycle", "dismount"); } + public boolean isSidewalk() { + return isTag("footway", "sidewalk") && isTag("highway", "footway"); + } + protected boolean doesTagAllowAccess(String tag) { if (tags == null) { return false; @@ -580,7 +585,14 @@ public boolean isWheelchairAccessible() { * Does this entity has a name of its own or if it needs to have a fallback one assigned? */ public boolean needsFallbackName() { - return !hasTag("name") && !hasTag("ref"); + return !isNamed(); + } + + /** + * Does this entity have tags that allow extracting a name? + */ + public boolean isNamed() { + return hasTag("name") || hasTag("ref"); } /** @@ -600,7 +612,7 @@ public Set getLevels() { var levels = getMultiTagValues(LEVEL_TAGS); if (levels.isEmpty()) { // default - return Set.of("0"); + return DEFAULT_LEVEL; } return levels; } From bac74c97ccfba54360636d49d0ce6175b043e0e4 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 25 Mar 2024 09:58:25 +0100 Subject: [PATCH 06/30] Finetune sidewalk naming --- .../apis/vectortiles/DebugStyleSpec.java | 16 ++-- .../apis/vectortiles/model/StyleBuilder.java | 14 +++- .../module/osm/naming/SidewalkNamer.java | 73 +++++++++++++++++-- .../openstreetmap/model/OSMWithTags.java | 7 ++ 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java index 1558346493a..16e3b247bc7 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java @@ -50,13 +50,15 @@ public class DebugStyleSpec { 1, List.of(new ZoomStop(15, 0.2f), new ZoomStop(MAX_ZOOM, 3)) ); - private static final Class[] ALL_EDGES = new Class[]{StreetEdge.class, + private static final Class[] ALL_EDGES = new Class[] { + StreetEdge.class, AreaEdge.class, EscalatorEdge.class, PathwayEdge.class, ElevatorHopEdge.class, TemporaryPartialStreetEdge.class, - TemporaryFreeEdge.class}; + TemporaryFreeEdge.class, + }; static StyleSpec build( VectorSourceLayer regularStops, @@ -81,9 +83,7 @@ static StyleSpec build( .typeLine() .vectorSourceLayer(edges) .lineColor(MAGENTA) - .edgeFilter( - ALL_EDGES - ) + .edgeFilter(ALL_EDGES) .lineWidth(LINE_WIDTH) .minZoom(13) .maxZoom(MAX_ZOOM) @@ -93,10 +93,8 @@ static StyleSpec build( .typeSymbol() .lineText("name") .vectorSourceLayer(edges) - .edgeFilter( - ALL_EDGES - ) - .minZoom(15) + .edgeFilter(ALL_EDGES) + .minZoom(17) .maxZoom(MAX_ZOOM) .intiallyHidden(), StyleBuilder diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java index c7f259b0c9b..28c1e792fd1 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/model/StyleBuilder.java @@ -42,7 +42,7 @@ public enum LayerType { Line, Raster, Fill, - Symbol + Symbol, } private StyleBuilder(String id) { @@ -106,12 +106,18 @@ private StyleBuilder type(LayerType type) { return this; } - public StyleBuilder lineText(String name){ + public StyleBuilder lineText(String name) { layout.put("symbol-placement", "line"); + layout.put("symbol-spacing", 500); layout.put("text-field", "{%s}".formatted(name)); layout.put("text-font", List.of("KlokanTech Noto Sans Regular")); - layout.put("text-size", new ZoomDependentNumber(14, List.of(new ZoomStop(14, 12), new ZoomStop(20, 14))).toJson()); - layout.put("text-offset", List.of(0, 0.9)); + layout.put( + "text-size", + new ZoomDependentNumber(14, List.of(new ZoomStop(14, 12), new ZoomStop(20, 14))).toJson() + ); + layout.put("text-max-width", 5); + layout.put("text-keep-upright", true); + layout.put("text-rotation-alignment", "map"); paint.put("text-color", "#000"); paint.put("text-halo-color", "#fff"); paint.put("text-halo-blur", 4); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index e32df244530..fa988df6032 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -4,9 +4,13 @@ import java.util.Collection; import java.util.Comparator; import java.util.List; +import java.util.Objects; import org.locationtech.jts.algorithm.distance.DiscreteHausdorffDistance; import org.locationtech.jts.index.SpatialIndex; -import org.locationtech.jts.index.quadtree.Quadtree; +import org.locationtech.jts.operation.distance.DistanceOp; +import org.opentripplanner.framework.geometry.HashGridSpatialIndex; +import org.opentripplanner.framework.geometry.SphericalDistanceLibrary; +import org.opentripplanner.framework.geometry.WgsCoordinate; import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.logging.ProgressTracker; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; @@ -18,8 +22,9 @@ public class SidewalkNamer implements EdgeNamer { private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamer.class); + private static final int MAX_DISTANCE_TO_SIDEWALK = 50; - private SpatialIndex streetEdges = new Quadtree(); + private SpatialIndex streetEdges = new HashGridSpatialIndex(); private Collection unnamedSidewalks = new ArrayList<>(); @Override @@ -29,7 +34,7 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdge(OSMWithTags way, StreetEdge edge) { - if (way.isSidewalk() && way.needsFallbackName()) { + if (way.isSidewalk() && way.needsFallbackName() && !way.isExplicitlyUnnamed()) { unnamedSidewalks.add(edge); } if (way.isNamed()) { @@ -44,24 +49,45 @@ public void postprocess() { 500, unnamedSidewalks.size() ); + + List edges = new ArrayList<>(); unnamedSidewalks .parallelStream() .forEach(sidewalk -> { var envelope = sidewalk.getGeometry().getEnvelopeInternal(); - envelope.expandBy(0.0002); + envelope.expandBy(0.000002); var candidates = (List) streetEdges.query(envelope); candidates .stream() - .min(lowestHausdorffDistance(sidewalk)) + .map(c -> { + var hausdorff = DiscreteHausdorffDistance.distance( + sidewalk.getGeometry(), + c.getGeometry(),0.5 + ); + + var points = DistanceOp.nearestPoints( c.getGeometry(),sidewalk.getGeometry()); + double fastDistance = SphericalDistanceLibrary.fastDistance(points[0], points[1]); + + return new EdgeWithDistance(hausdorff, fastDistance, candidates.size(), c, sidewalk); + }) + .filter(e -> e.distance < MAX_DISTANCE_TO_SIDEWALK) + .min(Comparator.comparingDouble(EdgeWithDistance::hausdorff)) .ifPresent(named -> { - sidewalk.setName(named.getName()); + edges.add(named); + sidewalk.setName(Objects.requireNonNull(named.namedEdge.getName())); }); //Keep lambda! A method-ref would cause incorrect class and line number to be logged //noinspection Convert2MethodRef progress.step(m -> LOG.info(m)); }); + + edges + .stream() + .sorted(Comparator.comparingDouble(EdgeWithDistance::hausdorff).reversed()) + .limit(100) + .forEach(EdgeWithDistance::logDebugString); LOG.info(progress.completeMessage()); // set the indices to null so they can be garbage-collected @@ -74,4 +100,39 @@ private static Comparator lowestHausdorffDistance(StreetEdge sidewal DiscreteHausdorffDistance.distance(sidewalk.getGeometry(), candidate.getGeometry()) ); } + + record EdgeWithDistance( + double hausdorff, + double distance, + int numberOfCandidates, + StreetEdge namedEdge, + StreetEdge sidewalk + ) { + void logDebugString() { + LOG.info("Name '{}' applied with low Hausdorff distance ", namedEdge.getName()); + LOG.info("Hausdorff: {}", hausdorff); + LOG.info("Distance: {}m", distance); + LOG.info("OSM: {}", osmUrl()); + LOG.info("Debug client: {}", debugClientUrl()); + LOG.info("<-------------------------------------------------------------------------------->"); + } + + String debugClientUrl() { + var c = new WgsCoordinate(sidewalk.getFromVertex().getCoordinate()); + return "http://localhost:8080/debug-client-preview/#19/%s/%s".formatted( + c.latitude(), + c.longitude() + ); + } + + String osmUrl() { + var c = new WgsCoordinate(sidewalk.getFromVertex().getCoordinate()); + return "https://www.openstreetmap.org/?mlat=%s&mlon=%s#map=17/%s/%s".formatted( + c.latitude(), + c.longitude(), + c.latitude(), + c.longitude() + ); + } + } } diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index a4922400b04..056acafb5d6 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -168,6 +168,13 @@ public boolean isSidewalk() { return isTag("footway", "sidewalk") && isTag("highway", "footway"); } + /** + * Whether this entity explicity doesn't have a name. + */ + public boolean isExplicitlyUnnamed() { + return isTagTrue("noname"); + } + protected boolean doesTagAllowAccess(String tag) { if (tags == null) { return false; From 22e102a77928283d994c2d0e3d3616e4bf77eb4a Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 25 Mar 2024 10:04:32 +0100 Subject: [PATCH 07/30] Finetune sidewalk naming --- .../graph_builder/module/osm/naming/SidewalkNamer.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index fa988df6032..30696a548f7 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -63,10 +63,11 @@ public void postprocess() { .map(c -> { var hausdorff = DiscreteHausdorffDistance.distance( sidewalk.getGeometry(), - c.getGeometry(),0.5 + c.getGeometry(), + 0.5 ); - var points = DistanceOp.nearestPoints( c.getGeometry(),sidewalk.getGeometry()); + var points = DistanceOp.nearestPoints(c.getGeometry(), sidewalk.getGeometry()); double fastDistance = SphericalDistanceLibrary.fastDistance(points[0], points[1]); return new EdgeWithDistance(hausdorff, fastDistance, candidates.size(), c, sidewalk); @@ -114,7 +115,9 @@ void logDebugString() { LOG.info("Distance: {}m", distance); LOG.info("OSM: {}", osmUrl()); LOG.info("Debug client: {}", debugClientUrl()); - LOG.info("<-------------------------------------------------------------------------------->"); + LOG.info( + "<-------------------------------------------------------------------------------->" + ); } String debugClientUrl() { From 1f106b14e5e4e7d7dc318b89a9ba1b9b2e13ad89 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 26 Mar 2024 17:20:50 +0100 Subject: [PATCH 08/30] Add documentation --- .../geometry/SphericalDistanceLibrary.java | 14 ++++++++++---- .../module/osm/naming/SidewalkNamer.java | 13 +++++++++---- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opentripplanner/framework/geometry/SphericalDistanceLibrary.java b/src/main/java/org/opentripplanner/framework/geometry/SphericalDistanceLibrary.java index 479163aefd1..38c80e86d79 100644 --- a/src/main/java/org/opentripplanner/framework/geometry/SphericalDistanceLibrary.java +++ b/src/main/java/org/opentripplanner/framework/geometry/SphericalDistanceLibrary.java @@ -30,10 +30,16 @@ public static double distance(Coordinate from, Coordinate to) { return distance(from.y, from.x, to.y, to.x); } + /** + * @see SphericalDistanceLibrary#fastDistance(double, double, double, double) + */ public static double fastDistance(Coordinate from, Coordinate to) { return fastDistance(from.y, from.x, to.y, to.x); } + /** + * @see SphericalDistanceLibrary#fastDistance(double, double, double, double) + */ public static double fastDistance(Coordinate from, Coordinate to, double cosLat) { double dLat = toRadians(from.y - to.y); double dLon = toRadians(from.x - to.x) * cosLat; @@ -105,8 +111,8 @@ public static double distance(double lat1, double lon1, double lat2, double lon2 } /** - * Compute an (approximated) distance between two points, with a known cos(lat). Be careful, this - * is approximated and never check for the validity of input cos(lat). + * Compute an (approximated) distance in meters between two points, with a known cos(lat). + * Be careful, this is approximated and never checks for the validity of input cos(lat). */ public static double fastDistance(double lat1, double lon1, double lat2, double lon2) { return fastDistance(lat1, lon1, lat2, lon2, RADIUS_OF_EARTH_IN_M); @@ -131,8 +137,8 @@ public static double distance(double lat1, double lon1, double lat2, double lon2 } /** - * Approximated, fast and under-estimated equirectangular distance between two points. Works only - * for small delta lat/lon, fall-back on exact distance if not the case. See: + * Approximated, fast and under-estimated equirectangular distance in meters between two points. + * Works only for small delta lat/lon, fall-back on exact distance if not the case. See: * http://www.movable-type.co.uk/scripts/latlong.html */ public static double fastDistance( diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 30696a548f7..495409a62a3 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -68,9 +68,9 @@ public void postprocess() { ); var points = DistanceOp.nearestPoints(c.getGeometry(), sidewalk.getGeometry()); - double fastDistance = SphericalDistanceLibrary.fastDistance(points[0], points[1]); + double distance = SphericalDistanceLibrary.fastDistance(points[0], points[1]); - return new EdgeWithDistance(hausdorff, fastDistance, candidates.size(), c, sidewalk); + return new EdgeWithDistance(hausdorff, distance, candidates.size(), c, sidewalk); }) .filter(e -> e.distance < MAX_DISTANCE_TO_SIDEWALK) .min(Comparator.comparingDouble(EdgeWithDistance::hausdorff)) @@ -84,11 +84,12 @@ public void postprocess() { progress.step(m -> LOG.info(m)); }); - edges + var worst = edges .stream() .sorted(Comparator.comparingDouble(EdgeWithDistance::hausdorff).reversed()) .limit(100) - .forEach(EdgeWithDistance::logDebugString); + .toList(); + worst.forEach(EdgeWithDistance::logDebugString); LOG.info(progress.completeMessage()); // set the indices to null so they can be garbage-collected @@ -109,6 +110,10 @@ record EdgeWithDistance( StreetEdge namedEdge, StreetEdge sidewalk ) { + EdgeWithDistance { + Objects.requireNonNull(namedEdge); + Objects.requireNonNull(sidewalk); + } void logDebugString() { LOG.info("Name '{}' applied with low Hausdorff distance ", namedEdge.getName()); LOG.info("Hausdorff: {}", hausdorff); From 1d83a07ccc6439d4cf4230c11bfe228cade66aa0 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 27 Mar 2024 15:37:03 +0100 Subject: [PATCH 09/30] Fine-tune sidewalk naming algorithm --- .../framework/geometry/GeometryUtils.java | 2 +- .../module/osm/naming/SidewalkNamer.java | 202 ++++++++++++------ .../openstreetmap/model/OSMWithTags.java | 5 +- 3 files changed, 144 insertions(+), 65 deletions(-) diff --git a/src/main/java/org/opentripplanner/framework/geometry/GeometryUtils.java b/src/main/java/org/opentripplanner/framework/geometry/GeometryUtils.java index cf634ad0e23..87a225fc0b3 100644 --- a/src/main/java/org/opentripplanner/framework/geometry/GeometryUtils.java +++ b/src/main/java/org/opentripplanner/framework/geometry/GeometryUtils.java @@ -264,7 +264,7 @@ public static Geometry convertGeoJsonToJtsGeometry(GeoJsonObject geoJsonGeom) } /** - * Extract individual line string from a mult-line string. + * Extract individual line strings from a multi-line string. */ public static List getLineStrings(MultiLineString mls) { var ret = new ArrayList(); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 495409a62a3..8c03eacbd67 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -5,13 +5,27 @@ import java.util.Comparator; import java.util.List; import java.util.Objects; -import org.locationtech.jts.algorithm.distance.DiscreteHausdorffDistance; -import org.locationtech.jts.index.SpatialIndex; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; +import org.geotools.api.referencing.FactoryException; +import org.geotools.api.referencing.crs.CoordinateReferenceSystem; +import org.geotools.api.referencing.operation.MathTransform; +import org.geotools.api.referencing.operation.TransformException; +import org.geotools.geometry.jts.JTS; +import org.geotools.referencing.CRS; +import org.geotools.referencing.crs.DefaultGeographicCRS; +import org.locationtech.jts.geom.Geometry; +import org.locationtech.jts.geom.LineString; +import org.locationtech.jts.geom.MultiLineString; +import org.locationtech.jts.geom.Point; +import org.locationtech.jts.operation.buffer.BufferParameters; import org.locationtech.jts.operation.distance.DistanceOp; +import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.framework.geometry.HashGridSpatialIndex; import org.opentripplanner.framework.geometry.SphericalDistanceLibrary; -import org.opentripplanner.framework.geometry.WgsCoordinate; import org.opentripplanner.framework.i18n.I18NString; +import org.opentripplanner.framework.lang.DoubleUtils; import org.opentripplanner.framework.logging.ProgressTracker; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; @@ -23,9 +37,10 @@ public class SidewalkNamer implements EdgeNamer { private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamer.class); private static final int MAX_DISTANCE_TO_SIDEWALK = 50; + private static final double MIN_PERCENT_IN_BUFFER = .85; - private SpatialIndex streetEdges = new HashGridSpatialIndex(); - private Collection unnamedSidewalks = new ArrayList<>(); + private HashGridSpatialIndex streetEdges = new HashGridSpatialIndex<>(); + private Collection unnamedSidewalks = new ArrayList<>(); @Override public I18NString name(OSMWithTags way) { @@ -35,10 +50,19 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdge(OSMWithTags way, StreetEdge edge) { if (way.isSidewalk() && way.needsFallbackName() && !way.isExplicitlyUnnamed()) { - unnamedSidewalks.add(edge); + unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels())); } - if (way.isNamed()) { - streetEdges.insert(edge.getGeometry().getEnvelopeInternal(), edge); + if (way.isNamed() && !way.isLink()) { + var containsReverse = streetEdges + .query(edge.getGeometry().getEnvelopeInternal()) + .stream() + .anyMatch(candidate -> candidate.edge.isReverseOf(edge)); + if (!containsReverse) { + streetEdges.insert( + edge.getGeometry().getEnvelopeInternal(), + new EdgeOnLevel(edge, way.getLevels()) + ); + } } } @@ -50,33 +74,58 @@ public void postprocess() { unnamedSidewalks.size() ); - List edges = new ArrayList<>(); + final AtomicInteger namesApplied = new AtomicInteger(0); unnamedSidewalks .parallelStream() - .forEach(sidewalk -> { + .forEach(sidewalkOnLevel -> { + var sidewalk = sidewalkOnLevel.edge; var envelope = sidewalk.getGeometry().getEnvelopeInternal(); envelope.expandBy(0.000002); - var candidates = (List) streetEdges.query(envelope); + var candidates = streetEdges.query(envelope); - candidates + var groups = candidates + .stream() + .collect(Collectors.groupingBy(e -> e.edge.getName())) + .entrySet() .stream() - .map(c -> { - var hausdorff = DiscreteHausdorffDistance.distance( - sidewalk.getGeometry(), - c.getGeometry(), - 0.5 + .map(entry -> { + var levels = entry + .getValue() + .stream() + .flatMap(e -> e.levels.stream()) + .collect(Collectors.toSet()); + return new CandidateGroup( + entry.getKey(), + entry.getValue().stream().map(e -> e.edge).toList(), + levels ); + }); - var points = DistanceOp.nearestPoints(c.getGeometry(), sidewalk.getGeometry()); - double distance = SphericalDistanceLibrary.fastDistance(points[0], points[1]); + var buffer = preciseBuffer(sidewalk.getGeometry(), 25); + var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); - return new EdgeWithDistance(hausdorff, distance, candidates.size(), c, sidewalk); + groups + .filter(g -> g.nearestDistanceTo(sidewalk.getGeometry()) < MAX_DISTANCE_TO_SIDEWALK) + .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) + .map(g -> { + var lengthInsideBuffer = g.intersectionLength(buffer); + double percentInBuffer = lengthInsideBuffer / sidewalkLength; + return new NamedEdgeGroup(percentInBuffer, candidates.size(), g.name, sidewalk); }) - .filter(e -> e.distance < MAX_DISTANCE_TO_SIDEWALK) - .min(Comparator.comparingDouble(EdgeWithDistance::hausdorff)) - .ifPresent(named -> { - edges.add(named); - sidewalk.setName(Objects.requireNonNull(named.namedEdge.getName())); + // remove those groups where less than a certain percentage is inside the buffer around + // the sidewalk. this safety mechanism for sidewalks that snake around the + // like https://www.openstreetmap.org/way/1059101564 + .filter(group -> group.percentInBuffer > MIN_PERCENT_IN_BUFFER) + .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) + .ifPresent(group -> { + namesApplied.incrementAndGet(); + var name = group.name.toString(); + + sidewalk.setName( + Objects.requireNonNull( + I18NString.of("%s, Percent in buffer: %s".formatted(name, group.percentInBuffer)) + ) + ); }); //Keep lambda! A method-ref would cause incorrect class and line number to be logged @@ -84,12 +133,13 @@ public void postprocess() { progress.step(m -> LOG.info(m)); }); - var worst = edges - .stream() - .sorted(Comparator.comparingDouble(EdgeWithDistance::hausdorff).reversed()) - .limit(100) - .toList(); - worst.forEach(EdgeWithDistance::logDebugString); + LOG.info( + "Assigned names to {} of {} of sidewalks ({})", + namesApplied.get(), + unnamedSidewalks.size(), + DoubleUtils.roundTo2Decimals((double) namesApplied.get() / unnamedSidewalks.size() * 100) + ); + LOG.info(progress.completeMessage()); // set the indices to null so they can be garbage-collected @@ -97,50 +147,76 @@ public void postprocess() { unnamedSidewalks = null; } - private static Comparator lowestHausdorffDistance(StreetEdge sidewalk) { - return Comparator.comparingDouble(candidate -> - DiscreteHausdorffDistance.distance(sidewalk.getGeometry(), candidate.getGeometry()) - ); + /** + * Taken from https://stackoverflow.com/questions/36455020 + */ + private Geometry preciseBuffer(Geometry geometry, double distanceInMeters) { + try { + var coordinate = geometry.getCentroid().getCoordinate(); + String code = "AUTO:42001,%s,%s".formatted(coordinate.x, coordinate.y); + CoordinateReferenceSystem auto = CRS.decode(code); + + MathTransform toTransform = CRS.findMathTransform(DefaultGeographicCRS.WGS84, auto); + MathTransform fromTransform = CRS.findMathTransform(auto, DefaultGeographicCRS.WGS84); + + Geometry pGeom = JTS.transform(geometry, toTransform); + + Geometry pBufferedGeom = pGeom.buffer(distanceInMeters, 4, BufferParameters.CAP_FLAT); + return JTS.transform(pBufferedGeom, fromTransform); + } catch (TransformException | FactoryException e) { + throw new RuntimeException(e); + } } - record EdgeWithDistance( - double hausdorff, - double distance, + record NamedEdgeGroup( + double percentInBuffer, int numberOfCandidates, - StreetEdge namedEdge, + I18NString name, StreetEdge sidewalk ) { - EdgeWithDistance { - Objects.requireNonNull(namedEdge); + NamedEdgeGroup { + Objects.requireNonNull(name); Objects.requireNonNull(sidewalk); } - void logDebugString() { - LOG.info("Name '{}' applied with low Hausdorff distance ", namedEdge.getName()); - LOG.info("Hausdorff: {}", hausdorff); - LOG.info("Distance: {}m", distance); - LOG.info("OSM: {}", osmUrl()); - LOG.info("Debug client: {}", debugClientUrl()); - LOG.info( - "<-------------------------------------------------------------------------------->" - ); + } + + record CandidateGroup(I18NString name, List edges, Set levels) { + double intersectionLength(Geometry polygon) { + return edges + .stream() + .mapToDouble(edge -> { + var intersection = polygon.intersection(edge.getGeometry()); + return length(intersection); + }) + .sum(); } - String debugClientUrl() { - var c = new WgsCoordinate(sidewalk.getFromVertex().getCoordinate()); - return "http://localhost:8080/debug-client-preview/#19/%s/%s".formatted( - c.latitude(), - c.longitude() + private double length(Geometry intersection) { + return switch (intersection) { + case LineString ls -> SphericalDistanceLibrary.length(ls); + case MultiLineString mls -> GeometryUtils + .getLineStrings(mls) + .stream() + .mapToDouble(this::intersectionLength) + .sum(); + case Point ignored -> 0; + case Geometry g -> throw new IllegalStateException( + "Didn't expect geometry %s".formatted(g.getClass()) ); + }; } - String osmUrl() { - var c = new WgsCoordinate(sidewalk.getFromVertex().getCoordinate()); - return "https://www.openstreetmap.org/?mlat=%s&mlon=%s#map=17/%s/%s".formatted( - c.latitude(), - c.longitude(), - c.latitude(), - c.longitude() - ); + double nearestDistanceTo(Geometry g) { + return edges + .stream() + .mapToDouble(e -> { + var points = DistanceOp.nearestPoints(e.getGeometry(), g); + return SphericalDistanceLibrary.fastDistance(points[0], points[1]); + }) + .min() + .orElse(Double.MAX_VALUE); } } + + record EdgeOnLevel(StreetEdge edge, Set levels) {} } diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 056acafb5d6..5cf5ce52a2f 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -533,7 +533,7 @@ public Set getMultiTagValues(Set refTags) { .flatMap(v -> Arrays.stream(v.split(";"))) .map(String::strip) .filter(v -> !v.isBlank()) - .collect(Collectors.toUnmodifiableSet()); + .collect(Collectors.toSet()); } public OsmProvider getOsmProvider() { @@ -570,6 +570,9 @@ public boolean isRoutable() { return false; } + /** + * Is this a link to another road, like a highway ramp. + */ public boolean isLink() { String highway = getTag("highway"); return highway != null && highway.endsWith(("_link")); From ea1c1697f8467da822940e138604c712b4502f8f Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 27 Mar 2024 15:47:07 +0100 Subject: [PATCH 10/30] Update tests and docs --- docs/BuildConfiguration.md | 2 +- .../apis/vectortiles/style.json | 55 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/docs/BuildConfiguration.md b/docs/BuildConfiguration.md index 3a3a6ff7ee4..c993016160f 100644 --- a/docs/BuildConfiguration.md +++ b/docs/BuildConfiguration.md @@ -35,7 +35,7 @@ Sections follow that describe particular settings in more depth. | maxTransferDuration | `duration` | Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph. | *Optional* | `"PT30M"` | 2.1 | | [multiThreadElevationCalculations](#multiThreadElevationCalculations) | `boolean` | Configuring multi-threading during elevation calculations. | *Optional* | `false` | 2.0 | | [osmCacheDataInMem](#osmCacheDataInMem) | `boolean` | If OSM data should be cached in memory during processing. | *Optional* | `false` | 2.0 | -| osmNaming | `string` | A custom OSM namer to use. | *Optional* | | 2.0 | +| osmNaming | `string` | A custom OSM namer to use. | *Optional* | | 1.5 | | platformEntriesLinking | `boolean` | Link unconnected entries to public transport platforms. | *Optional* | `false` | 2.0 | | [readCachedElevations](#readCachedElevations) | `boolean` | Whether to read cached elevation data. | *Optional* | `true` | 2.0 | | staticBikeParkAndRide | `boolean` | Whether we should create bike P+R stations from OSM data. | *Optional* | `false` | 1.5 | diff --git a/src/test/resources/org/opentripplanner/apis/vectortiles/style.json b/src/test/resources/org/opentripplanner/apis/vectortiles/style.json index 7e611171409..f8a6a80e44c 100644 --- a/src/test/resources/org/opentripplanner/apis/vectortiles/style.json +++ b/src/test/resources/org/opentripplanner/apis/vectortiles/style.json @@ -63,6 +63,56 @@ "visibility" : "none" } }, + { + "id" : "edge-name", + "type" : "symbol", + "source" : "vectorSource", + "source-layer" : "edges", + "minzoom" : 17, + "maxzoom" : 23, + "paint" : { + "text-color" : "#000", + "text-halo-color" : "#fff", + "text-halo-blur" : 4, + "text-halo-width" : 3 + }, + "filter" : [ + "in", + "class", + "StreetEdge", + "AreaEdge", + "EscalatorEdge", + "PathwayEdge", + "ElevatorHopEdge", + "TemporaryPartialStreetEdge", + "TemporaryFreeEdge" + ], + "layout" : { + "symbol-placement" : "line", + "symbol-spacing" : 500, + "text-field" : "{name}", + "text-font" : [ + "KlokanTech Noto Sans Regular" + ], + "text-size" : { + "base" : 14.0, + "stops" : [ + [ + 14, + 12.0 + ], + [ + 20, + 14.0 + ] + ] + }, + "text-max-width" : 5, + "text-keep-upright" : true, + "text-rotation-alignment" : "map", + "visibility" : "none" + } + }, { "id" : "link", "type" : "line", @@ -194,5 +244,6 @@ } } ], - "version" : 8 -} + "version" : 8, + "glyphs" : "https://cdn.jsdelivr.net/gh/klokantech/klokantech-gl-fonts@master/{fontstack}/{range}.pbf" +} \ No newline at end of file From daac6006f9e0cae79fb9b7a7bf5a0b75673aa727 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 27 Mar 2024 16:48:14 +0100 Subject: [PATCH 11/30] Improve documentation --- docs/BuildConfiguration.md | 10 ++++++++- .../module/osm/naming/SidewalkNamer.java | 8 +------ .../graph_builder/services/osm/EdgeNamer.java | 21 +++++++++++-------- .../services/osm/EdgeNamerTest.java | 12 +++++++++++ 4 files changed, 34 insertions(+), 17 deletions(-) create mode 100644 src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java diff --git a/docs/BuildConfiguration.md b/docs/BuildConfiguration.md index c993016160f..a95421e47f7 100644 --- a/docs/BuildConfiguration.md +++ b/docs/BuildConfiguration.md @@ -35,7 +35,7 @@ Sections follow that describe particular settings in more depth. | maxTransferDuration | `duration` | Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph. | *Optional* | `"PT30M"` | 2.1 | | [multiThreadElevationCalculations](#multiThreadElevationCalculations) | `boolean` | Configuring multi-threading during elevation calculations. | *Optional* | `false` | 2.0 | | [osmCacheDataInMem](#osmCacheDataInMem) | `boolean` | If OSM data should be cached in memory during processing. | *Optional* | `false` | 2.0 | -| osmNaming | `string` | A custom OSM namer to use. | *Optional* | | 1.5 | +| [osmNaming](#osmNaming) | `enum` | A custom OSM namer to use. | *Optional* | `"none"` | 1.5 | | platformEntriesLinking | `boolean` | Link unconnected entries to public transport platforms. | *Optional* | `false` | 2.0 | | [readCachedElevations](#readCachedElevations) | `boolean` | Whether to read cached elevation data. | *Optional* | `true` | 2.0 | | staticBikeParkAndRide | `boolean` | Whether we should create bike P+R stations from OSM data. | *Optional* | `false` | 1.5 | @@ -536,6 +536,14 @@ deployment depending on your infrastructure. Set the parameter to `true` to cach data, and to `false` to read the stream from the source each time. +

osmNaming

+ +**Since version:** `1.5` ∙ **Type:** `enum` ∙ **Cardinality:** `Optional` ∙ **Default value:** `"none"` +**Path:** / +**Enum values:** `none` | `portland` | `sidewalks` + +A custom OSM namer to use. +

readCachedElevations

**Since version:** `2.0` ∙ **Type:** `boolean` ∙ **Cardinality:** `Optional` ∙ **Default value:** `true` diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 8c03eacbd67..41e4fa09482 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -119,13 +119,7 @@ public void postprocess() { .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) .ifPresent(group -> { namesApplied.incrementAndGet(); - var name = group.name.toString(); - - sidewalk.setName( - Objects.requireNonNull( - I18NString.of("%s, Percent in buffer: %s".formatted(name, group.percentInBuffer)) - ) - ); + sidewalk.setName(Objects.requireNonNull(group.name)); }); //Keep lambda! A method-ref would cause incorrect class and line number to be logged diff --git a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java index 2733e705c70..249557ba1fe 100644 --- a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java @@ -52,25 +52,28 @@ public static EdgeNamer fromConfig(NodeAdapter root, String parameterName) { .of(parameterName) .summary("A custom OSM namer to use.") .since(OtpVersion.V1_5) - .asString(null); + .asEnum(EdgeNamerType.NONE); return fromConfig(osmNaming); } /** * Create a custom namer if needed, return null if not found / by default. */ - public static EdgeNamer fromConfig(String type) { - if (type == null) { + public static EdgeNamer fromConfig(EdgeNamerType type) { + if(type == null{ return new DefaultNamer(); } - return switch (type) { - case "portland" -> new PortlandCustomNamer(); - case "sidewalks" -> new SidewalkNamer(); - default -> throw new IllegalArgumentException( - String.format("Unknown osmNaming type: '%s'", type) - ); + case PORTLAND -> new PortlandCustomNamer(); + case SIDEWALKS -> new SidewalkNamer(); + case NONE -> new DefaultNamer(); }; } } + + enum EdgeNamerType { + NONE, + PORTLAND, + SIDEWALKS, + } } diff --git a/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java b/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java new file mode 100644 index 00000000000..70e9dbbc198 --- /dev/null +++ b/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java @@ -0,0 +1,12 @@ +package org.opentripplanner.graph_builder.services.osm; + +import org.junit.jupiter.api.Test; + +class EdgeNamerTest { + + @Test + void nullType(){ + var namer = EdgeNamer.EdgeNamerFactory.fromConfig(null); + } + +} \ No newline at end of file From 4d67575df277e8781af10c780d79b252b752e2ac Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 27 Mar 2024 21:40:05 +0100 Subject: [PATCH 12/30] Split up methods, add documentation --- .../module/osm/naming/SidewalkNamer.java | 122 ++++++++++-------- .../graph_builder/services/osm/EdgeNamer.java | 8 +- .../services/osm/EdgeNamerTest.java | 15 ++- 3 files changed, 84 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 41e4fa09482..a44b4551919 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -8,6 +8,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.geotools.api.referencing.FactoryException; import org.geotools.api.referencing.crs.CoordinateReferenceSystem; import org.geotools.api.referencing.operation.MathTransform; @@ -53,6 +54,8 @@ public void recordEdge(OSMWithTags way, StreetEdge edge) { unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels())); } if (way.isNamed() && !way.isLink()) { + // we generate two edges for each osm way: one there and one back. since we don't do any routing + // in this class we don't need the two directions and keep only one of them. var containsReverse = streetEdges .query(edge.getGeometry().getEnvelopeInternal()) .stream() @@ -78,49 +81,7 @@ public void postprocess() { unnamedSidewalks .parallelStream() .forEach(sidewalkOnLevel -> { - var sidewalk = sidewalkOnLevel.edge; - var envelope = sidewalk.getGeometry().getEnvelopeInternal(); - envelope.expandBy(0.000002); - var candidates = streetEdges.query(envelope); - - var groups = candidates - .stream() - .collect(Collectors.groupingBy(e -> e.edge.getName())) - .entrySet() - .stream() - .map(entry -> { - var levels = entry - .getValue() - .stream() - .flatMap(e -> e.levels.stream()) - .collect(Collectors.toSet()); - return new CandidateGroup( - entry.getKey(), - entry.getValue().stream().map(e -> e.edge).toList(), - levels - ); - }); - - var buffer = preciseBuffer(sidewalk.getGeometry(), 25); - var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); - - groups - .filter(g -> g.nearestDistanceTo(sidewalk.getGeometry()) < MAX_DISTANCE_TO_SIDEWALK) - .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) - .map(g -> { - var lengthInsideBuffer = g.intersectionLength(buffer); - double percentInBuffer = lengthInsideBuffer / sidewalkLength; - return new NamedEdgeGroup(percentInBuffer, candidates.size(), g.name, sidewalk); - }) - // remove those groups where less than a certain percentage is inside the buffer around - // the sidewalk. this safety mechanism for sidewalks that snake around the - // like https://www.openstreetmap.org/way/1059101564 - .filter(group -> group.percentInBuffer > MIN_PERCENT_IN_BUFFER) - .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) - .ifPresent(group -> { - namesApplied.incrementAndGet(); - sidewalk.setName(Objects.requireNonNull(group.name)); - }); + assignNameToSidewalk(sidewalkOnLevel, namesApplied); //Keep lambda! A method-ref would cause incorrect class and line number to be logged //noinspection Convert2MethodRef @@ -128,7 +89,7 @@ public void postprocess() { }); LOG.info( - "Assigned names to {} of {} of sidewalks ({})", + "Assigned names to {} of {} of sidewalks ({}%)", namesApplied.get(), unnamedSidewalks.size(), DoubleUtils.roundTo2Decimals((double) namesApplied.get() / unnamedSidewalks.size() * 100) @@ -141,6 +102,60 @@ public void postprocess() { unnamedSidewalks = null; } + private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger namesApplied) { + var sidewalk = sidewalkOnLevel.edge; + var buffer = preciseBuffer(sidewalk.getGeometry(), 25); + var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); + + var envelope = sidewalk.getGeometry().getEnvelopeInternal(); + envelope.expandBy(0.000002); + var candidates = streetEdges.query(envelope); + + groupEdgesByName(candidates) + .filter(g -> g.nearestDistanceTo(sidewalk.getGeometry()) < MAX_DISTANCE_TO_SIDEWALK) + .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) + .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) + // remove those groups where less than a certain percentage is inside the buffer around + // the sidewalk. this safety mechanism for sidewalks that snake around the corner + // like https://www.openstreetmap.org/way/1059101564 + .filter(group -> group.percentInBuffer > MIN_PERCENT_IN_BUFFER) + .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) + .ifPresent(group -> { + namesApplied.incrementAndGet(); + sidewalk.setName(Objects.requireNonNull(group.name)); + }); + } + + private static NamedEdgeGroup computePercentInsideBuffer( + CandidateGroup g, + Geometry buffer, + double sidewalkLength + ) { + var lengthInsideBuffer = g.intersectionLength(buffer); + double percentInBuffer = lengthInsideBuffer / sidewalkLength; + return new NamedEdgeGroup(percentInBuffer, g.name); + } + + private static Stream groupEdgesByName(List candidates) { + return candidates + .stream() + .collect(Collectors.groupingBy(e -> e.edge.getName())) + .entrySet() + .stream() + .map(entry -> { + var levels = entry + .getValue() + .stream() + .flatMap(e -> e.levels.stream()) + .collect(Collectors.toSet()); + return new CandidateGroup( + entry.getKey(), + entry.getValue().stream().map(e -> e.edge).toList(), + levels + ); + }); + } + /** * Taken from https://stackoverflow.com/questions/36455020 */ @@ -162,19 +177,17 @@ private Geometry preciseBuffer(Geometry geometry, double distanceInMeters) { } } - record NamedEdgeGroup( - double percentInBuffer, - int numberOfCandidates, - I18NString name, - StreetEdge sidewalk - ) { + private record NamedEdgeGroup(double percentInBuffer, I18NString name) { NamedEdgeGroup { Objects.requireNonNull(name); - Objects.requireNonNull(sidewalk); } } - record CandidateGroup(I18NString name, List edges, Set levels) { + /** + * A group of edges that are near a sidewalk that have the same name. These groups are used + * to figure out if the name of the group can be applied to a nearby sidewalk. + */ + private record CandidateGroup(I18NString name, List edges, Set levels) { double intersectionLength(Geometry polygon) { return edges .stream() @@ -200,6 +213,9 @@ private double length(Geometry intersection) { }; } + /** + * Get the closest distance in meters between any of the edges in the group and the given geometry. + */ double nearestDistanceTo(Geometry g) { return edges .stream() @@ -212,5 +228,5 @@ private double length(Geometry intersection) { } } - record EdgeOnLevel(StreetEdge edge, Set levels) {} + private record EdgeOnLevel(StreetEdge edge, Set levels) {} } diff --git a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java index 249557ba1fe..010b21ec2d1 100644 --- a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java @@ -52,7 +52,7 @@ public static EdgeNamer fromConfig(NodeAdapter root, String parameterName) { .of(parameterName) .summary("A custom OSM namer to use.") .since(OtpVersion.V1_5) - .asEnum(EdgeNamerType.NONE); + .asEnum(EdgeNamerType.DEFAULT); return fromConfig(osmNaming); } @@ -60,19 +60,19 @@ public static EdgeNamer fromConfig(NodeAdapter root, String parameterName) { * Create a custom namer if needed, return null if not found / by default. */ public static EdgeNamer fromConfig(EdgeNamerType type) { - if(type == null{ + if (type == null) { return new DefaultNamer(); } return switch (type) { case PORTLAND -> new PortlandCustomNamer(); case SIDEWALKS -> new SidewalkNamer(); - case NONE -> new DefaultNamer(); + case DEFAULT -> new DefaultNamer(); }; } } enum EdgeNamerType { - NONE, + DEFAULT, PORTLAND, SIDEWALKS, } diff --git a/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java b/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java index 70e9dbbc198..ff471fbfbf6 100644 --- a/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java +++ b/src/test/java/org/opentripplanner/graph_builder/services/osm/EdgeNamerTest.java @@ -1,12 +1,19 @@ package org.opentripplanner.graph_builder.services.osm; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; + import org.junit.jupiter.api.Test; +import org.opentripplanner.graph_builder.module.osm.naming.DefaultNamer; +import org.opentripplanner.graph_builder.services.osm.EdgeNamer.EdgeNamerType; class EdgeNamerTest { @Test - void nullType(){ - var namer = EdgeNamer.EdgeNamerFactory.fromConfig(null); + void nullType() { + assertInstanceOf(DefaultNamer.class, EdgeNamer.EdgeNamerFactory.fromConfig(null)); + assertInstanceOf( + DefaultNamer.class, + EdgeNamer.EdgeNamerFactory.fromConfig(EdgeNamerType.DEFAULT) + ); } - -} \ No newline at end of file +} From 86707901c27ba775a72f948d2c24954324c56148 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 28 Mar 2024 12:27:22 +0100 Subject: [PATCH 13/30] Add test, docs --- docs/BuildConfiguration.md | 6 +- .../apis/gtfs/GraphQLScalars.java | 7 +- .../framework/json/ObjectMappers.java | 9 ++ .../module/osm/naming/SidewalkNamer.java | 45 ++++++- .../module/osm/naming/SidewalkNamerTest.java | 114 ++++++++++++++++++ .../wayproperty/specifier/WayTestData.java | 9 +- .../test/support/GeoJsonIo.java | 26 ++++ 7 files changed, 204 insertions(+), 12 deletions(-) create mode 100644 src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java create mode 100644 src/test/java/org/opentripplanner/test/support/GeoJsonIo.java diff --git a/docs/BuildConfiguration.md b/docs/BuildConfiguration.md index a95421e47f7..fe3a2109ba4 100644 --- a/docs/BuildConfiguration.md +++ b/docs/BuildConfiguration.md @@ -35,7 +35,7 @@ Sections follow that describe particular settings in more depth. | maxTransferDuration | `duration` | Transfers up to this duration with the default walk speed value will be pre-calculated and included in the Graph. | *Optional* | `"PT30M"` | 2.1 | | [multiThreadElevationCalculations](#multiThreadElevationCalculations) | `boolean` | Configuring multi-threading during elevation calculations. | *Optional* | `false` | 2.0 | | [osmCacheDataInMem](#osmCacheDataInMem) | `boolean` | If OSM data should be cached in memory during processing. | *Optional* | `false` | 2.0 | -| [osmNaming](#osmNaming) | `enum` | A custom OSM namer to use. | *Optional* | `"none"` | 1.5 | +| [osmNaming](#osmNaming) | `enum` | A custom OSM namer to use. | *Optional* | `"default"` | 1.5 | | platformEntriesLinking | `boolean` | Link unconnected entries to public transport platforms. | *Optional* | `false` | 2.0 | | [readCachedElevations](#readCachedElevations) | `boolean` | Whether to read cached elevation data. | *Optional* | `true` | 2.0 | | staticBikeParkAndRide | `boolean` | Whether we should create bike P+R stations from OSM data. | *Optional* | `false` | 1.5 | @@ -538,9 +538,9 @@ data, and to `false` to read the stream from the source each time.

osmNaming

-**Since version:** `1.5` ∙ **Type:** `enum` ∙ **Cardinality:** `Optional` ∙ **Default value:** `"none"` +**Since version:** `1.5` ∙ **Type:** `enum` ∙ **Cardinality:** `Optional` ∙ **Default value:** `"default"` **Path:** / -**Enum values:** `none` | `portland` | `sidewalks` +**Enum values:** `default` | `portland` | `sidewalks` A custom OSM namer to use. diff --git a/src/main/java/org/opentripplanner/apis/gtfs/GraphQLScalars.java b/src/main/java/org/opentripplanner/apis/gtfs/GraphQLScalars.java index 42ffe992539..5ab24c89543 100644 --- a/src/main/java/org/opentripplanner/apis/gtfs/GraphQLScalars.java +++ b/src/main/java/org/opentripplanner/apis/gtfs/GraphQLScalars.java @@ -1,6 +1,5 @@ package org.opentripplanner.apis.gtfs; -import com.bedatadriven.jackson.datatype.jts.JtsModule; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import graphql.language.StringValue; @@ -16,15 +15,15 @@ import java.time.format.DateTimeFormatter; import javax.annotation.Nonnull; import org.locationtech.jts.geom.Geometry; -import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.framework.graphql.scalar.DurationScalarFactory; +import org.opentripplanner.framework.json.ObjectMappers; import org.opentripplanner.framework.model.Grams; import org.opentripplanner.framework.time.OffsetDateTimeParser; public class GraphQLScalars { - private static final ObjectMapper geoJsonMapper = new ObjectMapper() - .registerModule(new JtsModule(GeometryUtils.getGeometryFactory())); + private static final ObjectMapper geoJsonMapper = ObjectMappers.geoJson(); + public static GraphQLScalarType DURATION_SCALAR = DurationScalarFactory.createDurationScalar(); public static final GraphQLScalarType POLYLINE_SCALAR = GraphQLScalarType diff --git a/src/main/java/org/opentripplanner/framework/json/ObjectMappers.java b/src/main/java/org/opentripplanner/framework/json/ObjectMappers.java index 8802db0fc41..1670ae94fb3 100644 --- a/src/main/java/org/opentripplanner/framework/json/ObjectMappers.java +++ b/src/main/java/org/opentripplanner/framework/json/ObjectMappers.java @@ -1,7 +1,9 @@ package org.opentripplanner.framework.json; +import com.bedatadriven.jackson.datatype.jts.JtsModule; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; +import org.opentripplanner.framework.geometry.GeometryUtils; public class ObjectMappers { @@ -13,4 +15,11 @@ public static ObjectMapper ignoringExtraFields() { mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); return mapper; } + + /** + * Returns a mapper that can serialize JTS geometries into GeoJSON. + */ + public static ObjectMapper geoJson() { + return new ObjectMapper().registerModule(new JtsModule(GeometryUtils.getGeometryFactory())); + } } diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index a44b4551919..60d0f267337 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -34,11 +34,30 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * A namer that assigns names of nearby streets to sidewalks if they meet certain + * geometric similarity criteria. + *

+ * The algorithm works as follows: + * - for each sidewalk we look up (named) street edges nearby + * - group those edges into groups where each edge has the same name + * - draw a flat-capped buffer around the sidewalk, like this: https://tinyurl.com/4fpe882h + * - check how much of a named edge group is inside the buffer + * - remove those groups which are below MIN_PERCENT_IN_BUFFER + * - take the group that has the highest percentage (as a proportion of the sidewalk length) inside + * the buffer and apply its name to the sidewalk. + *

+ * This works very well for OSM data where the sidewalk runs a parallel to the street and at each + * intersection the sidewalk is also split. It doesn't work well for sidewalks that go around + * the corner, like https://www.openstreetmap.org/way/1059101564. These cases are, however, detected + * by the above algorithm and the sidewalk name remains the same. + */ public class SidewalkNamer implements EdgeNamer { private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamer.class); private static final int MAX_DISTANCE_TO_SIDEWALK = 50; private static final double MIN_PERCENT_IN_BUFFER = .85; + private static final int BUFFER_METERS = 25; private HashGridSpatialIndex streetEdges = new HashGridSpatialIndex<>(); private Collection unnamedSidewalks = new ArrayList<>(); @@ -102,9 +121,12 @@ public void postprocess() { unnamedSidewalks = null; } + /** + * The actual worker method that runs the business logic on an individual sidewalk edge. + */ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger namesApplied) { var sidewalk = sidewalkOnLevel.edge; - var buffer = preciseBuffer(sidewalk.getGeometry(), 25); + var buffer = preciseBuffer(sidewalk.getGeometry(), BUFFER_METERS); var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); var envelope = sidewalk.getGeometry().getEnvelopeInternal(); @@ -112,7 +134,9 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam var candidates = streetEdges.query(envelope); groupEdgesByName(candidates) + // remove edges that are far away .filter(g -> g.nearestDistanceTo(sidewalk.getGeometry()) < MAX_DISTANCE_TO_SIDEWALK) + // make sure we only compare sidewalks and streets that are on the same level .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) // remove those groups where less than a certain percentage is inside the buffer around @@ -126,6 +150,10 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam }); } + /** + * Compute the length of the group that is inside the buffer and return it as a percentage + * of the lenght of the sidewalk. + */ private static NamedEdgeGroup computePercentInsideBuffer( CandidateGroup g, Geometry buffer, @@ -136,6 +164,11 @@ private static NamedEdgeGroup computePercentInsideBuffer( return new NamedEdgeGroup(percentInBuffer, g.name); } + /** + * If a single street is split into several eges each individual part of the street would potentially + * have a low similarity with the (longer) sidewalk. For that reason we combined them into a group + * and have a better basis for comparison. + */ private static Stream groupEdgesByName(List candidates) { return candidates .stream() @@ -157,6 +190,13 @@ private static Stream groupEdgesByName(List candida } /** + * Add a buffer around a geometry that uses both meters as the input and makes sure + * that the buffer is the same distance (in meters) anywhere on earth. + *

+ * Background: If you call the regular buffer() method on a JTS geometry that uses WGS84 as the + * coordinate reference system, the buffer will be accurate at the equator but will become more + * and more elongated the furter north/south you go. + *

* Taken from https://stackoverflow.com/questions/36455020 */ private Geometry preciseBuffer(Geometry geometry, double distanceInMeters) { @@ -188,6 +228,9 @@ private record NamedEdgeGroup(double percentInBuffer, I18NString name) { * to figure out if the name of the group can be applied to a nearby sidewalk. */ private record CandidateGroup(I18NString name, List edges, Set levels) { + /** + * How much of this group intersects with the give geometry, in meters. + */ double intersectionLength(Geometry polygon) { return edges .stream() diff --git a/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java b/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java new file mode 100644 index 00000000000..3faa77ee531 --- /dev/null +++ b/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java @@ -0,0 +1,114 @@ +package org.opentripplanner.graph_builder.module.osm.naming; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.opentripplanner.framework.geometry.GeometryUtils; +import org.opentripplanner.framework.geometry.WgsCoordinate; +import org.opentripplanner.framework.i18n.I18NString; +import org.opentripplanner.graph_builder.services.osm.EdgeNamer; +import org.opentripplanner.openstreetmap.model.OSMWay; +import org.opentripplanner.openstreetmap.wayproperty.specifier.WayTestData; +import org.opentripplanner.street.model.StreetTraversalPermission; +import org.opentripplanner.street.model._data.StreetModelForTest; +import org.opentripplanner.street.model.edge.StreetEdge; +import org.opentripplanner.street.model.edge.StreetEdgeBuilder; +import org.opentripplanner.test.support.GeoJsonIo; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +class SidewalkNamerTest { + + private static final I18NString SIDEWALK = I18NString.of("sidewalk"); + private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamerTest.class); + + @Test + void postprocess() { + var builder = new ModelBuilder(); + var sidewalk = builder.addUnamedSidewalk( + new WgsCoordinate(33.75029, -84.39198), + new WgsCoordinate(33.74932, -84.39275) + ); + + LOG.info( + "Geometry of {}: {}", + sidewalk.edge.getName(), + GeoJsonIo.toUrl(sidewalk.edge.getGeometry()) + ); + + var pryorStreet = builder.addStreetEdge( + "Pryor Street", + new WgsCoordinate(33.75032, -84.39190), + new WgsCoordinate(33.74924, -84.39275) + ); + + LOG.info( + "Geometry of {}: {}", + pryorStreet.edge.getName(), + GeoJsonIo.toUrl(pryorStreet.edge.getGeometry()) + ); + + assertNotEquals(sidewalk.edge.getName(), pryorStreet.edge.getName()); + builder.postProcess(new SidewalkNamer()); + assertEquals(sidewalk.edge.getName(), pryorStreet.edge.getName()); + } + + private static class ModelBuilder { + + private final List pairs = new ArrayList<>(); + + EdgePair addUnamedSidewalk(WgsCoordinate... coordinates) { + var edge = edgeBuilder(coordinates) + .withName(SIDEWALK) + .withPermission(StreetTraversalPermission.PEDESTRIAN) + .withBogusName(true) + .buildAndConnect(); + + var way = WayTestData.footwaySidewalk(); + assertTrue(way.isSidewalk()); + var p = new EdgePair(way, edge); + pairs.add(p); + return p; + } + + EdgePair addStreetEdge(String name, WgsCoordinate... coordinates) { + var edge = edgeBuilder(coordinates) + .withName(I18NString.of(name)) + .withPermission(StreetTraversalPermission.ALL) + .buildAndConnect(); + var way = WayTestData.highwayTertiary(); + way.addTag("name", name); + assertFalse(way.isSidewalk()); + assertTrue(way.isNamed()); + var p = new EdgePair(way, edge); + pairs.add(p); + return p; + } + + void postProcess(EdgeNamer namer) { + pairs.forEach(p -> namer.recordEdge(p.way, p.edge)); + namer.postprocess(); + } + + private static StreetEdgeBuilder edgeBuilder(WgsCoordinate... c) { + var coordinates = Arrays.stream(c).toList(); + var ls = GeometryUtils.makeLineString(c); + return new StreetEdgeBuilder<>() + .withFromVertex( + StreetModelForTest.intersectionVertex(coordinates.getFirst().asJtsCoordinate()) + ) + .withToVertex( + StreetModelForTest.intersectionVertex(coordinates.getLast().asJtsCoordinate()) + ) + .withGeometry(ls); + } + } + + private record EdgePair(OSMWay way, StreetEdge edge) {} +} diff --git a/src/test/java/org/opentripplanner/openstreetmap/wayproperty/specifier/WayTestData.java b/src/test/java/org/opentripplanner/openstreetmap/wayproperty/specifier/WayTestData.java index 20dbcbb5a78..b09f690f794 100644 --- a/src/test/java/org/opentripplanner/openstreetmap/wayproperty/specifier/WayTestData.java +++ b/src/test/java/org/opentripplanner/openstreetmap/wayproperty/specifier/WayTestData.java @@ -121,9 +121,10 @@ public static OSMWithTags cyclewayBoth() { return way; } - public static OSMWithTags footwaySidewalk() { - var way = new OSMWithTags(); + public static OSMWay footwaySidewalk() { + var way = new OSMWay(); way.addTag("footway", "sidewalk"); + way.addTag("highway", "footway"); return way; } @@ -155,8 +156,8 @@ public static OSMWithTags highwayTrunk() { return way; } - public static OSMWithTags highwayTertiary() { - var way = new OSMWithTags(); + public static OSMWay highwayTertiary() { + var way = new OSMWay(); way.addTag("highway", "tertiary"); return way; } diff --git a/src/test/java/org/opentripplanner/test/support/GeoJsonIo.java b/src/test/java/org/opentripplanner/test/support/GeoJsonIo.java new file mode 100644 index 00000000000..8e25407fca5 --- /dev/null +++ b/src/test/java/org/opentripplanner/test/support/GeoJsonIo.java @@ -0,0 +1,26 @@ +package org.opentripplanner.test.support; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import org.locationtech.jts.geom.Geometry; +import org.opentripplanner.framework.json.ObjectMappers; + +/** + * Helper class for generating URLs to geojson.io. + */ +public class GeoJsonIo { + + private static final ObjectMapper MAPPER = ObjectMappers.geoJson(); + + public static String toUrl(Geometry geometry) { + try { + var geoJson = MAPPER.writeValueAsString(geometry); + var encoded = URLEncoder.encode(geoJson, StandardCharsets.UTF_8); + return "http://geojson.io/#data=data:application/json,%s".formatted(encoded); + } catch (JsonProcessingException e) { + throw new RuntimeException(e); + } + } +} From 4b6a27ea9f89ecefb5b6049de05688b40c81f961 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 28 Mar 2024 13:48:20 +0100 Subject: [PATCH 14/30] Sort by keys before comparing --- .../org/opentripplanner/test/support/JsonAssertions.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/test/java/org/opentripplanner/test/support/JsonAssertions.java b/src/test/java/org/opentripplanner/test/support/JsonAssertions.java index 2dab1e96190..f57e5a6741d 100644 --- a/src/test/java/org/opentripplanner/test/support/JsonAssertions.java +++ b/src/test/java/org/opentripplanner/test/support/JsonAssertions.java @@ -5,12 +5,17 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.SerializationFeature; import org.opentripplanner.standalone.config.framework.json.JsonSupport; public class JsonAssertions { private static final ObjectMapper MAPPER = new ObjectMapper(); + static { + MAPPER.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true); + } + /** * Take two JSON documents and reformat them before comparing {@code actual} with {@code expected}. */ From f596432db06b97c672a0906756c4c6c08151b850 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 28 Mar 2024 14:19:51 +0100 Subject: [PATCH 15/30] Sort order for JSON files --- .../test/support/JsonAssertions.java | 16 ++++++++++------ .../opentripplanner/apis/vectortiles/style.json | 4 ++-- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/test/java/org/opentripplanner/test/support/JsonAssertions.java b/src/test/java/org/opentripplanner/test/support/JsonAssertions.java index f57e5a6741d..1fe87268eee 100644 --- a/src/test/java/org/opentripplanner/test/support/JsonAssertions.java +++ b/src/test/java/org/opentripplanner/test/support/JsonAssertions.java @@ -5,17 +5,12 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.SerializationFeature; import org.opentripplanner.standalone.config.framework.json.JsonSupport; public class JsonAssertions { private static final ObjectMapper MAPPER = new ObjectMapper(); - static { - MAPPER.configure(SerializationFeature.ORDER_MAP_ENTRIES_BY_KEYS, true); - } - /** * Take two JSON documents and reformat them before comparing {@code actual} with {@code expected}. */ @@ -32,8 +27,17 @@ public static void assertEqualJson(String expected, String actual) { */ public static void assertEqualJson(String expected, JsonNode actual) { try { + var actualNode = MAPPER.readTree(actual.toString()); var exp = MAPPER.readTree(expected); - assertEquals(JsonSupport.prettyPrint(exp), JsonSupport.prettyPrint(actual)); + assertEquals( + exp, + actualNode, + () -> + "Expected '%s' but actual was '%s'".formatted( + JsonSupport.prettyPrint(exp), + JsonSupport.prettyPrint(actualNode) + ) + ); } catch (JsonProcessingException e) { throw new RuntimeException(e); } diff --git a/src/test/resources/org/opentripplanner/apis/vectortiles/style.json b/src/test/resources/org/opentripplanner/apis/vectortiles/style.json index f8a6a80e44c..c0e31a26f5d 100644 --- a/src/test/resources/org/opentripplanner/apis/vectortiles/style.json +++ b/src/test/resources/org/opentripplanner/apis/vectortiles/style.json @@ -244,6 +244,6 @@ } } ], - "version" : 8, - "glyphs" : "https://cdn.jsdelivr.net/gh/klokantech/klokantech-gl-fonts@master/{fontstack}/{range}.pbf" + "glyphs" : "https://cdn.jsdelivr.net/gh/klokantech/klokantech-gl-fonts@master/{fontstack}/{range}.pbf", + "version" : 8 } \ No newline at end of file From a312d2a1e4a1cadf4354b991366bf936827d155e Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Thu, 28 Mar 2024 14:42:20 +0100 Subject: [PATCH 16/30] Revert set conversion --- .../org/opentripplanner/openstreetmap/model/OSMWithTags.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 5cf5ce52a2f..51bc9da1dd8 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -533,7 +533,7 @@ public Set getMultiTagValues(Set refTags) { .flatMap(v -> Arrays.stream(v.split(";"))) .map(String::strip) .filter(v -> !v.isBlank()) - .collect(Collectors.toSet()); + .collect(Collectors.toUnmodifiableSet()); } public OsmProvider getOsmProvider() { From 1c66f062eac75e8da0db9f3e1b03e91a709f5b39 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 10 Apr 2024 11:37:32 +0200 Subject: [PATCH 17/30] Apply review feedback about documentation --- .../module/osm/naming/SidewalkNamer.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 60d0f267337..049c3cfee24 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -102,8 +102,8 @@ public void postprocess() { .forEach(sidewalkOnLevel -> { assignNameToSidewalk(sidewalkOnLevel, namesApplied); - //Keep lambda! A method-ref would cause incorrect class and line number to be logged - //noinspection Convert2MethodRef + // Keep lambda! A method-ref would cause incorrect class and line number to be logged + // noinspection Convert2MethodRef progress.step(m -> LOG.info(m)); }); @@ -139,9 +139,9 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam // make sure we only compare sidewalks and streets that are on the same level .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) - // remove those groups where less than a certain percentage is inside the buffer around - // the sidewalk. this safety mechanism for sidewalks that snake around the corner - // like https://www.openstreetmap.org/way/1059101564 + // Remove those groups where less than a certain percentage is inside the buffer around + // the sidewalk. This is a safety mechanism for sidewalks that snake around the corner, + // like https://www.openstreetmap.org/way/1059101564 . .filter(group -> group.percentInBuffer > MIN_PERCENT_IN_BUFFER) .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) .ifPresent(group -> { @@ -152,7 +152,7 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam /** * Compute the length of the group that is inside the buffer and return it as a percentage - * of the lenght of the sidewalk. + * of the length of the sidewalk. */ private static NamedEdgeGroup computePercentInsideBuffer( CandidateGroup g, @@ -165,8 +165,8 @@ private static NamedEdgeGroup computePercentInsideBuffer( } /** - * If a single street is split into several eges each individual part of the street would potentially - * have a low similarity with the (longer) sidewalk. For that reason we combined them into a group + * If a single street is split into several edges, each individual part of the street would potentially + * have a low similarity with the (longer) sidewalk. For that reason we combine them into a group * and have a better basis for comparison. */ private static Stream groupEdgesByName(List candidates) { @@ -190,12 +190,12 @@ private static Stream groupEdgesByName(List candida } /** - * Add a buffer around a geometry that uses both meters as the input and makes sure - * that the buffer is the same distance (in meters) anywhere on earth. + * Add a buffer around a geometry that makes sure that the buffer is the same distance (in meters) + * anywhere on earth. *

* Background: If you call the regular buffer() method on a JTS geometry that uses WGS84 as the * coordinate reference system, the buffer will be accurate at the equator but will become more - * and more elongated the furter north/south you go. + * and more elongated the further north/south you go. *

* Taken from https://stackoverflow.com/questions/36455020 */ @@ -229,7 +229,7 @@ private record NamedEdgeGroup(double percentInBuffer, I18NString name) { */ private record CandidateGroup(I18NString name, List edges, Set levels) { /** - * How much of this group intersects with the give geometry, in meters. + * How much of this group intersects with the given geometry, in meters. */ double intersectionLength(Geometry polygon) { return edges From c53e83801fb45ed030af0c920cac4df16e3a2577 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Fri, 12 Apr 2024 08:25:30 +0200 Subject: [PATCH 18/30] Fix more typos --- .../graph_builder/module/osm/naming/SidewalkNamer.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 049c3cfee24..e6eab4d3d0e 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -141,7 +141,7 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) // Remove those groups where less than a certain percentage is inside the buffer around // the sidewalk. This is a safety mechanism for sidewalks that snake around the corner, - // like https://www.openstreetmap.org/way/1059101564 . + // like https://www.openstreetmap.org/way/1059101564. .filter(group -> group.percentInBuffer > MIN_PERCENT_IN_BUFFER) .max(Comparator.comparingDouble(NamedEdgeGroup::percentInBuffer)) .ifPresent(group -> { @@ -195,7 +195,7 @@ private static Stream groupEdgesByName(List candida *

* Background: If you call the regular buffer() method on a JTS geometry that uses WGS84 as the * coordinate reference system, the buffer will be accurate at the equator but will become more - * and more elongated the further north/south you go. + * and more elongated the farther north/south you go. *

* Taken from https://stackoverflow.com/questions/36455020 */ From 70585decabdfe488947bb1ddf4aa7ef207b155ef Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 16 Apr 2024 09:40:41 +0200 Subject: [PATCH 19/30] Remove extra expandBy --- .../graph_builder/module/osm/naming/SidewalkNamer.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index e6eab4d3d0e..763a531250f 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -129,9 +129,7 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam var buffer = preciseBuffer(sidewalk.getGeometry(), BUFFER_METERS); var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); - var envelope = sidewalk.getGeometry().getEnvelopeInternal(); - envelope.expandBy(0.000002); - var candidates = streetEdges.query(envelope); + var candidates = streetEdges.query(buffer.getEnvelopeInternal()); groupEdgesByName(candidates) // remove edges that are far away From 3056805c63b67a93e16e4b206a0aef785cb275d8 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 16 Apr 2024 09:43:54 +0200 Subject: [PATCH 20/30] Use better name for variable --- .../opentripplanner/apis/vectortiles/DebugStyleSpec.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java index 16e3b247bc7..92d577480e2 100644 --- a/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java +++ b/src/main/java/org/opentripplanner/apis/vectortiles/DebugStyleSpec.java @@ -50,7 +50,7 @@ public class DebugStyleSpec { 1, List.of(new ZoomStop(15, 0.2f), new ZoomStop(MAX_ZOOM, 3)) ); - private static final Class[] ALL_EDGES = new Class[] { + private static final Class[] EDGES_TO_DISPLAY = new Class[] { StreetEdge.class, AreaEdge.class, EscalatorEdge.class, @@ -83,7 +83,7 @@ static StyleSpec build( .typeLine() .vectorSourceLayer(edges) .lineColor(MAGENTA) - .edgeFilter(ALL_EDGES) + .edgeFilter(EDGES_TO_DISPLAY) .lineWidth(LINE_WIDTH) .minZoom(13) .maxZoom(MAX_ZOOM) @@ -93,7 +93,7 @@ static StyleSpec build( .typeSymbol() .lineText("name") .vectorSourceLayer(edges) - .edgeFilter(ALL_EDGES) + .edgeFilter(EDGES_TO_DISPLAY) .minZoom(17) .maxZoom(MAX_ZOOM) .intiallyHidden(), From de0e38292a806fd0534009965abcf91ed5366d5b Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 16 Apr 2024 17:24:15 +0200 Subject: [PATCH 21/30] Compute UTM CRS only once --- .../module/osm/naming/SidewalkNamer.java | 91 +++++++++++++------ 1 file changed, 62 insertions(+), 29 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 763a531250f..d17de3f96c1 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -16,6 +16,8 @@ import org.geotools.geometry.jts.JTS; import org.geotools.referencing.CRS; import org.geotools.referencing.crs.DefaultGeographicCRS; +import org.locationtech.jts.geom.Coordinate; +import org.locationtech.jts.geom.Envelope; import org.locationtech.jts.geom.Geometry; import org.locationtech.jts.geom.LineString; import org.locationtech.jts.geom.MultiLineString; @@ -61,6 +63,7 @@ public class SidewalkNamer implements EdgeNamer { private HashGridSpatialIndex streetEdges = new HashGridSpatialIndex<>(); private Collection unnamedSidewalks = new ArrayList<>(); + private PreciseBuffer preciseBuffer; @Override public I18NString name(OSMWithTags way) { @@ -96,6 +99,8 @@ public void postprocess() { unnamedSidewalks.size() ); + this.preciseBuffer = new PreciseBuffer(computeCentroid(), BUFFER_METERS); + final AtomicInteger namesApplied = new AtomicInteger(0); unnamedSidewalks .parallelStream() @@ -121,12 +126,23 @@ public void postprocess() { unnamedSidewalks = null; } + /** + * Compute the centroid of all sidewalk edges. + */ + private Coordinate computeCentroid() { + var envelope = new Envelope(); + unnamedSidewalks.forEach(e -> + envelope.expandToInclude(e.edge.getGeometry().getEnvelopeInternal()) + ); + return envelope.centre(); + } + /** * The actual worker method that runs the business logic on an individual sidewalk edge. */ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger namesApplied) { var sidewalk = sidewalkOnLevel.edge; - var buffer = preciseBuffer(sidewalk.getGeometry(), BUFFER_METERS); + var buffer = preciseBuffer.preciseBuffer(sidewalk.getGeometry()); var sidewalkLength = SphericalDistanceLibrary.length(sidewalk.getGeometry()); var candidates = streetEdges.query(buffer.getEnvelopeInternal()); @@ -187,34 +203,6 @@ private static Stream groupEdgesByName(List candida }); } - /** - * Add a buffer around a geometry that makes sure that the buffer is the same distance (in meters) - * anywhere on earth. - *

- * Background: If you call the regular buffer() method on a JTS geometry that uses WGS84 as the - * coordinate reference system, the buffer will be accurate at the equator but will become more - * and more elongated the farther north/south you go. - *

- * Taken from https://stackoverflow.com/questions/36455020 - */ - private Geometry preciseBuffer(Geometry geometry, double distanceInMeters) { - try { - var coordinate = geometry.getCentroid().getCoordinate(); - String code = "AUTO:42001,%s,%s".formatted(coordinate.x, coordinate.y); - CoordinateReferenceSystem auto = CRS.decode(code); - - MathTransform toTransform = CRS.findMathTransform(DefaultGeographicCRS.WGS84, auto); - MathTransform fromTransform = CRS.findMathTransform(auto, DefaultGeographicCRS.WGS84); - - Geometry pGeom = JTS.transform(geometry, toTransform); - - Geometry pBufferedGeom = pGeom.buffer(distanceInMeters, 4, BufferParameters.CAP_FLAT); - return JTS.transform(pBufferedGeom, fromTransform); - } catch (TransformException | FactoryException e) { - throw new RuntimeException(e); - } - } - private record NamedEdgeGroup(double percentInBuffer, I18NString name) { NamedEdgeGroup { Objects.requireNonNull(name); @@ -270,4 +258,49 @@ private double length(Geometry intersection) { } private record EdgeOnLevel(StreetEdge edge, Set levels) {} + + /** + * A class to cache the expensive construction of a Universal Traverse Mercator coordinate + * reference system. + * Re-using the same CRS for all edges might introduce tiny imprecisions for OTPs use cases + * but speeds up the processing enormously and is a price well worth paying. + */ + private static final class PreciseBuffer { + + private final double distanceInMeters; + private final MathTransform toTransform; + private final MathTransform fromTransform; + + private PreciseBuffer(Coordinate coordinate, double distanceInMeters) { + this.distanceInMeters = distanceInMeters; + String code = "AUTO:42001,%s,%s".formatted(coordinate.x, coordinate.y); + try { + CoordinateReferenceSystem auto = CRS.decode(code); + this.toTransform = CRS.findMathTransform(DefaultGeographicCRS.WGS84, auto); + this.fromTransform = CRS.findMathTransform(auto, DefaultGeographicCRS.WGS84); + } catch (FactoryException e) { + throw new RuntimeException(e); + } + } + + /** + * Add a buffer around a geometry that makes sure that the buffer is the same distance (in + * meters) anywhere on earth. + *

+ * Background: If you call the regular buffer() method on a JTS geometry that uses WGS84 as the + * coordinate reference system, the buffer will be accurate at the equator but will become more + * and more elongated the farther north/south you go. + *

+ * Taken from https://stackoverflow.com/questions/36455020 + */ + private Geometry preciseBuffer(Geometry geometry) { + try { + Geometry pGeom = JTS.transform(geometry, toTransform); + Geometry pBufferedGeom = pGeom.buffer(distanceInMeters, 4, BufferParameters.CAP_FLAT); + return JTS.transform(pBufferedGeom, fromTransform); + } catch (TransformException e) { + throw new RuntimeException(e); + } + } + } } From 1d749c3d3642f4e7e4223db8af136a13bd0fbc54 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 17 Apr 2024 14:25:14 +0200 Subject: [PATCH 22/30] Always record edges in pairs in the custom namer --- .../graph_builder/module/osm/OsmModule.java | 9 ++-- .../module/osm/StreetEdgePair.java | 35 +++++++++++++ .../module/osm/naming/DefaultNamer.java | 4 +- .../osm/naming/PortlandCustomNamer.java | 49 ++++++++++--------- .../module/osm/naming/SidewalkNamer.java | 22 ++++----- .../graph_builder/services/osm/EdgeNamer.java | 6 +-- .../module/islandpruning/TestNamer.java | 4 +- .../module/osm/naming/SidewalkNamerTest.java | 3 +- 8 files changed, 85 insertions(+), 47 deletions(-) create mode 100644 src/main/java/org/opentripplanner/graph_builder/module/osm/StreetEdgePair.java diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java index 0cd53b70fc6..79bcacda660 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java @@ -113,8 +113,6 @@ public Map elevationDataOutput() { return elevationData; } - private record StreetEdgePair(StreetEdge main, StreetEdge back) {} - private void build() { var parkingProcessor = new ParkingProcessor( graph, @@ -390,8 +388,10 @@ private void buildBasicGraph() { geometry ); - StreetEdge street = streets.main; - StreetEdge backStreet = streets.back; + params.edgeNamer().recordEdges(way, streets); + + StreetEdge street = streets.main(); + StreetEdge backStreet = streets.back(); normalizer.applyWayProperties(street, backStreet, wayData, way); applyEdgesToTurnRestrictions(way, startNode, endNode, street, backStreet); @@ -545,7 +545,6 @@ private StreetEdge getEdgeForStreet( .withBogusName(way.needsFallbackName()); StreetEdge street = seb.buildAndConnect(); - params.edgeNamer().recordEdge(way, street); return street; } diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/StreetEdgePair.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/StreetEdgePair.java new file mode 100644 index 00000000000..7e87838c4d5 --- /dev/null +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/StreetEdgePair.java @@ -0,0 +1,35 @@ +package org.opentripplanner.graph_builder.module.osm; + +import java.util.ArrayList; +import org.opentripplanner.street.model.edge.StreetEdge; + +public record StreetEdgePair(StreetEdge main, StreetEdge back) { + /** + * Return the non-null elements of this pair as an Iterable. + */ + public Iterable asIterable() { + var ret = new ArrayList(2); + if (main != null) { + ret.add(main); + } + if (back != null) { + ret.add(back); + } + return ret; + } + + /** + * Select one of the edges contained in this pair that is not null. No particular algorithm is + * guaranteed, and it may change in the future. + */ + public StreetEdge pickAny() { + if (main != null) { + return main; + } else if (back != null) { + return back; + } + throw new IllegalStateException( + "%s must not contain two null elements".formatted(getClass().getSimpleName()) + ); + } +} diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/DefaultNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/DefaultNamer.java index b1639608d4c..ff7b7d17666 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/DefaultNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/DefaultNamer.java @@ -1,9 +1,9 @@ package org.opentripplanner.graph_builder.module.osm.naming; import org.opentripplanner.framework.i18n.I18NString; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; -import org.opentripplanner.street.model.edge.StreetEdge; public class DefaultNamer implements EdgeNamer { @@ -13,7 +13,7 @@ public I18NString name(OSMWithTags way) { } @Override - public void recordEdge(OSMWithTags way, StreetEdge edge) {} + public void recordEdges(OSMWithTags way, StreetEdgePair edge) {} @Override public void postprocess() {} diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java index 9d550d51ae6..fb0d3f583e0 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java @@ -3,6 +3,7 @@ import java.util.HashSet; import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.i18n.NonLocalizedString; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; import org.opentripplanner.street.model.edge.StreetEdge; @@ -69,28 +70,32 @@ public I18NString name(OSMWithTags way) { } @Override - public void recordEdge(OSMWithTags way, StreetEdge edge) { - if (!edge.hasBogusName()) { - return; // this edge already has a real name so there is nothing to do - } - String highway = way.getTag("highway"); - if ("motorway_link".equals(highway) || "trunk_link".equals(highway)) { - if (edge.isBack()) { - nameByDestination.add(edge); - } else { - nameByOrigin.add(edge); - } - } else if ( - "secondary_link".equals(highway) || - "primary_link".equals(highway) || - "tertiary_link".equals(highway) - ) { - if (edge.isBack()) { - nameByOrigin.add(edge); - } else { - nameByDestination.add(edge); - } - } + public void recordEdges(OSMWithTags way, StreetEdgePair edgePair) { + edgePair + .asIterable() + .forEach(edge -> { + if (!edge.hasBogusName()) { + return; // this edge already has a real name so there is nothing to do + } + String highway = way.getTag("highway"); + if ("motorway_link".equals(highway) || "trunk_link".equals(highway)) { + if (edge.isBack()) { + nameByDestination.add(edge); + } else { + nameByOrigin.add(edge); + } + } else if ( + "secondary_link".equals(highway) || + "primary_link".equals(highway) || + "tertiary_link".equals(highway) + ) { + if (edge.isBack()) { + nameByOrigin.add(edge); + } else { + nameByDestination.add(edge); + } + } + }); } @Override diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index d17de3f96c1..990d1d73442 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -30,6 +30,7 @@ import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.lang.DoubleUtils; import org.opentripplanner.framework.logging.ProgressTracker; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; import org.opentripplanner.street.model.edge.StreetEdge; @@ -71,23 +72,20 @@ public I18NString name(OSMWithTags way) { } @Override - public void recordEdge(OSMWithTags way, StreetEdge edge) { + public void recordEdges(OSMWithTags way, StreetEdgePair pair) { if (way.isSidewalk() && way.needsFallbackName() && !way.isExplicitlyUnnamed()) { - unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels())); + pair + .asIterable() + .forEach(edge -> unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels()))); } if (way.isNamed() && !way.isLink()) { // we generate two edges for each osm way: one there and one back. since we don't do any routing // in this class we don't need the two directions and keep only one of them. - var containsReverse = streetEdges - .query(edge.getGeometry().getEnvelopeInternal()) - .stream() - .anyMatch(candidate -> candidate.edge.isReverseOf(edge)); - if (!containsReverse) { - streetEdges.insert( - edge.getGeometry().getEnvelopeInternal(), - new EdgeOnLevel(edge, way.getLevels()) - ); - } + var edge = pair.pickAny(); + streetEdges.insert( + edge.getGeometry().getEnvelopeInternal(), + new EdgeOnLevel(edge, way.getLevels()) + ); } } diff --git a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java index 010b21ec2d1..60cf780f714 100644 --- a/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/services/osm/EdgeNamer.java @@ -3,13 +3,13 @@ import javax.annotation.Nonnull; import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.i18n.NonLocalizedString; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.module.osm.naming.DefaultNamer; import org.opentripplanner.graph_builder.module.osm.naming.PortlandCustomNamer; import org.opentripplanner.graph_builder.module.osm.naming.SidewalkNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; import org.opentripplanner.standalone.config.framework.json.NodeAdapter; import org.opentripplanner.standalone.config.framework.json.OtpVersion; -import org.opentripplanner.street.model.edge.StreetEdge; /** * Interface responsible for naming edges of the street graph. It allows you to write your own @@ -25,11 +25,11 @@ public interface EdgeNamer { * Callback function for each way/edge combination so that more complicated names can be built * in the post-processing step. */ - void recordEdge(OSMWithTags way, StreetEdge edge); + void recordEdges(OSMWithTags way, StreetEdgePair edge); /** * Called after each edge has been named to build a more complex name out of the relationships - * tracked in {@link EdgeNamer#recordEdge(OSMWithTags, StreetEdge)}. + * tracked in {@link EdgeNamer#recordEdges(OSMWithTags, StreetEdgePair)}. */ void postprocess(); diff --git a/src/test/java/org/opentripplanner/graph_builder/module/islandpruning/TestNamer.java b/src/test/java/org/opentripplanner/graph_builder/module/islandpruning/TestNamer.java index 0d2ad370541..4002da919d8 100644 --- a/src/test/java/org/opentripplanner/graph_builder/module/islandpruning/TestNamer.java +++ b/src/test/java/org/opentripplanner/graph_builder/module/islandpruning/TestNamer.java @@ -2,9 +2,9 @@ import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.i18n.NonLocalizedString; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWithTags; -import org.opentripplanner.street.model.edge.StreetEdge; class TestNamer implements EdgeNamer { @@ -14,7 +14,7 @@ public I18NString name(OSMWithTags way) { } @Override - public void recordEdge(OSMWithTags way, StreetEdge edge) {} + public void recordEdges(OSMWithTags way, StreetEdgePair edge) {} @Override public void postprocess() {} diff --git a/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java b/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java index 3faa77ee531..c1c3d7e5e73 100644 --- a/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java +++ b/src/test/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamerTest.java @@ -12,6 +12,7 @@ import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.framework.geometry.WgsCoordinate; import org.opentripplanner.framework.i18n.I18NString; +import org.opentripplanner.graph_builder.module.osm.StreetEdgePair; import org.opentripplanner.graph_builder.services.osm.EdgeNamer; import org.opentripplanner.openstreetmap.model.OSMWay; import org.opentripplanner.openstreetmap.wayproperty.specifier.WayTestData; @@ -92,7 +93,7 @@ EdgePair addStreetEdge(String name, WgsCoordinate... coordinates) { } void postProcess(EdgeNamer namer) { - pairs.forEach(p -> namer.recordEdge(p.way, p.edge)); + pairs.forEach(p -> namer.recordEdges(p.way, new StreetEdgePair(p.edge, null))); namer.postprocess(); } From 4013d70e0baca11a42e053ffaa76daa5ce188e62 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 6 May 2024 14:26:37 +0200 Subject: [PATCH 23/30] Remove unneeded operation --- .../module/osm/naming/SidewalkNamer.java | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 990d1d73442..a8dd169dfe5 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -23,7 +23,6 @@ import org.locationtech.jts.geom.MultiLineString; import org.locationtech.jts.geom.Point; import org.locationtech.jts.operation.buffer.BufferParameters; -import org.locationtech.jts.operation.distance.DistanceOp; import org.opentripplanner.framework.geometry.GeometryUtils; import org.opentripplanner.framework.geometry.HashGridSpatialIndex; import org.opentripplanner.framework.geometry.SphericalDistanceLibrary; @@ -146,8 +145,6 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam var candidates = streetEdges.query(buffer.getEnvelopeInternal()); groupEdgesByName(candidates) - // remove edges that are far away - .filter(g -> g.nearestDistanceTo(sidewalk.getGeometry()) < MAX_DISTANCE_TO_SIDEWALK) // make sure we only compare sidewalks and streets that are on the same level .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) @@ -240,19 +237,6 @@ private double length(Geometry intersection) { }; } - /** - * Get the closest distance in meters between any of the edges in the group and the given geometry. - */ - double nearestDistanceTo(Geometry g) { - return edges - .stream() - .mapToDouble(e -> { - var points = DistanceOp.nearestPoints(e.getGeometry(), g); - return SphericalDistanceLibrary.fastDistance(points[0], points[1]); - }) - .min() - .orElse(Double.MAX_VALUE); - } } private record EdgeOnLevel(StreetEdge edge, Set levels) {} From e28a69c75fd5d4681f2f8a6213caba640671327c Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 6 May 2024 14:34:47 +0200 Subject: [PATCH 24/30] Improve performance of Portland custom namer --- .../module/osm/naming/PortlandCustomNamer.java | 11 ++++------- .../module/osm/naming/SidewalkNamer.java | 1 - .../openstreetmap/model/OSMWithTags.java | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java index fb0d3f583e0..b178adaab6b 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java @@ -71,24 +71,21 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdges(OSMWithTags way, StreetEdgePair edgePair) { + final boolean isHighwayLink = way.isHighwayLink(); + final boolean isLowerLink = way.isLowerLink(); edgePair .asIterable() .forEach(edge -> { if (!edge.hasBogusName()) { return; // this edge already has a real name so there is nothing to do } - String highway = way.getTag("highway"); - if ("motorway_link".equals(highway) || "trunk_link".equals(highway)) { + if (isHighwayLink) { if (edge.isBack()) { nameByDestination.add(edge); } else { nameByOrigin.add(edge); } - } else if ( - "secondary_link".equals(highway) || - "primary_link".equals(highway) || - "tertiary_link".equals(highway) - ) { + } else if (isLowerLink) { if (edge.isBack()) { nameByOrigin.add(edge); } else { diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index a8dd169dfe5..19b750106fa 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -236,7 +236,6 @@ private double length(Geometry intersection) { ); }; } - } private record EdgeOnLevel(StreetEdge edge, Set levels) {} diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index 636584eb770..d0f0dea764e 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -574,6 +574,20 @@ public boolean isLink() { return highway != null && highway.endsWith(("_link")); } + public boolean isHighwayLink() { + String highway = getTag("highway"); + return "motorway_link".equals(highway) || "trunk_link".equals(highway); + } + + public boolean isLowerLink() { + String highway = getTag("highway"); + return ( + "secondary_link".equals(highway) || + "primary_link".equals(highway) || + "tertiary_link".equals(highway) + ); + } + public boolean isElevator() { return isTag("highway", "elevator"); } From f995865fbe33c64691c5ea91f3f546057facc60b Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 6 May 2024 14:40:36 +0200 Subject: [PATCH 25/30] Improve computation of center point --- .../module/osm/naming/SidewalkNamer.java | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 19b750106fa..5245a9e9887 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -96,7 +96,7 @@ public void postprocess() { unnamedSidewalks.size() ); - this.preciseBuffer = new PreciseBuffer(computeCentroid(), BUFFER_METERS); + this.preciseBuffer = new PreciseBuffer(computeEnvelopeCenter(), BUFFER_METERS); final AtomicInteger namesApplied = new AtomicInteger(0); unnamedSidewalks @@ -126,11 +126,12 @@ public void postprocess() { /** * Compute the centroid of all sidewalk edges. */ - private Coordinate computeCentroid() { + private Coordinate computeEnvelopeCenter() { var envelope = new Envelope(); - unnamedSidewalks.forEach(e -> - envelope.expandToInclude(e.edge.getGeometry().getEnvelopeInternal()) - ); + unnamedSidewalks.forEach(e -> { + envelope.expandToInclude(e.edge.getFromVertex().getCoordinate()); + envelope.expandToInclude(e.edge.getToVertex().getCoordinate()); + }); return envelope.centre(); } From e9a0722d9b6c4a6e28a48ca85a799fb4ece45e62 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 6 May 2024 14:51:01 +0200 Subject: [PATCH 26/30] Improve if/else logic --- .../graph_builder/module/osm/naming/SidewalkNamer.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 5245a9e9887..c1080a66772 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -57,7 +57,6 @@ public class SidewalkNamer implements EdgeNamer { private static final Logger LOG = LoggerFactory.getLogger(SidewalkNamer.class); - private static final int MAX_DISTANCE_TO_SIDEWALK = 50; private static final double MIN_PERCENT_IN_BUFFER = .85; private static final int BUFFER_METERS = 25; @@ -72,14 +71,16 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdges(OSMWithTags way, StreetEdgePair pair) { + // this way is a sidewalk and hasn't been named yet (and is not explicitly unnamed) if (way.isSidewalk() && way.needsFallbackName() && !way.isExplicitlyUnnamed()) { pair .asIterable() .forEach(edge -> unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels()))); } - if (way.isNamed() && !way.isLink()) { + // the way is _not_ a sidewalk and does have a name + else if (way.isNamed() && !way.isLink()) { // we generate two edges for each osm way: one there and one back. since we don't do any routing - // in this class we don't need the two directions and keep only one of them. + // in this class we don't need the two directions and index only one of them. var edge = pair.pickAny(); streetEdges.insert( edge.getGeometry().getEnvelopeInternal(), From 8a88c9932f5fdfcd7eb643730e8f18a38dd6f03f Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 6 May 2024 14:55:40 +0200 Subject: [PATCH 27/30] Update comment --- .../graph_builder/module/osm/naming/SidewalkNamer.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index c1080a66772..6016e8e1d44 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -79,8 +79,9 @@ public void recordEdges(OSMWithTags way, StreetEdgePair pair) { } // the way is _not_ a sidewalk and does have a name else if (way.isNamed() && !way.isLink()) { - // we generate two edges for each osm way: one there and one back. since we don't do any routing - // in this class we don't need the two directions and index only one of them. + // We generate two edges for each osm way: one there and one back. This spatial index only + // needs to contain one item for each road segment with a unique geometry and name, so we + // add only one of the two edges. var edge = pair.pickAny(); streetEdges.insert( edge.getGeometry().getEnvelopeInternal(), From 2b2d36505ab952ffb6bcb2ec7c7ac84c4f609361 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 8 May 2024 11:00:25 +0200 Subject: [PATCH 28/30] Clarify hasName/hasNoName/isExplicitlyUnnamed --- .../graph_builder/module/osm/OsmModule.java | 2 +- .../module/osm/WalkableAreaBuilder.java | 4 +-- .../module/osm/naming/SidewalkNamer.java | 2 +- .../openstreetmap/model/OSMWithTags.java | 33 ++++++++++++------- .../openstreetmap/model/OSMWithTagsTest.java | 4 +-- 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java index 79bcacda660..3a745dddf52 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java @@ -542,7 +542,7 @@ private StreetEdge getEdgeForStreet( .withSlopeOverride(way.getOsmProvider().getWayPropertySet().getSlopeOverride(way)) .withStairs(way.isSteps()) .withWheelchairAccessible(way.isWheelchairAccessible()) - .withBogusName(way.needsFallbackName()); + .withBogusName(way.hasNoName()); StreetEdge street = seb.buildAndConnect(); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java index 6244a50321f..89b71403232 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java @@ -532,7 +532,7 @@ private Set createSegments( .withBack(false) .withArea(edgeList) .withCarSpeed(carSpeed) - .withBogusName(areaEntity.needsFallbackName()) + .withBogusName(areaEntity.hasNoName()) .withWheelchairAccessible(areaEntity.isWheelchairAccessible()) .withLink(areaEntity.isLink()); @@ -555,7 +555,7 @@ private Set createSegments( .withBack(true) .withArea(edgeList) .withCarSpeed(carSpeed) - .withBogusName(areaEntity.needsFallbackName()) + .withBogusName(areaEntity.hasNoName()) .withWheelchairAccessible(areaEntity.isWheelchairAccessible()) .withLink(areaEntity.isLink()); diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 6016e8e1d44..196ebe73946 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -72,7 +72,7 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdges(OSMWithTags way, StreetEdgePair pair) { // this way is a sidewalk and hasn't been named yet (and is not explicitly unnamed) - if (way.isSidewalk() && way.needsFallbackName() && !way.isExplicitlyUnnamed()) { + if (way.isSidewalk() && way.hasNoName() && !way.isExplicitlyUnnamed()) { pair .asIterable() .forEach(edge -> unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels()))); diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index d0f0dea764e..b4c9c4b01b5 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -168,13 +168,6 @@ public boolean isSidewalk() { return isTag("footway", "sidewalk") && isTag("highway", "footway"); } - /** - * Whether this entity explicity doesn't have a name. - */ - public boolean isExplicitlyUnnamed() { - return isTagTrue("noname"); - } - protected boolean doesTagAllowAccess(String tag) { if (tags == null) { return false; @@ -602,17 +595,33 @@ public boolean isWheelchairAccessible() { } /** - * Does this entity has a name of its own or if it needs to have a fallback one assigned? + * Does this entity have tags that allow extracting a name? */ - public boolean needsFallbackName() { + public boolean isNamed() { + return hasTag("name") || hasTag("ref"); + } + + /** + * Is this entity unnamed? + *

+ * Perhaps this entity has a name that isn't in the source data, but it's also possible that + * it's explicitly tagged as not having one. + * + * @see OSMWithTags#isExplicitlyUnnamed() + */ + public boolean hasNoName() { return !isNamed(); } /** - * Does this entity have tags that allow extracting a name? + * Whether this entity explicitly doesn't have a name. This is different to no name being + * set on the entity in OSM. + * + * @see OSMWithTags#isNamed() + * @see https://wiki.openstreetmap.org/wiki/Tag:noname%3Dyes */ - public boolean isNamed() { - return hasTag("name") || hasTag("ref"); + public boolean isExplicitlyUnnamed() { + return isTagTrue("noname"); } /** diff --git a/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java b/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java index 3c060150714..3b50d0bff0d 100644 --- a/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java +++ b/src/test/java/org/opentripplanner/openstreetmap/model/OSMWithTagsTest.java @@ -266,9 +266,9 @@ void testGenerateI18NForPattern() { @Test void fallbackName() { var nameless = WayTestData.cycleway(); - assertTrue(nameless.needsFallbackName()); + assertTrue(nameless.hasNoName()); var namedTunnel = WayTestData.carTunnel(); - assertFalse(namedTunnel.needsFallbackName()); + assertFalse(namedTunnel.hasNoName()); } } From dbd8f2f1053c89ca183b5fa37e12612882394a70 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Mon, 13 May 2024 17:45:56 +0200 Subject: [PATCH 29/30] Remove separate section about OSM naming --- doc-templates/BuildConfiguration.md | 16 ---------------- docs/BuildConfiguration.md | 16 ---------------- 2 files changed, 32 deletions(-) diff --git a/doc-templates/BuildConfiguration.md b/doc-templates/BuildConfiguration.md index 756593a57a3..43a29e1fb79 100644 --- a/doc-templates/BuildConfiguration.md +++ b/doc-templates/BuildConfiguration.md @@ -140,22 +140,6 @@ The mechanism is that for any two identical tags, OTP will use the first one. } ``` -### Custom naming - -You can define a custom naming scheme for elements drawn from OSM by defining an `osmNaming` field -in `build-config.json`, such as: - -```JSON -// build-config.json -{ - "osmNaming": "portland" -} -``` - -There is currently only one custom naming module called `portland` (which has no parameters). - - - ## Elevation data OpenTripPlanner can "drape" the OSM street network over a digital elevation model (DEM). This allows diff --git a/docs/BuildConfiguration.md b/docs/BuildConfiguration.md index fe3a2109ba4..d2e7833c251 100644 --- a/docs/BuildConfiguration.md +++ b/docs/BuildConfiguration.md @@ -238,22 +238,6 @@ The mechanism is that for any two identical tags, OTP will use the first one. } ``` -### Custom naming - -You can define a custom naming scheme for elements drawn from OSM by defining an `osmNaming` field -in `build-config.json`, such as: - -```JSON -// build-config.json -{ - "osmNaming": "portland" -} -``` - -There is currently only one custom naming module called `portland` (which has no parameters). - - - ## Elevation data OpenTripPlanner can "drape" the OSM street network over a digital elevation model (DEM). This allows From 60c4ee925f51689bacd596f517ea709984b36615 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Tue, 14 May 2024 11:52:48 +0200 Subject: [PATCH 30/30] Incorporate review feedback --- .../graph_builder/module/osm/OsmModule.java | 4 +--- .../module/osm/naming/PortlandCustomNamer.java | 18 ++++++++++++++++-- .../module/osm/naming/SidewalkNamer.java | 8 ++++---- .../openstreetmap/model/OSMWithTags.java | 14 -------------- 4 files changed, 21 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java index 3a745dddf52..10c215ee448 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java @@ -544,9 +544,7 @@ private StreetEdge getEdgeForStreet( .withWheelchairAccessible(way.isWheelchairAccessible()) .withBogusName(way.hasNoName()); - StreetEdge street = seb.buildAndConnect(); - - return street; + return seb.buildAndConnect(); } private float getMaxCarSpeed() { diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java index b178adaab6b..c0ca595fbd7 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/PortlandCustomNamer.java @@ -71,8 +71,8 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdges(OSMWithTags way, StreetEdgePair edgePair) { - final boolean isHighwayLink = way.isHighwayLink(); - final boolean isLowerLink = way.isLowerLink(); + final boolean isHighwayLink = isHighwayLink(way); + final boolean isLowerLink = isLowerLink(way); edgePair .asIterable() .forEach(edge -> { @@ -185,4 +185,18 @@ private static String nameAccordingToOrigin(StreetEdge e, int maxDepth) { } return null; } + + private static boolean isHighwayLink(OSMWithTags way) { + String highway = way.getTag("highway"); + return "motorway_link".equals(highway) || "trunk_link".equals(highway); + } + + private static boolean isLowerLink(OSMWithTags way) { + String highway = way.getTag("highway"); + return ( + "secondary_link".equals(highway) || + "primary_link".equals(highway) || + "tertiary_link".equals(highway) + ); + } } diff --git a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java index 196ebe73946..eafccc0da1b 100644 --- a/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java +++ b/src/main/java/org/opentripplanner/graph_builder/module/osm/naming/SidewalkNamer.java @@ -71,13 +71,13 @@ public I18NString name(OSMWithTags way) { @Override public void recordEdges(OSMWithTags way, StreetEdgePair pair) { - // this way is a sidewalk and hasn't been named yet (and is not explicitly unnamed) + // This way is a sidewalk and hasn't been named yet (and is not explicitly unnamed) if (way.isSidewalk() && way.hasNoName() && !way.isExplicitlyUnnamed()) { pair .asIterable() .forEach(edge -> unnamedSidewalks.add(new EdgeOnLevel(edge, way.getLevels()))); } - // the way is _not_ a sidewalk and does have a name + // The way is _not_ a sidewalk and does have a name else if (way.isNamed() && !way.isLink()) { // We generate two edges for each osm way: one there and one back. This spatial index only // needs to contain one item for each road segment with a unique geometry and name, so we @@ -120,7 +120,7 @@ public void postprocess() { LOG.info(progress.completeMessage()); - // set the indices to null so they can be garbage-collected + // Set the indices to null so they can be garbage-collected streetEdges = null; unnamedSidewalks = null; } @@ -148,7 +148,7 @@ private void assignNameToSidewalk(EdgeOnLevel sidewalkOnLevel, AtomicInteger nam var candidates = streetEdges.query(buffer.getEnvelopeInternal()); groupEdgesByName(candidates) - // make sure we only compare sidewalks and streets that are on the same level + // Make sure we only compare sidewalks and streets that are on the same level .filter(g -> g.levels.equals(sidewalkOnLevel.levels)) .map(g -> computePercentInsideBuffer(g, buffer, sidewalkLength)) // Remove those groups where less than a certain percentage is inside the buffer around diff --git a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java index b4c9c4b01b5..b53739bf6a1 100644 --- a/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java +++ b/src/main/java/org/opentripplanner/openstreetmap/model/OSMWithTags.java @@ -567,20 +567,6 @@ public boolean isLink() { return highway != null && highway.endsWith(("_link")); } - public boolean isHighwayLink() { - String highway = getTag("highway"); - return "motorway_link".equals(highway) || "trunk_link".equals(highway); - } - - public boolean isLowerLink() { - String highway = getTag("highway"); - return ( - "secondary_link".equals(highway) || - "primary_link".equals(highway) || - "tertiary_link".equals(highway) - ); - } - public boolean isElevator() { return isTag("highway", "elevator"); }