Clean up Convex geometry (including fixing AABB computation error)

[SeanCurtis-TRI]

The Convex geometry had an error in computing its local AABB. This fixes that error.

It includes a unit test that exposes the error without the fix and validates the fix.

This also, incidentally, cleans up some of the documentation.

NOTE: The unit tests on the Convex geometry are incomplete. This is noted in the new tests.
replaces #250 (this is part of that PR).

resolves #248

---

This change is 

[Levi-Armstrong]

This was one thing I was concerned about. It is possible to have several triangles in the convex mesh that lie on a single plane. So based on this description a user must loop through a mesh and combine all triangle that are on a given plane in to a single polytope. Do I understand this correctly?

[Levi-Armstrong]

Missing brief description

[Levi-Armstrong]

@SeanCurtis-TRI I built everything using this PR along with #288 and all of my test now pass. 👍

[SeanCurtis-TRI]

Thanks for looking. I'll patch up the comments first thing in the morning.

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 67 at r1 (raw file):

Previously, Levi-Armstrong (Levi Armstrong) wrote…

This was one thing I was concerned about. It is possible to have several triangles in the convex mesh that lie on a single plane. So based on this description a user must loop through a mesh and combine all triangle that are on a given plane in to a single polytope. Do I understand this correctly?

Several thoughts:

1. You have the spirit right (but I'd tweak the terminology a bit). If you have multiple triangles that lie on the same plane, they should all merge together into one convex face. If they don't, then you don't truly have a convex geometry.
2. I made some initial assumptions about how it handles redundant planes. When i get through the unit tests for volume, center of mass, etc., I'll confirm it. I also have to look into the various support functionality for GJK/MPR.

---

include/fcl/geometry/shape/convex.h, line 103 at r1 (raw file):

Previously, Levi-Armstrong (Levi Armstrong) wrote…

Missing brief description

Ooops...I'll fix this tomorrow.

[sherm1]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @Levi-Armstrong)

---

include/fcl/geometry/shape/convex.h, line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Several thoughts:

1. You have the spirit right (but I'd tweak the terminology a bit). If you have multiple triangles that lie on the same plane, they should all merge together into one convex face. If they don't, then you don't truly have a convex geometry.
2. I made some initial assumptions about how it handles redundant planes. When i get through the unit tests for volume, center of mass, etc., I'll confirm it. I also have to look into the various support functionality for GJK/MPR.

A "no parallel triangles" rule seems to leave out some very simple convex shapes -- like a box! Each face of a box needs two co-planar triangles to represent its quad face. It seems unreasonable to prohibit that, or am I misunderstanding the restriction here?

[SeanCurtis-TRI]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @Levi-Armstrong)

---

include/fcl/geometry/shape/convex.h, line 67 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

A "no parallel triangles" rule seems to leave out some very simple convex shapes -- like a box! Each face of a box needs two co-planar triangles to represent its quad face. It seems unreasonable to prohibit that, or am I misunderstanding the restriction here?

This representation seems to allow one convex polygon per plane. The polytope's surface should not be triangulated. So, box's are naturally represented as squares associated with each plane.

[SeanCurtis-TRI]

+@sherm1 for review, please

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @Levi-Armstrong)

[sherm1]

Checkpoint.

Reviewed 5 of 6 files at r1.
Reviewable status: 5 of 6 files reviewed, 11 unresolved discussions (waiting on @Levi-Armstrong, @SeanCurtis-TRI, and @sherm1)

---

include/fcl/geometry/shape/convex.h, line 67 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This representation seems to allow one convex polygon per plane. The polytope's surface should not be triangulated. So, box's are naturally represented as squares associated with each plane.

Per f2f and above, not clear to me that this is a convenient form for a robotics user nor that it is a necessary restriction here. But also not clear that I'm right!

---

include/fcl/geometry/shape/convex.h, line 36 at r2 (raw file):

 */

/** @author Jia Pan */

BTW consider adding something like

@author Sean Curtis (2018) Added documentation of existing code.

or whatever.

---

include/fcl/geometry/shape/convex.h, line 65 at r2 (raw file):

  /// `Π(x, y, z) = Ax + By + Cz + d = 0` where `n̂ = [A, B, C]` and |n̂| = 1.
  /// The normal points *outside* of the convex region. There should be *no*
  /// redundant planes in the set.

