Update the convex-half space primitive collision algorithm

[SeanCurtis-TRI]

No primitive algorithm was registered for half space-convex intersection.
There was an existing function defined, but with an incompatible interface.
This introduces a new function (with compatible interface), fully documents
it, tests it, and connects it in as a registered primitive function.

There are also incidental formatting changes related to the effort of
writing the new primitive algorithm implementation.

Resolves #468

---

This change is 

[SeanCurtis-TRI]

+@DamrongGuoy for review, please.

Reviewable status: 0 of 8 files reviewed, all discussions resolved (waiting on @DamrongGuoy)

[DamrongGuoy]

Checkpoint. I'll try to finish reviewing test_half_space_convex.cpp tomorrow (Tuesday).

Reviewed 7 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 10 unresolved discussions (waiting on @DamrongGuoy and @SeanCurtis-TRI)

a discussion (no related file):
I'm not sure how to resolve this. If users call these two versions of convexHalfspaceIntersect with the same pair of intersecting Convex and Halfspace, would they get penetration_depth (first version) and ContactPoint::penetration_depth (second version) with opposite signs?

template <typename S>
FCL_EXPORT
bool convexHalfspaceIntersect(const Convex<S>& s1, const Transform3<S>& tf1,
                              const Halfspace<S>& s2, const Transform3<S>& tf2,
                              Vector3<S>* contact_points, S* penetration_depth, Vector3<S>* normal);

template <typename S>
FCL_EXPORT bool convexHalfspaceIntersect(
    const Convex<S>& convex_C, const Transform3<S>& X_FC,
    const Halfspace<S>& half_space_H, const Transform3<S>& X_FH,
    std::vector<ContactPoint<S>>* contacts);

The (older) first version mistakenly returns a negative number for penetration depth. The (newer) second version returns the correct positive number for penetration depth. (I am assuming that by defintion, penetration depth is positive or zero. I cannot find its definition in Wikipedia, which has it for electromagnetic radiation only. https://en.wikipedia.org/wiki/Penetration_depth)

Perhaps some kinds of function documentation or deprecation to warn users in fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h ?

---

---

include/fcl/narrowphase/detail/gjk_solver_indep-inl.h, line 206 at r1 (raw file):

// +------------+-----+--------+-----------+---------+------+----------+-------+------------+----------+----------+
// | convex     |/////|////////|///////////|/////////|//////|//////////|///////|////////////|//////////|          |
// +------------+-----+--------+-----------+---------+------+----------+-------+------------+----------+----------+

BTW, what do "//////", "<blank>", and "O" mean?

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 36 at r1 (raw file):

 */

/** @author Jia Pan */

BTW, I wonder whether you should add your name here? It looks like this is the first time we added our notations (X_FC) into this file.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 134 at r1 (raw file):

///
/// If the two geometries are intersecting and `contacts` is not `nullptr`, the
/// new ContactPoint.normal will be antiparallel to the half space normal

BTW, I feel like "opposite" would sound easier than "antiparallel". It's just my feeling that "antiparallel" makes me think in two steps: "anti" and "parallel".

No need to change it.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 134 at r1 (raw file):

///
/// If the two geometries are intersecting and `contacts` is not `nullptr`, the
/// new ContactPoint.normal will be antiparallel to the half space normal

BTW, I checked struct ContactPoint. It said:

  /// @brief Contact normal, pointing from o1 to o2
  Vector3<S> normal;

I'm not sure what from o1 to o2 means and whether it's consistent with ContactPoint.normal will be antiparallel to the half space normal here.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r1 (raw file):

/// all such points. The ContactPoint.penetration_depth value is the depth of P.
/// ContactPoint.pos is defined as the point between P and the nearest point on
/// the boundary of the half space, measured and exppressed in F.

nit: s/exppressed/expressed.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r1 (raw file):

/// all such points. The ContactPoint.penetration_depth value is the depth of P.
/// ContactPoint.pos is defined as the point between P and the nearest point on
/// the boundary of the half space, measured and exppressed in F.

In struct ContactPoint it said:

  /// @brief Contact position, in world space
  Vector3<S> pos;

Does world space mean World frame? Or we should ignore the doc in ContactPoint since we explicitly say it's in frame F here.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 156 at r1 (raw file):

    const Convex<S>& convex_C, const Transform3<S>& X_FC,
    const Halfspace<S>& half_space_H, const Transform3<S>& X_FH,
    std::vector<ContactPoint<S>>* contacts);

