Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rental searches when destination is in a no-drop-off zone #6233

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

tkalvas
Copy link
Contributor

@tkalvas tkalvas commented Nov 6, 2024

Summary

Fixes #6054

It turns out that the egress search ended up on a rental bike at all possible stops so StreetNearbyStopFinder.findNearbyStops didn't find anything. Based on my reading and debug runs, it is caused by the branch in StreetEdge.splitStatesAfterHavingExitedNoDropOffZoneWhenReverseSearching not having a walk state as one of the option.

I further think, but did not include in this PR, that the networks generated here should be filtered by what networks are allowed in the request. Now it generates everything allowed by the vertex, ignoring the request. This is not a correctness issue but affects performance.

Issue

Closes #6054

Also closes #6244

Unit tests

I added a short test which failed before but succeeds now.

Maybe there should be a larger scope test which replicates the situation as findNearestStops sees it?

Documentation

No changes.

Changelog

Changelog-worthy bug fix, maybe?

Bumping the serialization version id

No need.

@tkalvas tkalvas requested a review from a team as a code owner November 6, 2024 10:35
@tkalvas tkalvas marked this pull request as draft November 6, 2024 10:35
@tkalvas tkalvas changed the title No stops found Egress routing with BICYCLE_RENTAL sometimes breaks fixed Nov 6, 2024
@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 6, 2024

Probably best reviewed by Leonard.

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.73%. Comparing base (cee960f) to head (481c3b8).
Report is 322 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
.../opentripplanner/street/model/edge/StreetEdge.java 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6233      +/-   ##
=============================================
- Coverage      69.90%   69.73%   -0.17%     
+ Complexity     17723    17681      -42     
=============================================
  Files           1998     2007       +9     
  Lines          75443    75776     +333     
  Branches        7718     7755      +37     
=============================================
+ Hits           52740    52845     +105     
- Misses         20025    20218     +193     
- Partials        2678     2713      +35     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leonardehrenfried leonardehrenfried self-requested a review November 6, 2024 10:46
@leonardehrenfried
Copy link
Member

leonardehrenfried commented Nov 6, 2024

So the problem appeared only when the destination of the routing request was in a no-drop-off zone?

@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 6, 2024

Yeah, that is my understanding.

We might have a separate issue that a commercial operator's gbfs feed has spurious ride_allowed: falses. I'll have to track that separately. Anyway, that's probably why it was so hard to find a test case, it comes and goes for any particular coordinates in our production.

@tkalvas tkalvas marked this pull request as ready for review November 7, 2024 08:43
@optionsome optionsome self-requested a review November 7, 2024 15:58
@tkalvas
Copy link
Contributor Author

tkalvas commented Nov 8, 2024

See an alternative fix #6244

Its discussion includes my motivation on why I don't prefer it.

@tkalvas tkalvas changed the title Egress routing with BICYCLE_RENTAL sometimes breaks fixed Also continue currentMode WALK when reverse searching through splitStatesAfterHavingExitedNoDropOffZoneWhenReverseSearching Nov 8, 2024
@t2gran t2gran added the Bug label Nov 12, 2024
@t2gran t2gran added this to the 2.7 (next release) milestone Nov 12, 2024
@tkalvas tkalvas changed the title Also continue currentMode WALK when reverse searching through splitStatesAfterHavingExitedNoDropOffZoneWhenReverseSearching Fix rental searches when destination is in a no-drop-off zone Nov 12, 2024
@optionsome optionsome merged commit 37a4d12 into opentripplanner:dev-2.x Nov 15, 2024
5 checks passed
@optionsome optionsome deleted the no-stops-found branch November 15, 2024 11:08
t2gran pushed a commit that referenced this pull request Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Egress routing with BICYCLE_RENTAL sometimes breaks in GTFS planConnection
4 participants