Skip to content

Conversation

@hongkai-dai
Copy link
Contributor

@hongkai-dai hongkai-dai commented May 6, 2019

Fixes #399


This change is Reviewable

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I recall correctly, you wanted to discuss this. I've looked at your solution and it seems this masks the true bug (in doSimplex3()). As I read the code:

  1. The triangle BCD was supposed to have been the previous simplex.
  2. We should already know that the origin doesn't lie on the triangle (otherwise, we would already have returned "non-separated").
  3. Therefore, we must assume that A and O lie on the same side of BCD. Because going from simplex BCD to tet ABCD we created a support vector which pointed from the triangle toward O (I won't say "normal" to BCD...but...probably?). So, A must either lie on the plane or beyond the plane in O's direction.

If item 3 is not strictly true, then the pre-requisites of doSimplex4() are invalidated which means the bug is further upstream.

Your thoughts?

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


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

  // https://github.com/danfis/libccd/issues/55 for an explanation.
  ccd_vec3_t point_projection_on_triangle_unused;
  ccd_real_t dist_squared = ccdVec3PointTriDist2(

BTW It strikes me that the condition revealed by this test is sufficient but not necessary. It's testing the point A against the triangle BCD. If a lies inside the triangle BCD, this test correctly detects zero-volume. If A lies outside BCD, but lies on the plane, then this will be incorrect. The real query should be: are these points co-planar?

In theory, it can be co-planar without A being inside BCD -- it depends on the support function. If BCD are not vertices of the minkowski difference, but, instead, lie on the interior of a face of the difference, the a new support vertex could be a linear combination of the three -- for all the reasons we've just encountered. Sound familiar?


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

  if (isAbsValueLessThanEpsSquared(dist_squared)) return 1;

  // compute AO, AB, AC, AD segments and ABC, ACD, ADB normal vectors

nit; You should probably extend this comment to include the fact that you're computing additional quantities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Box box collision incorrectly calls EPA

2 participants