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

Adding swept-sphere radius + removing dead code #558

Merged
merged 76 commits into from
Apr 3, 2024

Conversation

lmontaut
Copy link
Contributor

@lmontaut lmontaut commented Mar 29, 2024

Ok, it's a big PR, haven't finished it yet but it's almost done.
Before I rant, note that there are absolutely no changes to HPP-FCL API, it's only internal stuff.

It started with a simple premise: adding swept-sphere radius for all shapes of HPP-FCL.
However, it got a bit out of hand.

When making simple modifications to the library, I would always encounter compilation errors which were coming from dead code.
This is very frustrating to deal with because 90% of the time is spent fixing code that is not used and not supported.
Most of the problems come from a single culprit: the GJKSolver. This solver had specializations for specific pairs of shapes.
However, these specializations were already dealt with in the ShapeShapeCollide/Distance internal functions. So this means having to keep track of multiple specializations. On top of that, most of these specializations were located in a details.h file which contained about 2000 LOC of dead code. Also, many functions had conflicting convention on their return value.

The solution: I moved all the specialized algorithms to the ShapeShapeCollide/Distance internal functions and skinned the GJKSolver. Now GJKSolver, as its name indicates, is only used to run GJK/EPA.
This also has the benefit of being able to test the specialized algorithms against GJK/EPA.

The overall philosophy is the following: no internal functions in HPP-FCL should directly call the GJKSolver.
All internal functions should use either ShapeShapeCollide or ShapeShapeDistance, which take care of either using a specialized algorithm or GJK/EPA.

TODOs regarding the original swept-sphere PR:

  • Finish swept-sphere for specialized functions (sphere-box, sphere-cylinder, plane-plane, halfspace-halfspace, plane-halfspace)
  • Rename the template parameter of support functions from Inflation[...] to SweptSphere[...]
  • Export template specializations of MinkowskiDiff::set and getSupport

Next PR:

  • Take into account swept-sphere in AABB/OBB computation
  • Add swept-sphere for BVHs, Hfields and Octrees. Also make sure Broadphase takes swept-sphere into account (it should thanks to computeLocalAABB).

@lmontaut lmontaut requested a review from jcarpent March 29, 2024 09:53
@florent-lamiraux
Copy link
Contributor

Hello @lmontaut
By swept sphere radius, do you mean the Minkowski sum of a shape with a sphere ?

@lmontaut
Copy link
Contributor Author

lmontaut commented Mar 29, 2024

Hello @lmontaut By swept sphere radius, do you mean the Minkowski sum of a shape with a sphere ?

Hi @florent-lamiraux, yes!
Just to clarify, this was already a feature of the MinkowskiDiff in HPP-FCL, I simply extracted it to make it available for all shapes (other than spheres and capsules).
As was already done in HPP-FCL, there is no need to take the swept-sphere radius into account in the iterations of GJK/EPA. You can simply correct the solution found by GJK/EPA after they have converged.
This mathematical property is verified in the test/swept_sphere_radius.cpp I created while working on this PR. It runs GJK/EPA with and without taking into account the inflation of the supports in the iterations of the algos, and check the solutions.

However, it is sometimes usefull to compute the support function of shapes while taking into account their swept-sphere radius, so I added the support for that.

@florent-lamiraux
Copy link
Contributor

Isn't it redundant with security margins ?

@lmontaut
Copy link
Contributor Author

Isn't it redundant with security margins ?

Yes, it's a redundant information if only the distance between the shapes is considered.
But it does change the witness points (by translating them along the normal).
I agree it's a simple transformation but it would require all the algos using the GJKSolver to factor in whether or not to modify the witness points (certain algos like HeightField-Shape do some internal bin corrections which need multiple call to GJK/EPA).

In the end, I think swept-sphere radius and security margin are two complementary information:

  • The security margin is aimed at a pair of collision. It's used to make sure two shapes don't get closer than a certain value.
  • A swept-sphere radius is a property of a shape. Sometimes it's usefull to described a rounded box by its underlying box + swept sphere radius, and not have to manually correct the witness points/distance by using the security margin.