BTW, do we want frame notation in contacts like contacts_F ? Or it's not necessary?

---

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

 */

/** @author Jia Pan */

BTW, should you add your name here?

---

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

                              Vector3<S>* contact_points, S* penetration_depth, Vector3<S>* normal)
{
// TODO(SeanCurtis-TRI): This is generally unused in FCL. Consider killing it.

BTW, I think this TODO is for the entire function not just the following line, right? How about moving this TODO to six lines above, just before the function definition?

---

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

  Vector3<S> v;
  S depth = std::numeric_limits<S>::max();

BTW, should we change the name depth to signed_distance ?

---

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

    // TODO(SeanCurtis-TRI): This appears to have the wrong sign for depth.
    //  We've actually computed *signed distance* which is -depth.
    if(penetration_depth) *penetration_depth = depth;

Do we have documentation that defines penetration_depth somewhere? I can't judge whether it should be positive or negative number for sure.

---

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

  Halfspace<S> half_space_C = transform(half_space_H, X_FC.inverse() * X_FH);

  Vector3<S> p_CV_deepest;

BTW, just a sanity check. We have a doc somewhere saying that we will make FCL using frame notation, right? I can imagine someone unfamiliar with frames would scratch their heads here.

---

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

  Vector3<S> p_CV_deepest;
  S minimum_distance = std::numeric_limits<S>::max();

BTW, would min_signed_distance be better? I like the signed because it clearly allows negative number. Otherwise, distance alone sounds to me like only zero+positive number is allowed: https://en.wikipedia.org/wiki/Distance#Distance_in_Euclidean_space

---

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

    const Vector3<S> normal_F = X_FH.linear() * half_space_H.n;
    const Vector3<S> p_FV = X_FC * p_CV_deepest;
    // NOTE: penetration depth is defined as negative signed distance.

BTW, how about this?

// NOTE: Here penetration depth is defined as negative of signed distance,
// so the penetration depth is positive or zero.

penetration depth is negative signed distance sounds to me like penetration depth is a negative number.

Here the signed distance is negative or zero, but we want the penetration depth to be positive or zero, correct?

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 4 at r1 (raw file):

 * Software License Agreement (BSD License)
 *
 *  Copyright (c) 2018. Toyota Research Institute

I know little about Copyright. Is it supposed to be 2020 this year? Or 2018 when TRI takes over FCL perhaps?

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 37 at r1 (raw file):

/** @author Sean Curtis (sean@tri.global) (2020) */

// Tests the custom half space-convex collision test.

nit: would "halfspace-convex collision test" sound better? As it is, it sounds like "half" of "space-convex collision".

I'll be happy with using the class names: "Halfspace-Convex collision test" too.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 65 at r1 (raw file):

/*
 Creates a unit tetrahedron as a Convex geometry.

I cannot find a unit tetrahedron in Wikipedia. How bout we just say a tetrahedron and let the picture says the rest?

The closest term I found is trirectangular tetrahedron https://en.wikipedia.org/wiki/Trirectangular_tetrahedron

In case it's relevant, a simple example of a regular tetrahedron (all edges of the same length, all faces as equilateral triangles) is:

(0,0,0),
(1,0,1),
(1,1,0),
(0,1,1)

All the six edges has length sqrt(2).

[DamrongGuoy]

with some questions.

Reviewed 1 of 8 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @SeanCurtis-TRI)

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 112 at r1 (raw file):

 A Convex shape (the tetrahedron returned by MakeTetrahedron) is posed in
 the half space frame H (such that the half space lies on the Hz = 0 plane
 with the normal pointing in the Hz direction). */

BTW, I kept confusing myself about the letter T and C. I know that T is for tetrahedron, but I kept mistaking C for Convex because the name of this file has convex in it. How about we prepare the readers here for the names of the frames like this?

These are the names of the frames that we use:
C = frame of the configuration
H = frame of the half space
T = frame of the tetrahedron

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 190 at r1 (raw file):

  // and submit *two* configurations: one in collision, one not.

  Transform3<S> X_CH =

nit: const ?

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 200 at r1 (raw file):

  configurations.emplace_back(
      "non-trivial half space, non-colliding",
      X_CH * MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(1.01)}), X_CH);

