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.

Sign up for GitHub

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

mark edge segments with interpolated match points #3670

Merged
merged 12 commits into from
Jun 30, 2022
Merged

mark edge segments with interpolated match points #3670

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0a71dce
revert back to old logic with just some slight adjustments
nilsnolde May 29, 2022
26b55fa
fix test
nilsnolde Jun 1, 2022
31c2269
changelog
nilsnolde Jun 1, 2022
79676ce
a refactor to allow modifying segments indices for match points that …
kevinkreiser Jun 29, 2022
faf7868
do the intuitive thing which is to set the index whenever its not set
kevinkreiser Jun 30, 2022
f01b71e
todo about where we should create the segments and not modify them la…
kevinkreiser Jun 30, 2022
ef28f74
Merge remote-tracking branch 'origin/master' into kk_interpolated_mat…
kevinkreiser Jun 30, 2022
354888f
properly build timezones on geos 3.9*
kevinkreiser Jun 30, 2022
0524544
remove old test and add more extensive, new test
nilsnolde Jun 30, 2022
881af5f
revert pybind11 update
nilsnolde Jun 30, 2022
322ca4a
test also matched points
nilsnolde Jun 30, 2022
cea174c
Update CHANGELOG.md
kevinkreiser Jun 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
* FIXED: openstreetmapspeeds global config with `null`s now supported [#3621](https://github.com/valhalla/valhalla/pull/3621)
* FIXED: valhalla_run_matrix was failing (could not find proper max_matrix_distance) [#3635](https://github.com/valhalla/valhalla/pull/3635)
* FIXED: Removed duplicate degrees/radians constants [#3642](https://github.com/valhalla/valhalla/pull/3642)
* FIXED: Some interpolated points had invalid edge_index in trace_attributes response [#3646](https://github.com/valhalla/valhalla/pull/3646)
* FIXED: Forgot to adapt driving side and country access rules in [#3619](https://github.com/valhalla/valhalla/pull/3619) [#3652](https://github.com/valhalla/valhalla/pull/3652)
* FIXED: DateTime::is_conditional_active(...) incorrect end week handling [#3655](https://github.com/valhalla/valhalla/pull/3655)
* FIXED: TimeDistanceBSSMatrix: incorrect initialization for destinations[#3659](https://github.com/valhalla/valhalla/pull/3659)
Expand Down
2 changes: 1 addition & 1 deletion scripts/valhalla_build_timezones
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

error_exit() {
echo "error: $1" 1>&2
exit $(pkg-config geos --modversion | grep -cvF "3.9")
#exit $(pkg-config geos --modversion | grep -cvF "3.9")
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah so we have a bug on geos 3.9*. and in a previous pr we patched it without really checking it. the best thing that we can do here is to not exit if its 3.9. ill change this script to do that. @nilsnolde are you cool if this comes along for the ride? im kind of sick of having to deal with it and have been too lazy to pr it seprately

Copy link
Member

Choose a reason for hiding this comment

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

sure!

}

if ! which spatialite >/dev/null; then
Expand Down
51 changes: 33 additions & 18 deletions src/meili/match_route.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,16 @@ EdgeSegment::EdgeSegment(baldr::GraphId the_edgeid,
* Here we return the vector of edge segments between the source and target states. If its a node to
* node route (meaning no realy edge is traversed) then we use the target_result to say what edge the
* segment should use
*
* TODO: we should modify this function to take the range of MatchResults between the source and
* target. we should then use those to property initialize the segments source/target/indices to
* match those of the MatchResults that lie on them rather than doing it downstream in an otherwise
* superfluous operation
* @param source source state to use to find the route
* @param target target state which candidate in the next column to fetch the route for
* @param route a place to put the edge segments as we create them
* @param target_result in case we have a node to node route we have a no-op edge segment to return
* @return the vector of segments reprsenting the route between source and target
* @return the vector of segments representing the route between source and target
*/
bool MergeRoute(const State& source,
const State& target,
Expand Down Expand Up @@ -129,38 +134,47 @@ bool MergeRoute(const State& source,
void cut_segments(const std::vector<MatchResult>& match_results,
int first_idx,
int last_idx,
const std::vector<EdgeSegment>& segments,
std::vector<EdgeSegment>& segments,
std::vector<EdgeSegment>& new_segments) {
auto first_segment = segments.begin();
auto search_segment = first_segment;
int prev_idx = first_idx;
for (int curr_idx = first_idx + 1; curr_idx <= last_idx; ++curr_idx) {
const MatchResult& curr_match = match_results[curr_idx];
const MatchResult& prev_match = match_results[prev_idx];

// skip if we do not need to cut
if (!curr_match.is_break_point && curr_idx != last_idx) {
continue;
}

// Allow for some fp-fuzz in this gt comparison
// does the previous match point look further along its edge than the current
bool prev_gt_curr = prev_match.distance_along > curr_match.distance_along + 1e-3;

// we want to handle to loop by locating the correct target edge by comparing the distance alone
// if they are on the same edge and previous comes after then its a loop
bool loop = (prev_match.edgeid == curr_match.edgeid) && prev_gt_curr;

// if it is a loop, we start the search after the first edge
auto last_segment = std::find_if(first_segment + static_cast<size_t>(loop), segments.end(),
[&curr_match](const EdgeSegment& segment) {
return (segment.edgeid == curr_match.edgeid);
});
for (search_segment += static_cast<size_t>(loop); search_segment != segments.end();
++search_segment) {
if (search_segment->edgeid == curr_match.edgeid)
break;
}

if (last_segment == segments.cend()) {
if (search_segment == segments.cend()) {
throw std::logic_error("In meili::cutsegments(), unexpectedly unable to locate target edge.");
}

// if its not a break point nor the last segment, then we dont need to split it into two
if (!curr_match.is_break_point && curr_idx != last_idx) {
Copy link
Member Author

Choose a reason for hiding this comment

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

so the main trick is we moved this continue down past where we did the loop to find the segment upon which the current match point lies. this way we have access to it, even if we arent going to cut it below. because i keep the search_segment around for each iteration of the main loop we are gauranteed that the inner loop does no more iterations than it otherwise would have and so this change shouldnt really cost us anything

// but if it isnt a breakpoint and its not the last stateful match point then its interpolated
// and we still need to mark the segments in such a way that we which match points lie on them
if (search_segment->first_match_idx < 0)
search_segment->first_match_idx = curr_idx;
if (search_segment->last_match_idx < 0)
search_segment->last_match_idx = curr_idx;
Comment on lines +167 to +170
Copy link
Member Author

Choose a reason for hiding this comment

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

i'll admit that i didnt think too hard about how correct this is but rather did the conservative thing, that is only set the index if unset. its possible that i should just always set both of them because they will be corrected below and wont have been set up to this point (ie we can probably remove the ifs) but i didnt want to have to prove that to myself at 10pm 😄

// we are done marking it no need to split this one
continue;
}

// we need to close the previous edge
size_t old_size = new_segments.size();
new_segments.insert(new_segments.cend(), first_segment, last_segment + 1);
new_segments.insert(new_segments.cend(), first_segment, search_segment + 1);
new_segments[old_size].first_match_idx = prev_idx;
// when the points got interpolated, we want to use the match results' distance along
// otherwise, we use the segment's source or target because if it is a node snap, the
Expand All @@ -169,9 +183,9 @@ void cut_segments(const std::vector<MatchResult>& match_results,
prev_match.HasState() ? first_segment->source : prev_match.distance_along;
new_segments.back().last_match_idx = curr_idx;
new_segments.back().target =
curr_match.HasState() ? last_segment->target : curr_match.distance_along;
curr_match.HasState() ? search_segment->target : curr_match.distance_along;

first_segment = last_segment;
first_segment = search_segment;
prev_idx = curr_idx;
}
}
Expand Down Expand Up @@ -201,6 +215,7 @@ std::vector<EdgeSegment> ConstructRoute(const MapMatcher& mapmatcher,
for (int curr_idx = 0, n = static_cast<int>(match_results.size()); curr_idx < n; ++curr_idx) {
const MatchResult& match = match_results[curr_idx];

// unmatched or interpolated
if (!match.edgeid.Is_Valid() || !match.HasState()) {
continue;
}
Expand All @@ -214,7 +229,7 @@ std::vector<EdgeSegment> ConstructRoute(const MapMatcher& mapmatcher,
// minimum number of segments. in this case we could at minimum end up with 1 segment
segments.clear();
if (!MergeRoute(prev_state, state, segments, match)) {
// we are only discontinuous but only if this isnt the beginning of the route
// we are only discontinuous if this isnt the beginning of the route
if (!route.empty())
route.back().discontinuity = true;
// next pair
Expand Down
48 changes: 33 additions & 15 deletions test/gurka/test_match.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,30 +11,48 @@ TEST(Standalone, BasicMatch) {

const std::string ascii_map = R"(
1
A---2B-3-4C
|
|5
D
A---2B-3-45C
|
|6
D
)";

const gurka::ways ways = {{"AB", {{"highway", "primary"}}},
{"BC", {{"highway", "primary"}}},
{"CD", {{"highway", "primary"}}}};

const double gridsize = 10;
const double gridsize = 9;
const auto layout = gurka::detail::map_to_coordinates(ascii_map, gridsize);
auto map = gurka::buildtiles(layout, ways, {}, {}, "test/data/basic_match");

auto result = gurka::do_action(valhalla::Options::trace_route, map, {"1", "2", "3", "4", "5"},
"auto", {}, {}, nullptr, "via");

gurka::assert::osrm::expect_match(result, {"AB", "CD"});
gurka::assert::raw::expect_path(result, {"AB", "BC", "CD"});
gurka::assert::raw::expect_maneuvers(result, {DirectionsLeg::Maneuver::kStart,
DirectionsLeg::Maneuver::kRight,
DirectionsLeg::Maneuver::kDestination});

gurka::assert::raw::expect_path_length(result, 0.100, 0.001);
// trace_route
auto result_route =
gurka::do_action(valhalla::Options::trace_route, map, {"1", "2", "3", "4", "5", "6"}, "auto",
{}, {}, nullptr, "via");

gurka::assert::osrm::expect_match(result_route, {"AB", "CD"});
gurka::assert::raw::expect_path(result_route, {"AB", "BC", "CD"});
gurka::assert::raw::expect_maneuvers(result_route, {DirectionsLeg::Maneuver::kStart,
DirectionsLeg::Maneuver::kRight,
DirectionsLeg::Maneuver::kDestination});

gurka::assert::raw::expect_path_length(result_route, 0.099, 0.001);

// trace_attributes
std::string json_str;
auto result_attributes =
gurka::do_action(valhalla::Options::trace_attributes, map, {"1", "2", "3", "4", "5", "6"},
"auto", {}, {}, &json_str, "via");

// confirm one of the interpolated points has the right edge index
rapidjson::Document result_doc;
result_doc.Parse(json_str);
ASSERT_EQ(result_doc["matched_points"].GetArray().Size(), 6);
ASSERT_EQ(result_doc["edges"].GetArray().Size(), 3);

ASSERT_EQ(static_cast<std::string>(result_doc["matched_points"][4]["type"].GetString()),
"interpolated");
ASSERT_EQ(result_doc["matched_points"][4]["edge_index"].GetInt(), 1);
}

TEST(Standalone, UturnMatch) {
Expand Down