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

Exclude cash-only-tolls #3341

Merged
merged 13 commits into from Oct 1, 2021
Merged

Exclude cash-only-tolls #3341

merged 13 commits into from Oct 1, 2021

Conversation

ktatterso
Copy link
Contributor

@ktatterso ktatterso commented Sep 28, 2021

Issue

Support to avoid routing through/on cash-only tolls.

A new request parameter is added: exclude_cash_only_tolls.

Tolls appear on nodes as barrier=toll_booth.

There are 60 types of payment keys, e.g., payment:coins and payment:credit_card. There are only three payment keys that are cash:
payment:cash
payment:coins
payment:notes

On a given tollednode there are frequently multiple payment keys specified, indicating the types of payment dis/allowed.

This PR examines the types of payments on a tolled node and determines if the toll is cash-only. This detail is saved in the tile. At runtime we can then inspect/honor this detail in the Allowed methods.

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?

@ktatterso ktatterso changed the title Work in progress - draft of exclude cash-only-tolls Exclude cash-only-tolls Sep 28, 2021
@ktatterso ktatterso marked this pull request as ready for review September 28, 2021 20:42
@kevinkreiser
Copy link
Member

can you say more about this:

Tolls can appear on nodes and ways.

looking at what you are trying to achieve i dont understand why this isnt sequestered to just nodes in the graph. surely toll collection only ever happens at a node. if osm supports tagging this on the way (i saw a tiny minority did), then i think we should either ignore it or try to mark the node at one end of the way as the collection point. in other words i dont understand why we need this on the directededge. to me it seems we can avoid cash only toll collection by disallowing costing to pass through a node with cash only collection and completely forget about marking the edges with this info.

also i looked at your entry for taginfo and it doesnt seem to match what osm tagging exists. you mentioned it in your description and i see the parser code is correctly pulling out the payment tags but the taginfo entry is talking about our internal boolean that we make in lua not the actual tags from the osm wiki. you need to change the taginfo entry to have the tag payment:* and a value of * i guess or something more like that.

@ktatterso
Copy link
Contributor Author

@kevinkreiser I did some more digging. There appear to be very few ways tagged as toll=yes and requiring cash-only payment.

Just out of interest I also dug into ways tagged as barrier=toll_booth. (These make up 6% of the total entities with this tag, nodes making up the majority.) While there are 1000's of ways with this tag, at least in the cases I examined it appears that those ways are frequently connected to a node tagged barrier=toll_booth.

Point taken, I think it is safe to restrict this to nodes.

@ktatterso
Copy link
Contributor Author

@kevinkreiser Re taginfo, I'm still learning how to update it.

taginfo.json Outdated
{
"key": "payment:*",
"object_types": [
"way",
Copy link
Member

Choose a reason for hiding this comment

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

we are ignoring it on the way for now right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, right

Copy link
Member

@gknisely gknisely left a comment

Choose a reason for hiding this comment

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

looks good now.

@ktatterso ktatterso merged commit 72598d9 into master Oct 1, 2021
@ktatterso ktatterso deleted the exclude-cash-only-tolls branch October 1, 2021 01:06
Comment on lines +208 to +209
!(exclude_cash_only_tolls_ && node->cash_only_toll())) ||
ignore_access_;
Copy link
Member

Choose a reason for hiding this comment

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

this doesnt look right, i think it should be:

return ((node->access() & access_mask_) || ignore_access_) &&
            !(exclude_cash_only_tolls_ && node->cash_only_toll());

basically the ignoring access has to do with the access and the cash toll is a separate concern

cc @gknisely @ktatterso

Copy link
Member

Choose a reason for hiding this comment

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

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