From cc95cdcf0ba763d4518da549c5b9b5237f26cc37 Mon Sep 17 00:00:00 2001 From: Leonard Ehrenfried Date: Wed, 27 Mar 2024 21:40:05 +0100 Subject: [PATCH] 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 +}