Bike rental edge split refactor

[evansiroky]

To be completed by pull request submitter:

• issue: Link to or create an issue that describes the relevant feature or bug. Add GitHub keywords to this PR's description (e.g., closes #45).
• roadmap: Check the roadmap for this feature or bug. If it is not already on the roadmap, PLC will discuss as part of the review process.
• tests: Have you added relevant test coverage? Are all the tests passing on the continuous integration service (Travis CI)?
• formatting: Have you followed the suggested code style?
• documentation: If you are adding a new configuration option, have you added an explanation to the configuration documentation tables and sections?
• changelog: add a bullet point to the changelog file with description and link to the linked issue

To be completed by @opentripplanner/plc:

• reviews and approvals by 2 members, ideally from different organizations
• after merging: update the relevant card on the roadmap

This PR refactors the way that StreetEdges are handled in order to properly insert and remove bike/car/vehicle rentals.

This PR is built on top of #2762 which should be reviewed, approved and merged first.

Motivation and problem description

In the latest work for TriMet, we added in the ability to plan trips with eScooter rentals in the Porltand, OR area. We were running the graph with a total of 9 rental updaters: 1 bicycle rental operator, 2 car rental operators and 6 eScooter rental operators. Hundreds of vehicles were in operation at any point in time and all of these companies had rental vehicles concentrated in the downtown Portland area. At times there could be a dozen or more vehicles on a particular street corner. See this example:

The implementation of connecting the vehicle rentals to the graph prior to this PR was as follows:

Before insertion of bike/car/vehicle rental:

After insertion of bike/car/vehicle rental:

This one insertion may seem harmless, but it caused numerous problems:

1. More and more edges were permanently being created in the graph that were required to be traversed in order to go what was originally just one edge.
2. Each of the newly created vertices were treated as intersections with turn costs. Each of these insertions added time to route searches, especially with the car mode which has a minimum of 5 seconds added for straight turns.
3. The permanently split edges lost all elevation data as noted in StreetEdges split destructively lose elevation data #2790

Perhaps the worst thing about the original implementation, however, was that the split edges were never unsplit once the rental station disappeared from the updater data. Therefore, there was a constant accumulation of unneeded StreetEdges that would stay in the graph forever. As noted in #2787, this ultimately resulted in requiring to reboot OTP every-so-often so it wouldn't grind to a halt.

Sample trip plan with a fresh start of OTP

Sample trip plan with an OTP instance running for a few days

Proposed solution

As was seen with the original implementation, the first priority was to actually remove the vertices and edges that were no longer needed from the graph. However, this would involve a very complex analysis of which edges were split and how to rejoin edges. For example, if one vehicle initially split an edge and then another vehicle split the resulting edge and the first vehicle then needed to be removed, there would have to be an analysis of what edges there currently were that could be rejoined and the resulting distance and elevation recalculations would likely lose or have substantially changed information. Furthermore, there would still be the matter of increased turn costs and many more edges required to be traversed for what used to be one edge.

So, the implementation I am proposing here is to create semi-permanent edges and vertices that are dedicated to a specific rental station. See the following diagram of the new implementation:

This implementation does result in 2 more edges being created than merely splitting edges in two. However, every other original problem is solved by doing so:

1. The original edge between the intersections is preserved and can be traversed.
2. There are no added turn costs from all the psuedo-intersections
3. The SemiPermanentPartialStreetEdges inherit from the PartialStreetEdge which recalculates the elevation profile

Furthermore, this added approach has even more benefits:

1. The removal of vertices and edges is very straightforward. All that needs to be done is to remove the bikeRentalStationVertex, the SemiPermanentSplitterVertex and all of the edges associated with those vertices. All of this has no side effects on any other edge or vertex.
2. The SemiPermanentPartialStreetEdge is now able to accommodate an overridden traverse method that stops further traversal if bike rentals aren't even enabled in the request options. When it is impossible to traverse these edges they won't get added to the priority queue of the AStar search which will result in significant computational savings. For example, in a simple transit+walk query, none of the SemiPermanentPartialStreetEdges will be traversable and won't be explored further.

Related issues

