Resolves the error in EPA when the query point is colinear with an edge #417

hongkai-dai · 2019-09-07T00:38:42Z

Resolves #415

This change is

…be visible

sherm1 · 2019-09-09T17:48:50Z

Is this ready for a reviewer, @hongkai-dai ?

SeanCurtis-TRI

+@SeanCurtis-TRI checkpoint -- having local reproduction issues.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hongkai-dai and @SeanCurtis-TRI)

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 696 at r1 (raw file):

                                               double rho) {
  // A new vertex is colinear with an edge e[0] of the tetrahedron. The border
  // edges should be  e[1], e[4], e[5]. The visible faces should be f[0], f[1],

nit: extra space between "be" and "e[1]".

SeanCurtis-TRI

It's perfectly happy on my laptop; everything behaves as one would expect. :-/ I've made some documentation/superficial requests for changes.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @hongkai-dai)

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1189 at r1 (raw file):
Based on our (now two) conversation(s), I'd like to see this comment greatly expanded reflecting some of the subtleties and nuance we've discussed. State the problem, state the choice for resolving it and the reasons/implications of that choice. For example:

For this triangle to have no area, the query point must be co-linear with a candidate border edge. That means it is simultaneously co-planar with the two faces adjacent to that edge. But to be in this branch, one face was considered to be visible and the other to not be visible -- an inconsistent classification.

The solution is to unify that classification. We can consider both faces as being visible or not. If we were to consider them not visible, we would shrink the size of the visible patch (making this slightly faster), but would risk introducing co-planar faces into the polytope. We choose to consider both faces as being visible. At the cost of a patch boundary with more edges, we guarantee that we don't add co-planar faces.

It may be that co-planar faces are permissible and a smaller patch is preferred. This is still an open problem. For now, we go with the "safe" choice.

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

      // If the query point is colinear with the edge, then both neighbouring
      // faces of this edge (namely face f and g) should be visible. This edge
      // is an internal edge. Othewise, this edge is a border edge.

nit: If you choose to keep some variant of this comment in place of my suggestion, you have a typo here on "Othewise".

test/test_fcl_signed_distance.cpp, line 319 at r1 (raw file):

  fcl::DistanceResult<S> result;

  fcl::distance(&box1, &box2, request, result);

nit: Any particular reason we removed the ASSERT_NO_THROW? None of the changes in this file suggested that is desirable.

test/test_fcl_signed_distance.cpp, line 430 at r1 (raw file):

}

GTEST_TEST(FCL_SIGNED_DISTANCE, box_box1_ccd) {

BTW This test name has clearly grown stale.

Perhaps it would be better to rename and document:

// A collection of scenarios observed in practice that have created error 
// conditions in previous commits of the code. Each test is a unique instance.
GTEST_TEST(FCL_SIGNED_DISTANCE, RealWorldRegression) {

And, perhaps, corresponding renamings of test_distance_box_box* to `test_distance_box_box_regression*'.

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 693 at r1 (raw file):
I'd recommend documenting this function:

Given an equilateral tetrahedron, create a query point that is co-linear with edge 0 as q = v₀ + ρ(v₀ - v₁), confirms that the correct tetrahedra faces are included in the visible patch. Point q is co-linear with edge 0 which is adjacent to faces f0 and f1. Face f3 is trivially visible from q.

If the query point is co-linear with a tet edge, then both adjacent faces should be visible. The behavior is sensitive to numerical accuracy issues and we expose rho (ρ) as a parameter so that different scenarios can easily be authored which exercise different code paths to determine visibility. (In the code, "visibility" may be determined by multiple tests.)

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 721 at r1 (raw file):

Case 1: Visibility of faces f0 and f1 is not immediately apparent -- requires recognition that q, v0, and v1 are colinear.

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 723 at r1 (raw file):

Case 2: Visibility of faces f0 and f1 are indpendently confirmed -- colinearity doesn't matter.

hongkai-dai

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @SeanCurtis-TRI)

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