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 scooter access #5361

Merged

Conversation

leonardehrenfried
Copy link
Member

Summary

This fixes the scooter rental access computation when geofencing zones are deactivated. The problem was that the StreetTransitStopLink edge didn't allow rental scooters to traverse.

Issue

Closes #5356

Unit tests

Lots of test cases added.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.01% 🎉

Comparison is base (a27baf3) 66.49% compared to head (7149a68) 66.51%.
Report is 2 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5361      +/-   ##
=============================================
+ Coverage      66.49%   66.51%   +0.01%     
- Complexity     15234    15239       +5     
=============================================
  Files           1785     1785              
  Lines          69269    69277       +8     
  Branches        7291     7293       +2     
=============================================
+ Hits           46063    46079      +16     
+ Misses         20737    20729       -8     
  Partials        2469     2469              
Files Changed Coverage Δ
...org/opentripplanner/street/search/state/State.java 87.34% <75.00%> (+5.52%) ⬆️
...ner/street/model/edge/StreetTransitEntityLink.java 82.45% <81.81%> (+3.21%) ⬆️

... and 1 file with indirect coverage changes

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

@leonardehrenfried leonardehrenfried requested review from vpaturet and optionsome and removed request for t2gran September 19, 2023 09:43
@vpaturet
Copy link
Contributor

A small request, not directly related to this PR, but useful for getting into the logic: could you add some javadoc in the AStarEdge interface. In particular the meaning of the traverse() method is not obvious and a short high-level description would help, as well as few words about the meaning of the return value State.empty(), and the special case of returning multiple states (#4841)

@leonardehrenfried
Copy link
Member Author

Does 9428ee7 make it any clearer, @vpaturet ?

@vpaturet
Copy link
Contributor

Perfect, thank you!

@vpaturet
Copy link
Contributor

This also fixes the issue I mentioned earlier where direct routing to a station would not return a scooter rental leg:

trip(
    from: {
      name: "Gulleråsen"
      place: "NSR:StopPlace:6294"
    },
    to: {
      name: "Telthusbakken" 
      place: "NSR:StopPlace:6290"
    },
    dateTime: "2023-08-11T12:48:50.467+02:00",
    modes: {
      directMode: scooter_rental
    }
  )

while using coordinates would succeeds:

trip(
  from: {
    name: "Gulleråsen"
    place: "NSR:StopPlace:6294"
  },
  to: {
    name: "Telthusbakken",
    coordinates: {
      latitude: 59.924565
      longitude: 10.750507
    }
    
 },
  dateTime: "2023-08-11T12:48:50.467+02:00",
  modes: {
    directMode: scooter_rental
  }
)

Now both queries return similar results.

vpaturet
vpaturet previously approved these changes Sep 22, 2023
/**
* Traverse the edge from a given state and return the result of the traversal.
*
* @param u The 'current' state when arriving at the fromVertex.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is u again some term from A* literature or java naming convention? Could we have a more descriptive name for this variable even if this is just an interface?

Copy link
Member Author

@leonardehrenfried leonardehrenfried Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. There must be a reason why Hannes called it u. In other places it's called s0.

The A* Wikipedia page mentions neither u nor s0.

Would s0 be a good name for you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's fine

@leonardehrenfried leonardehrenfried merged commit 20472f3 into opentripplanner:dev-2.x Sep 22, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Sep 22, 2023
@leonardehrenfried leonardehrenfried deleted the fix-scooter-access branch September 22, 2023 13:21
@t2gran t2gran added this to the 2.5 (next release) milestone Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rental scooter access does not work when geofencing is off
4 participants