use the actual distance instead of the squared distance to determine if the origin is on the facet of the simplex. #365
Conversation
…if the origin is on the facet of the simplex.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 390 at r1 (raw file):
dist = std::sqrt( ccdVec3PointTriDist2(ccd_vec3_origin, &B->v, &C->v, &D->v, nullptr)); if (ccdIsZero(dist))
BTW The pre-optimizer in me would advocate testing the squared distance against a squared epsilon (in place of a square root). Is there a mathematical reason why that would be bad? In terms of code, it would make ccdIsZero() unusable.
There was a problem hiding this comment.
Reviewable status:
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 390 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW The pre-optimizer in me would advocate testing the squared distance against a squared epsilon (in place of a square root). Is there a mathematical reason why that would be bad? In terms of code, it would make
ccdIsZero()unusable.
Per f2f -- Hongkai will switch to squared distance here, using a local method to compare with eps^2, and (I hope you're sitting down because this will be shocking!) ... he's going to rename the squared distance variable from dist to something like, er, dist2.
There was a problem hiding this comment.
Reviewable status:
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 390 at r1 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
Per f2f -- Hongkai will switch to squared distance here, using a local method to compare with eps^2, and (I hope you're sitting down because this will be shocking!) ... he's going to rename the squared distance variable from
distto something like, er, dist2.
BTW I could even be persuaded as to the value of the verbose variable squared_dist. :)
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @SeanCurtis-TRI)
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 390 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW I could even be persuaded as to the value of the verbose variable
squared_dist. :)
Done.
There was a problem hiding this comment.
Nice -- one bug, I think.
Reviewed 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 375 at r2 (raw file):
// found dist_squared = ccdVec3PointTriDist2(&A->v, &B->v, &C->v, &D->v, nullptr); if (ccdIsZero(dist_squared)) {
isAbsValueLessThanEpsSquared ?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sherm1)
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 375 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
isAbsValueLessThanEpsSquared ?
Done. Good catch.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sherm1)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hongkai-dai and @sherm1)
a discussion (no related file):
One thing this makes me wonder -- are there unit tests with really loose tolerances that could be tightened up because of this? I strongly suspect that it's not worth chasing through legacy tests to find such, but I would argue there should be a unit test in support of this change.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hongkai-dai)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
One thing this makes me wonder -- are there unit tests with really loose tolerances that could be tightened up because of this? I strongly suspect that it's not worth chasing through legacy tests to find such, but I would argue there should be a unit test in support of this change.
Given that I intend to rewrite the code touched by __ccdGJK, I think I will introduce another PR with good unit test coverage. Are you OK if I add the unit test in that PR instead of this one? My concern is that if I write the unit test for this PR, it would slow down the merging process, and the test might be outdated after we introduce the new PR.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hongkai-dai)
a discussion (no related file):
Previously, hongkai-dai (Hongkai Dai) wrote…
Given that I intend to rewrite the code touched by
__ccdGJK, I think I will introduce another PR with good unit test coverage. Are you OK if I add the unit test in that PR instead of this one? My concern is that if I write the unit test for this PR, it would slow down the merging process, and the test might be outdated after we introduce the new PR.
Well, as long as there is an incipient PR coming,
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Well, as long as there is an incipient PR coming,
The dev branch I am working is in https://github.com/hongkai-dai/fcl/tree/andres_failing_case. Actually Andres' test case is a good one to expose the problem in the old code. But there are more problems not exposed in that test yet.
There was a problem hiding this comment.
Reviewable status:
There was a problem hiding this comment.
The CI failure is on the mac build, for the test test_fcl_distance. That test always fails, and I think @jamiesnape is working on it (thanks Jamie!)?
Reviewable status:
|
I'm confirming that the CI failure is only the test_fcl_distance tolerance problem that we've been seeing on Macs, unrelated to this PR. |
|
The mac test hasn't failed in ages? Why did it suddenly start failing now? |
|
The last PR #362 merged before this one also has the same mac test failure. |
|
So, in November (#352) was the last PR that was about math. That passed (and several prior to that). Then we had a couple of CMake-related PRs that failed. Weird that that's come back. |
|
From looking at the closed-PR list it appears that #360 was the first one that failed. That did include some small Mac-only CMake changes. Hard to see how that would break anything though -- any thoughts @jamiesnape ? |
|
Not related. Master actually failed before then (see #361). |
|
FTR The historical CI failure on mac was not |
|
On #362 which was merged just before this PR. The mac failure was |
|
Precisely - it's the same mac error we've been getting since January 16 (https://travis-ci.org/flexible-collision-library/fcl/builds/480449098). That is not the failure we were consistently getting all of last year where we'd confirm it was the "known failure" and merge anyways. This is failure in a very specific test -- one related to #293. It requires a box-box distance test to be evaluated with GJK. Curiously, the expected answer and computed answer differ by 10^-7 which feels very theshold-ish; but I'm not sure what would cause a change in behavior. I'll post an issue. |
#361 is not that issue? |
|
@jamiesnape you are 100% right. I've assigned myself to that issue. Thanks. |
cc @avalenzu
This change is