Skip to content
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

planner_3d: fix cost calculation bug on in-place turning #725

Merged
merged 4 commits into from
Oct 31, 2023

Conversation

nhatao
Copy link
Collaborator

@nhatao nhatao commented Oct 27, 2023

This PR fixes a bug in GridAstarModel3D::cost().

When the movement between cur and next is an in-place turning, cm_[cur] is not evaluated. I think it is a simple mistake.
In contrast, when the movement between cur and next is not an in-place turning, MotionCache is used to interpolate the movement of the robot. Because MotionCache::Page::getMotion() does not return the last pose (I guess this is needed to avoid double counting of costs), the cm_[next] is not evaluated. Therefore, when only the cm_[next] is 100 and the costs of other poses are not 100, GridAstarModel3D::cost() returns a positive value mistakenly.

Because of these bugs, the planner_3d could create a path in which the robot collides with an obstacle in the following situation.
Assume that the cost of the grid (10, 5, 0) is 100, and the costs of other grids are not 100. For example, the path (5, 5, 0) -> (10, 5, 0) -> (10, 5, 3) can be generated.
The interpolated poses between (5, 5, 0) to (10, 5, 0) generated by MotionCache do not include (10, 5, 0) as it is the last pose, so the cost of the grid (10, 5, 0) is not evaluated.
The interpolated poses between (10, 5, 0) to (10, 5, 3) do not include the first pose (10, 5, 0) because of the bug in the cost calculation of in-place turning motion.
As a result, the cost of (10, 5, 0) is never evaluated and planner_3d can generate an untraversable path.

@at-wat

This comment has been minimized.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2023

Codecov Report

Merging #725 (2604942) into master (26b99ff) will increase coverage by 0.20%.
The diff coverage is 100.00%.

❗ Current head 2604942 differs from pull request most recent head 9b1f07c. Consider uploading reports for the commit 9b1f07c to get more accurate results

@@            Coverage Diff             @@
##           master     #725      +/-   ##
==========================================
+ Coverage   88.67%   88.87%   +0.20%     
==========================================
  Files          62       62              
  Lines        4582     4584       +2     
==========================================
+ Hits         4063     4074      +11     
+ Misses        519      510       -9     
Files Coverage Δ
planner_cspace/src/grid_astar_model_3dof.cpp 96.15% <100.00%> (+0.04%) ⬆️

... and 1 file with indirect coverage changes

@nhatao nhatao requested a review from at-wat October 27, 2023 09:20
Copy link
Owner

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

Could you add a test case if it's not too complicated?

@at-wat at-wat changed the title planner_3d: fix a bug in the cost calculation planner_3d: fix cost calculation bug on in-place turning Oct 30, 2023
@at-wat

This comment has been minimized.

@nhatao nhatao requested a review from at-wat October 30, 2023 09:19
@nhatao
Copy link
Collaborator Author

nhatao commented Oct 30, 2023

I added small tests in test_planner_3d_cost.cpp.

@at-wat

This comment has been minimized.

@at-wat
Copy link
Owner

at-wat commented Oct 30, 2023

[528] PASSED on noetic

