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

Add graph support for highway=busway #3413

Merged
merged 8 commits into from
Nov 28, 2021

Conversation

landonreed
Copy link
Contributor

@landonreed landonreed commented Nov 17, 2021

Issue

This PR targets #2574. While it may not handle all of the cases described in that issue, it aims to expand the routable bus-only ways for bus costing to include the OSM tag highway=busway, which according to OSM docs should imply bus=designated. This is specifically designed to support some ways in the Brisbane area that are correctly identified as highway=busway.

I'm not sure if this warrants a test, but am open to adding one if the maintainers think that would be helpful!

Tasklist

  • N/A Add tests
  • Add #fixes with the issue number that this PR addresses
  • N/A Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

N/A

@kevinkreiser
Copy link
Member

@gknisely maybe this is a good opportunity to throw in a quick gurka test for bus?

@gknisely
Copy link
Member

@gknisely maybe this is a good opportunity to throw in a quick gurka test for bus?

Sure as soon as I get a minute. Unless @landonreed wants to give it a try

@landonreed
Copy link
Contributor Author

Hey all, I don't have a ton of experience in the valhalla code base, but would be happy to give it a look if that would be helpful!

@kevinkreiser
Copy link
Member

We dont have any bus specific tests yet but a good place to add some might be in here: https://github.com/valhalla/valhalla/blob/master/test/gurka/test_access_psv.cc where we could confirm bus can but auto cannot take these ways. Or over in the general routing test: https://github.com/valhalla/valhalla/blob/master/test/gurka/test_route.cc

@landonreed take a look at the general lay out of the gurka tests and give this a read: https://github.com/valhalla/valhalla/tree/master/test/gurka#valhalla-gurka-tests to understand the general philosophy.

@landonreed
Copy link
Contributor Author

@kevinkreiser, thanks for the pointers! Does a54ac38 look to be on the right track? Happy to separate that out into its own TEST if that would be better.

@kevinkreiser
Copy link
Member

@landonreed that test change looks great to me. Now we just have to fix the rest of the build and resolve merge conflicts

@kevinkreiser
Copy link
Member

@landonreed i modified your testing to make the test cases a bit stronger and also fix the cases you were aiming at. we'll see what the build says but locally everything worked for me after these changes 🤞

@kevinkreiser kevinkreiser merged commit 8184824 into valhalla:master Nov 28, 2021
@landonreed landonreed deleted the highway-busway-support branch November 29, 2021 14:19
@landonreed
Copy link
Contributor Author

That looks great! Thanks for all of the help moving this along, @kevinkreiser!

@landonreed
Copy link
Contributor Author

Hi @kevinkreiser, thanks once again for helping shepherding this along!

I wanted to inquire as to when you expect to cut the next release of valhalla? We have some customers that are interested in some of these changes, but I think we're blocked until the new version with this PR ships.

We're happy to help with anything that might grease the wheels!

@kevinkreiser
Copy link
Member

kevinkreiser commented Jan 26, 2022

@landonreed im not exactly sure what your deployment process is but the only thing our release process does:

  1. tagging the repo
  2. pushing a tagged docker build to docker hub

You are free to pull and retag our run-latest docker image anytime, which has the tip of master in it, if you want to use this for your deployment. In other words nothing about our release cadence should hold you back from deploying this code. Does that make sense? Or maybe you just like the signal that an official tag sends?

@landonreed
Copy link
Contributor Author

@kevinkreiser, thanks for the info! That's very helpful. I'll check on how our deployment is handled. We use a third-party hosted service, so there's a little bit of grey area in how they handle releasing new code. An official tagged release might be helpful, but if that's become less common of a practice for the project (or has overhead) maybe we can recommend that they use run-latest.

I'll follow up here if a tagged release seems like our only path forward.

@kevinkreiser
Copy link
Member

It's just not always a perfect time for a tagged release to be honest. At the moment we have partial support for indoor routing such that you can rout on them but the routes you get don't have maneuvers fleshed out yet. That pr is in progress and once we merge it we can probably tag

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.

None yet

3 participants