Skip to content

add trolleybus & monorail support - #3658

Merged
abyrd merged 10 commits into
opentripplanner:dev-2.xfrom
vesavlad:feature/add-trolleybus-support
Nov 4, 2021
Merged

add trolleybus & monorail support#3658
abyrd merged 10 commits into
opentripplanner:dev-2.xfrom
vesavlad:feature/add-trolleybus-support

Conversation

@vesavlad

@vesavlad vesavlad commented Oct 19, 2021

Copy link
Copy Markdown
Contributor

PR Instructions

When creating a pull request, please follow the format below. For each section, replace the guidance text with your own text, keeping the section heading. If you have nothing to say in a particular section, you can completely delete the section including its heading to indicate that you have taken the requested steps. None of these instructions or the guidance text (non-heading text) should be present in the submitted PR. These sections serve as a checklist: when you have replaced or deleted all of them, the PR is considered complete. As of 2021, most regular OTP contributors participate in our twice-weekly conference calls. For all but the simplest and smallest PRs, participation in these discussions is necessary to facilitate the review and merge process. Other developers can ask questions and provide immediate feedback on technical design and code style, and resolve any concerns about long term maintenance and comprehension of new code.

Summary

Add support for correctly parsing gtfs feeds with route of type trolleybus or monorail

Issue

closes #3657

Unit tests

Write a few words on how the new code is tested.

  • Were unit tests added/updated?
  • Was any manual verification done?
  • []Any observations on changes to performance?
  • Was the code designed so it is unit testable?
  • Were any tests applied to the smallest appropriate unit?
  • Do all tests pass the continuous integration service?

Code style

Have you followed the suggested code style?

Documentation

  • [] Have you added documentation in code covering design and rationale behind the code?
  • Were all non-trivial public classes and methods documented with Javadoc?
  • [] Were any new configuration options added? If so were the tables in the configuration documentation updated?

Changelog

  • Was a bullet point added to the changelog file with description and link to the linked issue?

@vesavlad
vesavlad requested a review from a team as a code owner October 19, 2021 11:02
@leonardehrenfried

Copy link
Copy Markdown
Member

Presumably you want to include/exclude these modes in your routing requests. Am I seeing this correctly?

@leonardehrenfried

Copy link
Copy Markdown
Member

If you don't, then mapping to bus and rail respectively would already be enough to get OTP to use them.

@vesavlad

Copy link
Copy Markdown
Contributor Author

Indeed want to give user flexibility to query and filter as he desires.
Maybe I need to also need to change the graphql ext part. Just let me know.

@leonardehrenfried

Copy link
Copy Markdown
Member

There are a couple of proposals floating around the community to make the modes more flexible and add more precise sub-modes (which I think monorail and trolleybus are) therefore I'm not sure that the community will accept this PR. (Note: I'm not the final arbiter of these things.)

Perhaps it's best if you join the OTP developer meeting that is happening Tuedays at 10am CEST and Thursdays at 3pm CEST on Google Hangout: https://meet.google.com/qwh-xzbz-atr

@t2gran

t2gran commented Oct 19, 2021

Copy link
Copy Markdown
Member

If you don't, then mapping to bus and rail respectively would already be enough to get OTP to use them.

Mapping these two modes as @leonardehrenfried suggest above would make this PR simple to approve, then we can discuss introducing new modes as a separate thing and decide on that in the issue #3657. I am not saying we will not accept it, I am just saying it need more discussion.

FYI: We (Entur) are likely to propose a configurable submodes as a Sandbox in the near future.

Comment thread src/main/java/org/opentripplanner/routing/core/TraverseMode.java Outdated
Comment thread src/main/java/org/opentripplanner/model/TransitMode.java Outdated
Comment thread src/main/java/org/opentripplanner/model/TransitMode.java Outdated
@t2gran t2gran added this to the 2.1 milestone Oct 22, 2021
@t2gran t2gran added the !New Feature A functional feature targeting the end user. label Oct 22, 2021
@vesavlad
vesavlad requested a review from t2gran October 25, 2021 17:23
Comment thread docs/Changelog.md Outdated
Co-authored-by: Thomas Gran <t2gran@gmail.com>
@hannesj
hannesj requested a review from abyrd November 2, 2021 11:06
@vesavlad
vesavlad requested a review from t2gran November 2, 2021 14:18
Comment thread src/main/java/org/opentripplanner/routing/core/TraverseModeSet.java Outdated

@abyrd abyrd 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.

This looks good to me, especially considering that these modes were added to the official base GTFS spec, and this PR also maps these new modes from the TPEG extension modes.

Comment on lines +45 to +47
private static final int MODE_TROLLEYBUS = 8192;

private static final int MODE_MONORAIL = 16384;

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.

Now that we have so many of these it might be better to define them by bit-shifting 1 by successive integers, to avoid any errors in writing out the powers of 2 as integer literals.

@abyrd
abyrd merged commit 52a9b31 into opentripplanner:dev-2.x Nov 4, 2021
t2gran pushed a commit that referenced this pull request Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!New Feature A functional feature targeting the end user.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for trolleybus & monorail route types

4 participants