Fix face normal pointing outward #336

hongkai-dai · 2018-09-10T22:58:20Z

Fix two problems in faceNormalPointingOutward

One problem is exposed in @Levi-Armstrong's PR Add convexhull to convehull test #334, that when we find a vertex as a good witness point that can determine the direction of the face normal, we should return directly, instead of break.
When computing the projection from a vertex to the plane, we incorrectly computed the projection to the origin.

This change is

…finitely on one side of the hyperplane.

… projection to the plane.

SeanCurtis-TRI · 2018-09-11T03:30:12Z

Looks like mac and windows are a bit less permissive in your use of std::array. Try doing a #include <array> -- that should make the CI errors go away.

I'll look at the PR in more detail in the morning.

sherm1

with a few minor comments

Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hongkai-dai)

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

    // flip it.
    ccdVec3Scale(&dir, ccd_real_t(-1));
  } else if (projection_to_origin >= -tol && projection_to_origin <= tol) {

BTW consider (but feel free to ignore):

if (-tol <= projection_to_origin && projection_to_origin <= tol)

To me that reads more like -tol ≤ proj ≤ tol which is what we wish we could write.

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

}

GTEST_TEST(FCL_GJK_EPA, faceNormalPointingOutwardOriginNearFace1) {

Minor: please add a short comment to this and the following test explaining why you added them.

hongkai-dai

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

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

Previously, sherm1 (Michael Sherman) wrote…

BTW consider (but feel free to ignore):
if (-tol <= projection_to_origin && projection_to_origin <= tol) 
To me that reads more like -tol ≤ proj ≤ tol which is what we wish we could write.

Done, good point!

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

Previously, sherm1 (Michael Sherman) wrote…

Minor: please add a short comment to this and the following test explaining why you added them.

Done.

sherm1

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

Levi-Armstrong · 2018-09-13T15:38:09Z

I ran this version for multiple motion plans and I have not had an assert failure for faceNormalPointingOutward.

SeanCurtis-TRI

First pass with lots of cosmetic requests.

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

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

  const ccd_real_t dist_tol = 0.01;
  ccd_real_t tol = dist_tol * dir_norm;
  ccd_real_t projection_to_origin =

BTW I'm not persuaded that this rename helps. The former projection was vague, but projection_to_origin feels misleading. As I see it, it's the position vector from the origin to v, projected onto dir. I don't know that that can be captured in a single name. So, perhaps the generic "projection" with a comment would serve better.

Although, reading the code below, this could simply be called origin_distance_to_plane and documented as the distance from the origin to the face's plane.

I've realized that my concern comes down to the conflation of the operation (projection) with the semantics of the result (signed distance). Variable names reflecting the semantics are much more helpful.

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

    // The origin is close to the plane of the face. Pick another vertex to test
    // the normal direction.
    ccd_real_t max_projection_to_plane = -CCD_REAL_MAX;

BTW As with the comment below: max_distance_to_plane and min_distance_to_plane, perhaps?

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

    ccd_pt_vertex_t* v;
    // If the magnitude of the projection_to_plane is larger than tolerance,
    // then it means one of the vertices is at least 1cm away from the plane

BTW This comment seems to be brittle (if dist_tol = 0.01 ever changes to a different value, this comment will be incorrect (about the "1cm"). If we replaced "1cm" with "dist_tol", then the sentence is a tautology.

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

    // coinciding with the face.
    ccdListForEachEntry(&polytope->vertices, v, ccd_pt_vertex_t, list) {
      ccd_real_t projection_to_plane =

BTW How about distance_to_plane?

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

    ccdListForEachEntry(&polytope->vertices, v, ccd_pt_vertex_t, list) {
      ccd_real_t projection_to_plane =
          ccdVec3Dot(&dir, &(v->v.v)) - projection_to_origin;

BTW In combination with the previous comments replacing projection with distance, this could be documented as the distance from v->v.v to a plane passing through the origin, parallel with the face and then offset by the distance from origin to the face.

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

        return dir;
      } else if (projection_to_plane < -tol) {
        // The vertex is at least 1cm away from the face plane, on the opposite

BTW Same comment as before about "1cm" being brittle. You can make this comment match the comment for the previous dist > tolcase.

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

/**
 * A tetrahedron with some specific ordering on its edges, and faces.

BTW this documentation doesn't do it for me. While the derived type can play with things by re-ordering the vertices, this class still defines a lot based on vertex order. I feel that should be documented here. Because the tetrahedron represents a complete graph of connectivity, we know we have edges between all vertices and each face is comprised of all but one vertex.

However, the winding of the face, the orientation of the tetrahedron, all depend on the ordering of the provided vertices.

I provide this rambling to see if you have any thoughts. That said, because I couldn't come up with anything that obviously improved the documentation, I would certainly accept your dismissal of this concern.

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 224 at r3 (raw file):
BTW the face being "right above the origin" already implies that the "origin is near the face". So, the sentence is a bit redundant.

Also, "top face" doesn't seem to be well defined.

Perhaps:

Creates a downward pointing tetrahedron which contains the origin. The origin is just inside "top" face of this tetrahedron. The remaining vertex is far enough away from the top face that it is considered a reliable witness to determine the direction of the face's normal. The top face is not quite parallel with the z = 0 plane. This test captures the failure condition reported in PR 334 -- a logic error made it so the reliable witness could be ignored.

test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 259 at r3 (raw file):
BTW My version of this documentation:

Similar to faceNormalPointingOutwardOriginNearFace1 with an important difference: the fourth vertex is no longer a reliable witness; it lies within the distance tolerance. However, it is unambiguously farther off the plane of the top face than those that form the face. This confirms when there are no obviously reliable witness that the most distant point serves.

hongkai-dai

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI and @sherm1)

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I'm not persuaded that this rename helps. The former projection was vague, but projection_to_origin feels misleading. As I see it, it's the position vector from the origin to v, projected onto dir. I don't know that that can be captured in a single name. So, perhaps the generic "projection" with a comment would serve better.

Although, reading the code below, this could simply be called origin_distance_to_plane and documented as the distance from the origin to the face's plane.

I've realized that my concern comes down to the conflation of the operation (projection) with the semantics of the result (signed distance). Variable names reflecting the semantics are much more helpful.

Done, I renamed the variable, and added the documentation.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW As with the comment below: max_distance_to_plane and min_distance_to_plane, perhaps?

Done.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This comment seems to be brittle (if dist_tol = 0.01 ever changes to a different value, this comment will be incorrect (about the "1cm"). If we replaced "1cm" with "dist_tol", then the sentence is a tautology.

Done.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW How about distance_to_plane?

Done.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW In combination with the previous comments replacing projection with distance, this could be documented as the distance from v->v.v to a plane passing through the origin, parallel with the face and then offset by the distance from origin to the face.

Done, I added the documentation. There is no offset.

I want to compute n.dot(v - face_point), where n is the normal vector. This gives me the signed (and scaled) distance. since origin_distance_to_plane = n.dot(face_point)

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Same comment as before about "1cm" being brittle. You can make this comment match the comment for the previous dist > tolcase.

Done.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW the face being "right above the origin" already implies that the "origin is near the face". So, the sentence is a bit redundant.

Also, "top face" doesn't seem to be well defined.

Perhaps:

Creates a downward pointing tetrahedron which contains the origin. The origin is just inside "top" face of this tetrahedron. The remaining vertex is far enough away from the top face that it is considered a reliable witness to determine the direction of the face's normal. The top face is not quite parallel with the z = 0 plane. This test captures the failure condition reported in PR 334 -- a logic error made it so the reliable witness could be ignored.

Done, thanks for writing up the documentation!

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW My version of this documentation:

Similar to faceNormalPointingOutwardOriginNearFace1 with an important difference: the fourth vertex is no longer a reliable witness; it lies within the distance tolerance. However, it is unambiguously farther off the plane of the top face than those that form the face. This confirms when there are no obviously reliable witness that the most distant point serves.

Done.

SeanCurtis-TRI

With one last passing thought.

Your CI failure seems to be a transitory logging error. Currently, I don't have the bits to retrigger it. So, you might consider going with my final passing thought as an opportunity to retrigger CI. :) It's not an obstacle; it's an opportunity. :D

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

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

  // this corner case.
  ccdVec3Cross(&dir, &e1, &e2);
  const ccd_real_t dir_norm = std::sqrt(ccdVec3Len2(&dir));

The new documentation is good and drew my attention to aspects I hadn't paid attention to.

All of the distances are "scaled" but only one name refers to that. That makes it seem special. I'd remove the word scaled from that one variable.
It seems the introduction of signed distance is so you can avoid the construction of a unit length normal. Given that you're already computing the length of dir (square root and all), all we're really talking about is:
- a division in place of a multiply and
- a little extra space to store the unit vector.
  As costs go, this seems like a negligible price to pay for code that would be more intuitive -- plus, it allows us to delete tol and just use dist_tol.

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

Previously, hongkai-dai (Hongkai Dai) wrote…

Done, thanks for writing up the documentation!

I hate it when I notice the typos in my own writing....

"...origin is just below the "top" face of..."

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

  // difference: the fourth vertex is no longer a reliable witness; it lies
  // within the distance tolerance. However, it is unambiguously farther off the
  // plane of the top face than those that form the face. This confirms when

again, my own typo:

"This confirms that when there are no..."

SeanCurtis-TRI

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

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The new documentation is good and drew my attention to aspects I hadn't paid attention to.

All of the distances are "scaled" but only one name refers to that. That makes it seem special. I'd remove the word scaled from that one variable.

It seems the introduction of signed distance is so you can avoid the construction of a unit length normal. Given that you're already computing the length of dir (square root and all), all we're really talking about is:

a division in place of a multiply and

a little extra space to store the unit vector.
As costs go, this seems like a negligible price to pay for code that would be more intuitive -- plus, it allows us to delete tol and just use dist_tol.

Oops...I didn't mean "signed distance", I meant "scaled distance".

hongkai-dai

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

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Oops...I didn't mean "signed distance", I meant "scaled distance".

Good point. I removed the "scaled", and now compute the signed distance, instead of the scaled signed distance.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW this documentation doesn't do it for me. While the derived type can play with things by re-ordering the vertices, this class still defines a lot based on vertex order. I feel that should be documented here. Because the tetrahedron represents a complete graph of connectivity, we know we have edges between all vertices and each face is comprised of all but one vertex.

However, the winding of the face, the orientation of the tetrahedron, all depend on the ordering of the provided vertices.

I provide this rambling to see if you have any thoughts. That said, because I couldn't come up with anything that obviously improved the documentation, I would certainly accept your dismissal of this concern.

I add some documentation here. Not sure how much it helps.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I hate it when I notice the typos in my own writing....

"...origin is just below the "top" face of..."

Done.

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

again, my own typo:

"This confirms that when there are no..."

Done.

SeanCurtis-TRI

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

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

Previously, hongkai-dai (Hongkai Dai) wrote…

Good point. I removed the "scaled", and now compute the signed distance, instead of the scaled signed distance.

So, now you're doing multiple divisions. I had the idea that you would computeddir_hat with "one" division and then did the dot-product w.r.t. dir_hat instead of dir.

hongkai-dai

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

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

So, now you're doing multiple divisions. I had the idea that you would computeddir_hat with "one" division and then did the dot-product w.r.t. dir_hat instead of dir.

Done.

SeanCurtis-TRI

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

sherm1

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

hongkai-dai · 2018-09-18T18:00:16Z

I don't understand the appveryor build failure, have we seen this kind of failure before?

SeanCurtis-TRI

Same logging error -- I have to believe that there is something in the log that is different from a "successful" run that is causing the logging error.

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

SeanCurtis-TRI

There isn't; apparently the build logger tries calling home and it fails to call home. I don't know why it's only on the 64-bit machine or why it seems to be doing it repeatedly.

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

SeanCurtis-TRI

PR #338 doesn't appear to be dying on this appveyor logging failure. I'd say try pushing something to the PR to retrigger CI to see if it was a transient issue on Appveyor's side. If it's not, then, inexplicably enough, it might be part of this PR. The if retriggering CI doesn't work, I'll try doing a trivial PR to test CI against master's current state.

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

hongkai-dai

@SeanCurtis-TRI and @sherm1 , now the appveryor CI is happy. If you could review the latest changes, then this PR will be good to go.

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

SeanCurtis-TRI

I'm happy

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

hongkai-dai added 2 commits September 10, 2018 15:18

return the normal vector if we found a valid witness point that is de…

0890880

…finitely on one side of the hyperplane.

Fix the problem that the projection to the origin is mistaken for the…

3875e49

… projection to the plane.

hongkai-dai force-pushed the fix_faceNormalPointingOutward branch from 326d7e4 to 3875e49 Compare September 10, 2018 22:59

hongkai-dai mentioned this pull request Sep 10, 2018

Add convexhull to convehull test #334

Open

add include array

e53433f

sherm1 approved these changes Sep 11, 2018

View reviewed changes

Address Sherm's comments.

96ef611

hongkai-dai commented Sep 11, 2018

View reviewed changes

sherm1 approved these changes Sep 11, 2018

View reviewed changes

SeanCurtis-TRI approved these changes Sep 13, 2018

View reviewed changes

Address Sean's comments.

9b4738d

hongkai-dai commented Sep 18, 2018

View reviewed changes

SeanCurtis-TRI requested changes Sep 18, 2018

View reviewed changes

Address Sean's comments.

bc7fea3

hongkai-dai commented Sep 18, 2018

View reviewed changes

SeanCurtis-TRI requested changes Sep 18, 2018

View reviewed changes

Address Sean's comment.

b537af6

hongkai-dai commented Sep 18, 2018

View reviewed changes

SeanCurtis-TRI approved these changes Sep 18, 2018

View reviewed changes

sherm1 approved these changes Sep 18, 2018

View reviewed changes

SeanCurtis-TRI approved these changes Sep 18, 2018

View reviewed changes

SeanCurtis-TRI approved these changes Sep 25, 2018

View reviewed changes

linting.

b9c02f6

hongkai-dai commented Sep 25, 2018

View reviewed changes

SeanCurtis-TRI approved these changes Sep 25, 2018

View reviewed changes

hongkai-dai merged commit ef223df into flexible-collision-library:master Sep 25, 2018

hongkai-dai mentioned this pull request Sep 25, 2018

libccd EPA expandPolytope: "this should not happen" #319

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix face normal pointing outward #336

Fix face normal pointing outward #336

hongkai-dai commented Sep 10, 2018 •

edited by sherm1

Loading

SeanCurtis-TRI commented Sep 11, 2018

sherm1 left a comment

hongkai-dai left a comment

sherm1 left a comment

Levi-Armstrong commented Sep 13, 2018

SeanCurtis-TRI left a comment

hongkai-dai left a comment

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

hongkai-dai left a comment

SeanCurtis-TRI left a comment

hongkai-dai left a comment

SeanCurtis-TRI left a comment

sherm1 left a comment

hongkai-dai commented Sep 18, 2018

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

SeanCurtis-TRI left a comment

hongkai-dai left a comment

SeanCurtis-TRI left a comment

Fix face normal pointing outward #336

Fix face normal pointing outward #336

Conversation

hongkai-dai commented Sep 10, 2018 • edited by sherm1 Loading

SeanCurtis-TRI commented Sep 11, 2018

sherm1 left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

Levi-Armstrong commented Sep 13, 2018

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

sherm1 left a comment

Choose a reason for hiding this comment

hongkai-dai commented Sep 18, 2018

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai left a comment

Choose a reason for hiding this comment

SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

hongkai-dai commented Sep 10, 2018 •

edited by sherm1

Loading