ComputeVisiblePatch tracks all visited faces

[SeanCurtis-TRI]

The previous version of the code only tracked those faces that were explicitly known to be visible. This led to two problems:

1. A "hidden" face could be visited multiple times and the calculation to determine its visibility could be needlessly repeated.
2. The different visits could actually produce different results leading to inherently contradictory classifications of edges.

In this case, we track every face we've visited and lock its classification into the first classification computed.

Furthermore, we extend the sanity check to look for duplicate classifications and tweak documentation.

This bug was responsible for the intermittent failures in mac CI. They should now go away.

---

This change is 

[SeanCurtis-TRI]

+@hongkai-dai for feature review, please. (Assuming this passes CI, of course.)

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

[hongkai-dai]

just one minor comment.

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @hongkai-dai and @SeanCurtis-TRI)

---

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

    // g has not previously been classified as a visible face.
    if (hidden_faces->count(g) > 0) {
      // Also not classified as hidden; we need to classify the edge.

This face g is classified as hidden? Since f is visible, and g is hidden, hence the common edge is classified as a border edge.

---

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

    libccd_extension::ComputeVisiblePatchRecursive(
        polytope.polytope(), face, edge_index, new_vertex, &border_edges,
        &visible_faces, &hidden_faces, &internal_edges);

hidden face is not checked in CheckComputeVisiblePatchCommon. Not that we need to explicitly compare hidden_faces to a set of expected hidden faces, but just to make sure that the intersection between the visible_faces and hidden_faces is empty.

[SeanCurtis-TRI]

+@sherm1 for platform review, please.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @hongkai-dai and @sherm1)

---

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

Previously, hongkai-dai (Hongkai Dai) wrote…

This face g is classified as hidden? Since f is visible, and g is hidden, hence the common edge is classified as a border edge.

Done

---

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

Previously, hongkai-dai (Hongkai Dai) wrote…

hidden face is not checked in CheckComputeVisiblePatchCommon. Not that we need to explicitly compare hidden_faces to a set of expected hidden faces, but just to make sure that the intersection between the visible_faces and hidden_faces is empty.

Done

[hongkai-dai]

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

[sherm1]

Awesome work, Sean!
Platform pending a few minor doc requests.

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @SeanCurtis-TRI)

---

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

    FCL_THROW_FAILED_AT_THIS_CONFIGURATION(
        "An edge is being classified as border that has already been "
        "defined as internal");

nit: defined -> classified

---

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

    FCL_THROW_FAILED_AT_THIS_CONFIGURATION(
        "An edge is being classified as internal that has already been "
        "defined as border");

nit: defined -> classified

---

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

 */
static void ComputeVisiblePatchRecursive(
    const ccd_pt_t& polytope, ccd_pt_face_t& f, int edge_index,

minor: per f2f there is an invariant here that f is a visible face. Should make that clear in the documentation (and maybe assert that?).

---

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

      ClassifyBorderEdge(edge, border_edges, internal_edges);
      return;
    } else if (!isOutsidePolytopeFace(&polytope, g, &query_point)) {

minor: per f2f I was very surprised by the "!" here, leading to confusion in interpreting the code controlled by it until I backed up and re-parsed. I'm not suggesting fixing the oddly-defined function. But a comment at the call site mentioning the backwards semantics would be helpful. Also, if it is actually returning an int zero to mean true, would be easier to understand if written

// A zero return means "yes".
if (isOutsidePolytopeFace(...)  == 0)

to avoid implying that it has a sensible boolean return.

---

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

    // internal.
    ClassifyInternalEdge(edge, border_edges, internal_edges);
  }

BTW this code could be made a lot easier to follow by restructuring to get rid of the nested ifs. For example this little case where we know both f and g are visible could be at the top, with an early return, allowing undenting of the big chunk of code above it. And even that chunk has some early returns that could be exploited for easier readability. That restructuring would make it harder to review the changes here though -- maybe drop in a TODO suggesting it for some later cleanup?

---

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

  }

  // Note: this does not delete any vertices that were on the interior of the

nit: change to "any vertex that was" so that the "it"s below match in number.

[SeanCurtis-TRI]

Comments addressed (I've temporarily added a new commit in case mac CI gets angry at my refactor; I'll squash on merge.)

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @hongkai-dai and @sherm1)

---

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

Previously, sherm1 (Michael Sherman) wrote…

nit: defined -> classified

Done

---

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

Previously, sherm1 (Michael Sherman) wrote…

nit: defined -> classified

Done

---

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

Previously, sherm1 (Michael Sherman) wrote…

minor: per f2f there is an invariant here that f is a visible face. Should make that clear in the documentation (and maybe assert that?).

OK
In retrospect, I'm dismissing this. The statement: the parmeters are documented as in ComputeVisiblePatch() and that clearly defines the expectation of f being visible.

---

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

Previously, sherm1 (Michael Sherman) wrote…

minor: per f2f I was very surprised by the "!" here, leading to confusion in interpreting the code controlled by it until I backed up and re-parsed. I'm not suggesting fixing the oddly-defined function. But a comment at the call site mentioning the backwards semantics would be helpful. Also, if it is actually returning an int zero to mean true, would be easier to understand if written

// A zero return means "yes".
if (isOutsidePolytopeFace(...)  == 0)

to avoid implying that it has a sensible boolean return.

I guess I'm confused.

• It returns a bool. Why do you think int?
• Directly under the if statement was documentation explaining the case. I've modified the documentation to make the case clearer. PTAL.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW this code could be made a lot easier to follow by restructuring to get rid of the nested ifs. For example this little case where we know both f and g are visible could be at the top, with an early return, allowing undenting of the big chunk of code above it. And even that chunk has some early returns that could be exploited for easier readability. That restructuring would make it harder to review the changes here though -- maybe drop in a TODO suggesting it for some later cleanup?

I did a total refactor. PTAL.

---

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

Previously, sherm1 (Michael Sherman) wrote…

nit: change to "any vertex that was" so that the "it"s below match in number.

Done

I had a slight bias for the plurality, so I matched it up on the other side.

[sherm1]

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

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I guess I'm confused.

• It returns a bool. Why do you think int?
• Directly under the if statement was documentation explaining the case. I've modified the documentation to make the case clearer. PTAL.

Sorry, I didn't have a handy way to search FCL code and I lacked the imagination to think someone would misuse a bool that way! Your refactoring achieves the clarity I was looking for, thanks.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I did a total refactor. PTAL.

Sweet!

---

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

    ClassifyInternalEdge(edge, border_edges, internal_edges);
    return;
  } else if (is_hidden) {

BTW after an early return you don't actually need the else if. I think it is more readable to take advantage of that and avoid the nesting, but I think we talked about that before and you prefer it this way. Fine either way. FYI, I'd write it like this:

if (is_visible) {
  // ... do stuff
  return;
}
// At this point we know g is not visible.
if (is_hidden) {
}

[sherm1]

Ready for squash.

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

[SeanCurtis-TRI]

Slight tweak to the early exit.

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

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW after an early return you don't actually need the else if. I think it is more readable to take advantage of that and avoid the nesting, but I think we talked about that before and you prefer it this way. Fine either way. FYI, I'd write it like this:

if (is_visible) {
  // ... do stuff
  return;
}
// At this point we know g is not visible.
if (is_hidden) {
}

Done; I'm forgoing the comment because the example, as suggested, isn't accurate. And the accurate alternative doesn't contribute much.

[sherm1]

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

[hongkai-dai]

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