Better fallback in shape geometry processing

[vesameskanen]

Summary

Fallback to using shape geometry without shape_dist_traveled instead of using straight lines between stops.

OTP has two methods for extracting geometry for a trip segment between two stops. The first one relies on shape_dist_traveled information found from stop_times and shape geometries. For some reason, this method (or the respective data) is error prone and therefore we often see itinerary and route geometries where stops are connected by straight lines. Such geometries are almost always incorrect as they do not follow road network and sometimes pass through buildings.

However, shape_dist_traveled is optional and OTP seems to work very well without it. This PR changes geometry processing so that when shape_dist_traveled based geometry extraction fails, OTP no longer applies the simple line geometry but fallbacks to the more robust method.

Images below show route 633N of HSL GTFS data before and after the fallback change.

[codecov-commenter]

Codecov Report

Base: 60.49% // Head: 60.54% // Increases project coverage by +0.04% 🎉

Coverage data is based on head (818a1e1) compared to base (b402815).
Patch coverage: 72.02% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #4617      +/-   ##
=============================================
+ Coverage      60.49%   60.54%   +0.04%     
- Complexity     12190    12221      +31     
=============================================
  Files           1583     1583              
  Lines          63364    63462      +98     
  Branches        6990     7006      +16     
=============================================
+ Hits           38335    38424      +89     
- Misses         22846    22856      +10     
+ Partials        2183     2182       -1     

Impacted Files	Coverage Δ
...va/org/opentripplanner/routing/RoutingService.java	26.66% <0.00%> (-1.91%)	⬇️
...erconfig/updaters/VehicleParkingUpdaterConfig.java	95.23% <ø> (-0.51%)	⬇️
...hicle_parking/VehicleParkingDataSourceFactory.java	0.00% <ø> (ø)
...ater/vehicle_parking/VehicleParkingSourceType.java	100.00% <ø> (ø)
...ipplanner/routing/api/request/RouteViaRequest.java	59.03% <59.03%> (ø)
...entripplanner/routing/api/request/ViaLocation.java	63.63% <63.63%> (ø)
...aph_builder/module/geometry/GeometryProcessor.java	86.61% <85.71%> (+0.21%)	⬆️
...lanner/routing/algorithm/via/ViaRoutingWorker.java	90.90% <90.90%> (ø)
...anner/ext/vehicleparking/bikely/BikelyUpdater.java	100.00% <100.00%> (ø)
...ntripplanner/routing/api/request/RouteRequest.java	74.15% <100.00%> (+1.21%)	⬆️
... and 16 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[hannesj]

What kind of error do we get from the getHopGeometriesViaShapeDistTravelled in that case?

[vesameskanen]

Issue reporting did not change, the code still adds two kind of bogus issues:

OpenTripPlanner/src/main/java/org/opentripplanner/graph_builder/module/geometry/GeometryProcessor.java

Line 366 in 818a1e1

issueStore.add(new BogusShapeDistanceTraveled(st1));

OpenTripPlanner/src/main/java/org/opentripplanner/graph_builder/module/geometry/GeometryProcessor.java

Line 455 in 818a1e1

issueStore.add(new BogusShapeGeometryCaught(shapeId, st0, st1));

The issue seems to be incorrectly worded, as code does not now fallback to polylines. I will fix it, thanks for pointing this out!

[hannesj]

Can we add a test case for the geometry processor with the invalid geometry?

[vesameskanen]

OK I will add it.

[leonardehrenfried]

When you've added the test, you can request a review from me.

[hannesj]

Should we try to understand what the underlying cause for the getHopGeometriesViaShapeDistTravelled failing is. This could be due to errenous data?

[vesameskanen]

I did analyze one such error in detail. I found out that the shape_dist_traveled of a stoptime entry, when mapped to the shape using the shape_dist_traveled of that shape itself, was too far away of the stop. In other words, shape_dist_traveled parametrization of the stop_times chain is out of sync with the shape. So, it is indeed a data error.

In my opinion, the optional shape_dist_traveled parameterization is quite a tricky concept, and probably hard to maintain by data producers. I would still trust that the defined route shape geometry is better than a simple polyline, even though the unintuitive parametrization fails.

[vesameskanen]

Simple test added. One trip has bad stop_times.txt shape_dist_traveled values at start. Unit test fails unless the new better fallback is applied, because OTP would use straight line between stops.

[leonardehrenfried]

Could you please unzip the test GTFS file, like so: https://github.com/opentripplanner/OpenTripPlanner/tree/dev-2.x/src/test/resources/gtfs/interlining

That will make it way easier to inspect the input data.