python/gjk.cc Outdated Show resolved Hide resolved
jcarpent
jcarpent previously approved these changes Mar 29, 2024
Copy link
Contributor

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

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

Thanks @lmontaut for this massive and very useful clean up.
It will help us to remove outdated and unsupported codes.

@florent-lamiraux
Copy link
Contributor

Isn't it redundant with security margins ?

Yes, it's a redundant information if only the distance between the shapes is considered. But it does change the witness points (by translating them along the normal). I agree it's a simple transformation but it would require all the algos using the GJKSolver to factor in whether or not to modify the witness points (certain algos like HeightField-Shape do some internal bin corrections which need multiple call to GJK/EPA).

In the end, I think swept-sphere radius and security margin are two complementary information:

* The security margin is aimed at a pair of collision. It's used to make sure two shapes don't get closer than a certain value.

* A swept-sphere radius is a property of a shape. Sometimes it's usefull to described a rounded box by its underlying box + swept sphere radius, and not have to manually correct the witness points/distance by using the security margin.

All right. I understand.
I second @jcarpent : Thank you very much for cleaning up unused code.

@jcarpent
Copy link
Contributor

  • Serialize CollisionGeometry with getNodeType. For now, when we serialize it, we don't serialize the child object

Serialization works normally for derived class. @lmontaut Have you encountered any issue?

@lmontaut
Copy link
Contributor Author

  • Serialize CollisionGeometry with getNodeType. For now, when we serialize it, we don't serialize the child object

Serialization works normally for derived class. @lmontaut Have you encountered any issue?

Yes, I can't serialize a CollisionGeometry to a text file for example, but maybe I am doing something wrong... Also, on the python side, I forgot to add the serializable visitor to the python object CollisionGeometry.

Again, maybe I am doing something wrong on the C++ side. I will delete this todo after investigation :)

@lmontaut lmontaut force-pushed the topic/swept-sphere-primitives branch 2 times, most recently from 25dd169 to 2fa9a00 Compare March 31, 2024 17:53
There is never the need to normalize the support direction when computing
a support point. This is because the argmax support function (the support
point) is invariant by multiplication by a positive scalar.
So the support in the direction dir or alpha * dir (with alpha > 0) will
always be identical, no matter the shape.
GJKSolver has direct access to GJK's distance_upper_bound field, so
there is no need to store it in GJKSolver.
We don't remove max iterations and tolerance fields because they are
used in the constructor of GJK and EPA.
Of course we also don't remove things related to cached guess.
GJK and EPA don't need to inflate the supports by the swept sphere radii
of the involved shapes at each their iterations.
This is a very nice mathematical property, which allows to run GJK/EPA on
the original shapes and simply correct the solution GJK/EPA found after
they have converged.

However, it is sometimes usefull to have access to the "true" support
function of a shape swept by a sphere.
Hence the template parameter.

The rule is simple: when interacting with GJK/EPA, this template parameter
should always be `false` (except for debugging purposes).
When interacting directly with a shape and using its support function,
the template parameter should always be `true`.
The ShapeShapeDistance function is responsible for either calling directly GJK/EPA
or calling a specialized function.
I previously deleted these attributes (woopsie).
It's actually important that they appear as attributes as most methods of
`GJKSolver` are const.
The idea is that only the API functions of hpp-fcl (like collide/distance or
ComputeCollision/Distance functors) are the only one functions which can set up
the GJKSolver. Then, any other internal functions (like ShapeShapeCollide or BVHShapeCollide etc.)
only work with `const GJKSolver *`. This way, we make sure that the settings of the GJKSolver are
not modified.
- Certain primitives can be "inflated". This returns a pair of new primitive and a new Transform.
An inflated primitive is simply elongated/shrinked along its primary dimensions.
An inflated box is simply the original box which vertices have been elongated along the primary
axiis of the box.
Note that the inflation does not need to be positive. A primitive can be deflated ("inflated negatively").

- Swept-sphere radius is different: the shape is not modified. Swept-sphere radius is a property
of a shape which mainly softens its sharp corners (a box will become a box with rounded corners).
The swept-sphere radius cannot be negative.

