Add routing functionality for floating bikes

[gmellemstrand]

Summary

This is a proof of concept of floating bike functionality. It implements the minimum functionality needed in order to do floating bike routing requests.

This PR adds a new enum to better keep track of bike rental states.

enum BikeRentalState {
  /**
   * This is the state before any bike rental has been initiated.
   */
  BEFORE_RENTING,
  /**
   * A bike or scooter is being rented from a bike rental station. This means that the bike will
   * need to be dropped off at another station before the search terminates.
   */
  RENTING_FROM_STATION,
  /**
   * A floating bike or scooter is being rented. It will not need to be dropped off before
   * terminating the search, as it can be dropped off at any street.
   */
  RENTING_FLOATING,
  /**
   * After dropping off a bike or scooter, this state is entered.
   */
  HAVE_RENTED
}

When starting to rent at a BikeRentalStation, it checks whether the bike is floating or not, and enters the appropriate state. The criteria for terminating the search at the destination is then that you are in the RENTING_FLOATING or HAVE_RENTED states. More on this below.

Issue

#3379

[evansiroky]

There's a typo here. See suggested change.

Suggested change

	* terminating the search, as it can be dropped of at any street.
	* terminating the search, as it can be dropped off at any street.

[gmellemstrand]

@evansiroky Is there any work done or issues I should know about when it comes to this functionality?

[evansiroky]

@evansiroky Is there any work done or issues I should know about when it comes to this functionality?

Well, there are a lot of items we've implemented in our fork related to floating vehicle rentals, but I guess the largest thing that stands out to me right now is the process of letting OTP help floating bikes end a bike rental in a floating state (ie ending outside of a bike rental station). I don't believe that I see logic to support this in this PR so far.

The code we implemented was heavily influenced and partially derived from code that I manually extracted from #2596. To start with, the code to allow dropping off a bike at an arbitrary StreetEdge is somewhat similar to the Taxi code in OTP 2 that analyzes every StreetEdge (and also StreetToTransitLinks) as a potential transition point for starting (in case of "arrive by" search) or ending (in case of "depart at" search) a floating vehicle rental. In our fork here is the logic within the StreetEdge and StreetTransitLink that we used. Also, we consolidated a number of checks for whether a vehicle dropoff is allowed into the isVehicleRentalDropoffAllowed method in the State class.

Another big part of all this is determining where the boundaries were for vehicle rental dropoffs (and also jurisdiction-specific parking and travel restrictions). We implemented all of the logic related to this prior to GBFS 2.1-RC (or more specifically addition of the geofencing_zones.json file) being approved or formally discussed. Our implementation took different approaches for what we thought would be jurisdiction-specific restrictions that applied to every operator and also the operating boundaries of each operator. The logic to load jurisdiction-specific travel and parking restrictions that had GeoJSON describing parking and travel restrictions was implemented during graph build. Here is some high-level documentation for the build-config.json parameters one could set for this. However, the logic about operator's own operating boundaries is to be set in the router-config.json (see example in our docs). As you may notice, GBFS 2.1-RC now has travel, dropoff, and speed restrictions in the geofencing_zones.json file. At a GBFS developer workshop in 2019 we discussed the geofencing_zones.json file at length and sort of came to a consensus that it would be the operators' responsibility to include jurisdiction-specific speed, dropoff and travel regulations within their GBFS. If we refactor our code to be compatible with GBFS 2.1+ we'd implement something that would obtain those geometries from a GBFS 2.1+ compatible feed.

The logic for calculating the allowable areas of travel for a specific operator remains mostly unchanged from the method done in #2596. This involves updating every applicable StreetEdge with the allowable vehicle rental networks and also splitting certain StreetEdges at the boundaries to make sure a floating dropoff can be done at the exact edge of a boundary in case there is a StreetEdge that is partially inside the operator boundary. Most of this code can be found here in our fork.

I think that's the most relevant items to share for now.

[gmellemstrand]

Thanks for the detailed response, @evansiroky

I have created a new issue where we can continue discussing the overall GTFS 2.1 implementation, but I will reply to your post here, since it relates more to this specific PR.

