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

Renamming the library to Coal #596

Merged
merged 65 commits into from
Nov 20, 2024
Merged

Conversation

lmontaut
Copy link
Contributor

@lmontaut lmontaut commented Jun 24, 2024

This PR addresses #593 and aims at changing the name of the library from hpp-fcl to coal.

For the end-user, this renaming implies the following changes:

  • In CMake: find_package(hpp-fcl) -> find_package(coal).
  • In C++: #include <hpp/fcl/...> -> #include <coal/...>.
  • In Python: import hppfcl -> import coal.

C++ users
I have added a layer of compatibility such that the namespace hpp::fcl:: is still available and points to coal:: if the cmake option COAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL is activated.
This option is set to false by default to encourage the use of coal:: instead of hpp::fcl::.

I also took the opportunity to change and clarify certain fundamental types used throughout the library:

  • FCL_REAL -> CoalScalar.
  • Matrix/Vec[...]f (i.e. Vec3f) -> Matrix/Vec[...]s (i.e. Vec3s).
  • Transform3f -> Transform3s.
    The idea behind this change is that multiple people have reported that they found the f at the end of these types confusing because they thought it referred to float when the library is double by default.
    Like the namespace, I kept the old data types behind the COAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL if needed.

Things left to do:

  • update readme
  • github workflow
  • github repo name
  • update changelog

Things to do after PR is merged: conda build, package.xml, ROS.

@jcarpent
Copy link
Contributor

Should wait for #598 to be merged.

@wxmerkt
Copy link
Contributor

wxmerkt commented Jun 25, 2024

For ROS, we may need to make a set of fresh releases to avoid breaking existing packages (and it'll take some time to get dialed in during which any updated downstream projects may fail). Ping me when it's merged and I can get started on the weekend(s)

@wxmerkt
Copy link
Contributor

wxmerkt commented Jun 26, 2024

@jcarpent One more thought - would you be open to us first fixing all the warnings before renaming so we can stop the email flood for "unstable" builds for the then no-longer-releasable hpp-fcl library?

@jcarpent
Copy link
Contributor

@jcarpent One more thought - would you be open to us first fixing all the warnings before renaming so we can stop the email flood for "unstable" builds for the then no-longer-releasable hpp-fcl library?

Yes, sure. @lmontaut will integrate your change into the renamed lib.
Thanks a lot @wxmerkt for your nice contributions.

@nim65s
Copy link
Contributor

nim65s commented Jun 28, 2024

  • In CMake: find_package(hpp-fcl) -> find_package(coal).
  • In C++: #include <hpp/fcl/...> -> #include <coal/...>.
  • In Python: import hppfcl -> import coal.

Those alone can require quite significant work. I would be in favor of providing an hpp-fcl CMake package, hpp/fcl include dir, and hppfcl python module which would raise some warning when used, but then redirect / symlink / alias to coal, and automatically enable COAL_BACKWARD_COMPATIBILITY_WITH_HPP_FCL.

I think we should be able to least configure/build/test pinocchio in its current form without requiring any change.

@lmontaut lmontaut marked this pull request as draft July 1, 2024 09:50
nim65s and others added 2 commits July 24, 2024 12:01
@nim65s
Copy link
Contributor

nim65s commented Jul 24, 2024

I'd prefer if we could decouple the workspace change from the rename change. This PR is already impossible to review.

src/CMakeLists.txt Outdated Show resolved Hide resolved
python/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
Copy link
Contributor

@nim65s nim65s left a comment

Choose a reason for hiding this comment

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

Some more extensive tests show that a couple fixes are required. I pushed those on lmontaut#4.

@lmontaut
Copy link
Contributor Author

I'd prefer if we could decouple the workspace change from the rename change. This PR is already impossible to review.

Yes it's my bad, sorry. I should have waited for this PR to be merged to merge @jorisv's changes. @nim65s I can revert the changes if you want.

@nim65s
Copy link
Contributor

nim65s commented Jul 26, 2024

We should probably push the topic/coal branch to this repo, to have easier ways of making PR to that branch. I had to make some to your fork instead, which make the history harder to follow. I think this is fine for now, we can avoid the extra cost of undoing and re-doing again. But next time, I really think we need to use a cleaner workflow

@jorisv jorisv merged commit 4157202 into coal-library:devel Nov 20, 2024
4 of 28 checks passed
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)
nim65s added a commit to nim65s/robotpkg that referenced this pull request Dec 6, 2024
…3.0.0

    ## [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, recursive bump of PKGREVISION
- 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.

5 participants