Per location search filters for functional road class and forms of way #2289

danpaz · 2020-03-26T21:36:34Z

Issue

This PR enables excluding edge candidates from Loki's search results with a new per-location parameter search_filter. Unlike heading and preferred_side filters, search_filter removes edges from the set of candidates entirely. The idea is to customize snapping behavior for routing: in some cases you don't want to snap to a highway or tunnel if you know your destination is a building centroid, even if that building is closer to the highway than the nearest navigable road.

Supports filtering on the following parameters:

min_road_class (string, default "service_other"): min functional road class allowed, options are from highest to lowest: "motorway","trunk","primary","secondary","tertiary","unclassified","residential","service_other"
max_road_class (string, default "motorway"): max functional road class allowed, options are from highest to lowest: "motorway","trunk","primary","secondary","tertiary","unclassified","residential","service_other"
exclude_tunnel (boolean, default false) exclude roads with tunnel attribution
exclude_bridge (boolean, default false) exclude roads with bridge attribution
exclude_ramp (boolean, default false) exclude roads with ramp use (includes links and turn channels)

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Generally use squash merge to rebase and clean comments before merging
Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

danpaz · 2020-03-26T21:38:18Z

proto/tripcommon.proto

@@ -122,3 +133,14 @@ message TurnLane {
  optional uint32 directions_mask = 1;
  optional bool is_active = 2;
 }
