fix shape trimming for map matching (gurka/actor refactor + macos ccache) #2326
Conversation
| @@ -97,35 +97,28 @@ void trim_shape(float start, | |||
| std::vector<PointLL>& shape) { | |||
There was a problem hiding this comment.
this function is the crux of the fix. the diff is very hard to read but the previous code relied on finding the appropriate distance along the shape and only then setting the coordinate (ie trimming). the problem with that approach is if you are off by some small floating point amount you can end up never finding the spot to place the start vertex. instead we refactor to stop when we hit the tolerance or if we hit the end and then assign the vertex regardless.
previously what would happen is you'd get the whole shape and now we get that tiny little segment at the end.
| @@ -18,6 +18,10 @@ struct actor_t::pimpl_t { | |||
| : reader(new baldr::GraphReader(config.get_child("mjolnir"))), loki_worker(config, reader), | |||
| thor_worker(config, reader), odin_worker(config) { | |||
| } | |||
| pimpl_t(const boost::property_tree::ptree& config, baldr::GraphReader& graph_reader) | |||
There was a problem hiding this comment.
for gurka, and other users, we allow you to pass in a graphreader instead of creating one. this is handy when you already have a reader around and want to re-use it. especially if you want to return pointers into data structures you want them to persist after the call. this isnt the case in gurka but can be the case in other uses of the library
| auto* tile = reader.GetGraphTile(tile_id); | ||
| const std::string& end_node, | ||
| const baldr::GraphId& tile_id = baldr::GraphId{}) { | ||
| // if the tile was specified use it otherwise scan everything |
There was a problem hiding this comment.
i made it so you didnt have to specify the tile id if you didnt want to. for most tests the tilesets are so small we just let the function search over all edges in the graph. if you have a massive data set you can still specify the tile to speed it up
| const std::string& way_name, | ||
| const std::string& end_node) { | ||
| baldr::GraphReader reader(map.config.get_child("mjolnir")); | ||
| return findEdge(reader, map.nodes, tile_id, way_name, end_node); |
There was a problem hiding this comment.
we dont want to do this because we return pointers into data that will no longer exist once the graphreader goes out of scope at the end of this function
| odin_worker.cleanup(); | ||
|
|
||
| return request; | ||
| valhalla::tyr::actor_t actor(map.config, *reader, true); |
There was a problem hiding this comment.
now that we can pass the reader its much easier to just use the actor directly
|
|
||
| valhalla::Api unserialized_route(const std::string& request_str, | ||
| const std::function<void()>& interrupt = []() -> void {}); | ||
| valhalla::Api unserialized_trace_route(const std::string& request_str, |
There was a problem hiding this comment.
i removed the unserialized functions in favor of adding the api pointer to all of the functions. so if you pass a non null Api object to the function the actor will pass it back to you at that pointer.
kevinkreiser
changed the title
|
the lint build fails because of: diff --git a/src/midgard/util.cc b/src/midgard/util.cc
index b7c1598..d8365a4 100644
--- a/src/midgard/util.cc
+++ b/src/midgard/util.cc
@@ -91,9 +91,9 @@ template <class container_t> container_t trim_front(container_t& pts, const floa
}
void trim_shape(float start,
- PointLL start_vertex,
+ const PointLL& start_vertex,
float end,
- PointLL end_vertex,
+ const PointLL& end_vertex,
std::vector<PointLL>& shape) {
// clip up to the start point if the start_vertex is valid
float along = 0.f;
diff --git a/valhalla/midgard/util.h b/valhalla/midgard/util.h
index c84990c..8f20459 100644
--- a/valhalla/midgard/util.h
+++ b/valhalla/midgard/util.h
@@ -255,9 +255,9 @@ template <class container_t> container_t trim_front(container_t& pts, const floa
* @param shape Shape, as vector of PointLLs
*/
void trim_shape(float start,
- PointLL start_vertex,
+ const PointLL& start_vertex,
float end,
- PointLL end_vertex,
+ const PointLL& end_vertex,
std::vector<PointLL>& shape);Which we actually want because we pass points from the |
test/gurka/test_trimming.cc
Outdated
| return loc; | ||
| } | ||
|
|
||
| TEST(TimeTracking, routes) { |
axiom 1:
directed edges reference their actual shape but they also store an integer measure of meters of length rounded to the nearest meter. this is useful for costing so we can quickly look up length to the nearest half meter and use it to determine the amount of time spent on a given edge at a given speed.
axiom 2:
in map matching we disable node snapping when performing candidate search for a given trace point. this means that some edge candidates end up really really close to the beginning or end of an edge. this can happen in regular routing too if you disable node snapping but...
axiom 3:
in map matching we always measure the actual length of the shape of an edge to get the percentage along that edges shape the candidate (snap) location lies (as a percentage 0 - 1). in the candidate search for routing (its different, long story, worthy of tech debt cleanup but non trivial performance implications) we always make sure to use the rounded edge length rather than compute it (since thats expensive)
lemma 1:
if you snap near enough to the end of the shape, the percentage offset that the candidate search for map matcher computes, when multiplied by the rounded length will give a trim length that is longer than the actual shape if the rounding happened upwards.
to fix this we need to modify the trim function to be robust to this situation or modify map matching to use the rounded length rather than the measured.
as a final fix all of the candidate searches should probably use the actual shape to determine the offset percentage but this can wait for later as it stands to make a lot of RAD changes.