Merge pull request #6486 from HSLdevcom/keep-interior-rings

Improve street routing within concave parts of area holes

application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmArea.java

@@ -12,6 +12,8 @@
 import org.locationtech.jts.geom.Point;
 import org.locationtech.jts.geom.Polygon;
 import org.locationtech.jts.geom.TopologyException;
+import org.locationtech.jts.operation.valid.IsValidOp;
+import org.locationtech.jts.operation.valid.TopologyValidationError;
 import org.opentripplanner.framework.geometry.GeometryUtils;
 import org.opentripplanner.osm.model.OsmEntity;
 import org.opentripplanner.osm.model.OsmNode;
@@ -40,7 +42,7 @@ class OsmArea {
     List<TLongList> innerRingNodes = constructRings(innerRingWays);
     List<TLongList> outerRingNodes = constructRings(outerRingWays);
     if (innerRingNodes == null || outerRingNodes == null) {
-      throw new AreaConstructionException();
+      throw new AreaConstructionException("innerRing or outerRing nodes are null");
     }
     ArrayList<TLongList> allRings = new ArrayList<>(innerRingNodes);
     allRings.addAll(outerRingNodes);
@@ -75,7 +77,7 @@ class OsmArea {
         }
       }
     } catch (TopologyException ex) {
-      throw new AreaConstructionException();
+      throw new AreaConstructionException(ex.getMessage());
     }
 
     // Make outermostRings immutable