+
+enum RoadClass {


Refactored this enum out of TripLeg and into the top level namespace in 99998bd so I could use it in the Location SearchFilter message. Actually much of the diff in this PR is that one refactor commit.

Do we have any concerns about backward compatibility here? This will break any existing users of the protobuf definitions unless they too refactor.

I may be wrong but AFAIK the proto definitions are only used within Valhalla, and I'm not aware of a situation where valhalla processes of different versions would be communicating with each other.

That was my understanding as well, the external API of Valhalla doesn't include protobuf definitions.

correct we have not yet exposed the protobuf API externally so we are ok to do things like this until we cross that barrier

danpaz · 2020-03-26T21:39:42Z

src/loki/search.cc

@@ -22,6 +23,22 @@ template <typename T> inline T square(T v) {
  return v * v;
 }

+bool search_filter(const DirectedEdge* edge, const Location::SearchFilter& search_filter) {


The actual edge filtering happens in this function.

A highly subjective idea, take it or leave it :)

The function name I think could be something that reflects that it is a bool and what the outcome is without the user having to check the implementation. Something like is_search_filter_triggered or similar would convey to the reader in other parts of the code what it is doing and returning.

Agreed, generally any function that returns bool is clearer to prefix with is_ or has_.

danpaz · 2020-03-26T21:41:24Z

test/gurka/gurka.h

@@ -582,6 +582,11 @@ route(const map& map, const std::string& from, const std::string& to, const std:
  return route(map, {from, to}, costing);
 }

+valhalla::Api route(const map& map, const std::string& request_json) {


cc @danpat I added this for convenience of writing tests with additional parameters in the request. If you have a neater pattern in mind for custom request parameters let me know.

danpaz · 2020-03-26T21:42:33Z

valhalla/baldr/location.h

+  /**
+   * Optional filters supplied in the request.
+   */
+  struct SearchFilter {


This struct is a mirror of the protobuf message, per @kevinkreiser there's a larger refactor to be done here for Loki to use the proto generated valhalla::Location directly. I didn't tackle that refactor in this PR.

Could we add a HUGE TODO and CAUTION to the docstring with what your comment just said?

mandeepsandhu · 2020-03-27T05:22:21Z

Issue

This PR enables excluding edge candidates from Loki's search results with a new per-location parameter search_filter. Unlike heading and preferred_side filters, search_filter removes edges from the set of candidates entirely. The idea is to customize snapping behavior for routing: in some cases you don't want to snap to a highway or tunnel if you know your destination is a building centroid, even if that building is closer to the highway than the nearest navigable road.

Supports filtering on the following parameters:

min_road_class (string, default "service_other"): min functional road class allowed, options are from highest to lowest: "motorway","trunk","primary","secondary","tertiary","unclassified","residential","service_other"

max_road_class (string, default "motorway"): max functional road class allowed, options are from highest to lowest: "motorway","trunk","primary","secondary","tertiary","unclassified","residential","service_other"

exclude_tunnel (boolean, default false) exclude roads with tunnel attribution

exclude_bridge (boolean, default false) exclude roads with bridge attribution

exclude_ramp (boolean, default false) exclude roads with ramp use (includes links and turn channels)

Tasklist

Add tests

Add #fixes with the issue number that this PR addresses

Generally use squash merge to rebase and clean comments before merging

Update the changelog

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

Is this filtering applied only when its searching for the destination node? Eg if exclude_bridge is set and the destination is a bridge itself, then does it result in a "no route"?

purew · 2020-03-27T12:48:02Z

src/loki/search.cc

+        if (!c_itr->prefiltered) {
+          all_prefiltered = false;
+        }
+        // all_prefiltered = all_prefiltered || !c_itr->prefiltered;


Stray comment?

purew · 2020-03-27T13:01:12Z

test/gurka/test_search_filter.cc

+                              {"BC", {{"highway", "primary"}}},
+                              {"CD", {{"highway", "primary"}}},
+                              {"AD", {{"highway", "primary"}}}};
+    map = gurka::buildtiles(layout, ways, {}, {}, "test/data/search_filter_heading");


Does buildtiles do nothing if invoked again with the same tileset? If not, this pattern may result in several runs of the tile baking process. (Maybe it's lightning quick given the small graph).

It'll rebuild tiles on every run, and can be a bit slow (on the order of 5-10 seconds per build). We should try to group test maps together to minimize build times, so I simplified the tests to that end in a73ed02.

purew · 2020-03-27T13:21:57Z

test/gurka/test_search_filter.cc

+  auto result_filtered = gurka::route(map, request_filtered);
+
+  gurka::assert::osrm::expect_route(result_filtered, {"ABE", "BC", "CD"});
+}


These tests are a joy to read, great work @danpaz & @danpat

purew

Looks good to me overall, only added a comment to move your github comment into the code comment for the struct mirroring protobuf.

src/loki/search.cc

kevinkreiser · 2020-03-30T13:37:19Z

src/loki/search.cc

@@ -87,6 +105,7 @@ struct candidate_t {
  double sq_distance;
  PointLL point;
  size_t index;
+  bool prefiltered;


src/loki/search.cc

src/mjolnir/statistics.cc

…in SearchFilter

* master: Refactor some functions for easier re-use. (#2302) typo in bitfield struct directededge (#2301) Re-enables changelog validation (#2294) ref and directional fixes (#2298) Add resample_distance (#2261) Fix Some ASAN Bugs (#2296) Update codecov ignore directory to third_party (#2297) Per location search filters for functional road class and forms of way (#2289) Adds traffic::Tile accessor function to GraphTile (#2295) Properly set the not_thru_pruning flag on the destination edge (just as (#2291) Range updates for datetime and emergency updates. (#2290)

danpaz requested review from kevinkreiser and danpat March 26, 2020 21:36

danpaz commented Mar 26, 2020

View reviewed changes

danpaz force-pushed the search_filters branch from 026c95f to 2f1b4d0 Compare March 26, 2020 21:45

danpaz mentioned this pull request Mar 26, 2020

Snapped onto the motorway #1820

Open

danpaz requested a review from purew March 26, 2020 21:50

purew reviewed Mar 27, 2020

View reviewed changes

kevinkreiser reviewed Mar 30, 2020

View reviewed changes

src/loki/search.cc Show resolved Hide resolved

kevinkreiser reviewed Mar 30, 2020

View reviewed changes

src/loki/search.cc Outdated Show resolved Hide resolved

kevinkreiser reviewed Mar 30, 2020

View reviewed changes

src/mjolnir/statistics.cc Outdated Show resolved Hide resolved

danpaz added 12 commits April 2, 2020 12:54

Allow per-location search filtering by roadclass

e550bbb

Refactor RoadClass proto enum into tripcommon so we can later use it …

40d6854

…in SearchFilter

Use RoadClass enum in SearchFilter

6879a47

Cleanup tests

6b9e677

Add additional search filters: tunnel, bridge, ramp. [wip tests failing]

7df377c

Origin needs to be offset slightly to snap to AB

d098327

Add missing properties in toPBF

1760802

Changelog

480dfea

Simplify tests to reduce map builds

6dfd0f5

Cleanup and code comments

f06e43e

Back out namespace changes in mjolnir

a069266

Fix linker errors

594d308

danpaz added 2 commits April 2, 2020 13:58

Remove unnecessary if branch

7a374fe

Add docs around search_filter

9888ae0

danpaz force-pushed the search_filters branch from d596597 to 9888ae0 Compare April 2, 2020 18:13

danpaz requested a review from kevinkreiser April 2, 2020 18:31

kevinkreiser approved these changes Apr 3, 2020

View reviewed changes

Merge branch 'master' into search_filters

56edde6

danpaz merged commit ac22924 into master Apr 3, 2020

danpaz deleted the search_filters branch April 3, 2020 14:58

danpaz mentioned this pull request Apr 13, 2020

Fix break/continue typo in search filtering #2317

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per location search filters for functional road class and forms of way #2289

Per location search filters for functional road class and forms of way #2289

danpaz commented Mar 26, 2020 •

edited

Loading

danpaz Mar 26, 2020

danpat Mar 26, 2020

purew Mar 27, 2020

danpaz Mar 27, 2020

kevinkreiser Mar 30, 2020 •

edited

Loading

danpaz Mar 26, 2020

purew Mar 27, 2020

danpaz Mar 27, 2020

danpaz Mar 26, 2020

danpaz Mar 26, 2020

purew Mar 27, 2020

mandeepsandhu commented Mar 27, 2020

Issue

Tasklist

Requirements / Relations

purew Mar 27, 2020

purew Mar 27, 2020

danpaz Mar 27, 2020

purew Mar 27, 2020

purew left a comment

kevinkreiser Mar 30, 2020

Per location search filters for functional road class and forms of way #2289

Per location search filters for functional road class and forms of way #2289

Conversation

danpaz commented Mar 26, 2020 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinkreiser Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandeepsandhu commented Mar 27, 2020

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpaz commented Mar 26, 2020 •

edited

Loading

kevinkreiser Mar 30, 2020 •

edited

Loading