Add FCL external #3381

jamiesnape · 2016-09-07T21:53:44Z

Adds the Flexible Collision Library (FCL) as an external.
Adds a stubbed collision model that will use FCL and a basic integration test.

Obviously, much work to do implementing the model and adding more tests, but this is something to build upon.

This change is

sherm1 · 2016-09-23T15:05:17Z

/cc @SeanCurtis-TRI

jamiesnape · 2016-09-26T13:31:08Z

Upstreaming danfis/libccd#24 and flexible-collision-library/fcl#181.

jamiesnape · 2016-09-27T16:26:19Z

Upstreams have accepted necessary patches.

sherm1 · 2016-09-27T16:42:45Z

Upstreams have accepted necessary patches.

Awesome! Thanks, Jamie.

jamiesnape · 2016-09-27T20:46:41Z

Upstreaming flexible-collision-library/fcl#182 and OctoMap/octomap#137.

jamiesnape · 2016-09-28T20:14:31Z

FCL patch is in, waiting on the octomap patch.

jamiesnape · 2016-10-03T18:16:19Z

+@SeanCurtis-TRI for feature review and +@sherm1 for platform review.

SeanCurtis-TRI

Looks good

sherm1 · 2016-10-03T23:44:51Z

Thanks, @jamiesnape! A few comments -- PTAL.

+@amcastro-tri can you take a quick look at this please?

