[proximity] Fixes bug in HalfSpace-HalfSpace support for point-pair penetration #14639
Conversation
There was a problem hiding this comment.
+@DamrongGuoy for feature review, please.
Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)
There was a problem hiding this comment.
+@EricCousineau-TRI for platform review.
Reviewed 3 of 3 files at r1.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
geometry/query_object.h, line 197 at r1 (raw file):
collision. Currently for T = AutoDiffXd, we support the collision between a sphere with another simple geometry including box, cylinder, capsule and halfspace.*/
Minor. Here we already had a warning that AutoDiffXd of unsupported pairs will throw, could we also have a similar warning that double of HalfSpace pairs will throw too please? Would doxygen allow multiple throws like this?
@throws if T = AutoDiffXd and an unsupported pair of geometries are in
collision. Currently for T = AutoDiffXd, we support the collision between a...
@throws if T = double and we have a pair of HalfSpaces.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status: 3 unresolved discussions (waiting on @SeanCurtis-TRI)
a discussion (no related file):
BTW Just to check, is there an issue / workflow that should be identified as fixed? (did someone else run into this?)
geometry/query_object.h, line 184 at r1 (raw file):
- For scalar type AutoDiffXd, we only support query pairs Sphere-Box, Sphere-Capsule, Sphere-Cylinder, Sphere-HalfSpace, and Sphere-Sphere.
nit Trailing whitespace.
geometry/query_object.h, line 194 at r1 (raw file):
nit (existing) This should state the type, e.g. std::exception.
Otherwise, it renders a tad odd:
https://drake.mit.edu/doxygen_cxx/classdrake_1_1geometry_1_1_query_object.html#af3b16adde799eb804619dc86f3703f4f
The very special case of HalfSpace-HalfSpace was not considered. This should throw an error as being a meaningless query. Before this patch, the fcl query would be dispatched and we were at the mercy of how fcl handled it -- it handles it poorly. Now, we officially report a problem. This also cleans up the documentationin QueryObject to reflect the current state.
SeanCurtis-TRI
force-pushed
the
PR_fix_half_space_bug
branch
from
922622e
to
636d6ca
Compare
There was a problem hiding this comment.
Comments addressed.
Reviewable status:
complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),DamrongGuoy (waiting on @DamrongGuoy and @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
BTW Just to check, is there an issue / workflow that should be identified as fixed? (did someone else run into this?)
I ran into it writing some other code. And it was convenient to patch it here so that it doesn't clutter up the subsequent PR.
geometry/query_object.h, line 184 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Trailing whitespace.
Done
geometry/query_object.h, line 194 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit (existing) This should state the type, e.g.
std::exception.Otherwise, it renders a tad odd:
https://drake.mit.edu/doxygen_cxx/classdrake_1_1geometry_1_1_query_object.html#af3b16adde799eb804619dc86f3703f4f
Done
geometry/query_object.h, line 197 at r1 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Minor. Here we already had a warning that AutoDiffXd of unsupported pairs will throw, could we also have a similar warning that
doubleofHalfSpacepairs will throw too please? Would doxygen allow multiplethrowslike this?@throws if T = AutoDiffXd and an unsupported pair of geometries are in collision. Currently for T = AutoDiffXd, we support the collision between a... @throws if T = double and we have a pair of HalfSpaces.
Done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:
a discussion (no related file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I ran into it writing some other code. And it was convenient to patch it here so that it doesn't clutter up the subsequent PR.
OK Gotcha - thanks for clarifying!
|
@drake-jenkins-bot retest this please |
…enetration (RobotLocomotion#14639) The very special case of HalfSpace-HalfSpace was not considered. This should throw an error as being a meaningless query. Before this patch, the fcl query would be dispatched and we were at the mercy of how fcl handled it -- it handles it poorly. Now, we officially report a problem. This also cleans up the documentation in QueryObject to reflect the current state.
The very special case of
HalfSpace-HalfSpacewas not considered. This should throw an error as being a meaningless query. Before this patch, the fcl query would be dispatched and we were at the mercy of how fcl handled it -- it handles it poorly. Now, we officially report a problem.This also cleans up the documentation in
QueryObjectto reflect the current state.This change is