-
Notifications
You must be signed in to change notification settings - Fork 35
Feature/use kd tree for landmark map search #579
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.
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
Open
PaulVerhoeckx
wants to merge
8
commits into
Ekumen-OS:main
Choose a base branch
from
nobleo:feature/use-kd-tree-for-landmark-map-search
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
67a6d26
Implement kd-tree for landmark_map search
PaulVerhoeckx a8cbc12
Implement copy constructors to make change non-breaking
PaulVerhoeckx c1882c6
Add nanoflann dependency to bazel config
PaulVerhoeckx 2c0a8aa
Fix linting
PaulVerhoeckx b9e61ee
Support both Bazel and CMake builds for nanoflann include
PaulVerhoeckx 6d2b289
Update readmes on nanoflann dependency
PaulVerhoeckx ad324d2
Fix clang
PaulVerhoeckx d151687
Pass query pointer to nanoflann
PaulVerhoeckx File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,13 +16,21 @@ | |
| #define BELUGA_SENSOR_DATA_LANDMARK_MAP_HPP | ||
|
|
||
| // external | ||
| // Bazel exposes nanoflann with a prefix, CMake does not | ||
| #if __has_include(<nanoflann/nanoflann.hpp>) | ||
| #include <nanoflann/nanoflann.hpp> | ||
| #else | ||
| #include <nanoflann.hpp> | ||
| #endif | ||
| #include <range/v3/view/filter.hpp> | ||
| #include <range/v3/view/tail.hpp> | ||
| #include <sophus/se3.hpp> | ||
|
|
||
| // standard library | ||
| #include <algorithm> | ||
| #include <cstdint> | ||
| #include <memory> | ||
| #include <unordered_map> | ||
| #include <utility> | ||
| #include <vector> | ||
|
|
||
|
|
@@ -48,7 +56,9 @@ class LandmarkMap { | |
| /// @param boundaries Limits of the map. | ||
| /// @param landmarks List of landmarks that can be expected to be detected. | ||
| 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(); | ||
| } | ||
|
|
||
| /// @brief Constructor with implicit map boundaries (computed from landmarks). | ||
| /// @details Note that computing map boundaries from landmarks will effectively | ||
|
|
@@ -67,9 +77,46 @@ class LandmarkMap { | |
| map_boundaries_.min() = map_boundaries_.min().cwiseMin(position); | ||
| map_boundaries_.max() = map_boundaries_.max().cwiseMax(position); | ||
| } | ||
| build_category_indices(); | ||
| } | ||
| } | ||
|
|
||
| /// @brief Copy constructor. | ||
| /// @details Copying requires expensive reconstruction of the cached kd-tree indices. | ||
| /// @deprecated Use move semantics (std::move) instead to avoid performance overhead. | ||
| /// @param other Landmark map to copy from. | ||
| [[deprecated("LandmarkMap copying is expensive. Use std::move() instead.")]] LandmarkMap(const LandmarkMap& other) | ||
| : landmarks_(other.landmarks_), map_boundaries_(other.map_boundaries_) { | ||
| build_category_indices(); | ||
| } | ||
|
|
||
| /// @brief Copy assignment operator. | ||
| /// @details Copying requires expensive reconstruction of the cached kd-tree indices. | ||
| /// @deprecated Use move semantics (std::move) instead to avoid performance overhead. | ||
| /// @param other Landmark map to copy from. | ||
| /// @return Reference to this landmark map. | ||
| [[deprecated("LandmarkMap copying is expensive. Use std::move() instead.")]] LandmarkMap& operator=( | ||
| const LandmarkMap& other) { | ||
| if (this != &other) { | ||
| landmarks_ = other.landmarks_; | ||
| map_boundaries_ = other.map_boundaries_; | ||
| category_indices_.clear(); | ||
| build_category_indices(); | ||
| } | ||
| return *this; | ||
| } | ||
|
|
||
| /// @brief Move constructor. | ||
| /// @details Explicitly defaulted so landmark data and cached kd-tree indices | ||
| /// can be transferred efficiently while preserving unique ownership semantics. | ||
| LandmarkMap(LandmarkMap&&) = default; | ||
|
|
||
| /// @brief Move assignment operator. | ||
| /// @details Explicitly defaulted so ownership of the cached kd-tree indices can | ||
| /// be transferred efficiently without rebuilding them. | ||
| /// @return Reference to this landmark map. | ||
| LandmarkMap& operator=(LandmarkMap&&) = default; | ||
|
|
||
| /// @brief Returns the map boundaries. | ||
| /// @return The map boundaries. | ||
| [[nodiscard]] LandmarkMapBoundaries map_limits() const { return map_boundaries_; } | ||
|
|
@@ -81,32 +128,19 @@ class LandmarkMap { | |
| [[nodiscard]] std::optional<LandmarkPosition3> find_nearest_landmark( | ||
| const LandmarkPosition3& detection_position_in_world, | ||
| const LandmarkCategory& detection_category) const { | ||
| // only consider those that have the same id | ||
| auto same_category_landmarks_view = | ||
| landmarks_ | ranges::views::filter([detection_category = detection_category](const auto& l) { | ||
| return detection_category == l.category; | ||
| }); | ||
|
|
||
| // find the landmark that minimizes the distance to the detection position | ||
| // This is O(n). A spatial data structure should be used instead. | ||
| auto min = std::min_element( | ||
| same_category_landmarks_view.begin(), same_category_landmarks_view.end(), | ||
| [&detection_position_in_world](const auto& a, const auto& b) { | ||
| const auto& landmark_a_position_in_world = a.detection_position_in_robot; | ||
| const auto& landmark_b_position_in_world = b.detection_position_in_robot; | ||
|
|
||
| const auto landmark_b_squared_in_world_squared = | ||
| (landmark_a_position_in_world - detection_position_in_world).squaredNorm(); | ||
| const auto landmark_b_distance_in_world_squared = | ||
| (landmark_b_position_in_world - detection_position_in_world).squaredNorm(); | ||
| return landmark_b_squared_in_world_squared < landmark_b_distance_in_world_squared; | ||
| }); | ||
|
|
||
| if (min == same_category_landmarks_view.end()) { | ||
| const auto it = category_indices_.find(detection_category); | ||
| if (it == category_indices_.end()) { | ||
| return std::nullopt; | ||
| } | ||
|
|
||
| return min->detection_position_in_robot; | ||
| const auto& index = *it->second; | ||
| const std::array<double, 3> query = { | ||
| detection_position_in_world.x(), detection_position_in_world.y(), detection_position_in_world.z()}; | ||
| CategoryIndexType result_idx; | ||
| double result_dist_sq; | ||
| if (index.tree->knnSearch(query.data(), 1, &result_idx, &result_dist_sq) == 0) { | ||
| return std::nullopt; | ||
| } | ||
| return index.cloud.pts[result_idx]; | ||
| } | ||
|
|
||
| /// @brief Finds the landmark that minimizes the bearing error to a given detection and returns its data. | ||
|
|
@@ -161,8 +195,52 @@ class LandmarkMap { | |
| } | ||
|
|
||
| private: | ||
| /// Point cloud adapter used by the category kd-trees. | ||
| struct PositionCloud { | ||
| std::vector<LandmarkPosition3> pts; | ||
| [[nodiscard]] std::size_t kdtree_get_point_count() const { return pts.size(); } | ||
| [[nodiscard]] double kdtree_get_pt(std::size_t i, std::size_t dim) const { return pts[i](static_cast<int>(dim)); } | ||
| template <class BBox> | ||
| bool kdtree_get_bbox(BBox&) const { | ||
| return false; | ||
| } | ||
| }; | ||
|
|
||
| /// Index type used by the category kd-trees. | ||
| using CategoryIndexType = std::uint32_t; | ||
|
|
||
| /// kd-tree type used to query landmarks within each category. | ||
| using CategoryKDTree = nanoflann::KDTreeSingleIndexAdaptor< | ||
| nanoflann::L2_Simple_Adaptor<double, PositionCloud>, | ||
| PositionCloud, | ||
| 3, | ||
| CategoryIndexType>; | ||
|
|
||
| /// Cached landmark positions and search tree for a single category. | ||
| struct CategoryIndex { | ||
| PositionCloud cloud; | ||
| std::unique_ptr<CategoryKDTree> tree; | ||
| }; | ||
|
|
||
| landmarks_set_position_data landmarks_; | ||
| LandmarkMapBoundaries map_boundaries_; | ||
| std::unordered_map<LandmarkCategory, std::unique_ptr<CategoryIndex>> category_indices_; | ||
|
|
||
| /// @brief Builds per-category kd-tree indices for nearest-neighbor search. | ||
| void build_category_indices() { | ||
| for (const auto& l : landmarks_) { | ||
| auto& entry = category_indices_[l.category]; | ||
| if (!entry) { | ||
| entry = std::make_unique<CategoryIndex>(); | ||
| } | ||
| entry->cloud.pts.push_back(l.detection_position_in_robot); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
| for (auto& [_, entry] : category_indices_) { | ||
| entry->tree = std::make_unique<CategoryKDTree>( | ||
| 3, entry->cloud, nanoflann::KDTreeSingleIndexAdaptorParams(/*leaf_max_size=*/10)); | ||
| entry->tree->buildIndex(); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| } // namespace beluga | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thecategory_indices_storage. Maybe we would have to splitbuild_category_indicesin two, one step to fill theptsvectors, 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?