Per f2f it doesn't seem obvious to me that this condition is required. The code seems only to require that there be a plane defined for every polygon. But, if this is a real restriction then it is a strict-but-unenforced prerequisite of this constructor with a serious implication for a user who starts out with a tri-mesh: they must combine coplanar faces into larger polygons. If that is a requirement I think it needs to be spelled out since it is very common (in robotics anyway) for geometry to be supplied in the form of a tri mesh.

Might be worth a TODO here to figure out whether the shape is primarily defined as

• an intersection of non-redundant half planes, with a redundant data structure containing precalculated intersection points, OR
• as a collection of vertices forming planar faces that bound the polytope, with a redundant data structure providing precalculated planes for each face.

The latter interpretation would permit convex trimeshes to be used to create a Convex object, even if there were coplanar faces. The former would not.

My opinion: we should allow any convex trimesh if we can, since that's what our target users are likely to have.

---

include/fcl/geometry/shape/convex.h, line 84 at r2 (raw file):

  /// @param num_points     The number of vertices in `points`.
  /// @param polygons       Encoding of the faces for each plane. See member
  ///                       documentation for details on encoding.

Minor: there must be m of these also - consider saying that here.

---

include/fcl/geometry/shape/convex.h, line 120 at r2 (raw file):

  ///    `rᵢ × rᵢ₊₁ · n̂ₚ = |rᵢ × rᵢ₊₁|, ∀ 0 ≤ i < m, i ∈ ℤ`, where
  ///    `n̂ₚ` is the normal for plane `p` on which the face `f` lies.
  ///    `rᵢ = points[vᵢ] - points[vᵢ₋₁]` is the displacement of the edge of

Nit: "displacement vector"

---

include/fcl/geometry/shape/convex.h, line 128 at r2 (raw file):

  ///    2. The nᵗʰ encoded polygon corresponds with the nᵗʰ plane normal,
  ///    3. The indices of the face correspond to a proper counter-clockwise
  ///       ordering.

Is it OK if adjacent edges are parallel? That still satisfies the cross product condition you wrote above, but in an annoyingly trivial way.

---

include/fcl/geometry/shape/convex.h, line 147 at r2 (raw file):

  /// note: The name "center" is misleading; it is neither the centroid nor the
  /// center of mass.
  Vector3<S> center;

Minor: any hope that we could rename this mean_point? This name seems particularly awful since there really is a method that returns the centroid for this same object.

---

include/fcl/geometry/shape/convex-inl.h, line 62 at r2 (raw file):

  points = points_in;
  num_points = num_points_in;
  polygons = polygons_in;

