Addressing custom box-box intersection #259

SeanCurtis-TRI · 2018-02-21T20:05:59Z

This does several things:

Modifies the "correctness" of box-box intersection:
a. Addresses the interpretation of the contact position (as per
issue Interpretation of contact "position" needs to be unified. #258); the contact position lies between the two surfaces.
b. Correct the sign on the penetration depth so that colliding
objects report positive penetration depth.
Refactors box-box intersection code:
a. Removes redundant implementation to limit repeated bugs. This
encompasses completely redundant function implementations as well
as unnecessary duplication in branches.
b. Add additional documentation/todos on the implementation.
c. Replace the fudge2 parameter with something more reasoned.
Adds unit tests confirming the behavior.

Relates to #258

CI issues related to #260

NOTE: This changes the return values for collision queries between boxes. Code that currently relies on negative penetration depths and contact positions that lie on one box or the other will break.

This change is

sherm1 · 2018-02-22T05:56:25Z

Wow - I'm amazed at how much code you were able to get rid of! Nice!

Reviewed 1 of 3 files at r1.
Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h, line 792 at r1 (raw file):

  if(maxc < 1) maxc = 1;

  // The determination of these contact compuations are tested in:

BTW compuations -> computations

test/test_fcl_box_box.cpp, line 52 at r1 (raw file):

// Simple specification for defining a box collision object. Specifies the
// dimensions and pose of the box in some frame F (X_FB).

BTW consider referencing the Drake documentation that explains this notation: http://drake.mit.edu/doxygen_cxx/group__multibody__spatial__pose.html

test/test_fcl_box_box.cpp, line 72 at r1 (raw file):

   // must be a *cube* (all sides equal).
   BoxBoxTest(const BoxSpecification<S> &box_spec_1,
              const BoxSpecification<S> &box_spec_2)

BTW & placement?

Comments from Reviewable

sherm1 · 2018-02-22T05:58:07Z

CI troubles (test_fcl_geometric_shapes).

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

Comments from Reviewable

SeanCurtis-TRI · 2018-02-22T15:49:08Z

Thanks Sherm. Comments addressed and CI (ostensibly) corrected.

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

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h, line 792 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW compuations -> computations

Done

test/test_fcl_box_box.cpp, line 52 at r1 (raw file):

http://drake.mit.edu/doxygen_cxx/group__multibody__spatial__pose.html
Done

test/test_fcl_box_box.cpp, line 72 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW & placement?

Done

Comments from Reviewable

sherm1 · 2018-02-23T22:01:29Z

-- awesome! A few BTWs below.
FYI I talked f2f with Sean to understand how he was able to get rid of so much code. Turns out there was a complete 1:1 duplication of all the ugly box code for two different interfaces. That was easily replaced by having both interfaces invoke the same code after unpacking the arguments.

CI is still failing one test on Mac (box-capsule test which shouldn't have been affected by this PR). Sean just triggered CI for master via no-op PR #261 to see whether the same test fails without the present PR. If so, I propose that we merge this PR and treat the Mac problem separately as discussed in issue #260. If it does not fail then it should be investigated further first.

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

test/test_fcl_box_box.cpp, line 4 at r2 (raw file):

 * Software License Agreement (BSD License)
 *
 *  Copyright (c) 2014-2016, CNRS-LAAS and AIST

BTW copyright should be Toyota Research Institute?

test/test_fcl_box_box.cpp, line 249 at r2 (raw file):

//   Box 1: A cube with side length of 1, centered on the world origin (O) and
//          rotated 45 degrees around the y-axis.
//   Box 2: A cube with side length of 3, moved in the negative z-direction such

BTW text says 3, diagram says 2. I was confused by that (especially since the first box has a side labeled 1 with length 1) but now I think the 1 and 2 mean body 1, body 2 rather than dimensions. Please clarify (maybe put "Box1" "Box2" in the figure instead of the naked-and-ambiguous integers?).

test/test_fcl_geometric_shapes.cpp, line 687 at r2 (raw file):

//  the most deeply penetrating vertex on s2. The contact position will be
//  located at -z_max / 2 (with the assumption that all penetration happens
//  *below* the z = 0 axis.

FYI Wow! Thanks for adding this documentation. Hopefully it will serve as much-needed inspiration for other FCL contributors whose comments have tended to be ... um ... terse.

Comments from Reviewable

SeanCurtis-TRI · 2018-03-01T16:22:04Z

Comments addressed

Review status: 3 of 4 files reviewed at latest revision, 2 unresolved discussions.

test/test_fcl_box_box.cpp, line 4 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW copyright should be Toyota Research Institute?

Done

test/test_fcl_box_box.cpp, line 249 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW text says 3, diagram says 2. I was confused by that (especially since the first box has a side labeled 1 with length 1) but now I think the 1 and 2 mean body 1, body 2 rather than dimensions. Please clarify (maybe put "Box1" "Box2" in the figure instead of the naked-and-ambiguous integers?).

Done

Comments from Reviewable

sherm1 · 2018-03-01T17:20:20Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

test/test_fcl_box_box.cpp, line 4 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done

Bad news dude -- it's 2018 already.

Comments from Reviewable

SeanCurtis-TRI · 2018-03-01T17:26:30Z

Review status: 3 of 4 files reviewed at latest revision, all discussions resolved.

test/test_fcl_box_box.cpp, line 4 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Bad news dude -- it's 2018 already.

When did that happen?

Comments from Reviewable

scpeters · 2018-03-02T11:02:16Z

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h

+  // We are performing the mathematical test: 0 > 0 (which should always be
+  // false). However, zero can sometimes be 1e-16 or 1.5e-16. Thus,
+  // mathematically we would interpret a scenario as 0 > 0 but end up with
+  // 1e5e-16 > 1e-16. The former evaluates to false, the latter evaluates to


1e5e-16 -> 1.5e-16

scpeters · 2018-03-02T11:14:08Z

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h

-    // contacts.push_back(ContactPoint<S>(-normal, pointInWorld, -*depth));
-    contacts.emplace_back(normal, pb, -*depth);
+    Vector3<S> pointInWorld((pa + pb) * 0.5);
+    contacts.push_back(ContactPoint<S>(normal, pointInWorld, *depth));


I think @jslee02 switched from push_back to emplace_back in 3693e16

SeanCurtis-TRI · 2018-03-02T16:53:26Z

Final comments addressed.

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

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h, line 380 at r4 (raw file):

Previously, scpeters (Steven Peters) wrote…

1e5e-16 -> 1.5e-16

Done

include/fcl/narrowphase/detail/primitive_shape_algorithm/box_box-inl.h, line 614 at r4 (raw file):

Previously, scpeters (Steven Peters) wrote…

I think @jslee02 switched from push_back to emplace_back in 3693e16

Done

Comments from Reviewable

This does several things: 1. Modifies the "correctness" of box-box intersection: a. Addresses the interpretation of the contact position (as per issue flexible-collision-library#258); the contact position lies between the two surfaces. b. Correct the sign on the penetration depth so that colliding objects report positive penetration depth. 2. Refactors box-box intersection code: a. Removes redundant implementation to limit repeated bugs. This encompasses completely redundant function implementations as well as unnecessary duplicatio in branches. b. Add additional documentation/todos on the implementation. c. Replace the `fudge2` parameter with something more reasoned. 3. Adds unit tests confirming the behavior.

sherm1 · 2018-03-05T21:12:08Z

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

Comments from Reviewable

sherm1 · 2018-03-05T22:33:30Z

CI failure is just the known Mac problem -- will merge anyway.

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

Comments from Reviewable

SeanCurtis-TRI requested review from avalenzu and jslee02 February 21, 2018 20:06

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from d1af469 to 7805c35 Compare February 22, 2018 15:48

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch 5 times, most recently from b29b4fb to 0c73212 Compare February 23, 2018 16:02

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from 0c73212 to 3ac84ce Compare March 1, 2018 16:20

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch 3 times, most recently from fdf3f6e to 78bb30e Compare March 1, 2018 16:25

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from 78bb30e to 973f96e Compare March 1, 2018 17:26

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch 2 times, most recently from f942001 to f169fc3 Compare March 1, 2018 18:38

scpeters reviewed Mar 2, 2018

View reviewed changes

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from f169fc3 to 1acc54a Compare March 2, 2018 16:53

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from 1acc54a to 3a5f975 Compare March 2, 2018 18:08

SeanCurtis-TRI force-pushed the PR_box_box_collision_fix branch from 3a5f975 to 9463e5b Compare March 5, 2018 16:11

sherm1 merged commit 7c9f74a into flexible-collision-library:master Mar 5, 2018

SeanCurtis-TRI deleted the PR_box_box_collision_fix branch March 7, 2018 16:20

This was referenced Mar 9, 2018

Mac release failures -- box-box intersection #265

Closed

Replace non-portable M_PI reference with fcl::constants invocation #264

Merged

avalenzu mentioned this pull request May 11, 2018

FCL Integration Status RobotLocomotion/drake#7656

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addressing custom box-box intersection #259

Addressing custom box-box intersection #259

SeanCurtis-TRI commented Feb 21, 2018 •

edited

Loading

sherm1 commented Feb 22, 2018

sherm1 commented Feb 22, 2018

SeanCurtis-TRI commented Feb 22, 2018

sherm1 commented Feb 23, 2018

SeanCurtis-TRI commented Mar 1, 2018

sherm1 commented Mar 1, 2018

SeanCurtis-TRI commented Mar 1, 2018

scpeters Mar 2, 2018

scpeters Mar 2, 2018

SeanCurtis-TRI commented Mar 2, 2018

sherm1 commented Mar 5, 2018

sherm1 commented Mar 5, 2018

Addressing custom box-box intersection #259

Addressing custom box-box intersection #259

Conversation

SeanCurtis-TRI commented Feb 21, 2018 • edited Loading

sherm1 commented Feb 22, 2018

sherm1 commented Feb 22, 2018

SeanCurtis-TRI commented Feb 22, 2018

sherm1 commented Feb 23, 2018

SeanCurtis-TRI commented Mar 1, 2018

sherm1 commented Mar 1, 2018

SeanCurtis-TRI commented Mar 1, 2018

scpeters Mar 2, 2018

Choose a reason for hiding this comment

scpeters Mar 2, 2018

Choose a reason for hiding this comment

SeanCurtis-TRI commented Mar 2, 2018

sherm1 commented Mar 5, 2018

sherm1 commented Mar 5, 2018

SeanCurtis-TRI commented Feb 21, 2018 •

edited

Loading