All tests passed
build/test_results/costmap_cspace/gtest-test_costmap_3d.xml: 24 tests
build/test_results/costmap_cspace/gtest-test_pointcloud_accumulator.xml: 4 tests
build/test_results/costmap_cspace/roslint-costmap_cspace.xml: 1 tests
build/test_results/joystick_interrupt/roslint-joystick_interrupt.xml: 1 tests
build/test_results/joystick_interrupt/rostest-test_test_joystick_interrupt_rostest.xml: 1 tests
build/test_results/joystick_interrupt/rosunit-test_joystick_interrupt.xml: 12 tests
build/test_results/map_organizer/roslint-map_organizer.xml: 1 tests
build/test_results/map_organizer/rostest-test_test_map_organizer_rostest.xml: 1 tests
build/test_results/map_organizer/rostest-test_test_pointcloud_to_maps_rostest.xml: 1 tests
build/test_results/map_organizer/rosunit-test_map_organizer.xml: 8 tests
build/test_results/map_organizer/rosunit-test_pointcloud_to_maps.xml: 2 tests
build/test_results/neonavigation_common/roslint-neonavigation_common.xml: 1 tests
build/test_results/neonavigation_common/rostest-test_test_compat_rostest.xml: 1 tests
build/test_results/neonavigation_common/rosunit-test_compat.xml: 6 tests
build/test_results/neonavigation_metrics_msgs/gtest-test_helper.xml: 4 tests
build/test_results/neonavigation_metrics_msgs/roslint-neonavigation_metrics_msgs.xml: 1 tests
build/test_results/obj_to_pointcloud/roslint-obj_to_pointcloud.xml: 1 tests
build/test_results/obj_to_pointcloud/rostest-test_test_obj_to_pointcloud_rostest.xml: 1 tests
build/test_results/obj_to_pointcloud/rosunit-test_obj_to_pointcloud.xml: 2 tests
build/test_results/planner_cspace/gtest-test_blockmem_gridmap.xml: 10 tests
build/test_results/planner_cspace/gtest-test_costmap_bbf.xml: 4 tests
build/test_results/planner_cspace/gtest-test_cyclic_vec.xml: 14 tests
build/test_results/planner_cspace/gtest-test_distance_map.xml: 18 tests
build/test_results/planner_cspace/gtest-test_distance_map_fast_update.xml: 4 tests
build/test_results/planner_cspace/gtest-test_grid_astar.xml: 12 tests
build/test_results/planner_cspace/gtest-test_grid_metric_converter.xml: 4 tests
build/test_results/planner_cspace/gtest-test_motion_cache.xml: 2 tests
build/test_results/planner_cspace/gtest-test_motion_primitive_builder.xml: 2 tests
build/test_results/planner_cspace/gtest-test_path_interpolator.xml: 4 tests
build/test_results/planner_cspace/gtest-test_planner_3d_cost.xml: 2 tests
build/test_results/planner_cspace/gtest-test_start_pose_predictor.xml: 6 tests
build/test_results/planner_cspace/roslint-planner_cspace.xml: 1 tests
build/test_results/planner_cspace/rostest-navigation_rostest__antialias_start_true.xml: 1 tests
build/test_results/planner_cspace/rostest-navigation_rostest__antialias_start_true__fast_map_update_true.xml: 1 tests
build/test_results/planner_cspace/rostest-navigation_rostest__with_tolerance_true.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_abort_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_costmap_watchdog_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_debug_outputs_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_dynamic_parameter_change_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_navigation_boundary_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_navigation_compat_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_navigation_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_planner_2dof_serial_joints_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_planner_3d_map_size_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_preempt_rostest.xml: 1 tests
build/test_results/planner_cspace/rostest-test_test_tolerant_action_rostest.xml: 1 tests
build/test_results/planner_cspace/rosunit-test_abort.xml: 2 tests
build/test_results/planner_cspace/rosunit-test_costmap_watchdog.xml: 4 tests
build/test_results/planner_cspace/rosunit-test_debug_outputs.xml: 8 tests
build/test_results/planner_cspace/rosunit-test_dynamic_parameter_change.xml: 4 tests
build/test_results/planner_cspace/rosunit-test_navigate.xml: 12 tests
build/test_results/planner_cspace/rosunit-test_navigate_boundary.xml: 2 tests
build/test_results/planner_cspace/rosunit-test_planner_2dof_serial_joints.xml: 4 tests
build/test_results/planner_cspace/rosunit-test_planner_3d_map_size.xml: 12 tests
build/test_results/planner_cspace/rosunit-test_preempt.xml: 2 tests
build/test_results/planner_cspace/rosunit-test_tolerant_action.xml: 2 tests
build/test_results/safety_limiter/roslint-safety_limiter.xml: 1 tests
build/test_results/safety_limiter/rostest-test_test_safety_limiter2_rostest.xml: 1 tests
build/test_results/safety_limiter/rostest-test_test_safety_limiter_compat_rostest.xml: 1 tests
build/test_results/safety_limiter/rostest-test_test_safety_limiter_rostest.xml: 1 tests
build/test_results/safety_limiter/rosunit-test_safety_limiter.xml: 22 tests
build/test_results/safety_limiter/rosunit-test_safety_limiter2.xml: 2 tests
build/test_results/track_odometry/gtest-test_tf_projection.xml: 2 tests
build/test_results/track_odometry/roslint-track_odometry.xml: 1 tests
build/test_results/track_odometry/rostest-test_test_tf_projection_rostest.xml: 1 tests
build/test_results/track_odometry/rostest-test_test_track_odometry_rostest.xml: 1 tests
build/test_results/track_odometry/rosunit-test_tf_projection_node.xml: 8 tests
build/test_results/track_odometry/rosunit-test_track_odometry.xml: 10 tests
build/test_results/trajectory_tracker/gtest-test_trajectory_tracker_filter.xml: 6 tests
build/test_results/trajectory_tracker/gtest-test_trajectory_tracker_path2d.xml: 18 tests
build/test_results/trajectory_tracker/roslint-trajectory_tracker.xml: 1 tests
build/test_results/trajectory_tracker/rostest-test_test_trajectory_recorder_rostest.xml: 1 tests
build/test_results/trajectory_tracker/rostest-test_test_trajectory_tracker_overshoot_rostest.xml: 1 tests
build/test_results/trajectory_tracker/rostest-test_test_trajectory_tracker_rostest.xml: 1 tests
build/test_results/trajectory_tracker/rostest-test_test_trajectory_tracker_with_odom_rostest.xml: 1 tests
build/test_results/trajectory_tracker/rostest-trajectory_tracker_rostest__odom_delay_0.xml: 1 tests
build/test_results/trajectory_tracker/rosunit-test_trajectory_recorder.xml: 2 tests
build/test_results/trajectory_tracker/rosunit-test_trajectory_tracker.xml: 16 tests
build/test_results/trajectory_tracker/rosunit-test_trajectory_tracker_overshoot.xml: 12 tests
build/test_results/trajectory_tracker/rosunit-test_trajectory_tracker_with_odom.xml: 4 tests
build/test_results/trajectory_tracker_msgs/gtest-test_path_with_velocity_conversion.xml: 4 tests
build/test_results/trajectory_tracker_msgs/roslint-trajectory_tracker_msgs.xml: 1 tests
Summary: 342 tests, 0 errors, 0 failures, 0 skipped

@nhatao nhatao changed the title planner_3d: fix cost calculation bug on in-place turning planner_3d: fix cost calculation bug in GridAstarModel3D Oct 30, 2023
@nhatao nhatao changed the title planner_3d: fix cost calculation bug in GridAstarModel3D planner_3d: fix cost calculation bug on in-place turning Oct 30, 2023
Copy link
Owner

@at-wat at-wat left a comment

Choose a reason for hiding this comment

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

LGTM

@at-wat at-wat merged commit e632876 into master Oct 31, 2023
3 checks passed
@at-wat at-wat deleted the fix-planner-3d branch October 31, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants