Conversation
SteveMacenski
left a comment
There was a problem hiding this comment.
I'd recommend spending some time to think about how we can most optimally compute this since this is going to be a little heavy so its worth some performance considerations.
Off the cuff:
- We don't really need to store an internal representation of the layer at all, so this could derive from the
Layerrather thanCostmapLayersince we shouldn't really need the internal costmap structure. - We default assume all points are lethal unless proven otherwise in the
for eachloop in the update window. - Given the path, we shift the path left/right by the left/right tolerances by the base frame. If we enclose the start of the path and the end of the path segment, we should end up with a polygon.
- Using some polygon math, we can ID if a point is inside of that polygon or not. If inside, we set ignore. If outside, we set as lethal. We'd need to make sure this works with concave polygons since there's no promise that this is convex.
Also maybe ways that can improve the method you have now and/or convolved the path by some radius left/right?
We probably want this to mark more than just 1 cell around the boundary as lethal so that small quantization errors don't allow the system to break out. At least 3 cells thick, but also I was thinking perhaps it would be sensible to just default mark everything as occupied unless within the bounds of tracking. But maybe a "thick" line is fine actually if that gives us some computational gains :-)
| this, std::placeholders::_1)); | ||
|
|
||
| path_sub_ = node->create_subscription<nav_msgs::msg::Path>( | ||
| "/plan", |
There was a problem hiding this comment.
Remove forward slash in both subscriptions
| tracking_feedback_sub_ = node->create_subscription<nav2_msgs::msg::TrackingFeedback>( | ||
| "/tracking_feedback", | ||
| std::bind(&TrackingErrorLayer::trackingCallback, this, std::placeholders::_1), | ||
| rclcpp::QoS(10).reliable() |
There was a problem hiding this comment.
For all QoS: Use the established Nav2 policies in nav2_ros_common
| std::bind(&TrackingErrorLayer::trackingCallback, this, std::placeholders::_1), | ||
| rclcpp::QoS(10).reliable() | ||
| ); | ||
| tf_buffer_ = std::make_shared<tf2_ros::Buffer>(node->get_clock()); |
There was a problem hiding this comment.
You should have this already from base classes to this class. You shouldn't need to manually create another (which is very heavy)
| std::mutex path_mutex_; | ||
| std::mutex tracking_error_mutex_; |
There was a problem hiding this comment.
I think you can get away with a single data mutex
| return result; | ||
| } | ||
|
|
||
| TrackingErrorLayer::~TrackingErrorLayer() |
There was a problem hiding this comment.
Move destructor right after the constructor
| void TrackingErrorLayer::reset() {} | ||
| void TrackingErrorLayer::activate() {enabled_ = true;} | ||
| void TrackingErrorLayer::deactivate() {enabled_ = false;} | ||
|
|
||
| void TrackingErrorLayer::onFootprintChanged() {} | ||
| void TrackingErrorLayer::cleanup() {} |
There was a problem hiding this comment.
Dont override if not implementing
| void TrackingErrorLayer::activate() {enabled_ = true;} | ||
| void TrackingErrorLayer::deactivate() {enabled_ = false;} |
There was a problem hiding this comment.
These two should create / destroy the subscriptions
| dyn_params_handler_.reset(); | ||
| } | ||
|
|
||
| void TrackingErrorLayer::reset() {} |
There was a problem hiding this comment.
This should reset data and other states. Please check out other layers for example
936f1e4 to
97ed4c8
Compare
|
@SteveMacenski I deleted bresenham line drawer to make it faster. I think I fixed your review points. I think adding constraints as seperate obstacles is a better choice, cause if a user tries to use this on a bigger local costmap it can become a huge computation. Also with rasterization, sharp turns could create problems. Current version creates big distinct blocks (like watchtowers) one after another. This can be parameterized and improved later. Not looking good but would do the job I think. |
|
Related to your other PR, I'm passing off the first review stage to @mini-1235 and I'll come in afterwards |
Can you show an image of this behavior for example? |
SteveMacenski
left a comment
There was a problem hiding this comment.
OK I can't help myself, here's a few notes. I didn't look at the main get path segments / wall functions, but I looked at the layer boilerplate
nav2_costmap_2d/costmap_plugins.xml
Outdated
| <class type="nav2_costmap_2d::VoxelLayer" base_class_type="nav2_costmap_2d::Layer"> | ||
| <description>Similar to obstacle costmap, but uses 3D voxel grid to store data.</description> | ||
| </class> | ||
| <class type="nav2_costmap_2d::TrackingErrorLayer" base_class_type="nav2_costmap_2d::Layer"> |
There was a problem hiding this comment.
Maybe rename to BoundedTrackingErrorLayer since its adding bounds
| namespace nav2_costmap_2d | ||
| { | ||
|
|
||
| TrackingErrorLayer::TrackingErrorLayer() {} |
There was a problem hiding this comment.
You can just set this to TrackingErrorLayer() = default; in the header file instead
| void TrackingErrorLayer::trackingCallback( | ||
| const nav2_msgs::msg::TrackingFeedback::SharedPtr msg) | ||
| { | ||
| std::lock_guard<std::mutex> lock(data_mutex_); | ||
| last_tracking_feedback_ = *msg; | ||
| } | ||
|
|
There was a problem hiding this comment.
It would be good to check on timestamps to make sure these are sufficiently current
|
|
||
| // Create subscriptions when layer is activated | ||
| path_sub_ = node->create_subscription<nav_msgs::msg::Path>( | ||
| "/plan", |
There was a problem hiding this comment.
Remove forward slashes here and in the tracking feedback to let this work with namespaces
There was a problem hiding this comment.
I have tried without "/" but didn't work. It wasn't subscribing to those topics. Am I doing anything else wrong about this?
There was a problem hiding this comment.
Ah. I know why. The costmap namespaces things on its topic. I think maybe instead you should make these parameters + add those parameters to the yaml file where there you set it.
Just make sure to use this function to parse the parameter https://github.com/ros-navigation/navigation2/blob/main/nav2_costmap_2d/include/nav2_costmap_2d/layer.hpp#L180. Grep the obstacle / static layer for use examples
| last_path_.poses.clear(); | ||
| last_path_.header = std_msgs::msg::Header(); |
There was a problem hiding this comment.
| last_path_.poses.clear(); | |
| last_path_.header = std_msgs::msg::Header(); | |
| last_path_ = nav_msgs::msg::Path(); |
@SteveMacenski currently looks like this. with bresenham function it was a smooth straight line. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TrackingBoundsLayer costmap plugin that creates a corridor around the robot's path with obstacles, helping to constrain navigation within path boundaries. The implementation adds a new costmap layer, configuration parameters, and comprehensive unit tests.
Key Changes
- Added TrackingBoundsLayer plugin that subscribes to path and tracking feedback topics to dynamically create corridor boundaries
- Implemented wall point generation algorithm that calculates perpendicular boundaries around path segments
- Added unit tests covering initialization, path segment extraction, wall point generation, and edge cases
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| nav2_util/include/nav2_util/geometry_utils.hpp | Adds utility include for geometry operations |
| nav2_costmap_2d/include/nav2_costmap_2d/tracking_bounds_layer.hpp | Defines the TrackingBoundsLayer class interface with lifecycle methods and member variables |
| nav2_costmap_2d/plugins/tracking_bounds_layer.cpp | Implements the tracking bounds layer logic including callbacks, path segmentation, and costmap updates |
| nav2_costmap_2d/test/unit/tracking_bounds_layer_test.cpp | Provides comprehensive unit tests for the new layer functionality |
| nav2_costmap_2d/test/unit/CMakeLists.txt | Adds test target for tracking_bounds_layer_test |
| nav2_costmap_2d/CMakeLists.txt | Integrates tracking_bounds_layer.cpp into the layers library |
| nav2_costmap_2d/costmap_plugins.xml | Registers the TrackingBoundsLayer as a plugin |
| nav2_bringup/params/nav2_params.yaml | Configures the tracking_bounds_layer in the local costmap with default parameters |
nav2_costmap_2d/test/unit/bounded_tracking_error_layer_test.cpp
Outdated
Show resolved
Hide resolved
nav2_costmap_2d/include/nav2_costmap_2d/tracking_bounds_layer.hpp
Outdated
Show resolved
Hide resolved
| void TrackingBoundsLayer::pathCallback(const nav_msgs::msg::Path::SharedPtr msg) | ||
| { | ||
| std::lock_guard<std::mutex> lock(data_mutex_); | ||
| auto now = node_.lock()->now(); | ||
| auto msg_time = rclcpp::Time(msg->header.stamp); | ||
| auto age = (now - msg_time).seconds(); | ||
|
|
||
| if (age > 1.0) { | ||
| RCLCPP_WARN_THROTTLE( | ||
| node_.lock()->get_logger(), | ||
| *node_.lock()->get_clock(), | ||
| 5000, | ||
| "Path is %.2f seconds old", age); | ||
| return; |
There was a problem hiding this comment.
Multiple calls to node_.lock() without checking if the lock is successful. If the node has been destroyed, this could lead to a crash. Store the locked node in a local variable and check it once at the beginning of the function.
Example fix:
void TrackingBoundsLayer::pathCallback(const nav_msgs::msg::Path::SharedPtr msg)
{
std::lock_guard<std::mutex> lock(data_mutex_);
auto node = node_.lock();
if (!node) {
return;
}
auto now = node->now();
// ... rest of the function using 'node' instead of node_.lock()
nav2_bringup/params/nav2_params.yaml
Outdated
| enabled: True | ||
| look_ahead: 5.0 | ||
| step: 5 | ||
| corridor_width: 2.0 |
There was a problem hiding this comment.
[nitpick] The tracking_bounds_layer configuration is missing the tracking_feedback_topic and path_topic parameters. While these have defaults in the code ("tracking_feedback" and "plan"), they should be explicitly documented in the configuration file for clarity, especially since this is a new plugin. Add:
tracking_bounds_layer:
plugin: "nav2_costmap_2d::TrackingBoundsLayer"
enabled: True
look_ahead: 5.0
step: 5
corridor_width: 2.0
tracking_feedback_topic: "tracking_feedback"
path_topic: "plan"| corridor_width: 2.0 | |
| corridor_width: 2.0 | |
| tracking_feedback_topic: "tracking_feedback" | |
| path_topic: "plan" |
This doesn't look very dense / complete (?). I see large gaps there.
Please update the name - I think the "Error" bit is an important clarification. You are correct on the TF thing - Maurice convinced me that my internal AI was hallucinating about it. That seems like something we should add to Nav2 utils. |
|
Can you provide some updated picture + compute times? I think the density is worth it, but maybe there are other methods we could consider and/or optimize if the performance isn't high enough |
|
Can we make the line at least 2 pixels thick? 1 pixel, as before, could cause issues if the search window is larger than 1 cell length (like smac, which is |
|
Should I parameterize it? |
|
I think that might be good. Default at 2 though I think would be good. |
|
This pull request is in conflict. Could you fix it @silanus23? |
|
@SteveMacenski I will solve conflict and open a doc. About computational weight I can bring actually numbers with |
|
@mini-1235 is taking over the first set of reviews on this feature - please direct to him for now :-) |
52b0dba to
308b361
Compare
Codecov Report❌ Patch coverage is
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Hi @silanus23, regarding the visualization, I don't think it is very clear at the moment. It would also be helpful to get some metrics to see whether we are on the right track, especially to ensure this works well on weaker CPUs |
|
Hi @mini-1235 I am aware that a PC's cpu is enough to compensate diffirences so you are right about asking for metrics. I just wanted to point out no overflows and in complexity wise current one was the best I could find. I will try to bring out metrics . In addition Currently lines are parameterized and double thickness of those and the thickness is parameterized. As a last note: I might be away for a short period soon, but I will jump back into this and all other pending PRs as soon as I return. :-) |
fe89b10 to
3fd5bca
Compare
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
|
That video look really nice! I didn't look through the code, but mostly my questions would be around performance to make sure its as light weight as it should be // the technique used. But for the general visible output, that's perfect: a wall of some non-trivial thickness to stop the robot from going around outside of bounds |
There was a problem hiding this comment.
@silanus23 Could you explain the screenshot or the perf data? I am having a bit of trouble interpreting it on my end. Maybe you can also measure the CPU usage before and after enabling this layer + how long the heavy functions take to execute.
As a side note, I will be away starting next week to get some rest, so I might be a bit slower to respond here. I will still try to find some time to bring this to a complete state in my free time :)
| this, std::placeholders::_1)); | ||
| } | ||
|
|
||
| void BoundedTrackingErrorLayer::goalCallback(const geometry_msgs::msg::PoseStamped::SharedPtr msg) |
There was a problem hiding this comment.
I think you can use the last pose in the pathCallback to determine if the goal has changed
| if (!enabled_.load()) { | ||
| return; | ||
| } | ||
| *min_x = std::min(*min_x, robot_x - look_ahead_); |
There was a problem hiding this comment.
Does this really work for global_costmap?
There was a problem hiding this comment.
Sorry I couldn't understand you in here. My aim is to creating a corridor for only a limited path segment ahead of the robot so in case of drawing circles it won't interfere with path installation or other things. Am I wrong about this? Is code shouldn't be providing this? Can you help me?
| for (size_t i = 0; i < segment.poses.size(); ++i) { | ||
| const auto & pose = segment.poses[i]; | ||
| geometry_msgs::msg::PoseStamped transformed_pose; | ||
| double timeout = (i == 0) ? 0.1 : 0.0; |
There was a problem hiding this comment.
You can get the transform_tolerance parameter like we did in other layers
|
@mini-1235
above 2 are about costmap layer's specific usage Here is my complete usage with layer (I know nav2 shouldn't be taking this much in a laptop cpu)
This one is complete cpu usage without layer These ones are outputs when I put loggers My summary: I am working on your reviews locally but I think we should continue to take goal change from other channels cause when change hit path it's a bit late I think. I am trying to delete layer's effect ASAP so path can react quicker and better. Previously when I tested I sensed a bit stuttering when taken from path. As a last not: |
| this, std::placeholders::_1)); | ||
| } | ||
|
|
||
| void BoundedTrackingErrorLayer::goalCallback(const geometry_msgs::msg::PoseStamped::SharedPtr msg) |
| if (!last_path_.poses.empty() && msg->poses.size() >= 2) { | ||
| size_t old_size = last_path_.poses.size(); | ||
| size_t new_size = msg->poses.size(); | ||
|
|
||
| if (old_size != new_size) { | ||
| path_discontinuity = true; | ||
| RCLCPP_DEBUG( | ||
| node->get_logger(), | ||
| "Path size changed from %zu to %zu, clearing corridor", old_size, new_size); | ||
| } else if (old_size >= 2) { | ||
| double threshold = corridor_width_ * 0.5; | ||
|
|
||
| // Compare midpoint and endpoint positions between old and new paths | ||
| // to detect significant path changes even when size remains the same | ||
| size_t old_mid_idx = old_size / 2; | ||
| size_t new_mid_idx = new_size / 2; | ||
| double mid_dx = msg->poses[new_mid_idx].pose.position.x - | ||
| last_path_.poses[old_mid_idx].pose.position.x; | ||
| double mid_dy = msg->poses[new_mid_idx].pose.position.y - | ||
| last_path_.poses[old_mid_idx].pose.position.y; | ||
| double mid_dist = std::hypot(mid_dx, mid_dy); | ||
|
|
||
| double end_dx = msg->poses.back().pose.position.x - | ||
| last_path_.poses.back().pose.position.x; | ||
| double end_dy = msg->poses.back().pose.position.y - | ||
| last_path_.poses.back().pose.position.y; | ||
| double end_dist = std::hypot(end_dx, end_dy); | ||
|
|
||
| if (mid_dist > threshold || end_dist > threshold) { | ||
| path_discontinuity = true; |
There was a problem hiding this comment.
Is all of this really necessary? This seems overkill. Why not just change when the path is different using the geomery_msgs isPathUpdated method? Or just check if the goal has changed?
What's the benefit this draws so I understand - I'm sure there's something if you went through this effort
| { | ||
| std::lock_guard<std::mutex> lock(data_mutex_); | ||
| last_path_ = nav_msgs::msg::Path(); | ||
| last_tracking_feedback_ = nav2_msgs::msg::TrackingFeedback(); | ||
| last_goal_ = geometry_msgs::msg::PoseStamped(); | ||
| } |
There was a problem hiding this comment.
I'd just turn this into a function resetState that deactivate / reset uses. Right now reset() actually doesn't clear out all the stuff.
| return; | ||
| } | ||
|
|
||
| std::lock_guard<std::mutex> lock(data_mutex_); |
There was a problem hiding this comment.
We could simply make these few values atomics so we don't have to mutex lock internally for callback states.
|
@mini-1235 how does this compare to your work in terms of the wall generating method? |
|
@SteveMacenski gonna switch to isPathUpdated. In addition what do you think about the cpu usage? Is it bearable or should I try to lower it? |
|
I assume those gdb times are pre-optimized, yeah? Can you add timers in the costmap update in costmap ros layer updates and give the timer update metrics (and hell - the others too just for comparison). I'd just print all the times for update/costs and have the log include the layer's name. Include the keepout zone for that comparison. The TB4 demo would include that all. |
…ctivate Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
Sorry. Lint probably slipped last second I will fix it at |
…ling Signed-off-by: silanus23 <berkantali23@outlook.com>
It is unexpected to me that this is heavier than the obstacle layer, which should be processing alot more data (~1000 beams at 20hz). Anywhere we can improve the performance and/or rethink the underlying algorithm to be more light weight (or is my expectation unrealistic)? Where's the compute time in this layer specifically? I'm thinking maybe its using I just looked at the implementation in detail: |
Locally I did some changes I didn't push cause I need to be sure about these improvmeents. They can regress a bit but I expect them to stay relatively intact I need to put unit tests do styling etc. I droppped setConvexPolygon and making TF ops batch by batch and I have added various optimizations on top of these. I hope to be able to finish this today or tomorrow. |
…r draw line Filliing inside walls with specific funcs newly added Getting path segment with a resolutioned search Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
|
What were the improvements made from those changes? Are we now at or below the other layers (the logs are quite long and hard to read -- some analyzed values might be nice and easier for us to review 😉 ) |
|
I have written a python script to help us analyze the results I think current one is close to The only problem is if the Of course as a last I have implemented that TF loop thing you suggested above. It created diff too. I couldn't catch it in the first time :) As a last thing should I increase the log size and again put here. 2 maybe not that enough. |
|
Oh nice, that's definitely performant enough I think. I think fixing that crochet pattern would be the only last thing to resolve so that we have that wall, but not the sparse effects that don't seem related to the 3-deep wall thickness. Are all the right cells being touched though? Looking at the image you showed at the diagonal, it looks like there's some cells that would be equidistant from the others that aren't included. It would expect it to look a little more 'straight' |
…ccording to new approach Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
Signed-off-by: silanus23 <berkantali23@outlook.com>
|
Yes this looks good to me. I would ask though an eample of a non-striaght path to see & making sure that the thickness is added to the outside of the bound limitations set (not in the middle) Make sure to add test coverage to the functional methods like getting the wall / segment / filling / etc. Stong unit tests there with straight, curved, and random paths would be good to showcase it works correctly Generally avoid |
|
|
||
| void BoundedTrackingErrorLayer::reset() | ||
| { | ||
| std::lock_guard<std::mutex> lock(data_mutex_); |
There was a problem hiding this comment.
Why not add the lock to the inside of resetState so that you don't need to lock here manually / in the deactivate / other places
| */ | ||
| class BoundedTrackingErrorLayer : public nav2_costmap_2d::Layer | ||
| { | ||
| // Friend declaration for test access |
There was a problem hiding this comment.
I think this should be removed
| * @brief Callback for goal pose updates. | ||
| * @param msg Incoming goal pose message. | ||
| */ | ||
| void goalCallback(const geometry_msgs::msg::PoseStamped::SharedPtr msg); |
There was a problem hiding this comment.
ConstSharedPtr, also for other subscriptions
| return; | ||
| } | ||
|
|
||
| if (on_set_params_handler_) { |
There was a problem hiding this comment.
This should only be in deactivate()
| this, std::placeholders::_1)); | ||
| } | ||
|
|
||
| void BoundedTrackingErrorLayer::goalCallback(const geometry_msgs::msg::PoseStamped::SharedPtr msg) |
| return; | ||
| } | ||
|
|
||
| current_path_index_.store(msg->current_path_index); |
There was a problem hiding this comment.
If you only need the current path index, can't that be calculated in this layer instead of subscribing it?
| int /*min_i*/, int /*min_j*/, int /*max_i*/, int /*max_j*/) | ||
| { | ||
| nav2_util::ExecutionTimer timer; | ||
| timer.start(); |
There was a problem hiding this comment.
I understand this can be useful for testing performance, but please remove it after you tested
| if (param_type == rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE) { | ||
| if (param_name == name_ + "." + "look_ahead") { | ||
| const double new_value = parameter.as_double(); | ||
| // Values above 3.0m cause corridor rendering artifacts |
There was a problem hiding this comment.
It feels strange to me that it can only be 0-3m, can you explain why
| } | ||
|
|
||
| // Update cached resolution | ||
| resolution_ = layered_costmap_->getCostmap()->getResolution(); |
There was a problem hiding this comment.
You should not do this in every updateCosts call, maybe in matchSize?
| return; | ||
| } | ||
|
|
||
| const std::string costmap_frame = layered_costmap_->getGlobalFrameID(); |
There was a problem hiding this comment.
Same for this, this should not be called in every updatecosts loop
| return; | ||
| } | ||
|
|
||
| const double res = layered_costmap_->getCostmap()->getResolution(); |













Basic Info
Description of contribution in a few bullet points
I have added a layer that can draw a corridor around path with obstacles. In addition I added a geometry util that can draw smoother lines.
Description of documentation updates required from your changes
A costmap layer and it's params
Description of how this change was tested
Wrote a unit test and I have seen visually it is working
Future work that may be required in bullet points
Tests definitely need a see throgh.
Note:
I had to copy paste and carry changes to a new branch. Like I did on the prev 2 PR. This going to make me move with less burden. Commits look lesser and rushy but they ain't.
For Maintainers:
backport-*.