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

Added FCL to build and added FCLModel #1890

Closed
wants to merge 6 commits into from

Conversation

law12019
Copy link

Drake now builds with External/fcl as an option. It is off by default because fcl uses boost which conflicts with the Matlab boost. FCLModel can be constructed instead of BulletModel. BulletModel is still the default when newModel() is called.

.../collision/test/FCLModelTest.cpp exercises some of the tests in .../collision/test/ModelTest.cpp and compares the results of BulletModel and FCLModel with the same calls.

I did not implement fcl collision with mesh shapes because the API appears to only have access to points and not triangles. I did not implement margins or ray intersections because I could
not find these features in fcl. With tests or examples of features, I could implement more.

Drake now builds with External/fcl as an option. It is off by default
because fcl uses boost which conflicts with the Matlab boost.
FCLModel can be constructed instead of BulletModel. BulletModel is
still the default when newModel() is called.

.../collision/test/FCLModelTest.cpp excersizes some of the tests in
.../collision/test/ModelTest.cpp and compares the results of BulletModel
and FCLModel with the same calls.

I did not implement fcl collision with mesh shapes because the
API appears to only have access to points and not triangles.

I did not implement margins or ray intersections because I could
not find these features in fcl.

With tests or examples of features, I could implement more.
@@ -43,6 +43,10 @@ if (WIN32)
else()
option(WITH_DIRECTOR "vtk-based visualization tool and robot user interface" ON) # not win32 yet. it builds on windows, but requires manually installation of vtk, etc. perhaps make a precompiled director pod (a bit like snopt)
option(WITH_LIBBOT "simple open-gl visualizer + lcmgl for director" ON) # there is hope, but i spent a long time cleaning up c/c++ language errors trying to make msvc happy.. and still had a long way to go.
option(WITH_BULLET "used for collision detection" ON) # almost works. just one remaining link error..?
Copy link
Contributor

Choose a reason for hiding this comment

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

this already exists above.

@RussTedrake
Copy link
Contributor

apart from the boost issue, it looks good to me on a quick code inspection. @sherm1, @amcastro-tri -- PTAL?

@sherm1
Copy link
Member

sherm1 commented Mar 23, 2016

Drake now builds with External/fcl as an option. It is off by default because fcl uses boost which conflicts with the Matlab boost

Charles, with a little arm twisting the FCL guys merged your PR into fcl/master, so you may be able to avoid using your own fork now. I also proposed eliminating boost which was enthusiastically received. There is now a nuke boost project, and a substantial PR in review. So there is some hope that in the relatively near future the boost dependency will be history.

set(flann_IS_CMAKE_POD TRUE)
#set(fcl_GIT_REPOSITORY https://github.com/flexible-collision-library/fcl.git)
#set(fcl_GIT_TAG 0.4.0)
set(fcl_GIT_REPOSITORY https://github.com/law12019/fcl.git)
Copy link
Member

Choose a reason for hiding this comment

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

@law12019, since your PR was merged you could ask FCL folks to tag a release that you could use here instead of your own repo.

@sherm1
Copy link
Member

sherm1 commented Mar 23, 2016

has build failures; not sure if this PR caused them

If the boost dependency is a fatal flaw, the shortest path to getting this in might be for @law12019 to help the FCL folks get rid of it. It doesn't look too hard. What do you think Charles?

}

// TODO: Figure out how to convert mesh verts to fcl mesh.
BVHModel<OBBRSS>* FCLModel::newFCLMeshShape(const DrakeShapes::Mesh& geometry,
Copy link
Contributor

Choose a reason for hiding this comment

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

Drake's implementation of mesh shapes seems to be using convex hulls for this in BulletModel::newBulletMeshShape. Maybe we'd like to do that here as well for now?

@amcastro-tri
Copy link
Contributor

This is a great feature @law12019! is your implementation already ready to run for instance the Cars example?

I would love to see how this implementation performs when running the SimpleCollisionTests I just created.

@@ -90,7 +93,8 @@ set(bot_core_lcmtypes_dependencies lcm)
set(director_dependencies lcm bot_core_lcmtypes libbot)
set(iris_dependencies eigen mosek)
set(signalscope_dependencies director)
set(drake_dependencies cmake eigen googletest lcm bot_core_lcmtypes libbot bullet octomap snopt gurobi swig_matlab swigmake)
set(drake_dependencies cmake eigen googletest lcm bot_core_lcmtypes libbot bullet libccd flann fcl octomap snopt gurobi swig_matlab swigmake)
set(fcl_dependancies flann libccd)
Copy link
Contributor

Choose a reason for hiding this comment

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

dependencies is misspelled here; as a result, the fcl dependencies are effectively not set

Copy link
Member

Choose a reason for hiding this comment

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

BTW the upstream fcl master branch doesn't depend on flann any more.

mwoehlke-kitware and others added 4 commits April 1, 2016 11:22
Update FCL to incorporate upstream changes, including de-boost-ification
and a fix to how ccd is found when it can't be found via pkg-config.
FCL no longer requires FLANN; remove it from the branch so we aren't
adding unnecessary externals. Also, fix spelling of FCL dependencies
variable so that the dependencies are actually seen.
@jwnimmer-tri
Copy link
Collaborator

This branch has conflicts that must be resolved.

@sherm1
Copy link
Member

sherm1 commented Apr 6, 2016

This PR replaced by #2010 -- closing here.

@sherm1 sherm1 closed this Apr 6, 2016
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.

6 participants