OMG! Just for the record it is worth mentioning (per f2f) that this code NEVER WORKED because the assignments used the same names on either side of the assignment operator, meaning the arguments got ignored :(

---

include/fcl/geometry/shape/convex-inl.h, line 65 at r2 (raw file):

  edges = nullptr;

  // Compute an interior point: the mean vertex (we're calling it "center").

BTW if we're stuck with this name I think it's worth mentioning again here that this is not the centroid.

---

include/fcl/geometry/shape/convex-inl.h, line 102 at r2 (raw file):

{
  this->aabb_local.min_.setConstant(std::numeric_limits<S>::max());
  this->aabb_local.max_.setConstant(-std::numeric_limits<S>::max());

OMG!! This means there was never a meaningful bounding box :(

BTW this doesn't look like it works if S=AutoDiff. Maybe it should be

std::numeric_limits<typename constants<S>::Real>::max();

instead?

BTW2: any reason this shouldn't be +/- infinity rather than max?

---

test/geometry/CMakeLists.txt, line 1 at r2 (raw file):

add_subdirectory(shape)

Nit: missing newline at eof

---

test/geometry/shape/test_convex.cpp, line 193 at r2 (raw file):

  // 90-degree rotation around each axis, in turn.
  for (int i = 0; i < 3; ++i) {
    X_WB.linear() = AngleAxis<double>(constants<double>::pi(),

Minor: comment above says "90" degrees but this looks like 180?

[sherm1]

with a few minor things to resolve.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Levi-Armstrong and @SeanCurtis-TRI)

---

test/geometry/shape/test_convex.cpp, line 37 at r2 (raw file):

/** @author Sean Curtis ([email protected]) (2018) */

// Tests the implementation of a convex hull.

Consider calling this "convex shape" rather than "convex hull" -- that's somewhat less of a commitment (due to the possibility of parallel faces used to define a "shape"). Since Convex derives from ShapeBase that would also be consistent with the existing fcl terminology.

---

test/geometry/shape/test_convex.cpp, line 79 at r2 (raw file):

                                        {0, 0, -1}};   // -z
    for (int f = 0; f < kFaceCount; ++f) {
      normals_[f] = X_WB.linear() * normals_B[f];

BTW consider defining R_WB=X_WB.linear() before this loop just so you can say its a rotation matrix (cuz the Eigen API has such an awful name).

---

test/geometry/shape/test_convex.cpp, line 133 at r2 (raw file):

  static constexpr int kPointCount = 8;
  // The implicit equations of the six planes of the cube.
  Vector3<S> normals_[kFaceCount];

BTW consider naming this normal_W_.

---

test/geometry/shape/test_convex.cpp, line 137 at r2 (raw file):

                            S(-0.5)};
  // The intersecting points of the planes (i.e., corners of the cube).
  Vector3<S> points_[kPointCount];

BTW consider naming this points_W_.

---

test/geometry/shape/test_convex.cpp, line 201 at r2 (raw file):

  // Small angle away from identity.
  X_WB.linear() = AngleAxis<double>(1e-5, Vector3<double>{1, 2, 3}.normalized())
      .matrix();

If I'm understanding this correctly you're only testing the local AABB here and that ignores the orientation passed here. The test here made me think this was a new AABB about the re-oriented shape. If you think this test is useful it should at least explain that we expect the orientation to have no effect.

[SeanCurtis-TRI]

Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @Levi-Armstrong and @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 36 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider adding something like

@author Sean Curtis (2018) Added documentation of existing code.

or whatever.

Done

---

include/fcl/geometry/shape/convex.h, line 65 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Per f2f it doesn't seem obvious to me that this condition is required. The code seems only to require that there be a plane defined for every polygon. But, if this is a real restriction then it is a strict-but-unenforced prerequisite of this constructor with a serious implication for a user who starts out with a tri-mesh: they must combine coplanar faces into larger polygons. If that is a requirement I think it needs to be spelled out since it is very common (in robotics anyway) for geometry to be supplied in the form of a tri mesh.

Might be worth a TODO here to figure out whether the shape is primarily defined as

• an intersection of non-redundant half planes, with a redundant data structure containing precalculated intersection points, OR
• as a collection of vertices forming planar faces that bound the polytope, with a redundant data structure providing precalculated planes for each face.

The latter interpretation would permit convex trimeshes to be used to create a Convex object, even if there were coplanar faces. The former would not.

My opinion: we should allow any convex trimesh if we can, since that's what our target users are likely to have.

So, there are two issues here:

1. What should this class be?
2. What is this class currently?

Your comment was neither the one nor the other. I'm deferring all comments on what the class should be. However, I'm fully in support of making sure the documentation is consistent with what is in the code. So, I'm going to remove all inferences and speculation and limit the documentation to describe what the code actually does. (Liberally sprinkled with non-doxygen TODOs of how it should change. :))

[SeanCurtis-TRI]

Radically new version; ptal.

Reviewable status: 1 of 13 files reviewed, 7 unresolved discussions (waiting on @sherm1, @Levi-Armstrong, and @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 67 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Per f2f and above, not clear to me that this is a convenient form for a robotics user nor that it is a necessary restriction here. But also not clear that I'm right!

Done (should be resolved in new documentation).

---

include/fcl/geometry/shape/convex.h, line 103 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ooops...I'll fix this tomorrow.

Done

---

include/fcl/geometry/shape/convex.h, line 65 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

So, there are two issues here:

1. What should this class be?
2. What is this class currently?

Your comment was neither the one nor the other. I'm deferring all comments on what the class should be. However, I'm fully in support of making sure the documentation is consistent with what is in the code. So, I'm going to remove all inferences and speculation and limit the documentation to describe what the code actually does. (Liberally sprinkled with non-doxygen TODOs of how it should change. :))

See new docs/API.

---

include/fcl/geometry/shape/convex.h, line 84 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: there must be m of these also - consider saying that here.

Done

---

include/fcl/geometry/shape/convex.h, line 120 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nit: "displacement vector"

Done

---

include/fcl/geometry/shape/convex.h, line 128 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Is it OK if adjacent edges are parallel? That still satisfies the cross product condition you wrote above, but in an annoyingly trivial way.

It should be ok; it merely produces a computationally wasted, zero-volume tetrahedron.

---

include/fcl/geometry/shape/convex.h, line 147 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: any hope that we could rename this mean_point? This name seems particularly awful since there really is a method that returns the centroid for this same object.

Done

---

include/fcl/geometry/shape/convex-inl.h, line 62 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

OMG! Just for the record it is worth mentioning (per f2f) that this code NEVER WORKED because the assignments used the same names on either side of the assignment operator, meaning the arguments got ignored :(

I don't think "OMG!" is a valid reviewable code. :)

---

include/fcl/geometry/shape/convex-inl.h, line 65 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW if we're stuck with this name I think it's worth mentioning again here that this is not the centroid.

Done

---

include/fcl/geometry/shape/convex-inl.h, line 102 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

OMG!! This means there was never a meaningful bounding box :(

BTW this doesn't look like it works if S=AutoDiff. Maybe it should be

std::numeric_limits<typename constants<S>::Real>::max();

instead?

BTW2: any reason this shouldn't be +/- infinity rather than max?

BTW 1: This is an AutoDiff incompatibility that is everywhere.
BTW 2: No real reason. Whoever wrote this liked max, maybe?

---

test/geometry/CMakeLists.txt, line 1 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nit: missing newline at eof

Done

---

test/geometry/shape/test_convex.cpp, line 37 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Consider calling this "convex shape" rather than "convex hull" -- that's somewhat less of a commitment (due to the possibility of parallel faces used to define a "shape"). Since Convex derives from ShapeBase that would also be consistent with the existing fcl terminology.

Done

---

test/geometry/shape/test_convex.cpp, line 79 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider defining R_WB=X_WB.linear() before this loop just so you can say its a rotation matrix (cuz the Eigen API has such an awful name).

Done

This went away when the normals went away.

---

test/geometry/shape/test_convex.cpp, line 133 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider naming this normal_W_.

Done (problem has gone away).

---

test/geometry/shape/test_convex.cpp, line 137 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider naming this points_W_.

Hmm...this could get mucky. The test is a bit hacky in that regard. I'd like to think of them as points in the geometry frame G. But given that I bake the vertex positions based on a transform, that really muddies the waters. Probably, the right thing to do is to leave the verrtices as points_G_ and then store the transform and simply provided the transformed points for the convex shape....

But without a strong inclination to do so, I'm going to leave it as is.

---

test/geometry/shape/test_convex.cpp, line 193 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: comment above says "90" degrees but this looks like 180?

Done (oops)

---

test/geometry/shape/test_convex.cpp, line 201 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

If I'm understanding this correctly you're only testing the local AABB here and that ignores the orientation passed here. The test here made me think this was a new AABB about the re-oriented shape. If you think this test is useful it should at least explain that we expect the orientation to have no effect.

What the test made you think is correct. It invokes testAABBComputation with that pose - it creates a new box with this new orientation and tests the AABB on that shape. So, in fact, the test is useful because the orientation does have an affect. (For the same reason that all the other orientations in this function matter.)

Or am I missing something that my mental model is blinding me to?

[sherm1]

Re-reviewed, and re-. I like this new version a lot! A few new comments, mostly doc.

CI is not happy and branch needs a rebase to master apparently.

Reviewed 12 of 12 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @Levi-Armstrong and @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 57 at r3 (raw file):

/// properties:
///   - Each face, fᵢ ∈ F, must be planar (and lies on the plane πᵢ with a
///     plane normal that points *outside* the mesh). Each face is mapped has

"is mapped has" doesn't parse. I'm not sure what you meant here.

---

include/fcl/geometry/shape/convex.h, line 60 at r3 (raw file):

///     a corresponding supporting plane and the ordered list of all planes is
///     `Π` such that `πᵢ` is the supporting plane for `fᵢ`.
///   - It must be true that `π(v) ≤ 0, ∀ π ∈ Π ∧ v ∈ V`.

Minor: notation confusion: Are πᵢ and π(v) supposed to represent the same quantity? Should use one or the other notation if so. Otherwise I'm not sure what π(v) is supposed to mean. I'm guessing something to do with measuring all the vertices w.r.t. a plane and then insisting that they are on the negative side since they are convex?

---

include/fcl/geometry/shape/convex.h, line 116 at r3 (raw file):

  int num_vertices;

  /// @brief The vertex positions.

Minor: can you say in which frame?

---

test/geometry/shape/test_convex.cpp, line 201 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

What the test made you think is correct. It invokes testAABBComputation with that pose - it creates a new box with this new orientation and tests the AABB on that shape. So, in fact, the test is useful because the orientation does have an affect. (For the same reason that all the other orientations in this function matter.)

Or am I missing something that my mental model is blinding me to?

Oh, I think I get it now. You are using the transform first to create new vertices in the world frame, then calculating the geometry-local AABB which is now in world. I missed that -- comment withdrawn.

---

test/geometry/shape/test_convex.cpp, line 69 at r3 (raw file):

  const int* polygons() const { return &polygons_[0]; }
  Convex<S> MakeConvex() const {
    // NOTE: The plane normals and distances aren't actually used.

Nit: this comment appears to be out of date since those are gone.

---

test/geometry/shape/test_convex.cpp, line 213 at r3 (raw file):

  S scale() const final { return 1; }
  int face_count() const final { return 6; }
  int point_count() const final { return 8; }

BTW vertex_count() ?

---

test/geometry/shape/test_convex.cpp, line 236 at r3 (raw file):

  EXPECT_EQ(convex.getNodeType(), GEOM_CONVEX);

  // The constructor computes the mean point (which it calls "center").

Minor: comment out of date now.

---

test/geometry/shape/test_convex.cpp, line 241 at r3 (raw file):

template <template <typename> class Shape, typename S>
void testABBComputation(const Shape<S>& model, const Transform3<S>& X_WS) {

BTW is this missing an "A" ("AABB")?

---

test/geometry/shape/test_convex.cpp, line 279 at r3 (raw file):

  // the volume, so we take the cube of the scale factor. But for *large*
  // geometry we don't let the tolerance get arbitrarily small.
  S scale = shape.scale() * shape.scale() * shape.scale();

Per f2f, in general this should not be the case -- COM for a small object, calculated in the small objects local frame, should be well conditioned. Likely the behavior here is an artifact of the test setup where the local frame is defined far from the actual object?

Should either work locally, or change the comment so it doesn't sound like an inherent property of small objects.

---

test/geometry/shape/test_convex.cpp, line 300 at r3 (raw file):

  // single scalar indicating its scale; we assume the error is correlated to
  // the volume, so we take the cube of the scale factor. But for *large*
  // geometry we don't let the tolerance get arbitrarily small.

Same comment here.

[SeanCurtis-TRI]

Hopefully windows is happier now.

Dismissed @Levi-Armstrong from a discussion.
Reviewable status: 9 of 13 files reviewed, 5 unresolved discussions (waiting on @sherm1, @Levi-Armstrong, and @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 57 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

"is mapped has" doesn't parse. I'm not sure what you meant here.

Done

---

include/fcl/geometry/shape/convex.h, line 60 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: notation confusion: Are πᵢ and π(v) supposed to represent the same quantity? Should use one or the other notation if so. Otherwise I'm not sure what π(v) is supposed to mean. I'm guessing something to do with measuring all the vertices w.r.t. a plane and then insisting that they are on the negative side since they are convex?

I've tweaked the notation a bit. See if that helps.

---

include/fcl/geometry/shape/convex.h, line 116 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: can you say in which frame?

Done

---

test/geometry/shape/test_convex.cpp, line 69 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nit: this comment appears to be out of date since those are gone.

Done

---

test/geometry/shape/test_convex.cpp, line 213 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW vertex_count() ?

Done

---

test/geometry/shape/test_convex.cpp, line 236 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: comment out of date now.

Done

---

test/geometry/shape/test_convex.cpp, line 241 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW is this missing an "A" ("AABB")?

Done

---

test/geometry/shape/test_convex.cpp, line 279 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Per f2f, in general this should not be the case -- COM for a small object, calculated in the small objects local frame, should be well conditioned. Likely the behavior here is an artifact of the test setup where the local frame is defined far from the actual object?

Should either work locally, or change the comment so it doesn't sound like an inherent property of small objects.

Resolved - it's relative error based on the volume of the object.

---

test/geometry/shape/test_convex.cpp, line 300 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Same comment here.

Same response

[sherm1]

Looks good -- a few more comments. CI is still red but not it's Travis.

Reviewed 1 of 4 files at r4.
Reviewable status: 10 of 13 files reviewed, 2 unresolved discussions (waiting on @sherm1 and @Levi-Armstrong)

---

include/fcl/geometry/shape/convex.h, line 60 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I've tweaked the notation a bit. See if that helps.

Almost. How about this:

Each face `fᵢ` has a corresponding supporting plane `πᵢ`.
Define functions`πᵢ(x)` which report the signed
distance from a point `x` to the plane πᵢ. Define `Π={πᵢ ∀ i}` as the set of all
face planes. Then if the mesh is convex, it must be true
that `π(v) ≤ 0, ∀ π ∈ Π ∧ v ∈ V`.

---

test/geometry/shape/test_convex.cpp, line 284 at r4 (raw file):

                int bits_lost) {
  // If we're losing more than 10 bits, then we have a major problem.
  GTEST_ASSERT_LE(bits_lost, 10);

BTW do you want to tighten this up now or is it still right?

---

test/geometry/shape/test_convex.cpp, line 307 at r4 (raw file):

                      int bits_lost) {
  // If we're losing more than 10 bits, then we have a major problem.
  GTEST_ASSERT_LE(bits_lost, 10);

BTW same comment.

---

test/geometry/shape/test_convex.cpp, line 382 at r4 (raw file):

  // We don't test translation because that would imply the geometry is
  // defined away from its own frame's origin. And that's just a recklessly
  // stupid thing to do.

BTW Hee hee. Consider mentioning that doing that degrades the precision with which these computations can be done.
BTW2 I think it would be possible still to do these accurately if we want to by computing a good origin (like the mean point) prior to doing it.

[sherm1]

Reviewed 3 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Levi-Armstrong and @SeanCurtis-TRI)

[sherm1]

Appveyor isn't happy :(

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Levi-Armstrong and @SeanCurtis-TRI)

[SeanCurtis-TRI]

Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions (waiting on @sherm1, @Levi-Armstrong, and @SeanCurtis-TRI)

---

include/fcl/geometry/shape/convex.h, line 60 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Almost. How about this:

Each face `fᵢ` has a corresponding supporting plane `πᵢ`.
Define functions`πᵢ(x)` which report the signed
distance from a point `x` to the plane πᵢ. Define `Π={πᵢ ∀ i}` as the set of all
face planes. Then if the mesh is convex, it must be true
that `π(v) ≤ 0, ∀ π ∈ Π ∧ v ∈ V`.

I've given it my own spin. Try this one.

---

test/geometry/shape/test_convex.cpp, line 284 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW do you want to tighten this up now or is it still right?

This is "right" in the sense that no test gets to say it loses more than 10 bits. I don't know if it's "right". I have no basis for arguing what the absolute maximum amount of lost precision should be. I can imagine a highly-tesselated mesh could introduce all sorts of precision loss. You have thoughts on this?

---

test/geometry/shape/test_convex.cpp, line 307 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW same comment.

Same response

---

test/geometry/shape/test_convex.cpp, line 382 at r4 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW Hee hee. Consider mentioning that doing that degrades the precision with which these computations can be done.
BTW2 I think it would be possible still to do these accurately if we want to by computing a good origin (like the mean point) prior to doing it.

Done

[SeanCurtis-TRI]

Dismissed @Levi-Armstrong from a discussion.
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @sherm1)

[sherm1]

Reviewed 2 of 2 files at r5.
Dismissed @Levi-Armstrong from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved

---

include/fcl/geometry/shape/convex.h, line 60 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I've given it my own spin. Try this one.

Nice!

---

test/geometry/shape/test_convex.cpp, line 284 at r4 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This is "right" in the sense that no test gets to say it loses more than 10 bits. I don't know if it's "right". I have no basis for arguing what the absolute maximum amount of lost precision should be. I can imagine a highly-tesselated mesh could introduce all sorts of precision loss. You have thoughts on this?

Nope, I just wanted to make sure this wasn't leftover from when you were getting less-precise results earlier.

[SeanCurtis-TRI]

@sherm1 Apparently reviewable would be happiest if you acknowledge the latest revision before I merge.

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

[sherm1]

Acked!
I wonder if this was related to the switch from a 4x3 transform to the 4x4 isometry? Eigen likes to align things that are multiples of 4 (which they both are) but perhaps it matters whether the columns are such multiples.

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

[SeanCurtis-TRI]

It may or may not be. However, there was direct correlation with my placing the poses in a vector. Prior to that they had been instantiated and used. But to make the poses available to multiple polytope types, I refactored that and then it went to hell. Still, all's better. now. :)

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