I will refer to scooters as bikes, because they are not differentiated in OTP2 currently.

For this PR, I allowed the search to end in a floating bike state, without having a floating bike drop-off state at all. Maybe it is not explicit enough, but it is checked in the following code: https://github.com/entur/OpenTripPlanner/blob/otp2_floating_bikes/src/main/java/org/opentripplanner/routing/core/State.java#L196-L199

I looked at #2596 and seems to also allow for ending in a floating bike state, as long as drop-off is possible at that point.

When routing within a geofencing zone (or if no zones are defined at all), when the search encounters a StreetEdge that does not allow bike routing, it is hard to distinguish between the walking with bike state or just drop-off the bike and enter the has rented bike state. Of course, when exiting a geofencing zone, you have no other choice but to enter the has rented bike state.

I also see that if we want geofencing zones to be obtained from a GTFS feed after graph build time, we would need to split the edges at the boundaries using realtime edges.

[evansiroky]

This check doesn't seem to be correct. The bikeRentalIsFinished method will return true if the current state is in RENTING_FLOATING. However, in an arriveBy context, this means that a user is in possession of a floating bike that they have yet to pickup from an actual station (docked or floating). Therefore, the check here should be only if the bike rental state is BEFORE_RENTING or HAVE_RENTED.

[evansiroky]

This won't properly account for arriveBy searches where the user has dropped off a bike at the chronological end (therefore, first arriveBy state) of a journey. In our fork we added code that would equivalently set this BikeRentalState to RENTING_FLOATING if the first seen street edge had conditions that would allow a floating rental to end there. See our code here.

[evansiroky]

Thanks for the detailed response, @evansiroky

I have created a new issue where we can continue discussing the overall GTFS 2.1 implementation, but I will reply to your post here, since it relates more to this specific PR.

I will refer to scooters as bikes, because they are not differentiated in OTP2 currently.

For this PR, I allowed the search to end in a floating bike state, without having a floating bike drop-off state at all. Maybe it is not explicit enough, but it is checked in the following code: https://github.com/entur/OpenTripPlanner/blob/otp2_floating_bikes/src/main/java/org/opentripplanner/routing/core/State.java#L196-L199

I looked at #2596 and seems to also allow for ending in a floating bike state, as long as drop-off is possible at that point.

When routing within a geofencing zone (or if no zones are defined at all), when the search encounters a StreetEdge that does not allow bike routing, it is hard to distinguish between the walking with bike state or just drop-off the bike and enter the has rented bike state. Of course, when exiting a geofencing zone, you have no other choice but to enter the has rented bike state.

I also see that if we want geofencing zones to be obtained from a GTFS feed after graph build time, we would need to split the edges at the boundaries using realtime edges.

@gmellemstrand thanks for the reply. I hadn't fully wrapped my head around the fact that there are now 2 street searches where each can have a bike rental. The code does seem like it should mostly work with the two noted exceptions I added above. As you mentioned, additional work would need to be done to incorporate geofencing zones that have rules restricting travel or dropoff.

[gmellemstrand]

I have now gone through and refactored the RentABikeAbstractEdge, RentABikeOffEdge and RentABikeOnEdge. I replaced them with a new class called BikeRentalEdge. The previous structured seemed to assume that dropping off a bike was just the inverse of renting a bike. This does not work now that we have different states before and after bike rental, and also different states based on whether the bike rental is floating or not.

I have gone through all the possible state transitions in both directions, I have tested that they work with both floating and non-floating bike rental both with depart after and arrive by.

