Add test, docs

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.
 
 <h3 id="osmNaming">osmNaming</h3>
 
-**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.
 

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

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()));
+  }
 }

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.
+ * <p>
+ * 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.
+ * <p>
+ * 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<EdgeOnLevel> streetEdges = new HashGridSpatialIndex<>();
   private Collection<EdgeOnLevel> unnamedSidewalks = new ArrayList<>();
@@ -102,17 +121,22 @@ 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();
     envelope.expandBy(0.000002);
     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<CandidateGroup> groupEdgesByName(List<EdgeOnLevel> candidates) {
     return candidates
       .stream()
@@ -157,6 +190,13 @@ private static Stream<CandidateGroup> groupEdgesByName(List<EdgeOnLevel> 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.
+   * <p>
+   * 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.
+   * <p>
    * 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<StreetEdge> edges, Set<String> levels) {
+    /**
+     * How much of this group intersects with the give geometry, in meters.
+     */
     double intersectionLength(Geometry polygon) {
       return edges
         .stream()

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<EdgePair> 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) {}
+}

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

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