diff --git a/application/src/main/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModule.java b/application/src/main/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModule.java index 7e2ba334f2f..07c39cc2d8a 100644 --- a/application/src/main/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModule.java +++ b/application/src/main/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModule.java @@ -115,7 +115,6 @@ public void buildGraph() { private boolean connectVertexToStop(TransitStopVertex ts, StreetIndex index) { var stop = ts.getStop(); var stopCode = stop.getCode(); - var stopId = stop.getId().getId(); Envelope envelope = new Envelope(ts.getCoordinate()); double xscale = Math.cos(ts.getCoordinate().y * Math.PI / 180); @@ -151,17 +150,19 @@ private boolean connectVertexToStop(TransitStopVertex ts, StreetIndex index) { var nearbyEdges = new HashMap>(); for (var edge : index.getEdgesForEnvelope(envelope)) { - osmInfoGraphBuildService.findPlatform(edge).ifPresent(platform -> { - if (matchesReference(stop, platform.references())) { - if (!nearbyEdges.containsKey(platform)) { - var list = new ArrayList(); - list.add(edge); - nearbyEdges.put(platform, list); - } else { - nearbyEdges.get(platform).add(edge); + osmInfoGraphBuildService + .findPlatform(edge) + .ifPresent(platform -> { + if (matchesReference(stop, platform.references())) { + if (!nearbyEdges.containsKey(platform)) { + var list = new ArrayList(); + list.add(edge); + nearbyEdges.put(platform, list); + } else { + nearbyEdges.get(platform).add(edge); + } } - } - }); + }); } for (var platformEdgeList : nearbyEdges.entrySet()) { diff --git a/application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java b/application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java index 673611f367f..195d36b9ed1 100644 --- a/application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java +++ b/application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java @@ -92,7 +92,12 @@ public static OsmModuleBuilder of( OsmInfoGraphBuildRepository osmInfoGraphBuildRepository, VehicleParkingRepository vehicleParkingRepository ) { - return new OsmModuleBuilder(providers, graph, osmInfoGraphBuildRepository, vehicleParkingRepository); + return new OsmModuleBuilder( + providers, + graph, + osmInfoGraphBuildRepository, + vehicleParkingRepository + ); } public static OsmModuleBuilder of( diff --git a/application/src/main/java/org/opentripplanner/service/osminfo/model/Platform.java b/application/src/main/java/org/opentripplanner/service/osminfo/model/Platform.java index 81ff388f69e..91d78385a34 100644 --- a/application/src/main/java/org/opentripplanner/service/osminfo/model/Platform.java +++ b/application/src/main/java/org/opentripplanner/service/osminfo/model/Platform.java @@ -4,5 +4,4 @@ import org.locationtech.jts.geom.LineString; import org.opentripplanner.framework.i18n.I18NString; -public record Platform(I18NString name, LineString geometry, Set references) { -} \ No newline at end of file +public record Platform(I18NString name, LineString geometry, Set references) {} diff --git a/application/src/test/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModuleTest.java b/application/src/test/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModuleTest.java index e0cf32eda9b..a4d5e86ced8 100644 --- a/application/src/test/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModuleTest.java +++ b/application/src/test/java/org/opentripplanner/graph_builder/module/OsmBoardingLocationsModuleTest.java @@ -2,14 +2,21 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.File; +import java.util.List; +import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; +import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.opentripplanner.framework.geometry.SphericalDistanceLibrary; +import org.opentripplanner.framework.i18n.I18NString; import org.opentripplanner.framework.i18n.NonLocalizedString; import org.opentripplanner.graph_builder.module.osm.OsmModule; import org.opentripplanner.osm.OsmProvider; @@ -34,15 +41,11 @@ import org.opentripplanner.transit.service.SiteRepository; import org.opentripplanner.transit.service.TimetableRepository; -/** - * We test that the platform area at Herrenberg station (https://www.openstreetmap.org/way/27558650) - * is correctly linked to the stop even though it is not the closest edge to the stop. - */ class OsmBoardingLocationsModuleTest { private final TimetableRepositoryForTest testModel = TimetableRepositoryForTest.of(); - static Stream testCases() { + static Stream herrenbergTestCases() { return Stream.of( Arguments.of( false, @@ -55,10 +58,14 @@ static Stream testCases() { ); } + /** + * We test that the platform area at Herrenberg station (https://www.openstreetmap.org/way/27558650) + * is correctly linked to the stop even though it is not the closest edge to the stop. + */ @ParameterizedTest( name = "add boarding locations and link them to platform edges when skipVisibility={0}" ) - @MethodSource("testCases") + @MethodSource("herrenbergTestCases") void addAndLinkBoardingLocations(boolean areaVisibility, Set linkedVertices) { File file = ResourceLoader .of(OsmBoardingLocationsModuleTest.class) @@ -69,7 +76,7 @@ void addAndLinkBoardingLocations(boolean areaVisibility, Set linkedVerti .build(); RegularStop busStop = testModel.stop("de:08115:4512:5:C", 48.59434, 8.86452).build(); RegularStop floatingBusStop = testModel.stop("floating-bus-stop", 48.59417, 8.86464).build(); - + var deduplicator = new Deduplicator(); var graph = new Graph(deduplicator); var timetableRepository = new TimetableRepository(new SiteRepository(), deduplicator); @@ -182,6 +189,201 @@ void addAndLinkBoardingLocations(boolean areaVisibility, Set linkedVerti .forEach(e -> assertEquals("Platform 101;102", e.getName().toString())); } + /** + * We test that the underground platforms at Moorgate station (https://www.openstreetmap.org/way/1328222021) + * is correctly linked to the stop even though it is not the closest edge to the stop. + */ + @Test + void testLinearPlatforms() { + var deduplicator = new Deduplicator(); + var graph = new Graph(deduplicator); + var osmInfoRepository = new DefaultOsmInfoGraphBuildRepository(); + var osmModule = OsmModule + .of( + new OsmProvider( + ResourceLoader.of(OsmBoardingLocationsModuleTest.class).file("moorgate.osm.pbf"), + false + ), + graph, + osmInfoRepository, + new DefaultVehicleParkingRepository() + ) + .withBoardingAreaRefTags(Set.of("naptan:AtcoCode")) + .build(); + osmModule.buildGraph(); + + var factory = new VertexFactory(graph); + + class TestCase { + + /** + * The linear platform to be tested + */ + public final RegularStop platform; + + /** + * The label of a vertex where the centroid should be connected to + */ + public final VertexLabel beginLabel; + + /** + * The label of the other vertex where the centroid should be connected to + */ + public final VertexLabel endLabel; + + private TransitStopVertex platformVertex = null; + + public TestCase(RegularStop platform, VertexLabel beginLabel, VertexLabel endLabel) { + this.platform = platform; + this.beginLabel = beginLabel; + this.endLabel = endLabel; + } + + /** + * Get a TransitStopVertex for the platform in the graph. It is made and added to the graph + * on the first call. + */ + TransitStopVertex getPlatformVertex() { + if (platformVertex == null) { + platformVertex = factory.transitStop(TransitStopVertex.of().withStop(platform)); + } + return platformVertex; + } + } + + var testCases = List.of( + new TestCase( + testModel + .stop("9100MRGT9") + .withName(I18NString.of("Moorgate (Platform 9)")) + .withCoordinate(51.51922107872304, -0.08767468698832413) + .withPlatformCode("9") + .build(), + VertexLabel.osm(12288669589L), + VertexLabel.osm(12288675219L) + ), + new TestCase( + testModel + .stop("9400ZZLUMGT3") + .withName(I18NString.of("Moorgate (Platform 7)")) + .withCoordinate(51.51919235051611, -0.08769925990953176) + .withPlatformCode("7") + .build(), + VertexLabel.osm(12288669575L), + VertexLabel.osm(12288675230L) + ) + ); + + for (var testCase : testCases) { + // test that the platforms are not connected + var platformVertex = testCase.getPlatformVertex(); + assertEquals(0, platformVertex.getIncoming().size()); + assertEquals(0, platformVertex.getOutgoing().size()); + + // test that the vertices to be connected by the centroid are currently connected + var fromVertex = Objects.requireNonNull(graph.getVertex(testCase.beginLabel)); + var toVertex = Objects.requireNonNull(graph.getVertex(testCase.endLabel)); + assertTrue( + getEdge(fromVertex, toVertex).isPresent(), + "malformed test: the vertices where the centroid is supposed to be located between aren't connected" + ); + assertTrue( + getEdge(toVertex, fromVertex).isPresent(), + "malformed test: the vertices where the centroid is supposed to be located between aren't connected" + ); + } + + var timetableRepository = new TimetableRepository(new SiteRepository(), deduplicator); + new OsmBoardingLocationsModule( + graph, + new DefaultOsmInfoGraphBuildService(osmInfoRepository), + timetableRepository + ) + .buildGraph(); + + var boardingLocations = graph.getVerticesOfType(OsmBoardingLocationVertex.class); + + for (var testCase : testCases) { + var platformVertex = testCase.getPlatformVertex(); + var fromVertex = Objects.requireNonNull(graph.getVertex(testCase.beginLabel)); + var toVertex = Objects.requireNonNull(graph.getVertex(testCase.endLabel)); + + var centroid = boardingLocations + .stream() + .filter(b -> b.references.contains(testCase.platform.getId().getId())) + .findFirst() + .orElseThrow(); + + // TODO: we should ideally place the centroid vertex directly on the platform by splitting + // the platform edge, but it is too difficult to touch the splitter code to use a given + // centroid vertex instead of a generated split vertex, so what we actually do is to directly + // connect the platform vertex to the split vertex + + // the actual centroid isn't used + assertEquals(0, centroid.getDegreeIn()); + assertEquals(0, centroid.getDegreeOut()); + + for (var vertex : platformVertex.getIncoming()) { + assertSplitVertex(vertex.getFromVertex(), centroid, fromVertex, toVertex); + } + + for (var vertex : platformVertex.getOutgoing()) { + assertSplitVertex(vertex.getToVertex(), centroid, fromVertex, toVertex); + } + } + } + + /** + * Assert that a split vertex is near to the given centroid, and it is possible to travel between + * the original vertices through the split vertex in a straight line + */ + private static void assertSplitVertex( + Vertex splitVertex, + OsmBoardingLocationVertex centroid, + Vertex begin, + Vertex end + ) { + var distance = SphericalDistanceLibrary.distance( + splitVertex.getCoordinate(), + centroid.getCoordinate() + ); + // FIXME: I am not sure why the calculated centroid from the original OSM geometry is about 2 m + // from the platform + assertTrue(distance < 4, "The split vertex is more than 4 m apart from the centroid"); + assertConnections(splitVertex, begin, end); + + if (splitVertex != begin && splitVertex != end) { + var forwardEdges = getEdge(begin, splitVertex) + .flatMap(first -> getEdge(splitVertex, end).map(second -> List.of(first, second))); + var backwardEdges = getEdge(end, splitVertex) + .flatMap(first -> getEdge(splitVertex, begin).map(second -> List.of(first, second))); + for (var edgeList : List.of(forwardEdges, backwardEdges)) { + edgeList.ifPresent(edges -> + assertEquals( + edges.getFirst().getOutAngle(), + edges.getLast().getInAngle(), + "The split vertex is not on a straight line between the connected vertices" + ) + ); + } + } + } + + /** + * Assert that there is a one-way path from the beginning through the given vertex to the end + * or vice versa. + */ + private static void assertConnections(Vertex vertex, Vertex beginning, Vertex end) { + if (vertex == beginning || vertex == end) { + assertTrue(beginning.isConnected(end)); + } + + assertTrue( + (getEdge(beginning, vertex).isPresent() && getEdge(vertex, end).isPresent()) || + (getEdge(end, vertex).isPresent() && getEdge(vertex, beginning).isPresent()) + ); + } + private void assertConnections( OsmBoardingLocationVertex busBoardingLocation, Set> expected @@ -192,4 +394,12 @@ private void assertConnections( assertEquals(expected, edges.stream().map(Edge::getClass).collect(Collectors.toSet())) ); } + + private static Optional getEdge(Vertex from, Vertex to) { + return from + .getOutgoingStreetEdges() + .stream() + .filter(edge -> edge.getToVertex() == to) + .findFirst(); + } } diff --git a/application/src/test/resources/org/opentripplanner/graph_builder/module/moorgate.osm.pbf b/application/src/test/resources/org/opentripplanner/graph_builder/module/moorgate.osm.pbf new file mode 100644 index 00000000000..ee95a0e7c51 Binary files /dev/null and b/application/src/test/resources/org/opentripplanner/graph_builder/module/moorgate.osm.pbf differ