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: Implicit vehicle access handling for ferries #1954

Merged
merged 7 commits into from
Jan 23, 2025
Merged

Conversation

koebi
Copy link
Collaborator

@koebi koebi commented Jan 22, 2025

Pull Request Checklist

  • 1. I have rebased the latest version of the main branch into my feature branch and all conflicts
    have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
    [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset
    (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
    importer etc.), I have generated longer distance routes for the affected profiles with different options
    (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
    points generated from the current live ORS.
    If there are differences then the reasoning for these MUST be documented in the pull request.
  • 12. I have written in the Pull Request information about the changes made including their intended usage
    and why the change was needed.

Not sure if there are any cases that would be covered by the or-part of the original if-statement that are not caught by the new statement…

One would probably have to change hgv as well, I think?

Information about the changes

  • Key functionality added: Use ferries that do not have any general or vehicle-specific access tag (such as foot, motorcar, access, …), but only the route=ferry tag.
  • Reason for change: According to the tag filtering logic we accept these ways, which I think makes sense.
    Whether it makes sense to only do that if foot or bike are absent, I don't know…

Examples and reasons for differences between live ORS routes, and those generated from this pull request

This route using such a ferry in norway will now be available.

@aoles aoles self-requested a review January 22, 2025 12:42
@aoles aoles self-assigned this Jan 22, 2025
@aoles aoles changed the title Fix ferry tag handling fix: Ferry implicit vehicle access handling Jan 22, 2025
@github-actions github-actions bot added the fix label Jan 22, 2025
@aoles
Copy link
Member

aoles commented Jan 22, 2025

FYI: The logic to allow access only in the absence of foot or bike tags has been adopted from graphhopper and has been originally introduced in graphhopper/graphhopper@f87fba4.

Copy link
Member

@aoles aoles left a comment

Choose a reason for hiding this comment

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

Many thanks for the prompt fix! 🚀

Looks aready quite good. Would you mind adding:

  • corresponding changes to the HGV flag encoder/tests
  • an entry in the CHANGELOG file?

Cheers! ❤️

@aoles aoles changed the title fix: Ferry implicit vehicle access handling fix: Implicit vehicle access handling for ferries Jan 22, 2025
@github-actions github-actions bot added fix and removed fix labels Jan 22, 2025
@aoles aoles force-pushed the fix/ferry-tag-handling branch from b422824 to b509045 Compare January 22, 2025 16:56
@aoles
Copy link
Member

aoles commented Jan 23, 2025

Hi @koebi,
thanks for implementing the suggested changes! Unfortunately, a few API tests started failing, some with quite big deviations from the expected values 💀 I'm a bit puzzled by this, especially as this PR effectively restores the original behavior that got broken in #1291. There the influence on API tests was negligible.

@aoles
Copy link
Member

aoles commented Jan 23, 2025

Some of the proposed changes seem to break the routing 💀 This is evident e.g. when looking at the route from testHGVHazmatRestriction api test for hazmat = true (blue is the reference as on main, and red the results calculated with this branch)

image

Apparently some sort of secret tram routing option got activated, see below 😂

image

Not sure what the following is about though 🙈

image

@aoles aoles enabled auto-merge January 23, 2025 13:50
Copy link
Member

@aoles aoles left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@aoles aoles merged commit b59881b into main Jan 23, 2025
27 checks passed
@aoles aoles deleted the fix/ferry-tag-handling branch January 23, 2025 13:55
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.

2 participants