-
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
Resolves the error in EPA when the query point is colinear with an edge #417
Resolves the error in EPA when the query point is colinear with an edge #417
Conversation
Is this ready for a reviewer, @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.
+@SeanCurtis-TRI checkpoint -- having local reproduction issues.
Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @hongkai-dai and @SeanCurtis-TRI)
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 696 at r1 (raw file):
double rho) { // A new vertex is colinear with an edge e[0] of the tetrahedron. The border // edges should be e[1], e[4], e[5]. The visible faces should be f[0], f[1],
nit: extra space between "be" and "e[1]".
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.
It's perfectly happy on my laptop; everything behaves as one would expect. :-/ I've made some documentation/superficial requests for changes.
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @hongkai-dai)
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1189 at r1 (raw file):
Based on our (now two) conversation(s), I'd like to see this comment greatly expanded reflecting some of the subtleties and nuance we've discussed. State the problem, state the choice for resolving it and the reasons/implications of that choice. For example:
For this triangle to have no area, the query point must be co-linear with a candidate border edge. That means it is simultaneously co-planar with the two faces adjacent to that edge. But to be in this branch, one face was considered to be visible and the other to not be visible -- an inconsistent classification.
The solution is to unify that classification. We can consider both faces as being visible or not. If we were to consider them not visible, we would shrink the size of the visible patch (making this slightly faster), but would risk introducing co-planar faces into the polytope. We choose to consider both faces as being visible. At the cost of a patch boundary with more edges, we guarantee that we don't add co-planar faces.
It may be that co-planar faces are permissible and a smaller patch is preferred. This is still an open problem. For now, we go with the "safe" choice.
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1191 at r1 (raw file):
// If the query point is colinear with the edge, then both neighbouring // faces of this edge (namely face f and g) should be visible. This edge // is an internal edge. Othewise, this edge is a border edge.
nit: If you choose to keep some variant of this comment in place of my suggestion, you have a typo here on "Othewise".
test/test_fcl_signed_distance.cpp, line 319 at r1 (raw file):
fcl::DistanceResult<S> result; fcl::distance(&box1, &box2, request, result);
nit: Any particular reason we removed the ASSERT_NO_THROW
? None of the changes in this file suggested that is desirable.
test/test_fcl_signed_distance.cpp, line 430 at r1 (raw file):
} GTEST_TEST(FCL_SIGNED_DISTANCE, box_box1_ccd) {
BTW This test name has clearly grown stale.
Perhaps it would be better to rename and document:
// A collection of scenarios observed in practice that have created error
// conditions in previous commits of the code. Each test is a unique instance.
GTEST_TEST(FCL_SIGNED_DISTANCE, RealWorldRegression) {
And, perhaps, corresponding renamings of test_distance_box_box*
to `test_distance_box_box_regression*'.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 693 at r1 (raw file):
I'd recommend documenting this function:
Given an equilateral tetrahedron, create a query point that is co-linear with edge 0 as
q = v₀ + ρ(v₀ - v₁)
, confirms that the correct tetrahedra faces are included in the visible patch. Pointq
is co-linear with edge 0 which is adjacent to faces f0 and f1. Face f3 is trivially visible from q.If the query point is co-linear with a tet edge, then both adjacent faces should be visible. The behavior is sensitive to numerical accuracy issues and we expose
rho (ρ)
as a parameter so that different scenarios can easily be authored which exercise different code paths to determine visibility. (In the code, "visibility" may be determined by multiple tests.)
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 721 at r1 (raw file):
Case 1: Visibility of faces f0 and f1 is not immediately apparent -- requires recognition that q, v0, and v1 are colinear.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 723 at r1 (raw file):
Case 2: Visibility of faces f0 and f1 are indpendently confirmed -- colinearity doesn't matter.
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @SeanCurtis-TRI)
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1189 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Based on our (now two) conversation(s), I'd like to see this comment greatly expanded reflecting some of the subtleties and nuance we've discussed. State the problem, state the choice for resolving it and the reasons/implications of that choice. For example:
For this triangle to have no area, the query point must be co-linear with a candidate border edge. That means it is simultaneously co-planar with the two faces adjacent to that edge. But to be in this branch, one face was considered to be visible and the other to not be visible -- an inconsistent classification.
The solution is to unify that classification. We can consider both faces as being visible or not. If we were to consider them not visible, we would shrink the size of the visible patch (making this slightly faster), but would risk introducing co-planar faces into the polytope. We choose to consider both faces as being visible. At the cost of a patch boundary with more edges, we guarantee that we don't add co-planar faces.
It may be that co-planar faces are permissible and a smaller patch is preferred. This is still an open problem. For now, we go with the "safe" choice.
Done. Thanks for the nice documentation.
test/test_fcl_signed_distance.cpp, line 319 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: Any particular reason we removed the
ASSERT_NO_THROW
? None of the changes in this file suggested that is desirable.
That was a mistake. Restored.
test/test_fcl_signed_distance.cpp, line 430 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This test name has clearly grown stale.
Perhaps it would be better to rename and document:
// A collection of scenarios observed in practice that have created error // conditions in previous commits of the code. Each test is a unique instance. GTEST_TEST(FCL_SIGNED_DISTANCE, RealWorldRegression) {And, perhaps, corresponding renamings of
test_distance_box_box*
to `test_distance_box_box_regression*'.
Done.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 693 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'd recommend documenting this function:
Given an equilateral tetrahedron, create a query point that is co-linear with edge 0 as
q = v₀ + ρ(v₀ - v₁)
, confirms that the correct tetrahedra faces are included in the visible patch. Pointq
is co-linear with edge 0 which is adjacent to faces f0 and f1. Face f3 is trivially visible from q.If the query point is co-linear with a tet edge, then both adjacent faces should be visible. The behavior is sensitive to numerical accuracy issues and we expose
rho (ρ)
as a parameter so that different scenarios can easily be authored which exercise different code paths to determine visibility. (In the code, "visibility" may be determined by multiple tests.)
Done. Thanks for the documentation.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 721 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Case 1: Visibility of faces f0 and f1 is not immediately apparent -- requires recognition that q, v0, and v1 are colinear.
Done.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 723 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Case 2: Visibility of faces f0 and f1 are indpendently confirmed -- colinearity doesn't matter.
Done.
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 you wanna take a look?
Reviewed 3 of 3 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.
Will look today.
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.
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.
pending a few minor comments, ptal
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hongkai-dai)
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1208 at r2 (raw file):
// It may be that co-planar faces are permissible and a smaller // patch is preferred. This is still an open problem.For now, we go with // the "safe" choice.
minor: per f2f this comment should move down. Here it should just say something like "we're outside the face, we have a good triangle, so we're done".
test/test_fcl_signed_distance.cpp, line 383 at r2 (raw file):
template <typename S> void test_distance_box_box_regression3() {
btw consider adding a comment here referencing the Drake issue that reported this problem?
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 709 at r2 (raw file):
// A new vertex is colinear with an edge e[0] of the tetrahedron. The border // edges should be e[1], e[4], e[5]. The visible faces should be f[0], f[1], // f[3], and the internal edges should be e[0], e[2], e[3].
BTW is there a picture somewhere in FCL that explains this tet numbering scheme? Would be good to link to it or copy it here for reference.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 737 at r2 (raw file):
EquilateralTetrahedron tet1(-0.05, -0.13, 0.12); CheckComputeVisiblePatchColinearNewVertex(tet1, 1.9); // Case 2: Visibility of faces f0 and f1 are indpendently confirmed --
nit: indpendently -> independently
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.
Reviewable status: complete! all files reviewed, all discussions resolved
include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1208 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
minor: per f2f this comment should move down. Here it should just say something like "we're outside the face, we have a good triangle, so we're done".
Done.
test/test_fcl_signed_distance.cpp, line 383 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
btw consider adding a comment here referencing the Drake issue that reported this problem?
Done. I referred to the FCL issue.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 709 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
BTW is there a picture somewhere in FCL that explains this tet numbering scheme? Would be good to link to it or copy it here for reference.
No we don't have that picture. I point to the documentation of EquilateralTetrahedron
.
test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl_epa.cpp, line 737 at r2 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
nit: indpendently -> independently
Done.
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.
CI trouble on Mac (both Release and Debug):
18/36 Test #21: test_fcl_signed_distance ........................***Exception: SegFault 0.51 sec
[==========] Running 7 tests from 2 test cases.
[----------] Global test environment set-up.
[----------] 4 tests from FCL_NEGATIVE_DISTANCE
[ RUN ] FCL_NEGATIVE_DISTANCE.sphere_sphere_ccd
[ OK ] FCL_NEGATIVE_DISTANCE.sphere_sphere_ccd (2 ms)
[ RUN ] FCL_NEGATIVE_DISTANCE.sphere_sphere_indep
[ OK ] FCL_NEGATIVE_DISTANCE.sphere_sphere_indep (0 ms)
[ RUN ] FCL_NEGATIVE_DISTANCE.sphere_capsule_ccd
[ OK ] FCL_NEGATIVE_DISTANCE.sphere_capsule_ccd (0 ms)
[ RUN ] FCL_NEGATIVE_DISTANCE.sphere_capsule_indep
[ OK ] FCL_NEGATIVE_DISTANCE.sphere_capsule_indep (0 ms)
[----------] 4 tests from FCL_NEGATIVE_DISTANCE (2 ms total)
[----------] 3 tests from FCL_SIGNED_DISTANCE
[ RUN ] FCL_SIGNED_DISTANCE.cylinder_sphere1_ccd
[ OK ] FCL_SIGNED_DISTANCE.cylinder_sphere1_ccd (0 ms)
[ RUN ] FCL_SIGNED_DISTANCE.cylinder_box_ccd
[ OK ] FCL_SIGNED_DISTANCE.cylinder_box_ccd (0 ms)
[ RUN ] FCL_SIGNED_DISTANCE.RealWorldRegression
Reviewed 3 of 3 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved
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.
CI seems flaky. I retriggered both jobs. One has already completed to the success, the other failed with an obvious flake. Retriggered it again.
Reviewable status: complete! all files reviewed, all discussions resolved
Resolves #415
This change is