To fully distinguish between these two features, I made sure any comment/template/variable referring
to swept-sphere radius did not have the word "inflation", as this word is reserved to talk about
the "inflated" methods of certain primitives.
Previously `GJKSolver::shapeDistance` was specialized for Triangle-Triangle`.
This was removed, and now `ShapeShapeDistance` is the one taking care of the
specialization.
To remove any possible confusion, a signature of ShapeShapeDistance is created
which has no mention of `DistanceRequest/Result`.
This function is used in complex structures like BVHs, Hfields and octrees.
Added a comment in EPA that explains it all.
In a future PR, I will remove the `NonConvex` status of EPA, as it useless.
Even when EPA is called "when it should not be" (i.e called on non-colliding shapes),
it will work just fine and return the correct result.
This is a very very cool property of EPA.
The doxygen_xml_parser.py script complains when an empty template
param is found (`typename = `). This happens for `GJKSolver::shapeDistance`
applied to TriangleP-TriangleP.
In any case, the `GJKSolver::shapeDistance` should not be template instanciated
for this pair of shapes as EPA does not work for TriangleP-TriangleP.
This fix actually makes sure the specialized TriangleP-TriangleP algo is used
instead.
@jcarpent jcarpent force-pushed the topic/swept-sphere-primitives branch from 85958db to a7e5f31 Compare April 3, 2024 04:46
@jcarpent jcarpent merged commit f639699 into coal-library:devel Apr 3, 2024
29 of 34 checks passed
@lmontaut lmontaut deleted the topic/swept-sphere-primitives branch April 4, 2024 12:53
nim65s added a commit to nim65s/robotpkg that referenced this pull request Nov 21, 2024
    ## [3.0.0] - 2024-11-20

    ### Added
    - Renaming the library from `hpp-fcl` to `coal`. Created a `COAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL` CMake option for retro compatibility. This allows to still do `find_package(hpp-fcl)` and `#include <hpp/fcl/...>` in C++ and it allows to still do `import hppfcl` in python (coal-library/coal#596).
    - Added `Transform3f::Random` and `Transform3f::setRandom` (coal-library/coal#584)
    - New feature: computation of contact surfaces for any pair of primitive shapes (triangle, sphere, ellipsoid, plane, halfspace, cone, capsule, cylinder, convex) (coal-library/coal#574).
    - Enhance Broadphase DynamicAABBTree to better handle planes and halfspace (coal-library/coal#570)
    - (coal-library/coal#558):
      - [internal] Removed dead code in `narrowphase/details.h` (coal-library/coal#558)
      - [internal] Removed specializations of methods of `GJKSolver`. Now the specializations are all handled by `ShapeShapeDistance` in `shape_shape_func.h`.
      - [new feature] Added support for Swept-Sphere primitives (sphere, box, capsule, cone, ellipsoid, triangle, halfspace, plane, convex mesh).
    - [API change] Renamed default convergence criterion from `VDB` to `Default` (coal-library/coal#556)
    - Fixed EPA returning nans on cases where it could return an estimate of the normal and penetration depth. (coal-library/coal#556)
    - Fixed too low tolerance in GJK/EPA asserts (coal-library/coal#554)
    - Fixed `normal_and_nearest_points` test (no need to have Eigen 3.4) (coal-library/coal#553)
    - (coal-library/coal#549)
    - Optimize EPA: ignore useless faces in EPA's polytope; warm-start support computation for `Convex`; fix edge-cases witness points computation.
    - Add `Serializable` trait to transform, collision data, collision geometries, bounding volumes, bvh models, hfields. Collision problems can now be serialized from C++ and sent to python and vice versa.
    - CMake: allow use of installed jrl-cmakemodules (coal-library/coal#564)
    - CMake: Add compatibility with jrl-cmakemodules workspace (coal-library/coal#610)
    - Python: add id() support for geometries (coal-library/coal#618).

Packaging changes:
- renamed package
- removed patch 522, merged upstream
- updated patch aa
- replaced common Makefile from HPP to JRL
- turned on backward compatibility with hpp-fcl
- updated required version to 3.0.0 (coal < 3.0.0 does not exist)
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