Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In DoSimplex4, check if O and A are on the same the same side of BCD #400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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