Feature/use kd tree for landmark map search - #579
Conversation
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
4ce4fb8 to
2c0a8aa
Compare
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
|
Thanks for the PR, @PaulVerhoeckx . Notice that you can run the test suite very similarly to how CI builds and runs it by executing |
Signed-off-by: Paul Verhoeckx <paul.verhoeckx@nobleo.nl>
Thanks for the tip, I think it will pass now;) |
glpuga
left a comment
There was a problem hiding this comment.
Nice job! we created this model for a proof of concept demo a long time ago and we did not put a lot performance into it, it's good to see it getting some dedication.
| if (!entry) { | ||
| entry = std::make_unique<CategoryIndex>(); | ||
| } | ||
| entry->cloud.pts.push_back(l.detection_position_in_robot); |
There was a problem hiding this comment.
It may be worth it to do a first pass counting how many landmarks for each category are there, to reserve the size in the pts vector and that way avoid reallocations as it grows.
| explicit LandmarkMap(const LandmarkMapBoundaries& boundaries, landmarks_set_position_data landmarks) | ||
| : landmarks_(std::move(landmarks)), map_boundaries_(std::move(boundaries)) {} | ||
| : landmarks_(std::move(landmarks)), map_boundaries_(std::move(boundaries)) { | ||
| build_category_indices(); |
There was a problem hiding this comment.
Feels wasteful to store the landmarks along with the nanoflann index, since both contain the same information in different forms, and we only use the index later. We only use the landmarks struct to recreate the index in case of copy/move operations.
We can probably remove landmarks_ if we refactor the category_indices_ storage. Maybe we would have to split build_category_indices in two, one step to fill the pts vectors, and another separate one to create the indices.
The first one is only done in the constructor that takes in the landmarks data. The map of categories and the points for each become trivially copyable/movable in all constructor/assignment operators, and we need only build the nanoflann index from that data.
Does it make sense?
|
@PaulVerhoeckx ignore the rolling failure. If you rebase to the current main we no longer run CI against rolling as it is a moving target. |
Proposed changes
Implement efficient nearest-neighbor landmark search using per-category KD-trees with nanoflann. Provides O(log n) spatial queries instead of O(n) linear search for position-based landmark matching.
Tested for a particle filter with:
This reduced the filter update time from 1.4 seconds to 0.005 seconds. (12th Gen Intel® Core™ i7-1255U × 12)
Changes
find_nearest_landmark()using KD-tree queries for efficient position-based matchingType of change
Checklist
Put an
xin the boxes that apply. This is simply a reminder of what we will require before merging your code.Additional comments
Note on
find_closest_bearing_landmark()implementation:This method currently uses O(n) linear search instead of the KD-tree indices because bearing-based matching requires angular distance metrics, which are fundamentally incompatible with the Euclidean L2 distance metric used by the position-based KD-trees.