This PR fixes #2787
This PR basically makes #2790 a non-issue. Comments have been added to the code regarding this.

[jwoyame]

I think this is a great PR that is useful for our needs in Columbus.

We experienced a problem using it that is actually related to a deeper issue. You can see below that we had a bug where a path sometimes has an extra appendage on the initial edge. Here, it goes right through a building footprint.

I figured this had something to do with integer offsets (compacted geometry form) being relative to the wrong vertex after the temporary origin edge split.

If you specify that you want destructive splitting, the StreetEdge constructor will inherit its "back" (reversed direction) property from the parent edge:

if (destructive) {
    e1 = new StreetEdge((StreetVertex) fromv, splitterVertex, geoms.first, name, 0, permission, this.isBack());
    e2 = new StreetEdge(splitterVertex, (StreetVertex) tov, geoms.second, name, 0, permission, this.isBack());

The copyAttributes function this PR creates will -- like the original code -- call to set the back property again, which is a no-op:

private void copyAttributes(StreetEdge other) {
    this.wayId = other.wayId;
    this.setBicycleSafetyFactor(other.getBicycleSafetyFactor());
    this.setHasBogusName(other.hasBogusName());
    this.setStairs(other.isStairs());
    this.setWheelchairAccessible(other.isWheelchairAccessible());
    this.setBack(other.isBack()); <-- won't change anything for destructive splits.
    this.setCarSpeed(other.getCarSpeed());
    this.setVehicleNetworks(other.getVehicleNetworks());
    this.setFloatingVehicleDropoffSuitability(other.getFloatingVehicleDropoffSuitability());
    this.setNoThruTraffic(other.isNoThruTraffic());
    this.setStreetClass(other.getStreetClass());
}

This is okay for destructive splits, but redundant. However it introduces an error when this function is called on Temporary or SemiPermanent edges. When these edges are constructed, they always pass false to the StreetEdge constructor for the back property. The temporary / semi-permanent edges will never match the direction of a reversed parent edge. So, the copyAttributes is marking them as being reversed AFTER they have already been created and compacted. Then, when they get uncompacted, the CompactLineString class will think that they need to be un-reversed and will make them relative to the END vertex rather than the start.

When I delete the setBack call, the result is the following:

The current code for TemporaryPartialStreetEdge always passes false to the StreetEdge constructor. This may be an issue because the directionality of the segment will simply match the source geometry's directionality for that edge. I'm uncertain what consequences this has, but I feel like the split edges should always match the parent's direction when the from / to vertices are being derived from the fromv and tov of the parent.

My second proposed change (besides removing the setBack call) is to set the direction to the parent edge's direction in the PartialStreetEdge (the base class for temporary and semi-permanent edges):

super(v1, v2, geometry, name, length, parentEdge.getPermission(), parentEdge.isBack());

Finally, to eliminate this form of bug (in lieu of a new test), I also propose that we get rid of the setBack function for StreetEdge completely, and move that functionality into the setGeometry function for the edge. We should only allow the property to be set when the compacted geometry is created. As this problem shows, it is very error-prone to allow the direction flag to be changed independently of the geometry. This might be for a separate PR... that's up to you all.

Thanks @evansiroky!

[evansiroky]

Thanks, @jwoyame, for looking at this. I'll have to try out your recommended changes at some point.

[evansiroky]

Thanks again @jwoyame for the comments. You definitely identified the cause of the problem and recommended a good solution. I have implemented most of your changes into this PR, but did not opt for removing the setBack method entirely. I believe the setBack method is needed for properly setting the back flag during graph builds. I think it'd definitely be a larger effort to find a different way to recalculate the geometry. The introduction of this bug was mostly from me mistakenly adding in the call to setBack where it shouldn't have been added since I was basically adding other set methods to make sure all Street attributes were assigned to split StreetEdges without considering or knowing the full effects of that.

[leonardehrenfried]

The street splitter has been refactored in OTP2 where the original edge is restored when the rental vehicle is no longer there.

Also, there hasn't been any development on the 1.5 branch for a few years.

Therefore I'm closing this issue. If that was a mistake, please re-open.