Patch bad-epsilon-for-zero-test libccd bug for Drake#23060
Patch bad-epsilon-for-zero-test libccd bug for Drake#23060SeanCurtis-TRI merged 1 commit intoRobotLocomotion:masterfrom
Conversation
SeanCurtis-TRI
added
the
release notes: fix
SeanCurtis-TRI
left a comment
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
+(release notes: fix)
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
SeanCurtis-TRI
left a comment
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
+@cohnt for feature review, please.
Reviewable status: LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers
SeanCurtis-TRI
force-pushed
the
PR_patch_libccd
branch
from
da3bb91 to
f8a6f78
Compare
cohnt
left a comment
cohnt
left a comment
There was a problem hiding this comment.
Note that this PR does not fix #21234 -- I verified that bug is still present. So we're not completely out of the woods on FCL/libccd yet. But I tested one of the problematic instances in #21673 and verified that is working now, so this should be a big help already!
Aside from a question about where test cases should be put,
Reviewed 4 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
tools/workspace/ccd_internal/test/is_zero_uses_relative_epsilon_test.cc line 14 at r1 (raw file):
/* Unit test for the is_zero_uses_relative_epsilon.patch for ccd_internal. */ GTEST_TEST(LibCcdPatchTest, IsZeroUsesRelativeEpsilonTest) {
btw regarding testing here, I know that a bunch of tests were merged into FCL with this PR. Is this one of those tests? If so, why does it need to be duplicated? If not, should it be added to FCL as well?
I guess I'm just not sure where the different tests should be put.
SeanCurtis-TRI
left a comment
SeanCurtis-TRI
left a comment
There was a problem hiding this comment.
+@sammy-tri for platform review, please.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
tools/workspace/ccd_internal/test/is_zero_uses_relative_epsilon_test.cc line 14 at r1 (raw file):
Previously, cohnt (Thomas Cohn) wrote…
btw regarding testing here, I know that a bunch of tests were merged into FCL with this PR. Is this one of those tests? If so, why does it need to be duplicated? If not, should it be added to FCL as well?
I guess I'm just not sure where the different tests should be put.
This is the one test that could not be submitted to FCL.
- This test requires the patch.
- The patch hasn't merged into libccd yet.
- Even if the patch merged into libccd, FCL isn't ready to upgrade to current libccd where it would finally get access to the fix.
- So, Drake is the only place that brings the patch and FCL together in a way where this test can pass.
cohnt
left a comment
cohnt
left a comment
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee sammy-tri(platform)
tools/workspace/ccd_internal/test/is_zero_uses_relative_epsilon_test.cc line 14 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
This is the one test that could not be submitted to FCL.
- This test requires the patch.
- The patch hasn't merged into libccd yet.
- Even if the patch merged into libccd, FCL isn't ready to upgrade to current libccd where it would finally get access to the fix.
- So, Drake is the only place that brings the patch and FCL together in a way where this test can pass.
That makes sense, thank you for explaining!
sammy-tri
left a comment
sammy-tri
left a comment
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees sammy-tri(platform),cohnt
3 participants
Resolves #21673
This change is