Fix bounds calculation for rolling costmaps in StaticLayer#6064
Conversation
Signed-off-by: Tony Najjar <tony.najjar@dexory.com>
|
@SteveMacenski this fixes the issue but please check my logic in detail here, I'm not 100% sure of the diagnosis (as is usually the case with |
|
@tonynajjar is this a modern PR to address what #5194 did (and the author just never followed up)? You mentioned not sure about the approach, if this is the case, here's another example method (which may or may not be better) |
There was a problem hiding this comment.
This does seem at face value to work well - I'd just check the edge details on what if or is it possible for the min/maxes to be out of bounds or miss a bit. I think (?) its correct, but double checking the types and that everything are exact values and not being cast to/from unsigned int / ints that could cause truncation or approximation by the cell sizes.
|
Is this related / to be addressed as well? #4911 |
Definitely looks like it yes
I don't know for sure but I don't think so |
|
LMK when you're ready to merge |
* Fix memory leak in AmclNode by deleting Laser objects before clearing vector (#5902) * fix: add deletion of each element of lasers before lasers_.clear to prevent memory leak Signed-off-by: Xinyu Yu <rosaxinyu@gmail.com> * feat: change type of lasers_ to unique_ptr to prevent memory leak when clearing the vector Signed-off-by: Xinyu Yu <rosaxinyu@gmail.com> --------- Signed-off-by: Xinyu Yu <rosaxinyu@gmail.com> * fix: ensure proper reinflation handling to avoid double full-map updates (#5915) Signed-off-by: Tony Najjar <tony.najjar@dexory.com> * Use a child logger in ros_topic_logger for better log categorization + nicer logging (#5949) * Update logger initialization to use a child logger for better log categorization Signed-off-by: Tony Najjar <tony.najjar@dexory.com> * better logging Signed-off-by: Tony Najjar <tony.najjar@dexory.com> --------- Signed-off-by: Tony Najjar <tony.najjar@dexory.com> * Merge commit from fork Signed-off-by: Rosayxy <rosaxinyu@gmail.com> * Invalidate current_ on dynamic parameter changes (#5927) * fix(inflation_layer): set current_ = false on dynamic param changes Fixes #5872 When inflation_radius, cost_scaling_factor, inflate_unknown, or inflate_around_unknown parameters are changed at runtime, the layer now marks itself as not current (current_ = false). This ensures the planner/controller servers wait for the costmap to recompute before using it. Previously, these dynamic parameter changes would update internal state but not trigger the currentness flag, causing the planner/controller to use stale costmap data. Signed-off-by: MFarooq07 <mfarooqjuly16@gmail.com> * fix(costmap_layers): set current_ = false on dynamic param changes Fixes #5872 When costmap layer parameters that affect output are changed at runtime, the layers now set current_ = false to signal that the costmap needs recomputation before the planner/controller can use it. Parameters that invalidate per layer: - inflation_layer: inflation_radius, cost_scaling_factor, enabled, inflate_unknown, inflate_around_unknown - obstacle_layer: min/max_obstacle_height, enabled - static_layer: enabled (triggers full update with bounds + has_updated_data) - voxel_layer: min/max_obstacle_height, origin_z, z_resolution, z_voxels, unknown_threshold, mark_threshold, enabled - plugin_container_layer: enabled Parameters that do NOT invalidate (apply from next cycle onward): - footprint_clearing_enabled, restore_cleared_footprint - combination_method Signed-off-by: MFarooq07 <mfarooqjuly16@gmail.com> * fix(costmap_layers): extend current_ = false to all affected layers Extends the fix from #5872 to obstacle, static, voxel, and plugin_container layers. Parameters that invalidate per layer: - obstacle_layer: min/max_obstacle_height, enabled - static_layer: enabled (triggers full update with bounds + has_updated_data) - voxel_layer: min/max_obstacle_height, origin_z, z_resolution, z_voxels, unknown_threshold, mark_threshold, enabled - plugin_container_layer: enabled Parameters that do NOT invalidate (apply from next cycle onward): - footprint_clearing_enabled, restore_cleared_footprint - combination_method Signed-off-by: MFarooq07 <mfarooqjuly16@gmail.com> * style(nav2_costmap_2d): apply uncrustify formatting * unindent to fix the formatting RCLCPP_WARN( RCLCPP_WARN( logger_, "%s is not a dynamic parameter " logger_, "%s is not a dynamic parameter " "cannot be changed while running. Rejecting parameter update.", param_name.c_str()); Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * function name indented Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * reverted variable declaration Removed the unused variable 'needs_full_update' from the parameter update function. Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * Remove unnecessary lines in static_layer.cpp empty lines and extra line updating the needs_full_update for enabled Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * Reset dimensions and state on parameter update removed the needs full update if condition block Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * Fix formatting in static_layer.cpp extra space on line 538 deleted Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> * Fix formatting and readability in obstacle_layer.cpp Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> * Fix formatting issues in voxel_layer.cpp Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> * Fix formatting issues in obstacle_layer.cpp Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> * Fix formatting and alignment in voxel_layer.cpp Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> --------- Signed-off-by: MFarooq07 <mfarooqjuly16@gmail.com> Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com> * fix(nav2_collision_monitor): use velocity magnitude for holonomic checks (#5954) * fix(nav2_collision_monitor): use velocity magnitude for holonomic checks Previously, VelocityPolygon only checked cmd_vel.x, ignoring lateral motion. This fix calculates the velocity vector magnitude when holonomic is set to true. Signed-off-by: Jaerak Son <sjr9017@naver.com> * style: fix lint whitespace errors and typos Signed-off-by: Jaerak Son <sjr9017@naver.com> * Revert nav2_util usage to match main branch requirements Signed-off-by: Jaerak Son <sjr9017@naver.com> * Revert nav2_util usage in velocity_polygons_test to match main branch requirements Signed-off-by: Jaerak Son <sjr9017@naver.com> * Add missing copyright header to nav2_collision_monitor/test/velocity_polygons_test.cpp Signed-off-by: Jaerak Son <sjr9017@naver.com> --------- Signed-off-by: Jaerak Son <sjr9017@naver.com> * Revert "Invalidate current_ on dynamic parameter changes (#5927)" This reverts commit 261c04b. * Fix CostmapSubscriber isCostmapReceived data race (#6009) (#6026) * Add mutex guard to isCostmaFix CostmapSubscriber isCostmapReceived data race (#6009)pReceived method Added atomic include and mutex guard for costmap check. Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> * Format isCostmapReceived function for clarity Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> * fixup! Fix CostmapSubscriber isCostmapReceived data race (#6009) Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> * Fix isCostmapReceived function formatting Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> * Fix CostmapSubscriber formatting for lint Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> --------- Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> * Enhance bounds calculation for rolling costmaps in StaticLayer (#6064) Signed-off-by: Tony Najjar <tony.najjar@dexory.com> * fix: add world-distance pre-filter to ObstacleLayer (#6092) * fix: add world-distance pre-filter to ObstacleLayer Fixes #6084 Signed-off-by: cheny <1426183440@qq.com> * test: adjust obstacle_layer test for world-distance filter Signed-off-by: cheny <1426183440@qq.com> --------- Signed-off-by: cheny <1426183440@qq.com> * check if a file was loaded before clearing the graph (#6103) Signed-off-by: EricoMeger <ericomeger9@gmail.com> * Bumping to 1.3.12 for release Signed-off-by: SteveMacenski <stevenmacenski@gmail.com> --------- Signed-off-by: Xinyu Yu <rosaxinyu@gmail.com> Signed-off-by: Tony Najjar <tony.najjar@dexory.com> Signed-off-by: Rosayxy <rosaxinyu@gmail.com> Signed-off-by: MFarooq07 <mfarooqjuly16@gmail.com> Signed-off-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> Signed-off-by: Tony Najjar <tony.najjar.1997@gmail.com> Signed-off-by: Jaerak Son <sjr9017@naver.com> Signed-off-by: Sania Shah <s77shah@uwaterloo.ca> Signed-off-by: cheny <1426183440@qq.com> Signed-off-by: EricoMeger <ericomeger9@gmail.com> Signed-off-by: SteveMacenski <stevenmacenski@gmail.com> Co-authored-by: Xinyu Yu <xy-yu22@mails.tsinghua.edu.cn> Co-authored-by: Tony Najjar <tony.najjar.1997@gmail.com> Co-authored-by: Xinyu Yu <rosaxinyu@gmail.com> Co-authored-by: Muhammad Farooq Memon <93149539+MFarooq07@users.noreply.github.com> Co-authored-by: Jaerak Son <40343097+mach0312@users.noreply.github.com> Co-authored-by: Sania Shah <s77shah@uwaterloo.ca> Co-authored-by: Ian <40204259+Cheny5863@users.noreply.github.com> Co-authored-by: Érico Meger <86668447+EricoMeger@users.noreply.github.com>
Basic Info
I have the issue that my rolling local costmap doesn't seem to get fully populated with data:
My setup (roughly):
According to my investigation, here is what is happening:
The
StaticLayer::updateBounds()method usesmapToWorld()to compute the region that needs updating. InCostmap2D, mapToWorld()converts grid cell indices to metric coordinates using the costmap instance's own origin:wx = origin_x + mx * resolution. This function has no awareness of TF frames — the output is implicitly in whatever frame the costmap's origin was defined in.For the static layer's internal costmap, the origin comes from the map YAML file, so
mapToWorld()produces coordinates in the map frame. However, the local costmap is a rolling window whose global_frame is odom. TheLayeredCostmapinterprets the bounds reported byupdateBounds()as being in its global_frame (odom), then converts them to grid cell ranges(min_i, min_j, max_i, max_j)that are passed toupdateCosts().When map → odom is non-identity (which happens as soon as an initial pose is set at a non-origin location), the map-frame bounds returned by the static layer are misinterpreted as odom-frame coordinates. This causes the update region to be offset, resulting in only a portion of the rolling window being populated with static map data — or none at all if the offset is large enough.
The
updateCosts()method already handles the frame difference correctly by doing a per-cell TF lookup from odom to map, but it only iterates over the cells within the (incorrect) bounds, so the correct transform logic is applied to the wrong region.Description of contribution in a few bullet points
Fix: When rolling, compute bounds relative to the robot pose (which is already in the odom frame) spanning the full rolling window, instead of using
mapToWorld():Description of documentation updates required from your changes
Description of how this change was tested
Future work that may be required in bullet points
For Maintainers:
backport-*.