-
Notifications
You must be signed in to change notification settings - Fork 417
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
Correct ill-formed test - to be compatible with libccd 2.0 and 2.1 #371
Conversation
There was a problem hiding this 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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@sherm1 for platform review please.
Thanks @SeanCurtis-TRI so muuuuuch for fixing the CI failure!
Reviewed 1 of 1 files at r1.
Reviewable status: 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.
02d1867
to
43628b8
Compare
There was a problem hiding this 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)
There was a problem hiding this 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: complete! all files reviewed, all discussions resolved (waiting on @sherm1)
There was a problem hiding this 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: 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!
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:
If a box-box distance primitive were introduced, the test would
become meaningless.
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