I have also made sure the isFinal method on State does the correct check in both directions. Also, since this search can end in two different states, both of those states are created as initial states for the reverse search (similar to the car pickup #3392).

[leonardehrenfried]

This looks very interesting to us. When we took Evan's code and ported it to our fork I also created a series of test cases that to figure out if everything works as expected: https://github.com/mfdz/OpenTripPlanner/blob/master/src/test/java/org/opentripplanner/routing/core/BikeRentalRoutingTest.java

Once I get the time I intend to apply these test cases to this branch to see if they work locally.

One question: i couldn't find any code that would let you drop off a bike near a station and then continue taking a train. Am I overlooking something?

And are combinations like rental bike -> train -> rental bike possible?

[evansiroky]

Thanks for the feedback, @evansiroky. I was not aware that it should be possible to combine floating pickup and station dropoff and vice versa. I will add your suggestions, so that this is possible.

I also see that in your table, you have set the cases that start/end in a floating dropoff to N/A. Do you think the simplification that floating vehicles are never dropped off is acceptable, or should we require dropoff? In that case, how do we differentiate it from walking with bike?

I believe this is a first good step. We at IBI Group will probably wait until GBFS 2.2+ support is added before using this in production to ensure that geofencing rules are in place, that way the simplification with this current code shouldn't be applicable anymore.

[gmellemstrand]

I have now adapted the CarPickup test setup by @flaktack to test the bike rental and floating bike functionality. I have also replaced the existing bike rental test, as I think this setup is simpler to work with.

I thought about adding a test for the floating bike dropoff at station, but as long as we don't have geofencing zones or a cost for dropping off at a street, it will never happen, and you will just get "walk with bike" all the way to your destionation.

[flaktack]

I've tested using two feeds, which worked as expected: Budapest's GBFS feed and voioslo. A few minor GBFS handling bugs surfaced, but those will be separate issues. ✍️

As mentioned above mixing free-floating and station rentals, along with explicit drop-off of vehicles is left as task for latter.

Is this comment relating to floating bikes and edge splitting still relevant?

OpenTripPlanner/src/main/java/org/opentripplanner/updater/bike_rental/datasources/GbfsBikeRentalDataSource.java

Lines 26 to 33 in 745bfda

	* Be aware that there are problems with the floating bike support. It therefore has to be
	* explicitly turned on by using OTPFeature.FloatingBike. Use at your own risk.
	*
	* See https://github.com/opentripplanner/OpenTripPlanner/issues/3316
	*
	* Leaving OTPFeature.FloatingBike turned off both prevents floating bike updaters added to
	* router-config.json from being used, but more importantly, floating bikes added by a
	* BikeRentalServiceDirectoryFetcher endpoint (which may be outside our control) will not be used.

[gmellemstrand]

Is this comment relating to floating bikes and edge splitting still relevant?

OpenTripPlanner/src/main/java/org/opentripplanner/updater/bike_rental/datasources/GbfsBikeRentalDataSource.java

Lines 26 to 33 in 745bfda

	* Be aware that there are problems with the floating bike support. It therefore has to be
	* explicitly turned on by using OTPFeature.FloatingBike. Use at your own risk.
	*
	* See https://github.com/opentripplanner/OpenTripPlanner/issues/3316
	*
	* Leaving OTPFeature.FloatingBike turned off both prevents floating bike updaters added to
	* router-config.json from being used, but more importantly, floating bikes added by a
	* BikeRentalServiceDirectoryFetcher endpoint (which may be outside our control) will not be used.

I have removed that warning, as it is not valid anymore. I think there is still use for the floating bike feature flag, but we might want to change the default setting.

[flaktack]

Thanks for the changes. I have no preference with regard to the default state of the feature flag, though enabling it by default currently could cause spurious warnings in GBFS feeds (#3442).

[gmellemstrand]

Thanks for the review. I will leave the feature flag off for now, and then we can reconsider in the future.

[t2gran]

A minor comment about DTOs and Collections:

Leg is a DataTransferObject(DTO), not use for much business logic. Hence, in OTP we mostly use List even for collection with unique values. This allow navigation in the collection. This is not a big deal, but if a Set is used in a DTO it should trigger the question: "What is this used for", after all restrictions on the values are applied to the collection BEFORE creating the DTO. The DTO should be as general as possible. I am happy to use both Set and/or List but it should be used consistent - and maybe also with a comment why - if set is used. Maybe document if the bikeRentalNetworks are ids or names also.

[t2gran]

This PR is approved by @flaktack and the issues reported by @evansiroky seams to be fixed. I will approve this now so we can move forward.