Don't penalize gates that have allowed access#3078
Conversation
2a2b6a9 to
078ebcb
Compare
| sif::Cost c; | ||
| c += country_crossing_cost_ * (node->type() == baldr::NodeType::kBorderControl); | ||
| c += gate_cost_ * (node->type() == baldr::NodeType::kGate); | ||
| c += gate_cost_ * (node->type() == baldr::NodeType::kGate) * (!InitiallyAllowed(node)); |
There was a problem hiding this comment.
why do you use Allowed method instead of simple access_indication flag ?
| c += gate_cost_ * (node->type() == baldr::NodeType::kGate) * (!InitiallyAllowed(node)); | |
| c += gate_cost_ * (node->type() == baldr::NodeType::kGate) * (!node->access_indication()); |
| * @param node Pointer to node information. | ||
| * @return Returns true if access was specified for the node and access is allowed. | ||
| */ | ||
| virtual bool InitiallyAllowed(const baldr::NodeInfo* node) const { |
There was a problem hiding this comment.
what is the purpose of this method ? how is supposed to be used ?
There was a problem hiding this comment.
i'll add that we must be very careful when considering expanding the scope of the costing interface and we should consider existing semantics and algorithms that make use of those and if perhaps they already could handle the functionality we are looking to achieve
from the title and a bit of looking at the diff i see that we are basically trying keep track of whether or not access was "tagged" or "inferred" for barrier nodes. i guess the idea is that if its tagged as access yes then we dont need to penalize it. if that is the case it seems to me all we need to do is:
- make the changes to nodeinfo to have the extra bit that says the access is tagged (i would consider naming it as such as well since we have the same concept for speeds on ways being "tagged")
- modify base_transition_cost to check that flag on the nodeinfo and not penalize if its there
There was a problem hiding this comment.
The main idea was to penalise the gates with no access information and additionally the gates that have restricted access. But since the fine does not affect the restricted nodes I agree that it is clearer to use the flag directly.
7b3d6be to
acd2f60
Compare
acd2f60 to
b84725e
Compare
| * CHANGED: Use std::shared_ptr in case if ENABLE_THREAD_SAFE_TILE_REF_COUNT is ON. [#3067](https://github.com/valhalla/valhalla/pull/3067) | ||
| * CHANGED: Reduce stop impact when driving in parking lots [#3051](https://github.com/valhalla/valhalla/pull/3051) | ||
| * ADDED: Added another through route test [#3074](https://github.com/valhalla/valhalla/pull/3074) | ||
| * ADDED: Do not penalise gates that have allowed access [#3078](https://github.com/valhalla/valhalla/pull/3078) |
There was a problem hiding this comment.
| * ADDED: Do not penalise gates that have allowed access [#3078](https://github.com/valhalla/valhalla/pull/3078) | |
| * ADDED: Do not penalize gates that have allowed access [#3078](https://github.com/valhalla/valhalla/pull/3078) |
kevinkreiser
left a comment
There was a problem hiding this comment.
looks good to me as well! thank you!
Issue
Gates that have specified access information should not be penalised.
The solution takes 1 spare bit from NodeInfo and OSMNode. But the collected info can also be used for other node types like bollard, sump buster and etc.
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?