Address EPA failure from issue 493

[SeanCurtis-TRI]

Address EPA failure from issue 493

• Incidentally, make the constants::eps* methods constexpr.
• Introduce two regression tests (the two posted in the issue).
• Correct a few confusing whitepsace formatting issues.
• Modify the validateNearestFeatureOfPolytopeBeingEdge() function in two ways
  • Distance from origin to face must now be greater than epsilon (instead of zero). Justificaiton for this change provided.
  • Test to determine if the edge truly is the nearest feature has been simplified and documentation updated. This led to the removal of the is_point_on_line_segment() function.

The eps constexpr changes and the EPA fixes are in two separate commits. Let me know if you'd like them broken into two PRs.

resolves #493

---

This change is 

[SeanCurtis-TRI]

Note to reviewer:

R1: changes to constants::fcl.
R2: Fix for issue in #493 (which makes use of constants::eps()).

Reviewable status: 0 of 7 files reviewed, all discussions resolved

[SeanCurtis-TRI]

Apparently std::pow is not constexpr in the clang available in CI. I've tweaked the constants::eps commit to accommodate this fact. Hopefully passing CI now.

R3 is part of R1.

Reviewable status: 0 of 6 files reviewed, all discussions resolved

[SeanCurtis-TRI]

Looks like CI is happy. +@hongkai-dai willing to feature review?

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @hongkai-dai)

[hongkai-dai]

+@sherm1 for platform review please, thanks!

Reviewed 2 of 6 files at r1, 3 of 3 files at r2, 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sherm1)

[SeanCurtis-TRI]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sherm1)

---

CHANGELOG.md, line 14 at r3 (raw file):

* Math

  * constants::eps (and variants) are now constexr:

nit: Forgot to update this when I removed the variants.

[hongkai-dai]

Reviewed 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @sherm1)

[sherm1]

Thanks, Sean! Platform with a few nits and a question.

Reviewed 2 of 6 files at r1, 2 of 3 files at r2, 2 of 2 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SeanCurtis-TRI)

---

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1702 at r4 (raw file):

    // We account for those differences by allowing a very small positive signed
    // distance to be considered zero. We assume that the GJK/EPA code
    // ultimately clasifies inside/outside around *zero* and *not* epsilon.

nit: clasifies -> classifies

---

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1703 at r4 (raw file):

    // distance to be considered zero. We assume that the GJK/EPA code
    // ultimately clasifies inside/outside around *zero* and *not* epsilon.
    if (origin_to_face_distance[i] > kEps) {

BTW this is a very tight tolerance (one roundoff bit). Is that enough?

---

test/test_fcl_signed_distance.cpp, line 487 at r4 (raw file):

  X_WB2.translation() << 2, 0, 1.25;
  const double expected_distance{0};
  test_distance_box_box_helper(box_size, X_WB1, box_size, X_WB2, &expected_distance);

nit: long line

---

test/test_fcl_signed_distance.cpp, line 514 at r4 (raw file):

  X_WB2.translation() << 0, 0, 1.25;
  const double expected_distance{0};
  test_distance_box_box_helper(box_size, X_WB1, box_size, X_WB2, &expected_distance);

nit: long line

[SeanCurtis-TRI]

Thanks for the review.

Reviewable status: 4 of 6 files reviewed, all discussions resolved (waiting on @hongkai-dai and @sherm1)

---

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1702 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: clasifies -> classifies

Done

---

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1703 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW this is a very tight tolerance (one roundoff bit). Is that enough?

Done

So, there are two issues here:

1. Is one bit enough for properly scaled problems?
2. What about badly scaled problems?

I directly addressed item 2. As for item 1, I'm going to suggest that all the tests pass, and that's a pretty good starting point. I'm loathe to dial it up without some specific rationale (theoretical or empirical).

---

test/test_fcl_signed_distance.cpp, line 487 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: long line

Done

---

test/test_fcl_signed_distance.cpp, line 514 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: long line

Done

[sherm1]

Reviewed 2 of 2 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

[sherm1]

Ready to merge -- I'll leave that to you @SeanCurtis-TRI in case you want to revise the long commit message upon squashing.

Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

Thanks. I'm running it through "other CI" to make sure that nothing is obviously hurt. Once that gets through, I'll merge here.

Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

Reviewers, I decided to update the test prior to merging based on changes due to reviewer comments and an additional failure case documented in the issue. PTAL.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @sherm1)

[sherm1]

Just to make sure -- did you verify that the new more-generic set of tests fails without the fix here?

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

Of course. :) I pulled the test onto master and confirmed failure.

Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

Blech...mac failure.

Reviewable status: complete! all files reviewed, all discussions resolved

[sherm1]

😠

Reviewable status: complete! all files reviewed, all discussions resolved

[SeanCurtis-TRI]

The mac failure was due to one particular case where the error was ever so slightly larger than epsilon. So, I bumped the value of epsilon by 1 bit and it made it happy on macsim. Let's see if that makes CI happy.

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @sherm1)

[SeanCurtis-TRI]

@sherm1 Can you take a look at the latest revision to address mac CI failure and let me know if you buy it?

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @sherm1)

[sherm1]

I'm happy with that, Sean. The 1-bit eps was making me nervous anyway -- that's a lot to ask out of a numerical calculation!

Reviewed 1 of 1 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved