Add utility for setting contact material properties generally #12595
Conversation
There was a problem hiding this comment.
+@tehbelinda for feature review
cc @amcastro-tri FYI
Reviewable status: LGTM missing from assignee tehbelinda, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @tehbelinda)
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)
examples/scene_graph/simple_contact_surface_vis.cc, line 256 at r1 (raw file):
rigid_props.AddProperty(geometry::internal::kMaterialGroup, geometry::internal::kElastic, std::numeric_limits<double>::infinity());
is the infinity elastic modulus just no longer needed? since AddRigidHydroelasticProperties doesn't do this either
geometry/proximity_properties.h, line 90 at r1 (raw file):
@throws std::logic_error if `elastic_modulus` is not positive, `dissipation` is negative, or any of the contact material
nit: should this indenting line up with the 'if'?
multibody/parsing/detail_scene_graph.cc, line 395 at r1 (raw file):
} // Note: this explicit use of kMaterialGroup, kFriction will go away when
should this be a TODO to retire it?
SeanCurtis-TRI
force-pushed
the
PR_contact_proximity_properties
branch
from
8421c63
to
bd83599
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @tehbelinda)
examples/scene_graph/simple_contact_surface_vis.cc, line 256 at r1 (raw file):
Previously, tehbelinda (Bel) wrote…
is the infinity elastic modulus just no longer needed? since AddRigidHydroelasticProperties doesn't do this either
It isn't; technically, there was an earlier PR (#12529) that rendered the infinity value meaningless, it just messed this call site. When I went searching for kElastic everywhere, I found these call sites that I'd missed previously.
geometry/proximity_properties.h, line 90 at r1 (raw file):
Previously, tehbelinda (Bel) wrote…
nit: should this indenting line up with the 'if'?
OK
Optionally. This is a relic from when I used to enumerate the conditions and the were all inset from the if. tl;dr this certainly isn't the only example of this alignment in geometry.
multibody/parsing/detail_scene_graph.cc, line 395 at r1 (raw file):
Previously, tehbelinda (Bel) wrote…
should this be a TODO to retire it?
Done Issues and TODO added.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
geometry/proximity_properties.h, line 90 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
OK
Optionally. This is a relic from when I used to enumerate the conditions and the were all inset from the
if. tl;dr this certainly isn't the only example of this alignment ingeometry.
Since this one doesn't enumerate them, I'm inclined to say this shouldn't have the extra indent but I'll leave it up to you
There was a problem hiding this comment.
Reviewed 7 of 14 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
SeanCurtis-TRI
force-pushed
the
PR_contact_proximity_properties
branch
2 times, most recently
from
dd83e43
to
97b188a
Compare
There was a problem hiding this comment.
+@sherm1 for platform review
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @amcastro-tri, @sherm1, and @tehbelinda)
geometry/proximity_properties.h, line 90 at r1 (raw file):
Previously, tehbelinda (Bel) wrote…
Since this one doesn't enumerate them, I'm inclined to say this shouldn't have the extra indent but I'll leave it up to you
Done
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1 and @tehbelinda)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)
There was a problem hiding this comment.
-@sherm1 +@sammy-tri for platform review, please.
Reviewable status: LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri)
There was a problem hiding this comment.
Reviewed 11 of 14 files at r1, 1 of 2 files at r2, 2 of 2 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @SeanCurtis-TRI)
geometry/BUILD.bazel, line 172 at r3 (raw file):
deps = [ ":geometry_roles", "//multibody/plant:coulomb_friction",
BTW this doesn't appear create a circular dependency between //multibody and //geometry yet, so no changes requested in this PR. Just raising the slippery slope risk for the future to encourage thoughts about things like material properties which might be relevant to both subsystems (not that I think this discussion has been absent)
multibody/plant/test/multibody_plant_hydroelastic_test.cc, line 87 at r3 (raw file):
double radius, double elastic_modulus, double dissipation,
The new parameters don't appear to be used below?
Previously, we had `AddSoftHydroelasticProperties` and `AddRigidHydroelasticProperties` to facilitate setting valid properties without having to worry about the nitty gritty. This does the same for more general contact material properties and then makes use of it throughout the code base.
SeanCurtis-TRI
force-pushed
the
PR_contact_proximity_properties
branch
from
97b188a
to
679f7fd
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform) (waiting on @sammy-tri and @tehbelinda)
geometry/BUILD.bazel, line 172 at r3 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
BTW this doesn't appear create a circular dependency between
//multibodyand//geometryyet, so no changes requested in this PR. Just raising the slippery slope risk for the future to encourage thoughts about things like material properties which might be relevant to both subsystems (not that I think this discussion has been absent)
A valid point...well worth keeping in mind.
multibody/plant/test/multibody_plant_hydroelastic_test.cc, line 87 at r3 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
The new parameters don't appear to be used below?
Done (well spotted)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4.
Reviewable status:complete! all discussions resolved, LGTM from assignees sammy-tri(platform),tehbelinda
Previously, we had
AddSoftHydroelasticPropertiesandAddRigidHydroelasticPropertiesto facilitate setting valid properties without having to worry about the nitty gritty. This does the same formore general contact material properties and then makes use of it throughout the code base.
This change is