Heading filter fix for node search #2058
Conversation
…ading filter. Need to use opposite logic and make sure the tile and edgeinfo for the opposing edge is used in case it was from a different tile and the forward flag is the same as the current edge.
src/loki/search.cc
Outdated
| // index is opposite the logic above, plus we need to use the edgeinfo and shape | ||
| // from the other tile since we use the other_edge forward flag | ||
| auto index = other_edge->forward() ? info.shape().size() - 2 : 0; | ||
| if (heading_filter(other_edge, other_tile->edgeinfo(other_edge->edgeinfo_offset()), |
There was a problem hiding this comment.
the only time the other edge info could be different is if they had different names, it technically isnt a bug to use the edge info from not this edge as in terms of the shape and direction of the shape, there will be no difference other than where it is in memory or disk.
only reason im saying this is because i purposely didnt use the opposing edges shape because the decoded shape gets cached on access, so in the case that it is a differetn tile with a different edge info (with the same shape), then we needlessly decode it again. i get that this is more clear, but it cant be less performant.
probably a comment mentioning why its ok to use the same shape from the other edge would be sufficient? or we can ignore the potential performance ramifications. what are your thoughts?
There was a problem hiding this comment.
also, thanks for catching the index thing!
There was a problem hiding this comment.
@dnesbitt61 the same bug exists on line 380 with regard to the index. we'll need to amend the pr to subtract the candidate index from the total shape size in that case right?
There was a problem hiding this comment.
problem with using the shape from the original edge is that if the opposing edge is in a different tile it will have its own edge info and shape with the forward flag set to true. So I'm pretty sure there can be an incompatibility here since both directed edges will have forward == true. Would need to bypass the forward flag (e.g., send it in as an argument rather than using the directed edge).
There was a problem hiding this comment.
also note that you could compute the angle once and then use the angle 180 degrees for the opposing edge...trick is how to do this when other edge filters are in play...
… based on orientation of OSM nodes on the original "edge" so opposing edges in adjacent tiles should have opposing forward flags.
Fix logic for shape index on the opposing (other) edge in the node heading filter. Need to use opposite logic and make sure the tile and edgeinfo for the opposing edge is used in case it was from a different tile and the forward flag is the same as the current edge.
Issue
fixes #2057
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?