Updated parsing of hydroelastic properties #12491
Conversation
|
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please |
SeanCurtis-TRI
force-pushed
the
PR_parse_hydroelastic
branch
from
b517e4f
to
e9f7498
Compare
|
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please |
SeanCurtis-TRI
force-pushed
the
PR_parse_hydroelastic
branch
3 times, most recently
from
a0a93a7
to
e804ea4
Compare
|
SeanCurtis-TRI
added
the
status: single reviewer ok
There was a problem hiding this comment.
+@ggould-tri +(status: single reviewer ok)
Could I get a single check from you. This is the PR you reviewed last week and approved. However, it had an asan failure and we simply reverted it. The first revision in this PR is that PR, verbatim. The subsequent revisions show the changes to address the asan failures (just a few lines). You'll note I've explicitly called one of the failing tests to confirm happiness.
Thanks
Reviewable status: LGTM missing from assignee ggould-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri)
|
The TODO mentions #12455, but that PR is already merged. |
This introduces a new XML sub-tree that is largely common between both SDF
and URDF.
SDF version:
<drake:proximity_properties>
<drake:mesh_resolution_hint>1.3</drake:hydroelastic_resolution_hint>
<drake:elastic_modulus>1e8</drake:elastic_modulus>
<drake:hunt_crossley_dissipation>0.25</drake:hunt_crossley_dissipation>
<drake:mu_dynamic>0.7</drake:mu_dynamic>
<drake:mu_static>0.7</drake:mu_static>
</drake:proximity_properties>
URDF version:
<drake:proximity_properties>
<drake:mesh_resolution_hint value="1.3" />
<drake:elastic_modulus value="1e8" />
<drake:hunt_crossley_dissipation value="0.25" />
<drake:mu_dynamic value="0.7" />
<drake:mu_static value="0.7" />
</drake:proximity_properties>
(URDF differs because URDF has a style that uses attributes whereas SDF
eschews them.)
- Parsing collision geometry now creates a populated ProximityProperties
and stores the parsed properties there.
- There is some code that provides legacy compatibility with old
mechanisms for specifying coefficients of friction so that existing
files will still work (albeit with warnings).
- MBP API has been extended to accept proximity properties compatible with
this parsing logic. The old API needs to be deprecated (in a follow up
commit).
We still need to add "soft" deprecation for when we use the legacy
functionality. It can't be compile-time, so it'll have to be warnings with
a time stamp.
`coulomb_friction` accessed a reference into the input properties. Those properties get *moved* into SceneGraph. However, there is one case where the moved properties are *discarded* -- when the shape is a mesh. In this case the captured reference was destroyed leading to referencing freed memory. This mitigates the immediate symptom.
SeanCurtis-TRI
force-pushed
the
PR_parse_hydroelastic
branch
from
e804ea4
to
c35b7c6
Compare
There was a problem hiding this comment.
TODO removed. I restored the reference and started writing a note about a potential danger when I decided it's not worth it. Copying a small struct at initialization time is a perfectly satisfactory, safe move to make.
Reviewable status: LGTM missing from assignee ggould-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri)
There was a problem hiding this comment.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri)
There was a problem hiding this comment.
Thanks
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @ggould-tri)
|
|
@drake-jenkins-bot linux-bionic-clang-bazel-experimental-address-sanitizer please |
SeanCurtis-TRI
added
the
status: squashing now
There was a problem hiding this comment.
+(status: squashing now)
(This time without making the CI gods angry...)
Reviewable status:
complete! all discussions resolved, LGTM from assignee ggould-tri(platform) (waiting on @ggould-tri)
PR #12464 merged with asan errors. This PR contains the original approved PR (as the first commit) and the fixes for the problem in the second commit.
This change is