Reviewed 10 of 10 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/collision/fcl_model.h, line 21 at r1 (raw file):

  bool closestPointsAllToAll(const std::vector<ElementId>& ids_to_check,
                             const bool use_margins,

const is meaningless here (in declaration), should be just bool use_margins. (and below) You can still use the const in the .cc implementation if you want; C++ considers that the same signature.

drake/systems/plants/collision/test/fcl_model_test.cc, line 12 at r1 (raw file):

GTEST_TEST(FCLModelTest, Test) {
  using namespace fcl;

Not allowed per styleguide -- just individual using fcl::blahs.

drake/systems/plants/collision/test/fcl_model_test.cc, line 19 at r1 (raw file):

  EXPECT_TRUE(v1.cross(v2).isApprox(v3));
  EXPECT_DOUBLE_EQ(26.0, v1.dot(v2));

This looks like it's testing Eigen rather than FCL?

Comments from Reviewable

jamiesnape · 2016-10-04T12:56:45Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/collision/fcl_model.h, line 21 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

const is meaningless here (in declaration), should be just bool use_margins. (and below) You can still use the const in the .cc implementation if you want; C++ considers that the same signature.

True. I was idly copying the superclass... I will fix both.

drake/systems/plants/collision/test/fcl_model_test.cc, line 19 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

This looks like it's testing Eigen rather than FCL?

Testing the wrapped versions really. I do not expect this test to persist once the model is implemented.

Comments from Reviewable

jamiesnape · 2016-10-04T13:24:24Z

Review status: 8 of 16 files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/collision/test/fcl_model_test.cc, line 19 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

Testing the wrapped versions really. I do not expect this test to persist once the model is implemented.

I will add something more meaningful.

Comments from Reviewable

amcastro-tri · 2016-10-04T14:46:51Z

It looks great to get started. Just a few comments.

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/collision/fcl_model.h, line 19 at r2 (raw file):

  FCLModel() {}
  virtual ~FCLModel() {}

BTW, Here missing override for addElement(). And I wonder if you could provide super basic functionality to add say a sphere so that we check it links correctly with FCL (and we can have a more FLC-like unit test in fcl_model_test.cc)

Comments from Reviewable

SeanCurtis-TRI · 2016-10-05T15:01:12Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

drake/systems/plants/collision/fcl_model.h, line 19 at r2 (raw file):

Previously, amcastro-tri (Alejandro Castro) wrote…

BTW, Here missing override for addElement(). And I wonder if you could provide super basic functionality to add say a sphere so that we check it links correctly with FCL (and we can have a more FLC-like unit test in fcl_model_test.cc)

BTW, I'm comfortable with leaving that as an exercise for tying FCL into drake. I believe the scope of this was getting FCL into the build.

Comments from Reviewable

jamiesnape · 2016-10-05T16:39:09Z

Review status: 8 of 16 files reviewed at latest revision, 3 unresolved discussions.

drake/systems/plants/collision/fcl_model.h, line 21 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

True. I was idly copying the superclass... I will fix both.

Done.

drake/systems/plants/collision/test/fcl_model_test.cc, line 12 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Not allowed per styleguide -- just individual using fcl::blahs.

Done.

drake/systems/plants/collision/test/fcl_model_test.cc, line 19 at r1 (raw file):

Previously, jamiesnape (Jamie Snape) wrote…

I will add something more meaningful.

Done.

Comments from Reviewable

sherm1 · 2016-10-05T17:24:24Z

-@amcastro-tri due to vacation (already reviewed)

Review status: 8 of 16 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

sherm1 · 2016-10-05T17:28:06Z

-- one comment, PTAL.

Reviewed 3 of 8 files at r2, 8 of 8 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

drake/systems/plants/collision/bullet_model.h, line 153 at r3 (raw file):

   */
  virtual PointPair findClosestPointsBetweenElements(
      const ElementId idA, const ElementId idB, bool use_margins);

BTW oops, the ElementIds also have a spurious const.

Comments from Reviewable

jamiesnape · 2016-10-05T17:37:42Z

Review status: 10 of 16 files reviewed at latest revision, 1 unresolved discussion.

drake/systems/plants/collision/bullet_model.h, line 153 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW oops, the ElementIds also have a spurious const.

Removed.

Comments from Reviewable

jamiesnape · 2016-10-05T17:38:05Z

@drake-jenkins-bot linux-gcc-ninja-experimental please
@drake-jenkins-bot mac-clang-experimental please
@drake-jenkins-bot mac-clang-ninja-experimental please

jamiesnape · 2016-10-05T18:06:26Z

SEGFAULT.

sherm1 · 2016-10-05T18:10:03Z

Reviewed 6 of 6 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

jamiesnape · 2016-10-10T14:23:24Z

@drake-jenkins-bot linux-gcc-ninja-experimental please
@drake-jenkins-bot mac-clang-experimental please
@drake-jenkins-bot mac-clang-ninja-experimental please

jamiesnape · 2016-10-12T16:47:30Z

@drake-jenkins-bot linux-gcc-ninja-experimental please
@drake-jenkins-bot mac-clang-experimental please
@drake-jenkins-bot mac-clang-ninja-experimental please

amcastro-tri · 2016-10-21T13:39:11Z

back from vacation. What's the status of this?. It'd be nice to get it merged!

jamiesnape · 2016-10-21T13:40:34Z

I need to sort out some Eigen alignment issues that are causing the test to SEGFAULT.

jamiesnape · 2016-10-21T14:32:45Z

Resolved merge conflicts.

jamiesnape · 2016-10-21T19:36:56Z

And more merge conflicts.

amcastro-tri · 2016-10-24T14:52:33Z

There still is a CI problem. Once solved this . Thanks!

Reviewed 2 of 12 files at r5.
Review status: 7 of 15 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

drake/systems/plants/collision/fcl_model.h, line 19 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW, I'm comfortable with leaving that as an exercise for tying FCL into drake. I believe the scope of this was getting FCL into the build.

I agree, I will mark myself satisfied.

Comments from Reviewable

jamiesnape · 2016-10-28T17:13:57Z

@drake-jenkins-bot retest this please
@drake-jenkins-bot linux-clang-experimental please
@drake-jenkins-bot linux-clang-experimental-minimal please
@drake-jenkins-bot linux-clang-ninja-experimental-open-source please
@drake-jenkins-bot linux-cpplint-experimental please
@drake-jenkins-bot linux-gcc-experimental-matlab please
@drake-jenkins-bot linux-gcc-experimental-open-source please
@drake-jenkins-bot linux-gcc-experimental-ros please
@drake-jenkins-bot linux-gcc-ninja-experimental please

jamiesnape · 2016-10-31T19:44:21Z

Problem seems to be upstream. Investigating further.

jwnimmer-tri · 2016-11-28T18:17:50Z

@jamiesnape Is this PR still on deck (though maybe down on the stack)? I don't want to close it because it has review in-progress, but it's also quite stale.

jamiesnape · 2016-11-28T18:18:26Z

It is still on-deck.

jamiesnape · 2016-12-23T18:52:56Z

@drake-jenkins-bot linux-clang-experimental-minimal please
@drake-jenkins-bot linux-clang-experimental-open-source please
@drake-jenkins-bot linux-clang-ninja-experimental please
@drake-jenkins-bot linux-gcc-bazel-experimental please
@drake-jenkins-bot linux-gcc-experimental please
@drake-jenkins-bot linux-gcc-experimental-minimal please
@drake-jenkins-bot linux-gcc-ninja-experimental-minimal please
@drake-jenkins-bot linux-gcc-ninja-experimental-open-source please
@drake-jenkins-bot linux-xenial-clang-experimental please
@drake-jenkins-bot linux-xenial-gcc-bazel-experimental please
@drake-jenkins-bot linux-xenial-gcc-experimental please
@drake-jenkins-bot linux-xenial-unprovisioned-clang-experimental please
@drake-jenkins-bot linux-xenial-unprovisioned-gcc-experimental please
@drake-jenkins-bot mac-clang-experimental please
@drake-jenkins-bot mac-clang-experimental-minimal please
@drake-jenkins-bot mac-clang-experimental-open-source please
@drake-jenkins-bot mac-clang-ninja-experimental please
@drake-jenkins-bot mac-clang-ninja-experimental-minimal please
@drake-jenkins-bot mac-clang-ninja-experimental-open-source please

jamiesnape · 2016-12-23T19:54:55Z

Finally.

sherm1 · 2016-12-23T21:01:43Z

Yay! Thanks, @jamiesnape.

@SeanCurtis-TRI, Jamie got FCL linked in.

jamiesnape added status: do not merge status: do not review labels Sep 7, 2016

RussTedrake assigned jamiesnape Sep 9, 2016

jamiesnape mentioned this pull request Sep 23, 2016

Use non-POD octomap #3569

Merged

jamiesnape added the priority: high label Sep 23, 2016

jamiesnape mentioned this pull request Sep 27, 2016

Added FCL to build and added FCLModel #2010

Closed

jamiesnape removed status: do not merge status: do not review labels Sep 27, 2016

jamiesnape assigned SeanCurtis-TRI and sherm1 and unassigned jamiesnape, sherm1 and SeanCurtis-TRI Oct 3, 2016

SeanCurtis-TRI approved these changes Oct 3, 2016

View reviewed changes

sherm1 assigned amcastro-tri Oct 3, 2016

sherm1 unassigned amcastro-tri Oct 5, 2016

jamiesnape mentioned this pull request Oct 11, 2016

Add libccd external #3762

Merged

Jamie Snape added 4 commits December 23, 2016 13:05

Fix const bool/ElementId in collision model member functions

eafab0d

Add FCL external

7e1d8c2

Add FCL collision model

5c580d1

Add FCL collision model test

18a8ff0

jamiesnape merged commit ff9294f into RobotLocomotion:master Dec 23, 2016

jamiesnape deleted the fcl branch December 23, 2016 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FCL external #3381

Add FCL external #3381

jamiesnape commented Sep 7, 2016 •

edited

Loading

sherm1 commented Sep 23, 2016

jamiesnape commented Sep 26, 2016

jamiesnape commented Sep 27, 2016

sherm1 commented Sep 27, 2016

jamiesnape commented Sep 27, 2016

jamiesnape commented Sep 28, 2016

jamiesnape commented Oct 3, 2016

SeanCurtis-TRI left a comment

sherm1 commented Oct 3, 2016

jamiesnape commented Oct 4, 2016

jamiesnape commented Oct 4, 2016

amcastro-tri commented Oct 4, 2016

SeanCurtis-TRI commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

sherm1 commented Oct 5, 2016

sherm1 commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

sherm1 commented Oct 5, 2016

jamiesnape commented Oct 10, 2016

jamiesnape commented Oct 12, 2016

amcastro-tri commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

amcastro-tri commented Oct 24, 2016

jamiesnape commented Oct 28, 2016

jamiesnape commented Oct 31, 2016

jwnimmer-tri commented Nov 28, 2016 •

edited

Loading

jamiesnape commented Nov 28, 2016

jamiesnape commented Dec 23, 2016 •

edited

Loading

jamiesnape commented Dec 23, 2016

sherm1 commented Dec 23, 2016

Add FCL external #3381

Add FCL external #3381

Conversation

jamiesnape commented Sep 7, 2016 • edited Loading

sherm1 commented Sep 23, 2016

jamiesnape commented Sep 26, 2016

jamiesnape commented Sep 27, 2016

sherm1 commented Sep 27, 2016

jamiesnape commented Sep 27, 2016

jamiesnape commented Sep 28, 2016

jamiesnape commented Oct 3, 2016

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

sherm1 commented Oct 3, 2016

jamiesnape commented Oct 4, 2016

jamiesnape commented Oct 4, 2016

amcastro-tri commented Oct 4, 2016

SeanCurtis-TRI commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

sherm1 commented Oct 5, 2016

sherm1 commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

jamiesnape commented Oct 5, 2016

sherm1 commented Oct 5, 2016

jamiesnape commented Oct 10, 2016

jamiesnape commented Oct 12, 2016

amcastro-tri commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

jamiesnape commented Oct 21, 2016

amcastro-tri commented Oct 24, 2016

jamiesnape commented Oct 28, 2016

jamiesnape commented Oct 31, 2016

jwnimmer-tri commented Nov 28, 2016 • edited Loading

jamiesnape commented Nov 28, 2016

jamiesnape commented Dec 23, 2016 • edited Loading

jamiesnape commented Dec 23, 2016

sherm1 commented Dec 23, 2016

jamiesnape commented Sep 7, 2016 •

edited

Loading

jwnimmer-tri commented Nov 28, 2016 •

edited

Loading

jamiesnape commented Dec 23, 2016 •

edited

Loading