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

Fix PlanningScene CollisionDetector diff handling #464

Merged
merged 3 commits into from
May 20, 2021

Conversation

henningkayser
Copy link
Member

@henningkayser henningkayser commented May 18, 2021

This fixes how PlanningScene diffs are created and managed (replaces #463 and #456).
#364 introduced a bug where a child scene would not copy-construct the collision environments so that it wouldn't include any of the parent's link geometries or attached objects. The link geometries aren't that bad because they would be constructed from the robot model anyway, but there was no way to sync back the attached objects. This PR fixes that by providing a private allocate function that allows passing the parent detector which implements the old behavior. I tested this and it fixes #429 and #407 for me. I didn't test #444 yet.

However, there is still a bug (or irritation) where the start state of the MotionPlanningPanel doesn't include the attached object right away after publishing the collision objects. The reason is that the internal start state of the panel is not updated automatically which is also exactly the case for MoveIt 1. The requested plan then solves a trajectory which is then being rejected right away because it's in collision. To make the plan succeed, the start state needs to be updated either by selecting "<current>" again or by changing it to a different state. I actually don't think that this needs fixing and it's definitely out of scope of this PR.

if (have_previous_coll_detector)
// If parent_detector is specified, copy-construct collision environments (copies link shapes and attached objects)
// Otherwise, construct new collision environment from world and robot model
if (parent_detector)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the key part of this fix. The collision environment of the parent is used to copy-construct the child environment so that attached objects are available as well.

collision_detector_->copyPadding(*parent_->collision_detector_);
}
collision_detector_->cenv_const_ = collision_detector_->cenv_;
// Reset collision detector to the the parent's version
Copy link
Member Author

Choose a reason for hiding this comment

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

clearDiffs() now also just allocates a fresh collision detector from parent

@codecov
Copy link

codecov bot commented May 18, 2021

Codecov Report

Merging #464 (3b6e0d1) into main (d788700) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
+ Coverage   51.41%   51.42%   +0.01%     
==========================================
  Files         223      223              
  Lines       23343    23343              
==========================================
+ Hits        12000    12001       +1     
+ Misses      11343    11342       -1     
Impacted Files Coverage Δ
...ene/include/moveit/planning_scene/planning_scene.h 55.18% <100.00%> (+1.61%) ⬆️
moveit_core/planning_scene/src/planning_scene.cpp 45.24% <100.00%> (-0.10%) ⬇️
moveit_core/collision_detection/src/world.cpp 80.96% <0.00%> (-0.68%) ⬇️
...nning_scene_monitor/src/planning_scene_monitor.cpp 56.24% <0.00%> (+0.13%) ⬆️
.../collision_detection_fcl/src/collision_env_fcl.cpp 90.00% <0.00%> (+0.53%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d788700...3b6e0d1. Read the comment docs.

@AndyZe
Copy link
Member

AndyZe commented May 19, 2021

Tested with the move_group_interface tutorial. Looks good to me!

@PrakarnJ
Copy link

@henningkayser I already test this with octomap . It can fixed #444

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.

move_group is not respecting the attached collision object
3 participants