Skip to content

Load additional data from GBFS and expose it - #3610

Merged
hannesj merged 20 commits into
opentripplanner:dev-2.xfrom
entur:otp2_simplify_vehicle_rental
Sep 30, 2021
Merged

Load additional data from GBFS and expose it#3610
hannesj merged 20 commits into
opentripplanner:dev-2.xfrom
entur:otp2_simplify_vehicle_rental

Conversation

@hannesj

@hannesj hannesj commented Sep 14, 2021

Copy link
Copy Markdown
Contributor

Summary

Read all fields from GBFS feeds and expose them through the Transmodel API

Issue

Closes #3227, #2814, #2778, #2618, #2563

Unit tests

Added additional feed to check compability with GBFS v1.0 feeds

Code style

Should be correct

Documentation

None changed

Changelog

Added

Open questions

  • Add everything to rental service vs having references between objects
  • Banned and allowed systems/vehicle types/floating/form factor/propulsion type
  • Should we map to new traverse modes
    • What about speed and permissions
  • Can we add new vertex types?
  • Stations with multiple types
  • Use max distance from vehicle
  • Astar dominance?
  • Which additional fields should be added?

@hannesj
hannesj requested a review from a team as a code owner September 14, 2021 06:13
Comment thread src/main/java/org/opentripplanner/routing/edgetype/VehicleRentalEdge.java Outdated
@hannesj
hannesj force-pushed the otp2_simplify_vehicle_rental branch from e47c0a3 to 22f059e Compare September 20, 2021 17:21

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have just a few questions, nothing major really.

I'm going compile and run this with out data set to give a final review.

AlertSeverityLevelType: String
BikePark: org.opentripplanner.routing.bike_park.BikePark#BikePark
BikeRentalStation: org.opentripplanner.routing.vehicle_rental.VehicleRentalStation#VehicleRentalStation
BikeRentalStation: org.opentripplanner.routing.vehicle_rental.VehicleRentalPlace#VehicleRentalPlace

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is a VehicleRentalPlace what used to be called VehicleRentalStation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, It has two classes implementing it, VehicleRentalStation and VehicleRentalVehicle

@leonardehrenfried

Copy link
Copy Markdown
Member

Screenshot from 2021-09-22 14-46-37

I've tested this with our dataset and it still works.

@t2gran t2gran added this to the 2.1 milestone Sep 22, 2021
Comment thread src/main/java/org/opentripplanner/routing/edgetype/VehicleRentalEdge.java Outdated
Comment thread src/main/java/org/opentripplanner/routing/vehicle_rental/RentalVehicleType.java Outdated
optionsome
optionsome previously approved these changes Sep 30, 2021

@optionsome optionsome left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think OTP1 supported defining network name in the updater config. I think that feature is not broken by this pull request and I haven't tested if the upstream OTP1 code actually allowed it but by quickly looking at the code it at least looked like it. Has it been an intentional change that defining the network name in config doesn't do anything anymore or was it broken by accident? I reviewed the code changes here and tested with a gbfs and smoove feed, and everything looked good otherwise.

@leonardehrenfried

Copy link
Copy Markdown
Member

The feature that you're talking about has been removed in a previous GBFS-related PR. The network name is now retrieved from system_information's system_id.

I think a contribution to put it back would be accepted. Or WDYT, @hannesj ?

@optionsome

Copy link
Copy Markdown
Member

Also to clarify, it still works with the smoove updater, just not with the GBFS updater.

@hannesj

hannesj commented Sep 30, 2021

Copy link
Copy Markdown
Contributor Author

@optionsome Do you have some use case for overriding the system id? It is meant exactly for things like this, as it is guaranteed to be globally unique. Also, for non-GBFS-updaters, network is still a configuration parameter.

@hannesj
hannesj enabled auto-merge September 30, 2021 09:52
@hannesj
hannesj merged commit d805f48 into opentripplanner:dev-2.x Sep 30, 2021
@optionsome

Copy link
Copy Markdown
Member

@optionsome Do you have some use case for overriding the system id? It is meant exactly for things like this, as it is guaranteed to be globally unique. Also, for non-GBFS-updaters, network is still a configuration parameter.

A numeric network id doesn't tell much by itself, we have had a standard in our project to name the networks after cities/operators so they can be easily recognized. Would it be ok for me to add back the support to give a custom network id for the GBFS updaters? Haven't checked yet how complicated it would be with the current codebase but I'm going to do some changes anyway now so I could do that addition as well unless it's unwanted or too troublesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OTP2 fails with insufficient logging when connecting to GBFS service

4 participants