@@ -148,7 +150,7 @@ public List<TLongList> constructRings(List<OsmWay> ways) {
 
   /**
    * Try to extract a point which is in  the middle of the area and
-   * also insaide the area geometry.
+   * also inside the area geometry.
    *
    * @return Point geometry inside the area
    */
@@ -168,8 +170,12 @@ private MultiPolygon calculateJTSMultiPolygon() {
     MultiPolygon jtsMultiPolygon = GeometryUtils
       .getGeometryFactory()
       .createMultiPolygon(polygons.toArray(new Polygon[0]));
-    if (!jtsMultiPolygon.isValid()) {
-      throw new AreaConstructionException();
+    var validOp = new IsValidOp(jtsMultiPolygon);
+    if (!validOp.isValid()) {
+      var validationError = validOp.getValidationError();
+      throw new AreaConstructionException(
+        "%s at %s".formatted(validationError.getMessage(), validationError.getCoordinate())
+      );
     }
 
     return jtsMultiPolygon;
@@ -252,5 +258,17 @@ private boolean constructRingsRecursive(
     return false;
   }
 
-  public static class AreaConstructionException extends RuntimeException {}
+  public static class AreaConstructionException extends RuntimeException {
+
+    private final String message;
+
+    public AreaConstructionException(String message) {
+      this.message = message;
+    }
+
+    @Override
+    public String getMessage() {
+      return message;
+    }
+  }
 }

application/src/main/java/org/opentripplanner/graph_builder/module/osm/WalkableAreaBuilder.java

@@ -72,7 +72,6 @@ class WalkableAreaBuilder {
   private final Set<String> boardingLocationRefTags;
   private final EdgeNamer namer;
   private final SafetyValueNormalizer normalizer;
-  private final VertexFactory vertexFactory;
 
   // template for AreaEdge names
   private static final String labelTemplate = "way (area) %s from %s to %s";
@@ -111,7 +110,6 @@ public WalkableAreaBuilder(
           .filter(this::isPlatformLinkingPoint)
           .collect(Collectors.toList())
         : List.of();
-    this.vertexFactory = new VertexFactory(graph);
   }
 
   /**
@@ -144,15 +142,16 @@ public void buildWithoutVisibility(OsmAreaGroup group) {
           }
         }
       }
-      edges
+      var vertices = edges
         .stream()
         .flatMap(v ->
           Stream
             .of(v.getFromVertex(), v.getToVertex())
             .filter(IntersectionVertex.class::isInstance)
             .map(IntersectionVertex.class::cast)
         )
-        .forEach(areaGroup::addVisibilityVertex);
+        .collect(Collectors.toSet());
+      areaGroup.addVisibilityVertices(vertices);
 
       createAreas(areaGroup, ring, group.areas);
     }
@@ -161,7 +160,6 @@ public void buildWithoutVisibility(OsmAreaGroup group) {
   public void buildWithVisibility(OsmAreaGroup group) {
     // These sets contain the nodes/vertices which can be used to traverse from the rest of the
     // street network onto the walkable area
-    Set<OsmNode> startingNodes = new HashSet<>();
     Set<Vertex> startingVertices = new HashSet<>();
 
     // List of edges belonging to the walkable area
@@ -188,9 +186,9 @@ public void buildWithVisibility(OsmAreaGroup group) {
       AreaGroup areaGroup = new AreaGroup(polygon);
 
       // the points corresponding to concave or hole vertices or those linked to ways
-      HashSet<OsmNode> visibilityNodes = new HashSet<>();
       HashSet<NodeEdge> alreadyAddedEdges = new HashSet<>();
       HashSet<IntersectionVertex> platformLinkingVertices = new HashSet<>();
+      HashSet<IntersectionVertex> visibilityVertices = new HashSet<>();
       GeometryFactory geometryFactory = GeometryUtils.getGeometryFactory();
 
       OsmEntity areaEntity = group.getSomeOsmObject();
@@ -209,9 +207,8 @@ public void buildWithVisibility(OsmAreaGroup group) {
         for (OsmNode node : entrances) {
           var vertex = vertexBuilder.getVertexForOsmNode(node, areaEntity);
           platformLinkingVertices.add(vertex);
-          visibilityNodes.add(node);
-          startingNodes.add(node);
-          areaGroup.addVisibilityVertex(vertex);
+          visibilityVertices.add(vertex);
+          startingVertices.add(vertex);
         }
 
         for (Ring outerRing : area.outermostRings) {
@@ -227,10 +224,8 @@ public void buildWithVisibility(OsmAreaGroup group) {
               .toList();
             platformLinkingVertices.addAll(verticesWithin);
             for (OsmVertex v : verticesWithin) {
-              OsmNode node = osmdb.getNode(v.nodeId);
-              visibilityNodes.add(node);
-              startingNodes.add(node);
-              areaGroup.addVisibilityVertex(v);
+              startingVertices.add(v);
+              visibilityVertices.add(v);
               linkPointsAdded = true;
             }
           }
@@ -255,39 +250,43 @@ public void buildWithVisibility(OsmAreaGroup group) {
               outerRing.isNodeConvex(i) ||
               (linkPointsAdded && (i == 0 || i == outerRing.nodes.size() / 2))
             ) {
-              visibilityNodes.add(node);
-              areaGroup.addVisibilityVertex(vertexBuilder.getVertexForOsmNode(node, areaEntity));
+              visibilityVertices.add(vertexBuilder.getVertexForOsmNode(node, areaEntity));
             }
             if (isStartingNode(node, osmWayIds)) {
-              visibilityNodes.add(node);
-              startingNodes.add(node);
-              areaGroup.addVisibilityVertex(vertexBuilder.getVertexForOsmNode(node, areaEntity));
+              var v = vertexBuilder.getVertexForOsmNode(node, areaEntity);
+              startingVertices.add(v);
+              visibilityVertices.add(v);
             }
           }
           for (Ring innerRing : outerRing.getHoles()) {
             for (int j = 0; j < innerRing.nodes.size(); ++j) {
               OsmNode node = innerRing.nodes.get(j);
-              edges.addAll(
-                createEdgesForRingSegment(areaGroup, area, innerRing, j, alreadyAddedEdges)
+              var newEdges = createEdgesForRingSegment(
+                areaGroup,
+                area,
+                innerRing,
+                j,
+                alreadyAddedEdges
               );
+              edges.addAll(newEdges);
+              ringEdges.addAll(newEdges);
               // A node can only be a visibility node only if it is an entrance to the
               // area or a convex point, i.e. the angle is over 180 degrees.
               // For holes, we must swap the convexity condition
               if (!innerRing.isNodeConvex(j)) {
-                visibilityNodes.add(node);
-                areaGroup.addVisibilityVertex(vertexBuilder.getVertexForOsmNode(node, areaEntity));
+                visibilityVertices.add(vertexBuilder.getVertexForOsmNode(node, areaEntity));
               }
               if (isStartingNode(node, osmWayIds)) {
-                visibilityNodes.add(node);
-                startingNodes.add(node);
-                areaGroup.addVisibilityVertex(vertexBuilder.getVertexForOsmNode(node, areaEntity));
+                var v = vertexBuilder.getVertexForOsmNode(node, areaEntity);
+                startingVertices.add(v);
+                visibilityVertices.add(v);
               }
             }
           }
         }
       }
 
-      if (areaGroup.visibilityVertices().isEmpty()) {
+      if (visibilityVertices.isEmpty()) {
         issueStore.add(new UnconnectedArea(group));
         // Area is not connected to graph. Remove it immediately before it causes any trouble.
         for (Edge edge : edges) {
@@ -296,41 +295,36 @@ public void buildWithVisibility(OsmAreaGroup group) {
         continue;
       }
 
+      areaGroup.addVisibilityVertices(visibilityVertices);
       createAreas(areaGroup, ring, group.areas);
 
-      if (visibilityNodes.size() > maxAreaNodes) {
-        issueStore.add(new AreaTooComplicated(group, visibilityNodes.size(), maxAreaNodes));
+      if (visibilityVertices.size() > maxAreaNodes) {
+        issueStore.add(new AreaTooComplicated(group, visibilityVertices.size(), maxAreaNodes));
       }
 
       // if area is too complex, consider only part of visibility nodes
       // so that at least some edges passing through the area are added
       // otherwise routing can use only area boundary edges
-      float skip_ratio = (float) maxAreaNodes / (float) visibilityNodes.size();
+      float skip_ratio = (float) maxAreaNodes / (float) visibilityVertices.size();
       int i = 0;
       float sum_i = 0;
-      for (OsmNode nodeI : visibilityNodes) {
+      for (IntersectionVertex vertex1 : visibilityVertices) {
         sum_i += skip_ratio;
         if (Math.floor(sum_i) < i + 1) {
           continue;
         }
         i = (int) Math.floor(sum_i);
-        IntersectionVertex vertex1 = vertexBuilder.getVertexForOsmNode(nodeI, areaEntity);
-        if (startingNodes.contains(nodeI)) {
-          startingVertices.add(vertex1);
-        }
         int j = 0;
         float sum_j = 0;
-        for (OsmNode nodeJ : visibilityNodes) {
+        for (IntersectionVertex vertex2 : visibilityVertices) {
           sum_j += skip_ratio;
           if (Math.floor(sum_j) < j + 1) {
             continue;
           }
           j = (int) Math.floor(sum_j);
-          if (shouldSkipEdge(nodeI, nodeJ, alreadyAddedEdges)) {
+          if (shouldSkipEdge(vertex1, vertex2, alreadyAddedEdges)) {
             continue;
           }
-          IntersectionVertex vertex2 = vertexBuilder.getVertexForOsmNode(nodeJ, areaEntity);
-
           Coordinate[] coordinates = new Coordinate[] {
             vertex1.getCoordinate(),
             vertex2.getCoordinate(),
@@ -429,12 +423,12 @@ private Set<AreaEdge> createEdgesForRingSegment(
   ) {
     OsmNode node = ring.nodes.get(i);
     OsmNode nextNode = ring.nodes.get((i + 1) % ring.nodes.size());
+    IntersectionVertex v1 = vertexBuilder.getVertexForOsmNode(node, area.parent);
+    IntersectionVertex v2 = vertexBuilder.getVertexForOsmNode(nextNode, area.parent);
 
-    if (shouldSkipEdge(node, nextNode, alreadyAddedEdges)) {
+    if (shouldSkipEdge(v1, v2, alreadyAddedEdges)) {
       return Set.of();
     }
-    IntersectionVertex v1 = vertexBuilder.getVertexForOsmNode(node, area.parent);
-    IntersectionVertex v2 = vertexBuilder.getVertexForOsmNode(nextNode, area.parent);
 
     return createSegments(v1, v2, List.of(area), areaGroup, false);
   }
@@ -598,22 +592,20 @@ public boolean shouldSkipEdge(State current, Edge edge) {
   }
 
   private boolean shouldSkipEdge(
-    OsmNode nodeI,
-    OsmNode nodeJ,
+    IntersectionVertex v1,
+    IntersectionVertex v2,
     HashSet<NodeEdge> alreadyAddedEdges
   ) {
-    if (nodeI == nodeJ) {
+    if (v1 == v2) {
       return true;
     }
-    NodeEdge edge = new NodeEdge(nodeI, nodeJ);
-    if (
-      alreadyAddedEdges.contains(edge) || alreadyAddedEdges.contains(new NodeEdge(nodeJ, nodeI))
-    ) {
+    NodeEdge edge = new NodeEdge(v1, v2);
+    if (alreadyAddedEdges.contains(edge) || alreadyAddedEdges.contains(new NodeEdge(v2, v1))) {
       return true;
     }
     alreadyAddedEdges.add(edge);
     return false;
   }
 
-  private record NodeEdge(OsmNode from, OsmNode to) {}
+  private record NodeEdge(IntersectionVertex from, IntersectionVertex to) {}
 }

application/src/main/java/org/opentripplanner/routing/linking/VertexLinker.java

@@ -634,7 +634,7 @@ public void addAreaVertex(
       }
     }
     if (scope == Scope.PERMANENT) {
-      areaGroup.addVisibilityVertex(newVertex);
+      areaGroup.addVisibilityVertices(Set.of(newVertex));
     }
   }
 

application/src/main/java/org/opentripplanner/street/model/edge/AreaGroup.java

@@ -2,11 +2,9 @@
 
 import java.io.Serializable;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
-import java.util.Objects;
 import java.util.Set;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import org.locationtech.jts.geom.Geometry;
 import org.locationtech.jts.geom.Polygon;
 import org.opentripplanner.street.model.vertex.IntersectionVertex;
@@ -52,18 +50,16 @@ public Set<IntersectionVertex> visibilityVertices() {
   }
 
   /**
-   * Add a visibility vertex to this edge.
+   * Add a set of visibility vertices to this area group
    */
-  public void addVisibilityVertex(IntersectionVertex toBeAdded) {
-    Objects.requireNonNull(toBeAdded);
+  public void addVisibilityVertices(Set<IntersectionVertex> toAdd) {
     synchronized (this) {
       if (visibilityVertices == EMPTY_SET) {
-        visibilityVertices = Set.of(toBeAdded);
+        visibilityVertices = Set.copyOf(toAdd);
       } else {
-        visibilityVertices =
-          Stream
-            .concat(visibilityVertices.stream(), Stream.of(toBeAdded))
-            .collect(Collectors.toUnmodifiableSet());
+        var temp = new HashSet<>(visibilityVertices);
+        temp.addAll(toAdd);
+        visibilityVertices = Set.copyOf(temp);
       }
     }
   }

application/src/test/java/org/opentripplanner/graph_builder/module/osm/moduletests/_support/NodeBuilder.java

@@ -0,0 +1,13 @@
+package org.opentripplanner.graph_builder.module.osm.moduletests._support;
+
+import org.opentripplanner.framework.geometry.WgsCoordinate;
+import org.opentripplanner.osm.model.OsmNode;
+
+public class NodeBuilder {
+
+  public static OsmNode node(long id, WgsCoordinate wgsCoordinate) {
+    var node = new OsmNode(wgsCoordinate.latitude(), wgsCoordinate.longitude());
+    node.setId(id);
+    return node;
+  }
+}

application/src/test/java/org/opentripplanner/graph_builder/module/osm/moduletests/_support/RelationBuilder.java

@@ -0,0 +1,30 @@
+package org.opentripplanner.graph_builder.module.osm.moduletests._support;
+
+import org.opentripplanner.osm.model.OsmMemberType;
+import org.opentripplanner.osm.model.OsmRelation;
+import org.opentripplanner.osm.model.OsmRelationMember;
+
+public class RelationBuilder {
+
+  private final OsmRelation relation = new OsmRelation();
+
+  public static RelationBuilder ofMultiPolygon() {
+    var builder = new RelationBuilder();
+    builder.relation.addTag("type", "multipolygon");
+    builder.relation.addTag("highway", "pedestrian");
+    return builder;
+  }
+
+  public RelationBuilder withWayMember(long id, String role) {
+    var member = new OsmRelationMember();
+    member.setRole(role);
+    member.setType(OsmMemberType.WAY);
+    member.setRef(id);
+    relation.addMember(member);
+    return this;
+  }
+
+  public OsmRelation build() {
+    return relation;
+  }
+}