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

Conversation

Projects
None yet
4 participants
@2scholz
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

BryceStevenWilley commented Oct 28, 2018

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. . 👍

@v4hn

v4hn approved these changes Nov 9, 2018

Copy link
Member

v4hn left a comment

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.

@rhaschke
Copy link
Contributor

rhaschke left a comment

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

This comment has been minimized.

Copy link
Contributor

rhaschke commented Nov 11, 2018

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

add support for fcl 0.6
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 2scholz force-pushed the 2scholz:new_fcl branch from 0fb1201 to b4e6445 Nov 16, 2018

@2scholz

This comment has been minimized.

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 ros-planning:melodic-devel Nov 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.