BTW, could we have the MakeTransform into this variable?

const Transform3<T> X_HT = MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(1.01)});

Then, this line could become:

X_CH * X_HT, X_CH);

Previously I was confused by X_CH * MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(1.01)}).

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 263 at r1 (raw file):

    // For a given configuration, we can set it up two ways:
    // X_CT and X_CH multiply mesh_T and half_space_H, respectively, or
    // identity multiplies mesh_C and half_space_C.

BTW, should mesh_T, mesh_C be convex_T, convex_C ?

[SeanCurtis-TRI]

Thanks for the careful review.

+@sherm1 for platform review, please.

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @DamrongGuoy and @sherm1)

a discussion (no related file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I'm not sure how to resolve this. If users call these two versions of convexHalfspaceIntersect with the same pair of intersecting Convex and Halfspace, would they get penetration_depth (first version) and ContactPoint::penetration_depth (second version) with opposite signs?

template <typename S>
FCL_EXPORT
bool convexHalfspaceIntersect(const Convex<S>& s1, const Transform3<S>& tf1,
                              const Halfspace<S>& s2, const Transform3<S>& tf2,
                              Vector3<S>* contact_points, S* penetration_depth, Vector3<S>* normal);

template <typename S>
FCL_EXPORT bool convexHalfspaceIntersect(
    const Convex<S>& convex_C, const Transform3<S>& X_FC,
    const Halfspace<S>& half_space_H, const Transform3<S>& X_FH,
    std::vector<ContactPoint<S>>* contacts);

The (older) first version mistakenly returns a negative number for penetration depth. The (newer) second version returns the correct positive number for penetration depth. (I am assuming that by defintion, penetration depth is positive or zero. I cannot find its definition in Wikipedia, which has it for electromagnetic radiation only. https://en.wikipedia.org/wiki/Penetration_depth)

Perhaps some kinds of function documentation or deprecation to warn users in fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h ?

You can put your mind at ease. This is in the detail namespace and is not intended for users to call. FCL seems to have lots of zombie code -- code that isn't correct but doesn't get called (or built now that it's templated). So, I've commented issues with the existing, unused function. It's still not exercised. Removing it is an exercise for some day in the future.

As for the definition of penetration (does it include zero or not?) there is no authoritative reference. There's not even really a clear piece of documentaiton in FCL about what it means and we occasionally find some code that doesn't line up with the others. So, as with the previous, this is too large a problem to tackle in this PR.

---

---

include/fcl/narrowphase/detail/gjk_solver_indep-inl.h, line 206 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, what do "//////", "<blank>", and "O" mean?

I infer from what was there before:

• ////// simply means no entry (because the table is symmetric over the diagonal axis, values there would be redundant.
• O means supported
• empty means not supported.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 36 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I wonder whether you should add your name here? It looks like this is the first time we added our notations (X_FC) into this file.

I don't worry about that -- git blame will tell people I worked on it. (If they care.)

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 134 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I feel like "opposite" would sound easier than "antiparallel". It's just my feeling that "antiparallel" makes me think in two steps: "anti" and "parallel".

No need to change it.

I like "opposite". Thanks for the suggestion.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 134 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I checked struct ContactPoint. It said:

  /// @brief Contact normal, pointing from o1 to o2
  Vector3<S> normal;

I'm not sure what from o1 to o2 means and whether it's consistent with ContactPoint.normal will be antiparallel to the half space normal here.

A good point. I didn't look at that. I looked at the implementation for sphereHalfspaceIntersect in the halfspace-inl.h file. It had the normal pointing inside the half space, so I copied that.

In this case, o1 and o2 obliquely refer to the convex and half space, respectively. So, pointing opposite the half space normal is pointing into the half space (o2) and out of the convex (o1).

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

nit: s/exppressed/expressed.

Done

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

In struct ContactPoint it said:

  /// @brief Contact position, in world space
  Vector3<S> pos;

Does world space mean World frame? Or we should ignore the doc in ContactPoint since we explicitly say it's in frame F here.

This function knows nothing about the semantics of frame F. If it is invoked with F = W, then ContactPoint documentation is satisfied. So, as long as it is being executed with X_WC and X_WH, pos will be in W.

Just chalk that up to another weakness in the documentation.

(Anecdotally, I've tested this in Drake and it produces the correct results.)

I've added a note underscoring what you've pointed out.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 156 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, do we want frame notation in contacts like contacts_F ? Or it's not necessary?

I liked the suggestion and started making the change, but because contacts is an accumulator, this function can't guarantee what frame the other entries are recorded in. So, I'm going to leave it painfully generic.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, should you add your name here?

See previous note.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I think this TODO is for the entire function not just the following line, right? How about moving this TODO to six lines above, just before the function definition?

Done

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, should we change the name depth to signed_distance ?

My goal was not to manipulate the implementaiton of the unused method.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

Do we have documentation that defines penetration_depth somewhere? I can't judge whether it should be positive or negative number for sure.

See previous note.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, just a sanity check. We have a doc somewhere saying that we will make FCL using frame notation, right? I can imagine someone unfamiliar with frames would scratch their heads here.

Great point.

The frame notation is gradually spreading throughout FCL. (sphere_box, sphere_cylinder, sphere_box, lots of tests). I've added the same comment we've placed elsewhere.

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, would min_signed_distance be better? I like the signed because it clearly allows negative number. Otherwise, distance alone sounds to me like only zero+positive number is allowed: https://en.wikipedia.org/wiki/Distance#Distance_in_Euclidean_space

Done

---

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

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, how about this?

// NOTE: Here penetration depth is defined as negative of signed distance,
// so the penetration depth is positive or zero.

penetration depth is negative signed distance sounds to me like penetration depth is a negative number.

Here the signed distance is negative or zero, but we want the penetration depth to be positive or zero, correct?

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 4 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I know little about Copyright. Is it supposed to be 2020 this year? Or 2018 when TRI takes over FCL perhaps?

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 37 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

nit: would "halfspace-convex collision test" sound better? As it is, it sounds like "half" of "space-convex collision".

I'll be happy with using the class names: "Halfspace-Convex collision test" too.

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 65 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I cannot find a unit tetrahedron in Wikipedia. How bout we just say a tetrahedron and let the picture says the rest?

The closest term I found is trirectangular tetrahedron https://en.wikipedia.org/wiki/Trirectangular_tetrahedron

In case it's relevant, a simple example of a regular tetrahedron (all edges of the same length, all faces as equilateral triangles) is:

(0,0,0),
(1,0,1),
(1,1,0),
(0,1,1)

All the six edges has length sqrt(2).

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 112 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, I kept confusing myself about the letter T and C. I know that T is for tetrahedron, but I kept mistaking C for Convex because the name of this file has convex in it. How about we prepare the readers here for the names of the frames like this?

These are the names of the frames that we use:
C = frame of the configuration
H = frame of the half space
T = frame of the tetrahedron

Done

Your confusion was justified -- the test (and the frames) evolved but I didn't clean up all of the documentation. What you propose was what the final intent of this file was...there were just some old relics I have now removed.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 190 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

nit: const ?

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 200 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, could we have the MakeTransform into this variable?

const Transform3<T> X_HT = MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(1.01)});

Then, this line could become:

X_CH * X_HT, X_CH);

Previously I was confused by X_CH * MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(1.01)}).

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 263 at r1 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

