-
Notifications
You must be signed in to change notification settings - Fork 938
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
prevent collision checking segfault if octomap has NULL root pointer #2104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Please use
static_piointer_cast
, the type is ensured bygetObjectType
. - Why would you return any distance here? I guess you could just check and
return false;
as in the other preliminary tests above. - use
nullptr
or boolean check, notNULL
- CI fails because your patch is incompatible with the old version of FCL. Please have a look at our compat header
I'm fine with catching this issue here, but please also file an issue in the fcl project and reference this request. Either we should never clear the root or they should fix it upstream.
Thanks for the guidance. I think I've made the modifications you described. |
Codecov Report
@@ Coverage Diff @@
## master #2104 +/- ##
==========================================
+ Coverage 54.49% 57.77% +3.28%
==========================================
Files 328 328
Lines 25661 25664 +3
==========================================
+ Hits 13983 14827 +844
+ Misses 11678 10837 -841
Continue to review full report at Codecov.
|
if ((o1->getObjectType() == fcl::OT_OCTREE && | ||
!std::static_pointer_cast<const fcl::OcTreed>(o1->collisionGeometry())->getRoot()) || | ||
(o1->getObjectType() == fcl::OT_OCTREE && | ||
!std::static_pointer_cast<const fcl::OcTreed>(o1->collisionGeometry())->getRoot())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the second check should be about o2
, right?
Also, please add a comment explaining that fcl::distance
misbehaves for this in the version you use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I copy-pasted that and failed to change o1 to o2. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. Thank you for creating the FCL issue too. 👍
…oveit#2104) (cherry picked from commit 7aa1699)
Description
After clearing the octomap, the root node is set to NULL, which then causes FCL to segfault in collision checking. Maybe it's actually an FCL bug, but this was the most straightforward way I found to work around it.
Checklist