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

Correct ill-formed test - to be compatible with libccd 2.0 and 2.1 #371

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Feb 7, 2019

The NearestPointFromDegenerateSimplex test was born of a specific
case, captured in the wild by a user. The test attempted to recreate
the circumstances of that failure. In its original formulation, the
test had two flaws:

  1. It relied on code paths for evaluating box-box distance via GJK.
    If a box-box distance primitive were introduced, the test would
    become meaningless.
  2. One of the box's poses is defined by an unnormalized quaternion.
    This issue was exposed when libccd updated its quaternion
    multiplication algorithm -- the final result was unduly impacted
    by the degree the quat wasn't normalized.

This commit addressed both of those issues.

resolves #361


This change is Reviewable

Copy link
Contributor Author

@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.

+@hongkai-dai for review, please. (Hopefully, this should sail past CI).

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @hongkai-dai)

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

:lgtm: +@sherm1 for platform review please.

Thanks @SeanCurtis-TRI so muuuuuch for fixing the CI failure!

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

The NearestPointFromDegenerateSimplex test was born of a specific
case, captured in the wild by a user. The test attempted to recreate
the circumstances of that failure. In its original formulation, the
test had two flaws:

  1. It relied on code paths for evaluating box-box distance via GJK.
     If a box-box distance primitive were introduced, the test would
     become meaningless.
  2. One of the box's poses is defined by an *unnormalized* quaternion.
     This issue was exposed when libccd updated its quaternion
     multiplication algorithm -- the final result was unduly impacted
     by the degree the quat wasn't normalized.

This commit addressed *both* of those issues.
Copy link
Contributor Author

@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.

Ooops -- minor change to the invocation of the distance query -- while the previous version had culled alot of the cruft in FCL's main code path, it stopped one step short of guaranteeing that an eventual box-box primitive wouldn't negate the test. now, it guarantees GJK evaluation on the two boxes.

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @hongkai-dai and @sherm1)

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

:lgtm: pending green CI

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


test/test_fcl_distance.cpp, line 380 at r2 (raw file):

                              DELTA<S>(), MatrixCompareType::absolute));
  EXPECT_NEAR(expected_dist, result.min_distance,
              constants<ccd_real_t>::eps_78());

BTW nice to see the tighter tolerance!

@SeanCurtis-TRI SeanCurtis-TRI merged commit e8fb8dd into flexible-collision-library:master Feb 7, 2019
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_compatible_with_ccd2.1 branch February 7, 2019 21:05
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.

test_fcl_distance NearestPointFromDegenerateSimplex fails on macOS
3 participants