BTW, should mesh_T, mesh_C be convex_T, convex_C ?

Done

[DamrongGuoy]

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

---

include/fcl/narrowphase/detail/gjk_solver_indep-inl.h, line 206 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I infer from what was there before:

• ////// simply means no entry (because the table is symmetric over the diagonal axis, values there would be redundant.
• O means supported
• empty means not supported.

Resolved.

I would feel ✔️ is better than O, which looks like 0 percent or 0 score or fail. On the other hand, in Japan, the red O means true, and the blue x means false:

[DamrongGuoy]

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

[DamrongGuoy]

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

[sherm1]

Very nice. Super readable. Platform with a few nits.

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

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r4 (raw file):

/// it will be arbitrarily selected from the set of all such points. The
/// ContactPoint.penetration_depth value is the depth of P. ContactPoint.pos is
/// defined as the point between P and the nearest point on the boundary of the

minor: "the point between" -- should that be "halfway between"? There are lots of points between P and the halfspace surface.

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 158 at r4 (raw file):

///                      boundary plane are measured and expressed in Frame H.
/// @param X_FH          The transform relating Frame H with the common frame F.
/// @param[out] contacts The optional accumulator for data characterizing the

nit: consider [in] for the other parameters since you did [out] here.

---

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

  //  That would also make computing normal_C cheaper; it could just be the
  //  product: X_FC.linear().transpose() * X_FH.linear() * half_space_H.n.
  for (const auto& p_CV : convex_C.getVertices()) {

BTW ouch! painful to have to visit all the vertices.
This would be an excellent small project for someone. Although if the convex mesh doesn't have edges represented I guess it wouldn't be all that small! Consider filing an FCL issue if there isn't one already.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 82 at r4 (raw file):
minor: consider

The tet is then posed in frame T, and the final mesh is produced with its vertices measured and expressed in frame T.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 104 at r4 (raw file):

  add_triangle(0, 1, 3);  // face on xz plane.
  add_triangle(0, 3, 2);  // face on yz plane.
  add_triangle(2, 3, 1);  // diagonal face.

minor: is there any significance to the vertex ordering here? Please add a comment saying whether or not it matters.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 141 at r4 (raw file):

        expected_pos_C(pos_C) {}

  string name;

nit: consider pose_name or test_name ? Or maybe label to match other parts of the test below?

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 203 at r4 (raw file):

      "non-trivial half space, non-colliding", X_CH * X_HT_separated, X_CH});

  // We pose the tet relative to the half plane, and the transform it again into

