Remove duplicated code between GJKDistance and GJKSignedDistance

[hongkai-dai]

Resolves #289

---

This change is 

[sherm1]

Nice -- with a few BTWs, and pending CI.

---

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

---

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

  typedef S (*type)(const void*, const void*, const ccd_t*, ccd_vec3_t*,
                    ccd_vec3_t*);
};

BTW you don't really need a struct here. Consider:

template <typename S>
using SignedDistanceFn = 
  std::function<S(const void*, const void*, const ccd_t*, 
                  ccd_vec3_t*, ccd_vec3_t*)>;

You could also use the C-style function type declaration -- it's the "using" rather than "typedef" that allows you to templatize.

---

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

template <typename S>
bool GJKDistanceImp(

BTW more common to use Impl as an abbreviation for "implementation". (An "Imp" is a "small, mischievous devil".)

---

Comments from Reviewable

[SeanCurtis-TRI]

You have some unique CI issues.

1. Clang seemed to have failed in a meaningless way -- I retriggered those builds to see if they played out differently.
2. Apple is having build errors:

In file included from /Users/travis/build/flexible-collision-library/fcl/src/narrowphase/detail/convexity_based_algorithm/gjk_libccd.cpp:38:
/Users/travis/build/flexible-collision-library/fcl/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h:1788:10: error: no matching function for call to 'GJKDistanceImp'
  return internal::GJKDistanceImp(obj1, supp1, obj2, supp2, max_iterations,
         ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/travis/build/flexible-collision-library/fcl/src/narrowphase/detail/convexity_based_algorithm/gjk_libccd.cpp:104:6: note: in instantiation of function template specialization 'fcl::detail::GJKDistance<double>' requested here
bool GJKDistance(
     ^
/Users/travis/build/flexible-collision-library/fcl/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h:1749:6: note: candidate function [with S = double] not viable: no known conversion from 'ccd_real_t (const void *, const void *, const ccd_t *, ccd_vec3_t *, ccd_vec3_t *)' (aka 'float (const void *, const void *, const _ccd_t *, _ccd_vec3_t *, _ccd_vec3_t *)') to 'typename SignedDistanceFn<double>::type' (aka 'double (*)(const void *, const void *, const _ccd_t *, _ccd_vec3_t *, _ccd_vec3_t *)') for 7th argument
bool GJKDistanceImp(
     ^
/Users/travis/build/flexible-collision-library/fcl/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h:1800:10: error: no matching function for call to 'GJKDistanceImp'
  return internal::GJKDistanceImp(
         ^~~~~~~~~~~~~~~~~~~~~~~~
/Users/travis/build/flexible-collision-library/fcl/src/narrowphase/detail/convexity_based_algorithm/gjk_libccd.cpp:116:6: note: in instantiation of function template specialization 'fcl::detail::GJKSignedDistance<double>' requested here
bool GJKSignedDistance(
     ^
/Users/travis/build/flexible-collision-library/fcl/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h:1749:6: note: candidate function [with S = double] not viable: no known conversion from 'ccd_real_t (const void *, const void *, const ccd_t *, ccd_vec3_t *, ccd_vec3_t *)' (aka 'float (const void *, const void *, const _ccd_t *, _ccd_vec3_t *, _ccd_vec3_t *)') to 'typename SignedDistanceFn<double>::type' (aka 'double (*)(const void *, const void *, const _ccd_t *, _ccd_vec3_t *, _ccd_vec3_t *)') for 7th argument
bool GJKDistanceImp(

It appears related to the fact that the mac is using a 32-bit build.

---

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

---

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

}

namespace internal {

BTW FCL doesn't have any other instances of internal namespace; but it does use a detail namespace. I'd suggest swapping.

---

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

namespace internal {
template <typename S>
struct SignedDistanceFn {

BTW A couple of things:

1. However the function pointer is implemented, it needs to have some documentation. I'd particularly emphasize the semantics of the returned value -- should be positive for non-penetrating values and should be negative for penetrating -- although the magnitude of the negative value does not have to be meaningful beyond the sign.
2. The name is misleading -- it's not strictly-speaking a "signed distance function"; it's a distance function. Whether or not a particular implementation of this function returns a meaningful distance value in penetration is up to the caller. This nuance is what particulary calls for documenting this.

---

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

template <typename S>
struct SignedDistanceFn {
  typedef S (*type)(const void*, const void*, const ccd_t*, ccd_vec3_t*,

BTW Related to your mac CI failure, you probably need to have this return a ccd_real_t type instead of an S. (Which generally raises the question of "how autodiffable is this, really?")

---

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

};

template <typename S>

BTW As before, I'd ask for docs. :) Let's leave the code better than when we found it.

---

Comments from Reviewable

[SeanCurtis-TRI]

Clang builds finished -- original failure does seem to have been a transient execution problem. Now you just have the Mac issue.

---

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

---

Comments from Reviewable

[hongkai-dai]

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

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW FCL doesn't have any other instances of internal namespace; but it does use a detail namespace. I'd suggest swapping.

I tried to swap to detail namespace, caused huge amount of compilation errors. I think the reason is that this function is already in fcl::detail namespace. Without conflicting with this namespace, I think maybe internal namespace is more clear?

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW A couple of things:

1. However the function pointer is implemented, it needs to have some documentation. I'd particularly emphasize the semantics of the returned value -- should be positive for non-penetrating values and should be negative for penetrating -- although the magnitude of the negative value does not have to be meaningful beyond the sign.
2. The name is misleading -- it's not strictly-speaking a "signed distance function"; it's a distance function. Whether or not a particular implementation of this function returns a meaningful distance value in penetration is up to the caller. This nuance is what particulary calls for documenting this.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Related to your mac CI failure, you probably need to have this return a ccd_real_t type instead of an S. (Which generally raises the question of "how autodiffable is this, really?")

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW you don't really need a struct here. Consider:

template <typename S>
using SignedDistanceFn = 
  std::function<S(const void*, const void*, const ccd_t*, 
                  ccd_vec3_t*, ccd_vec3_t*)>;

You could also use the C-style function type declaration -- it's the "using" rather than "typedef" that allows you to templatize.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW As before, I'd ask for docs. :) Let's leave the code better than when we found it.

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW more common to use Impl as an abbreviation for "implementation". (An "Imp" is a "small, mischievous devil".)

Done.

---

Comments from Reviewable

[sherm1]

A few more comments ...

---

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

---

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

 * @param tolerance The tolerance used in GJK. When the change of distance is
 * smaller than this tolerance, the algorithm terminates.
 * @param[out] res The distance between the objects. When the two objects are

BTW res -> dist

---

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

template <typename S>
FCL_EXPORT
bool GJKDistance(void* obj1, ccd_support_fn supp1,

BTW what is the bool return value for? (Add an @return to the doc?) Also for the other signature.

---

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

 * @param tolerance The tolerance used in GJK. When the change of distance is
 * smaller than this tolerance, the algorithm terminates.
 * @param[out] res The distance between the objects. When the two objects are

BTW res -> dist

---

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

 * @param distance_func The actual function that computes the distance.
 * Different functions should be passed in, depending on whether the user wants
 * to compute a signed distance (with penetration depth) of not.

BTW "of not" -> "or not"

---

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

 * Different functions should be passed in, depending on whether the user wants
 * to compute a signed distance (with penetration depth) of not.
 * @param[out] res The distance between the objects. When the two objects are

BTW res is correct here but did you want to rename it dist to match the API?

---

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

 * negative distance depends on the implementation.
 * @param[out] p1 The closest point on object 1 in the world frame.
 * @param[out] p2 The closest point on object 2 in the world frame.

FYI I like the [out]s here. Since it isn't obvious from the function signature that the input parameters are actually inputs (e.g. void* obj1 rather than const void* obj1) consider marking the other parameters with [in].

(Also for the API methods in gjk_libccd.h.)

---

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

 * @param[out] p1 The closest point on object 1 in the world frame.
 * @param[out] p2 The closest point on object 2 in the world frame.
 */

BTW return bool needs documentation.

---

Comments from Reviewable

[sherm1]

Also looks like it needs a manual merge.

---

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

---

Comments from Reviewable

[hongkai-dai]

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

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW res -> dist

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW what is the bool return value for? (Add an @return to the doc?) Also for the other signature.

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW res -> dist

Done.

---

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

Previously, hongkai-dai (Hongkai Dai) wrote…

I tried to swap to detail namespace, caused huge amount of compilation errors. I think the reason is that this function is already in fcl::detail namespace. Without conflicting with this namespace, I think maybe internal namespace is more clear?

Removed the namespace.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW "of not" -> "or not"

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW res is correct here but did you want to rename it dist to match the API?

Inside the function it has a local variable also named asdist, so I think it is better not to change the variable name.

---

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

Previously, sherm1 (Michael Sherman) wrote…

FYI I like the [out]s here. Since it isn't obvious from the function signature that the input parameters are actually inputs (e.g. void* obj1 rather than const void* obj1) consider marking the other parameters with [in].

(Also for the API methods in gjk_libccd.h.)

Done.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW return bool needs documentation.

Done.

---

Comments from Reviewable

[sherm1]

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[SeanCurtis-TRI]

Few last comments

---

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

---

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

 * @param[out] dist The distance between the objects. When the two objects are
 * not colliding, this is the actual distance, a positive number. When the two
 * objects are colliding, it is the nagation of the penetration depth, a

BTW s/nagation/negation

---

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

// number when the object is colliding, though the meaning of that negative
// number depends on the implementation.
using DistanceFn = std::function<ccd_real_t(

BTW missing space ccd_real_t (

---

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

 * not colliding, this is the actual distance, a positive number. When the two
 * objects are colliding, it is a negative value. The actual meaning of the
 * negative distance depends on the implementation.

BTW I'd say the "actual meaning of the negative distance is defined by distance_func.

---

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

                 S* res, Vector3<S>* p1, Vector3<S>* p2) {
  return detail::GJKDistanceImpl(obj1, supp1, obj2, supp2, max_iterations,
                                  tolerance, libccd_extension::ccdGJKDist2,

BTW funky indentation

---

Comments from Reviewable

[hongkai-dai]

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

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW s/nagation/negation

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW missing space ccd_real_t (

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'd say the "actual meaning of the negative distance is defined by distance_func.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW funky indentation

Done.

---

Comments from Reviewable

[SeanCurtis-TRI]

We'll do one last reality check on the CI when it's done. But should be in a mergeable state.

---

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

---

Comments from Reviewable

[SeanCurtis-TRI]

CI confirmed; ready for merging. @sherm1

---

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

---

Comments from Reviewable