bodies::Body::containsPoint() has non-consistent behavior for "surface" points about geometric_shapes HOT 17 CLOSED

I think the main issue here, is that spheres and cylinders are eventually approximated by triangle meshes. Hence, there is only a minimal chance to exactly hit a vertex point on the surface of a cylinder / sphere surface - which then will be considered inside.
I'm afraid this won't fix.

from geometric_shapes.

In what use-case are they approximated? In visuals? But that's no problem... we're talking about collision shapes here, and these guys are pretty often composed of these basic shapes to speed up computations... Or am I missing some point?

from geometric_shapes.

geometric_shapes is used by MoveIt for collision shapes, yes. But as MoveIt uses FCL for collision checking, everything is approximated using triangle meshes.

from geometric_shapes.

Hmm, that's interesting, I thought FCL is more "clever"... But anyways, the code of collision_shapes is used at some places anyways, regardless of whether it continues to FCL or not. And in these places, I still think consistency would be good, wouldn't it?

from geometric_shapes.

At least in narrowphase it looks like at least some collision checks are done precisely.

from geometric_shapes.

@guihomework I remember you mentioned FCL being slower with geometric collision checks. Do you remember the reference?

from geometric_shapes.

@rhaschke , the paper tested it, and converting geometric shapes to meshes led to similar or faster results, unfortunately approximating the cylinders (problem we faced at some point but not anymore apparently, maybe they changed the meshing)
I put here the table they have in the paper http://gamma.cs.unc.edu/FCL/fcl_docs/webpage/pdfs/fcl_icra2012.pdf

from geometric_shapes.

Thanks for the interesting paper reference.

But back to the original question - I still don't understand why should Body::containsPoint() be related to how FCL processes collisions.

I searched for usage in the ros-planning org, and these are the places I found:
https://github.com/ros-planning/moveit/blob/melodic-devel/moveit_ros/perception/point_containment_filter/src/shape_mask.cpp#L169
https://github.com/ros-planning/moveit_advanced/blob/indigo-devel/collision_detection_distance_field/src/static_distance_field.cpp#L62
https://github.com/ros-planning/moveit/blob/melodic-devel/moveit_core/kinematic_constraints/src/kinematic_constraint.cpp#L444
https://github.com/ros-planning/moveit_advanced/blob/indigo-devel/robot_sphere_representation/src/sphere_calc.cpp#L1822
https://github.com/ros-planning/moveit_core/blob/kinetic-devel/distance_field/src/find_internal_points.cpp#L56

Well, I haven't noticed anywhere in the code that there would be a situation when you first call containsPoint() and subsequently call FCL (at least not directly). Also, most of the methods using containsPoint() are called getInsidePoints or similar, so it seems that "users" of containsPoint() automatically assumed that it returns interior points.

If you'd like to keep 1:1 compatibility with FCL results, then this method would have to be rewritten using FCL, because I don't think it'd be wise to duplicate their code. Anyways, I say I'd be quite surprised to call a Spehere::containsPoint() method and get the results valid for its bounding icosahedron...

from geometric_shapes.

@guihomework Looks like your information stems from the original paper only (comparing FCL to ODE). Since then, FCL has added geometry-based collisions as well. We need to find more recent results on this!

@peci1 I can understand your point of view. Actually, I would prefer to switch to geometry-based collisions for FCL as well. However, this is probably a lot of work... Any volunteers?

from geometric_shapes.

@peci1 I can understand your point of view. Actually, I would prefer to switch to geometry-based collisions for FCL as well. However, this is probably a lot of work... Any volunteers?

Uhm, not me, sorry for that... This would be way more work than I can do for Moveit now...

from geometric_shapes.

@rhaschke I cannot follow completely. Do you know where exactly this conversion from primitive (eg sphere) to mesh happens on it's way to fcl? Looking here https://github.com/ros-planning/moveit/blob/abc4f4070e300352c851f84fb53fa6d06c9eacbb/moveit_core/collision_detection_fcl/src/collision_common.cpp#L784 I can see that a fcl::Sphered is created

from geometric_shapes.

@simonschmeisser Thanks for looking deeper. Indeed, MoveIt seems to have used basic shapes for ages and only FCL itself improved representation of basic shapes such that the issue flexible-collision-library/fcl#122 I remembered is solved now.
Hence, may I ask @peci1 to fix Cylinder::containsPoint() to consider all surface points inside.
Please feel free to directly add to #90.

from geometric_shapes.

@rhaschke yes, they added several special cases (primitive-primitive collisions instead of generic mesh ones) recently and also seem to have added quite some tests for them

from geometric_shapes.

Nice findings. Okay, I'll do the cylinder - but then I think sphere should also be fixed to include the surface points, shouldn't it?

from geometric_shapes.

I think sphere should also be fixed to include the surface points, shouldn't it?

Yes, of course. I didn't read the initial comment carefully enough. I had the impression, spheres were handled correctly already.

from geometric_shapes.

I updated #91 with the fix for this issue.

from geometric_shapes.

Given that #97 (which succeeds #90) has been merged, I'm closing this issue.

from geometric_shapes.