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

Support "access:conditional" conditional restrictions like "access:conditional"="no @ (Oct-May)" #5048

Merged
merged 13 commits into from
Jan 28, 2025

Conversation

kinkard
Copy link
Contributor

@kinkard kinkard commented Jan 10, 2025

Issue

fixes #2050

There are plenty of roads that are closed during some period of time and that are tagged by `"access:conditional". For example, this road in Alps is closed from October to May - https://www.openstreetmap.org/way/1012815911.

See more using use https://overpass-turbo.eu/

[out:json][timeout:25];
(
	way["access:conditional"](45.0, 5.0, 53.0, 15.0);
);
out body;
>;
out skel qt;

image

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • 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

Link any requirements here. Other pull requests this PR is based on?

@kinkard kinkard changed the title Support conditional access Support "access:conditional"="no @ winter" and other "access:conditional" Jan 10, 2025
Comment on lines 234 to 235
boost::algorithm::replace_all(condition, "winter", "Dec-Feb");
boost::algorithm::replace_all(condition, "summer", "Jun-Aug");
Copy link
Member

Choose a reason for hiding this comment

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

and if its the southern hemisphere we need to flip them 😄 i guess we dont have an easy way we could, once we know the latitude, make this choice... we'd have to store some other temp info about how we assumed northern hemisphere and how we need to shift the time by 6 months if it happened to be in the southern hemisphere? sounds like a pita but getting it wrong isnt really an option either right? or am i missing something?

Copy link
Contributor Author

@kinkard kinkard Jan 20, 2025

Choose a reason for hiding this comment

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

🤦 Now I see why Valhalla didn't support this in the first place.

Let me remove this change and keep only the "access:conditional" handling.

Copy link
Member

Choose a reason for hiding this comment

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

yeah its sad but it could be done, just a bit more involved passing state from at least the way/relation tagging until we get to the node tagging (where we finally know the lat lng). totally doable imho but def more involved (have to make storage for it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, now I'm thinkinking that seasons support is quite orthogonal to the tag support.

And for seasons, AFAIK, Valhalla aways processes ways after nodes, so a conditional substitution ("summer"/"winter" -> month range) somewhere in the pbfgraphparser.cc should do the job.

Copy link
Member

Choose a reason for hiding this comment

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

i was pretty sure we did ways, relations, then finally nodes. the reason to do this is because there are lots of ways and relations (with even more nodes) that we dont care about routing on at all and so by first filtering down the ways and relations to what we care about we can skip tons and tons of nodes that we wouldnt need to parse and then just throw away.

but i guess it doesnt matter that i dont understand what you're thinking 😄 if you think you can get the seasons straightened out please by all means do so!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right. Nodes get resolved after ways and relations =\

enum class BuildStage : int8_t {
  kInvalid = -1,
  kInitialize = 0,
  kParseWays = 1,
  kParseRelations = 2,
  kParseNodes = 3,
  kConstructEdges = 4,
  //...
  };

Well, then I have no idea how to do it without introducing additional complexity.

Copy link
Contributor Author

@kinkard kinkard Jan 22, 2025

Choose a reason for hiding this comment

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

I've removed this piece of code in b802645 and updated PR name & description to focus solery on "access:conditional" like "access:conditional"="no @ (Oct-May)" that still cover a lot of cases for winter restrictions, at least in Europe

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this be handled in the enhance stage? There, access restriction units are updated based on the admin they're in. You would have access to the node, and there are spare bits on AccessRestriction, so maybe use one to temporarily flip it to signal that it needs to be checked for the hemisphere it's in?

@kinkard kinkard changed the title Support "access:conditional"="no @ winter" and other "access:conditional" Support "access:conditional" conditional restrictions like "access:conditional"="no @ (Oct-May)" Jan 20, 2025
@kinkard kinkard requested a review from kevinkreiser January 20, 2025 11:18
@kevinkreiser kevinkreiser merged commit 790054e into valhalla:master Jan 28, 2025
7 checks passed
@kinkard kinkard deleted the support-conditional-access branch January 28, 2025 15:19
ianthetechie pushed a commit to ianthetechie/valhalla that referenced this pull request Feb 7, 2025
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.

Unparsed conditional access restriction
3 participants