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

add support for fcl 0.6 #1156

Merged
merged 1 commit into from
Nov 16, 2018
Merged

add support for fcl 0.6 #1156

merged 1 commit into from
Nov 16, 2018

Conversation

2scholz
Copy link
Contributor

@2scholz 2scholz commented Oct 26, 2018

The (still unreleased) fcl 0.6 changed paths to relevant headers as well as the name of many types.
This patch adds support for fcl 0.6 without breaking support for fcl 0.5.

This patch adds the header fcl_compat.h to check for the fcl version and include fcl headers accordingly.
The old types are exchanged to the newer ones throughout the package.
Compability to fcl 0.5 is ensured by creating type aliases that map the old types to the new ones.

The other way around might be the better choice for now, but the old type names are still in use in the new versions (as templates), so the alias trick only works this way around.

For this a lot of code from @Levi-Armstrong was used, who already created a branch with support for fcl 0.6 but without support for fcl 0.5: https://github.com/Levi-Armstrong/moveit/tree/newFCL

@v4hn

@BryceStevenWilley
Copy link
Contributor

A third option, besides aliasing old types to new or new types to old types, is to using typedefs that are not in the fcl namespace. This is the approach taken by OMPLapp (ompl/omplapp@b986838#diff-524e09a932cb1946ad510acbcd23bec5R56). This does make it more difficult to keep up with additions to FCL itself however. Both require lots of small changes in the codebase as well.

As long as we agree on how to handle the aliases, this is a great contribution. . 👍

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

I asked @2scholz to implement it the way he did and we discussed quite a bit while he worked on it.

I believe it would be better to merge this only after the FCL maintainers pushed a release tag to their repository. That way we can refer to their officially released API for the way we use it here.

However, if they don't manage to do this reasonably soon, I'm also happy with merging it right away.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Including all headers in fcl_compat.h reduces compiler performance and should be avoided.
I suggest the following:

  • Define a macro FCL_VERSION_CHECK to simplify version checking:
    #define FCL_VERSION_CHECK(major, minor, patch) ((major<<16)|(minor<<8)|(patch))
    #define FCL_VERSION FCL_VERSION_CHECK(FCL_MAJOR_VERSION, FCL_MINOR_VERSION, FCL_PATCH_VERSION)```
    
  • In fcl_compat.h provide class forwards to get rid of includes, but allow typedefs:
    namespace fcl {
    class CollisionGeometry;
    using CollisionGeometryd = fcl::CollisionGeometry;
    ...
    }
    
  • Restore individual includes, explicitly switching between old and new using the introduced macro:
    #if (FCL_VERSION >= FCL_VERSION_CHECK(0, 6, 0))
    #include <fcl/narrowphase/collision.h>
    #else
    #include <fcl/collision.h>
    #endif
    

@rhaschke
Copy link
Contributor

I agree to merge this before an official FCL 0.6 release. Backwards-compatibility is ensured ;-)
Please rebase.

fcl 0.6 changed paths to relevant headers as well as changed the
name of many types compared to fcl 0.5.
This patch adds the header fcl_compat.h to check for the fcl
version and include fcl headers accordingly.
The old types are exchanged to the newer ones throughout the
package.
Compability to fcl 0.5 is ensured by creating type aliases that
map the old types to the new ones.
@2scholz
Copy link
Contributor Author

2scholz commented Nov 16, 2018

I made the requested changes.
FCL_VERSION is already defined in fcl/config.h, so I named the macro MOVEIT_FCL_VERSION instead.

@rhaschke rhaschke merged commit b8b4814 into moveit:melodic-devel Nov 16, 2018
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.

None yet

4 participants