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

Check collisions during joint motions, too #2204

Merged
merged 7 commits into from
Aug 3, 2020
Merged

Check collisions during joint motions, too #2204

merged 7 commits into from
Aug 3, 2020

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Jul 9, 2020

  • Check collisions during joint motions (just like they are with Cartesian motions)

  • Print colliding objects, for debugging

@AndyZe AndyZe requested review from rhaschke and v4hn as code owners July 9, 2020 20:25
@AndyZe AndyZe requested a review from tylerjw July 9, 2020 20:26
@codecov
Copy link

codecov bot commented Jul 9, 2020

Codecov Report

Merging #2204 into master will increase coverage by 0.00%.
The diff coverage is 57.14%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2204   +/-   ##
=======================================
  Coverage   57.66%   57.66%           
=======================================
  Files         326      326           
  Lines       25662    25674   +12     
=======================================
+ Hits        14797    14804    +7     
- Misses      10865    10870    +5     
Impacted Files Coverage Δ
...oveit_servo/include/moveit_servo/collision_check.h 100.00% <ø> (ø)
moveit_ros/moveit_servo/src/servo_calcs.cpp 61.21% <50.00%> (-0.06%) ⬇️
moveit_ros/moveit_servo/src/collision_check.cpp 77.90% <60.00%> (-2.36%) ⬇️

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 89a6752...aa452d7. Read the comment docs.

moveit_ros/moveit_servo/src/collision_check.cpp Outdated Show resolved Hide resolved
@AndyZe AndyZe changed the title Check collisions during joint motions, too WIP: Check collisions during joint motions, too Jul 10, 2020
@AndyZe AndyZe changed the title WIP: Check collisions during joint motions, too Check collisions during joint motions, too Jul 11, 2020
Copy link
Contributor

@mlautman mlautman left a comment

Choose a reason for hiding this comment

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

Good improvements.

I would want to be sure that storing collision pairs won't slow down the code and at least have the option to turn off printing collision pairs. Other feedback is more minor.

if (!contact_map.empty())
{
// Throttled error message about the first contact in the list
ROS_ERROR_STREAM_THROTTLE_NAMED(ROS_LOG_THROTTLE_PERIOD, LOGNAME, "Objects in collision (among others, possibly): "
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really an error or should it be a warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't want to see people filtering out all debug info if this gets spammy. I think a rosparam for verbose collision checking would be a nice addition.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you modify the collision_request_.contacts = verbose_collision_checking_ (where verbose_collision_checking_ is set from a rosparam) than maybe if (!contact_map.empty()) will always be false and you won't have to add additional logic in this method. Something to investigate

Copy link
Member Author

Choose a reason for hiding this comment

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

will change it to a warning.

Not sure about making in configurable. It's already throttled so it doesn't get printed very often, and we already have a bunch of parameters to wade through (~40 of them)

moveit_ros/moveit_servo/src/collision_check.cpp Outdated Show resolved Hide resolved
moveit_ros/moveit_servo/src/servo_calcs.cpp Outdated Show resolved Hide resolved
@AndyZe
Copy link
Member Author

AndyZe commented Jul 20, 2020

@mlautman @tylerjw @henningkayser Is anybody going to click the merge button?

I guess it would help if tests were passing, but the failing tests aren't related.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

lgtm

@tylerjw tylerjw merged commit a07cb8a into moveit:master Aug 3, 2020
@rhaschke
Copy link
Contributor

rhaschke commented Aug 7, 2020

@tylerjw: Why did you merge this? This introduced a Travis regression in the Noetic build - as indicated by the failing build.

@tylerjw
Copy link
Member

tylerjw commented Aug 7, 2020

@rhaschke It was passing. It should have been rebased on master as the noetic build was added to travis after this PR was starting. I'll look into the regression and see if I can fix that.

@tylerjw
Copy link
Member

tylerjw commented Aug 7, 2020

If you look through the output of the failing Noetic build it seems like it has something to do either with a robot description or python: https://travis-ci.com/github/ros-planning/moveit/jobs/369428306

I don't see anything in there about servo.

@rhaschke
Copy link
Contributor

It was passing. It should have been rebased on master as the noetic build was added to travis after this PR was starting. I'll look into the regression and see if I can fix that.

@tylerjw, sorry for blaming you. I concluded from the last failing Travis build in this PR.
I fixed Travis by triggering a new build of the Noetic docker image(s).

@tylerjw
Copy link
Member

tylerjw commented Aug 10, 2020

Thank you, I'm glad you figured it out. I couldn't figure out what was broken. 👍

rhaschke pushed a commit that referenced this pull request Aug 13, 2020
* Check collisions during joint motions, too.
  Print colliding objects, for debugging.

* Small code simplification

* Use the argument to printCollisionPairs

* Clang format

* Fix misspelling

* Log all colliding pairs

* Adjustments to print formatting and log levels
@rhaschke rhaschke mentioned this pull request Aug 13, 2020
rhaschke pushed a commit that referenced this pull request Aug 13, 2020
* Check collisions during joint motions, too.
  Print colliding objects, for debugging.

* Small code simplification

* Use the argument to printCollisionPairs

* Clang format

* Fix misspelling

* Log all colliding pairs

* Adjustments to print formatting and log levels
@AndyZe AndyZe deleted the andyz/detailed_collision_output branch September 18, 2020 13:33
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.

7 participants