Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
58 changes: 41 additions & 17 deletions nav2_costmap_2d/plugins/obstacle_layer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -527,8 +527,8 @@ ObstacleLayer::updateBounds(

const sensor_msgs::msg::PointCloud2 & cloud = obs.cloud_;

double sq_obstacle_max_range = obs.obstacle_max_range_ * obs.obstacle_max_range_;
double sq_obstacle_min_range = obs.obstacle_min_range_ * obs.obstacle_min_range_;
const unsigned int max_range_cells = cellDistance(obs.obstacle_max_range_);
const unsigned int min_range_cells = cellDistance(obs.obstacle_min_range_);

sensor_msgs::PointCloud2ConstIterator<float> iter_x(cloud, "x");
sensor_msgs::PointCloud2ConstIterator<float> iter_y(cloud, "y");
Expand All @@ -549,28 +549,36 @@ ObstacleLayer::updateBounds(
continue;
}

// compute the squared distance from the hitpoint to the pointcloud's origin
double sq_dist =
(px -
obs.origin_.x) * (px - obs.origin_.x) + (py - obs.origin_.y) * (py - obs.origin_.y) +
(pz - obs.origin_.z) * (pz - obs.origin_.z);
// now we need to compute the map coordinates for the observation
unsigned int mx, my;
if (!worldToMap(px, py, mx, my)) {
RCLCPP_DEBUG(logger_, "Computing map coords failed");
continue;
}

// if the point is far enough away... we won't consider it
if (sq_dist >= sq_obstacle_max_range) {
RCLCPP_DEBUG(logger_, "The point is too far away");
// compute the distance from the hitpoint to the pointcloud's origin
// Calculate the distance in cell space to match the ray trace algorithm
// used for clearing obstacles (see Costmap2D::raytraceLine).
unsigned int x0, y0;
if (!worldToMap(obs.origin_.x, obs.origin_.y, x0, y0)) {
RCLCPP_DEBUG(logger_, "Sensor origin is out of map bounds");
continue;
}

// if the point is too close, do not conisder it
if (sq_dist < sq_obstacle_min_range) {
RCLCPP_DEBUG(logger_, "The point is too close");
const int dx = static_cast<int>(mx) - static_cast<int>(x0);
const int dy = static_cast<int>(my) - static_cast<int>(y0);
const unsigned int dist = static_cast<unsigned int>(
std::hypot(static_cast<double>(dx), static_cast<double>(dy)));

// if the point is far enough away... we won't consider it
if (dist > max_range_cells) {
RCLCPP_DEBUG(logger_, "The point is too far away");
continue;
}

// now we need to compute the map coordinates for the observation
unsigned int mx, my;
if (!worldToMap(px, py, mx, my)) {
RCLCPP_DEBUG(logger_, "Computing map coords failed");
// if the point is too close, do not consider it
if (dist < min_range_cells) {
RCLCPP_DEBUG(logger_, "The point is too close");
continue;
}

Expand Down Expand Up @@ -779,6 +787,22 @@ ObstacleLayer::raytraceFreespace(

unsigned int cell_raytrace_max_range = cellDistance(clearing_observation.raytrace_max_range_);
unsigned int cell_raytrace_min_range = cellDistance(clearing_observation.raytrace_min_range_);

const int dx = static_cast<int>(x1) - static_cast<int>(x0);
const int dy = static_cast<int>(y1) - static_cast<int>(y0);

unsigned int observation_dist = static_cast<unsigned int>(
std::hypot(static_cast<double>(dx), static_cast<double>(dy)));

if (observation_dist < 1) {
// If the observation is in the same cell as the origin, do not raytrace
continue;
}
// Reduce the observation distance by 1 cell so that the cell containing the
// observation is not cleared
cell_raytrace_max_range =
std::min(cell_raytrace_max_range, observation_dist - 1);
Copy link
Member

@SteveMacenski SteveMacenski Dec 4, 2025

Choose a reason for hiding this comment

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

Please remove - I'm not sure this is important considering the marking observation is triggered after the clearing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to check - I had added this to handle the case where raytrace_max_range > obstacle_max_range. In that case the cell might not be subsequently marked. Setting raytrace_max_range > obstacle_max_range helps avoid ghost obstacles lingering in the map when an obstacle is moving away from the sensor.
If that's not the behavior you'd expect with this configuration I'll remove.

Copy link
Member

Choose a reason for hiding this comment

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

That does beg a question, I agree. If the marking reading doesn't meet obstacle_max_range, then its not going to be marked no matter what. I don't think it makes sense to cull clearing when marking was set not to cover this spectrum.

I agree that I would be concerned about if they were equal and some small casting error resulted in marking being OOB but clearing was able to happen, but I think that as something in this PR we can make sure we compute the same as to make sure this doesn't happen.

We do marking after clearing, so I think there's not a validly possible case that this should cover:

  1. Mark after clear, so if marking is possible, it should mark even if cleared previously
  2. If the ranges are the same, we should ensure in this PR that there's no floating point / casting error that would result in a clearing operation that marking doesn't also cover, as a point of technical correctness
  3. If the obstacle range is smaller, then we know that we're going to clearing marked things 1-N cells further, so that seems like an intentional choice of settings

But, please, push back if I'm missing a key detail that you're looking out for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it comes down to interpretation of what should happen when raytracing and what a "clearing observation" means. My expectation is to clear up to but excluding the cell containing the observed point, since we have no evidence that the cell is empty. So for point 3, if we have an observation beyond obstacle_max_range I wouldn't expect it to be cleared, even though we have decided not to mark it. Happy to remove if you disagree or if you think it could cause problems.

Copy link
Member

@SteveMacenski SteveMacenski Dec 8, 2025

Choose a reason for hiding this comment

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

I think we should remove in this context. I generally agree with you, but this is 10+ year old code and I'd rather not disrupt things too much in such a sensitive location in the codebase. Unless there's a bug, I'd like to avoid potentially creating one for some subset of users that use the clearing / marking in unique ways.

One way I think this could fail is when the raymarching is aliased due to the ray not being aligned with the cell axes. If we back out one full cell, that could actually be multiple cells that are hit. I think our raycasting uses full 1 cell increments so it wouldn't change, but if we did it in say 0.5 cells (or later changed it to another GPU accelerated library with finer checks), then we'd have an obstacle that should have been cleared because we didn't raycast far enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood - I have removed this. I have also removed skipping raytracing the origin cell when observation_dist == 0 as it's just handling a special case of this change. See #5697 (comment)


MarkCell marker(costmap_, FREE_SPACE);
// and finally... we can execute our trace to clear obstacles along that line
nav2_util::raytraceLine(
Expand Down
6 changes: 6 additions & 0 deletions nav2_costmap_2d/test/unit/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -71,3 +71,9 @@ nav2_add_gtest(observation_buffer_test observation_buffer_test.cpp)
target_link_libraries(observation_buffer_test
nav2_costmap_2d_core
)

nav2_add_gtest(obstacle_layer_test obstacle_layer_test.cpp)
target_link_libraries(obstacle_layer_test
nav2_costmap_2d_core
layers
)
Loading
Loading