add display lat lon to locations and use for side of street if provided #1769
Conversation
src/baldr/location.cc
Outdated
| throw std::runtime_error{"lon is missing"}; | ||
| }; |
There was a problem hiding this comment.
this was just a simplification of the formatting. was driving me nuts
src/loki/search.cc
Outdated
| candidate.get_side(location.display_latlng_ ? *location.display_latlng_ : location.latlng_, | ||
| location.display_latlng_ | ||
| ? location.display_latlng_->DistanceSquared(candidate.point) | ||
| : candidate.sq_distance); |
There was a problem hiding this comment.
this is the meat of the change. if we provided a display ll we use that in conjunction with its distance from the candidate/snapped location to determine side of street, otherwise we fall back to the previous logic which was to use the input location ll as the display ll as well as the routing ll and its distance from the candidate/snapped location
There was a problem hiding this comment.
we really should handle that TODO before we ship this need to worry about the case where the display ll isnt really near the route ll
There was a problem hiding this comment.
Resolved that TODO in 599ba26. Let me know if that's along the lines of what you were thinking as well.
| lon = midgard::circular_range_clamp<float>(*lon, -180, 180); | ||
| location->mutable_display_ll()->set_lat(*lat); | ||
| location->mutable_display_ll()->set_lng(*lon); | ||
| } |
There was a problem hiding this comment.
this is where we parse the request from json into our internal pbf request object
| x.display_latlng_ = {answer.first - ortho.x(), answer.second - ortho.y()}; | ||
| search(x, false, answer, | ||
| {PE{{t, l, 3}, ratio, answer, 0, S::LEFT}, PE{{t, l, 8}, 1.f - ratio, answer, 0, S::RIGHT}}); | ||
|
|
There was a problem hiding this comment.
what we do here is we take a test for side of street that was relying on the routing ll to determine side of street and we put the routing ll on the road and then set the display ll to what was the routing ll. in this case the display ll is offset perpendicular to the road. we test a display ll on both sides of the road.
test/search.cc
Outdated
| search(x, false, answer, | ||
| {PE{{t, l, 3}, ratio, answer, 0, S::RIGHT}, PE{{t, l, 8}, 1.f - ratio, answer, 0, S::LEFT}}); | ||
|
|
||
| // try nuking display ll by setting street_side_max_distance less than display ll distance |
There was a problem hiding this comment.
Added a test here for street_side_max_distance
…ly consolidate these test configs into one)
|
@danpaz im going to answer your question to the outdated comment here so it doesnt get burried. the default distance seems a bit excessive imho. 5km is a very far distance, maybe something more like 1km? at any rate it does solve part of the problem i was alluding to with that TODO but i should have explained a bit further. let me take a stab at that. the unexpanded upon part of my worry in that TODO was an angular one. specifically what does "to the right" or "to the left" really mean? geometrically speaking you can say does a point lie on either side of this line (geodesic in our case) by looking at the sign of the area of the triangle formed by the point and 2 points along the line (we have functions for this already). however my intuition tells me that a human will not see it so simply. rather i suspect a human expects a cone to the left or to the right of their position within which they would consider something to the left or to the right of them.
so there you can see the tangent at the snap point is red and the cones are blue. geometrically a point inbetween the red and blue would still have a side of street but might be pretty confusing to a person. now you might say, a ha but the snap point is always perpendicular to the display ll so the angle from the snap to the display ll (green dot). that isnt the case sadly. the display ll can be in any orientation to the snap point. the snap point is usually (but also not always) perpendicular to the input ll but the display ll is any random ll that is provided. so anyway i think we need to do one more thing:
|
src/loki/search.cc
Outdated
| // add 360 degrees if angle becomes negative | ||
| auto angle_diff = (angle_to_point - tang_angle) >= 0.0f ? (angle_to_point - tang_angle) | ||
| : (angle_to_point - tang_angle + 360.0f); |
There was a problem hiding this comment.
i think this can be:
auto angle_diff = angle_to_point - tang_angle;
if(angle_diff < 0.f) {
angle_diff += 360.f;
}
…5000m as some tests use points > 1000m away from the road
| size_t index = edge->forward() ? 0 : info.shape().size() - 2; | ||
| float angle = | ||
| tangent_angle(index, candidate.point, info.shape(), | ||
| GetOffsetForHeading(edge->classification(), edge->use()), edge->forward()); |
There was a problem hiding this comment.
well this is unfortunate, but correct. we need to measure the tangent at the node for every edge. this means a bunch of tangent checks. if performance doesnt check out we shoudl think about putting the distance check first before the tangent... but then we have the fact that heading might need it... gah, lets see what performance numbers say before we complicate our lives..
|
@danpaz well i went to leave a reivew on this and then remember i made this PR originally.... anyway my review is: Approved pending performance regression testing |
masterkk_display |
* master: Protect against nan in uniform_resample_spherical_polyline (#2431) Update config.h.cmake nit: Adds missing cmake dependency for benchmarks (#2430) add display lat lon to locations and use for side of street if provided (#1769) Fixes includes for projects using Valhalla as a library (#2423) Add additional exit phrases (#2421)
this adds display ll to the input location so that in the case that you have both a routing coordinate and a display coordinate you can use the former to control how the route is generated and the latter to control the side of street output (narrative). in the event that a display ll is not provided, the required ll will be used, as it already was, as both the routing and display ll.