search

coal-library / coal

An extension of the Flexible Collision Library
Other
328 stars 93 forks source link

Renamming the library to `Coal` #596

Closed lmontaut closed 5 days ago

lmontaut commented 5 months ago

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:

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:

Things left to do:

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

jcarpent commented 5 months ago

Should wait for #598 to be merged.

wxmerkt commented 5 months ago

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago
  • 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.

jorisv commented 4 months ago

Hello @nim65s,

I agree that providing a full CMake/Python/C++ compatibility is a nice option.

I also think this option should not be activated in coal package (conda, nix, apt, ...). Since coal don't have the same name than hpp-fcl, we will be allowed by many packaging system to install coal and hpp-fcl at the same time. In this case, the compatibility files will conflict with the hpp-fcl files and create some hard to debug issues.

One option can be to release a hpp-fcl v3 package that depend of coal and provide the compatibility files. Another option is to forbid the installation of hpp-fcl and coal in the same time, but I don't know if all package manager provide this option.

nim65s commented 4 months ago

Yes, from a package manager point of view, coal should replace hpp-fcl. In archlinux terms, this is conflicts=('hpp-fcl') + provides=('hpp-fcl'). In python terms, this is Obsoletes-Dist: hpp-fcl for coal, and Provides-Dist: coal for hpp-fcl. For monorepo package managers like robotpkg or nix, we don't care.

One option can be to release a hpp-fcl v3 package that depend of coal and provide the compatibility files.

I already implemented that, and I'm testing this solution in various scenarii.

nim65s commented 4 months ago

ref. https://github.com/lmontaut/hpp-fcl/pull/1

nim65s commented 4 months ago

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

lmontaut commented 4 months ago

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 commented 4 months ago

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