nit "the transform" -> "then transform"

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 208 at r4 (raw file):

  const Transform3<S> X_HT_colliding =
      MakeTransform(R_HT_point_down, Vector3<S>{0, 0, S(0.5)});
  const Transform3<S> X_CT_collding = X_CH * X_HT_colliding;

nit: did you want X_CT_colliding rather than collding?

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 234 at r4 (raw file):

  EXPECT_EQ(colliding, config.expected_colliding) << label;
  if (config.expected_colliding) {
    EXPECT_EQ(results_W.size(), 1u) << label;

nit: you don't need the "u" here -- better to leave it off IMO

[DamrongGuoy]

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

[DamrongGuoy]

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

[SeanCurtis-TRI]

Thanks for the comments.

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

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 140 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: "the point between" -- should that be "halfway between"? There are lots of points between P and the halfspace surface.

Done

---

include/fcl/narrowphase/detail/primitive_shape_algorithm/halfspace.h, line 158 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: consider [in] for the other parameters since you did [out] here.

OK

I'm going to exercise my own bias here. [in] is generally implied. As such, adding it feels redundant. [out] provides new information. Even the thought that "forgetting to add an tag an out parameter" looks just like "relying on implicit encoding of an in parameter", I prefer the less cluttered look this way. If you feel more strongly, let me know.

---

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

Previously, sherm1 (Michael Sherman) wrote…

BTW ouch! painful to have to visit all the vertices.
This would be an excellent small project for someone. Although if the convex mesh doesn't have edges represented I guess it wouldn't be all that small! Consider filing an FCL issue if there isn't one already.

In fact, that's one of my OKRs this quarter. The collision with Convex is painfully slow and is hurting TRI users. So, fixing that will improve this as well.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 82 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: consider

The tet is then posed in frame T, and the final mesh is produced with its vertices measured and expressed in frame T.

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 104 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: is there any significance to the vertex ordering here? Please add a comment saying whether or not it matters.

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 141 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: consider pose_name or test_name ? Or maybe label to match other parts of the test below?

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 203 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit "the transform" -> "then transform"

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 208 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: did you want X_CT_colliding rather than collding?

Done

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 234 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

nit: you don't need the "u" here -- better to leave it off IMO

OK

Actually, You do. This is why I keep picking up the habit. FCL uses an older version of gtest and omitting the u is one of the causes of windows CI failure.

[sherm1]

One typo left.

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 234 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

OK

Actually, You do. This is why I keep picking up the habit. FCL uses an older version of gtest and omitting the u is one of the causes of windows CI failure.

Oh, yuck. TIL, thanks.

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 82 at r5 (raw file):

  The tet is defined in geometry frame G, shown above with vertex positions
  marked. The test is then posed in frame T, and the final convex mesh is

minor: test -> tet

[SeanCurtis-TRI]

Let's see if I can avoid introducing a typo into the typo correction.

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

---

test/narrowphase/detail/primitive_shape_algorithm/test_half_space_convex.cpp, line 82 at r5 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

minor: test -> tet

Done

[sherm1]

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