Handle reverse search when starting in no-drop-off zone #5201
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5201 +/- ##
=============================================
- Coverage 65.57% 65.57% -0.01%
- Complexity 14629 14633 +4
=============================================
Files 1762 1761 -1
Lines 68359 68368 +9
Branches 7289 7290 +1
=============================================
+ Hits 44827 44829 +2
- Misses 21045 21054 +9
+ Partials 2487 2485 -2
☔ View full report in Codecov by Sentry. |
leonardehrenfried
force-pushed
the
multiple-geofencing
branch
from
362ab54
to
c3bc4b6
Compare
leonardehrenfried
force-pushed
the
multiple-geofencing
branch
from
ac2d5d5
to
9f6ba60
Compare
There was a problem hiding this comment.
Seems to work on PDX data. A search for rental scooter starting in a no-parking (no-drop-off) area correctly returns an itinerary where you have to walk to a scooter outside the area boundary, whereas the reverse search (still) takes the scooter to inside the no-parking area.
src/main/java/org/opentripplanner/service/vehiclerental/street/VehicleRentalEdge.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/street/model/edge/StreetEdgeGeofencingTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/street/search/state/StateDataTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I tested this and often (if not always) the rental scooter suggestions slightly differ with arriveBy true/false when there are these geofencing zones. However, the suggestions still seemed quite reasonable so I don't know if we need to fix the issue within this pr.
| // null is a special rental network that speculatively assumes that you can take any vehicle | ||
| // you have to check in the rental edge if this has search has been started in a no-drop off zone | ||
| Stream.of((String) null), |
There was a problem hiding this comment.
I wonder if having this as null or some hardcoded value is better.
There was a problem hiding this comment.
I agree that using null is terrible for a number of reasons. My eventual goal is to refactor the StateData in a way that this is not needed anymore.
I'm thinking of something like this:
interface State
class Walking implements State
class ForwardRenting(String network) implements State
class BackwardsRentingLookingForVehicle implements State
class BackwardsSpeculativelyRenting implements StateThat way we can get rid of these magic values and don't carry around all the variables (rentalNetwork, formFactor, rentalState...) for the searches where we don't use them.
The big question is the performance of all of this.
There was a problem hiding this comment.
Right now documenting this nightmare is difficult and having explicit classes with Javadoc might help.
src/main/java/org/opentripplanner/street/search/state/StateData.java
Outdated
Show resolved
Hide resolved
| final State speculativeRenting = states[0]; | ||
| assertEquals(RENTING_FLOATING, speculativeRenting.getVehicleRentalState()); | ||
| assertEquals(BICYCLE, speculativeRenting.currentMode()); | ||
| // null means that the |
There was a problem hiding this comment.
:) Fleshed out in 0c103bc
Summary
When implementing GBFS geofencing zones in #4805 I had to cut out a complex feature, because a refactoring had to happen first (#4841) to make it work.
Since the preparatory work has not been completed, I can implement the following edge case: a reverse (arrive by) search has started in a no-drop off zone.
We note this case when speculatively starting the rental and have to make sure in the rental edge that we don't allow for the vehicle to be picked up.
Refactoring
getNonTransitModehas been renamed tocurrentModeStateDatawas moved into a separate method and made explicit@NotNullannotation for all edgesUnit tests
Lots added.