[hydroelastic] Modify contact surface visualization LCM #16102
Conversation
SeanCurtis-TRI
force-pushed
the
PR_upgrade_contact_surface_lcm
branch
2 times, most recently
from
e8ad6b1
to
6b7f5ac
Compare
There was a problem hiding this comment.
+@xuchenhan-tri for feature review, please.
Reviewable status: LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @xuchenhan-tri)
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @SeanCurtis-TRI)
examples/scene_graph/simple_contact_surface_vis.cc, line 239 at r1 (raw file):
msg.hydroelastic_contacts[i]; // We'll simulate MbP's model instance/body paradigm. We have a look up
BTW, why is this comment deleted? It still seems relevant.
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 44 at r1 (raw file):
double moment_C_W[3]; // The number of quadrature points. The number of quadrature points can vary
BTW, perhaps worth mentioning this is the total number of quadrature points. The second sentence led me to believe that this is number quadrature points per face for a second.
multibody/plant/contact_results_to_lcm.cc, line 150 at r1 (raw file):
namespace { // Writes a Vector3<T> to an array of doubles (with a converstion to double as
nit, s/converstion/conversion.
multibody/plant/contact_results_to_lcm.cc, line 168 at r1 (raw file):
const auto& contact_results = get_contact_result_input_port().template Eval<ContactResults<T>>(context); // TODO(SeanCurtis-TRI): Here, and below, the abbreviation "msg" is not
BTW, since you are already doing the good deed of cleaning up, why not clean this one up as well?
Seems like a find and replace "msg"->"message" and removing the TODO is enough.
multibody/plant/contact_results_to_lcm.cc, line 199 at r1 (raw file):
// Here we enable the standard triangulated contact patches, so the unit // test //multibody/plant:contact_results_to_lcm_test can pass. // However, you might want to switch `#if 1` to `#if 0` when visualizing
I wasn't too thrilled to read this, but then I realize this is probably a stale comment.
multibody/plant/test/contact_results_to_lcm_test.cc, line 697 at r1 (raw file):
// [3, i0, i1, i2] in the face data. Confirm size and contents. */ EXPECT_EQ(pair_message.poly_data_int_count, mesh.num_triangles() * 4); EXPECT_EQ(pair_message.poly_data.size(), mesh.num_triangles() * 4);
BTW, unexpected choice of EXPECT_EQ and ASSERT_EQ below. I would expect them to be swapped (i.e. assert here and expect in the loop). Could you explain?
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 1152 at r1 (raw file):
poly_indices = itertools.islice(surface.poly_data, i, i + vertex_count) poly_indices = surface.poly_data[i:i + vertex_count]
What's wrong with just using this line and getting rid of the line with itertools?
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 1157 at r1 (raw file):
vtk_polydata = vtk.vtkPolyData() vtk_polydata.SetPoints(vnp.getVtkPointsFromNumpy(p_WVs)) vtk_polydata.SetPolys(vtk_polys)
BTW, VTK can handle heterogeneous collection of polygons just fine, right?
bindings/pydrake/systems/lcm_py_bind_cpp_serializers.cc, line 83 at r1 (raw file):
BindCppSerializer<drake::lcmt_header>("drake"); BindCppSerializer<drake::lcmt_hydroelastic_contact_surface_for_viz>("drake"); BindCppSerializer<drake::lcmt_hydroelastic_contact_surface_tri_for_viz>(
So much I still don't know about python binding, but this is one fewer item today.
SeanCurtis-TRI
force-pushed
the
PR_upgrade_contact_surface_lcm
branch
from
6b7f5ac
to
374a42e
Compare
SeanCurtis-TRI
added
the
release notes: fix
There was a problem hiding this comment.
+@EricCousineau-TRI for platform review, per schedule.
+(release notes: yes)
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @xuchenhan-tri)
examples/scene_graph/simple_contact_surface_vis.cc, line 239 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, why is this comment deleted? It still seems relevant.
Done
A bit too much hack and slash there.
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 44 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, perhaps worth mentioning this is the total number of quadrature points. The second sentence led me to believe that this is number quadrature points per face for a second.
Done
multibody/plant/contact_results_to_lcm.cc, line 150 at r1 (raw file):
Previously, xuchenhan-tri wrote…
nit, s/converstion/conversion.
Done
multibody/plant/contact_results_to_lcm.cc, line 168 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, since you are already doing the good deed of cleaning up, why not clean this one up as well?
Seems like a find and replace "msg"->"message" and removing the TODO is enough.
Done
particularly since I did it elsewhere.
multibody/plant/contact_results_to_lcm.cc, line 199 at r1 (raw file):
Previously, xuchenhan-tri wrote…
I wasn't too thrilled to read this, but then I realize this is probably a stale comment.
Done
Yep. Stale.
multibody/plant/test/contact_results_to_lcm_test.cc, line 697 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, unexpected choice of EXPECT_EQ and ASSERT_EQ below. I would expect them to be swapped (i.e. assert here and expect in the loop). Could you explain?
Done
I've unified them because asserts on sizes is a fine thing.
The significance of putting it in the for loop is the idea that if one is wrong, there's a high probability that they are all wrong. So, an EXPECT in a for loop that fails will spew a mount of spam to the log.
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 1152 at r1 (raw file):
Previously, xuchenhan-tri wrote…
What's wrong with just using this line and getting rid of the line with
itertools?
Oops
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 1157 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, VTK can handle heterogeneous collection of polygons just fine, right?
OK
Yep. You'll notice the line vtk_polys.InsertNextCell(vertex_count, poly_indices) where for each "cell" (aka face) we explicitly declare the number of vertices it has.
There was a problem hiding this comment.
Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @SeanCurtis-TRI)
multibody/plant/test/contact_results_to_lcm_test.cc, line 697 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
Done
I've unified them because asserts on sizes is a fine thing.
The significance of putting it in the for loop is the idea that if one is wrong, there's a high probability that they are all wrong. So, an
EXPECTin a for loop that fails will spew a mount of spam to the log.
I see, makes sense!
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 1157 at r1 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
OK
Yep. You'll notice the line
vtk_polys.InsertNextCell(vertex_count, poly_indices)where for each "cell" (aka face) we explicitly declare the number of vertices it has.
Sweet!
tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 3 at r2 (raw file):
# Note that this script runs in the main context of drake-visualizer, # where many modules and variables already exist in the global scope. import itertools
nit, not needed anymore.
SeanCurtis-TRI
force-pushed
the
PR_upgrade_contact_surface_lcm
branch
from
374a42e
to
a5aa604
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewed 5 of 10 files at r1, 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @SeanCurtis-TRI)
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 50 at r3 (raw file):
int32_t num_quadrature_points; // The quadrature point data. No assumptions are made about which points are
nit Sorry for being naive, but I don't understand why having no assumptions is a good thing.
Is the association stored elsewhere?
Or does it really need no association? I assume that this is case - if so, can you add this to your text?,e.g.
// No assumptions are made about which points are
// associated with which polygons, because no association is needed.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion (waiting on @EricCousineau-TRI)
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 50 at r3 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit Sorry for being naive, but I don't understand why having no assumptions is a good thing.
Is the association stored elsewhere?
Or does it really need no association? I assume that this is case - if so, can you add this to your text?,e.g.// No assumptions are made about which points are // associated with which polygons, because no association is needed.
I can see why you'd request that, but I'm a bit wary. The current statement represents a fact and a minimum contract. No associations are promised. It seems providing a justification for that (not needed) makes it stricter without providing much benefit.
You are correct in your surmise that visualization doesn't require association (it just uses it to draw a bunch of vectors and doesn't care about the association -- the position of the vectors implicitly illustrates the actual association).
The true answer is, there is a correlation. The quadrature points are enumerated in sequence, ordered in the same order as the elements. But, generally, we can't provide a closed-form equation (e.g., j = 3 *i + 1) to define that relationship because, ultimately, the number of points per element may be variable.
So, rather than state the whys and whatfors, I just wanted to make it clear that people shouldn't rely on it.
Thoughts?
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees EricCousineau-TRI(platform),xuchenhan-tri (waiting on @SeanCurtis-TRI)
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 50 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I can see why you'd request that, but I'm a bit wary. The current statement represents a fact and a minimum contract. No associations are promised. It seems providing a justification for that (not needed) makes it stricter without providing much benefit.
You are correct in your surmise that visualization doesn't require association (it just uses it to draw a bunch of vectors and doesn't care about the association -- the position of the vectors implicitly illustrates the actual association).
The true answer is, there is a correlation. The quadrature points are enumerated in sequence, ordered in the same order as the elements. But, generally, we can't provide a closed-form equation (e.g., j = 3 *i + 1) to define that relationship because, ultimately, the number of points per element may be variable.
So, rather than state the whys and whatfors, I just wanted to make it clear that people shouldn't rely on it.
Thoughts?
I'm still not sure if I understand why "stricter" is important, b/c I can't see the message / contract staying as relaxed there were association (perhaps based on ordering, rather than fields).
That being said, perhaps this can be clarified a bit more as:
// There is no defined ordering to these points.
That way, association doesn't have to be mentioned, and wouldn't be invalidated if the points were to have fields that associated which polygon vertices it may be associated (vs. doing association via ordering).
(Will mark myself as satisfied anywho!)
This changes the representation of a mesh in the contact surface lcm message. This change makes it forward compatible with when we want to render polygonal meshes. It leads to the removal of a custom triangle message for contact surfaces. The various call sites that are responsible for generating such a message have been updated to write to the new format. Incidentally, various variable names (e.g., msg) that were *not* styleguide compliant have been updated to remove the bad abbreviation. The drake_visualizer plugin has been likewise modified to handle the new representation. In addition, how it handles drawing edges on the contact surface has been streamlined (it uses VTK functionality for drawing hidden line objects instead of creating multiple meshes).
SeanCurtis-TRI
force-pushed
the
PR_upgrade_contact_surface_lcm
branch
from
a5aa604
to
331b4d0
Compare
There was a problem hiding this comment.
Reviewable status:
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 50 at r3 (raw file):
I'm inclined to simply remove the sentence. Just a few lines up it says:
There is no guaranteed pattern of correspondence between the ith quadrature point and the jth mesh face.
So, this sentence (used to replace a statement that was simply no longer true), can simply be excised. Removal would have been better than replacement.
There was a problem hiding this comment.
Reviewable status:
lcmtypes/lcmt_hydroelastic_contact_surface_for_viz.lcm, line 50 at r3 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
I'm inclined to simply remove the sentence. Just a few lines up it says:
There is no guaranteed pattern of correspondence between the ith quadrature point and the jth mesh face.
So, this sentence (used to replace a statement that was simply no longer true), can simply be excised. Removal would have been better than replacement.
OK Sounds good!
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
|
examples/scene_graph/simple_contact_surface_vis.cc, line 237 at r4 (raw file):
Unfortunately, I've just looked at this PR after it's merged. This body of Please let me know if you'd like me to fix it. |
This changes the representation of a mesh in the contact surface lcm message. This change makes it forward compatible with when we want to render polygonal meshes. It leads to the removal of a custom triangle
message for contact surfaces.
The various call sites that are responsible for generating such a message have been updated to write to the new format. Incidentally, various variable names (e.g., msg) that were not styleguide compliant have been
updated to remove the bad abbreviation.
The
drake_visualizerplugin has been likewise modified to handle the new representation. In addition, how it handles drawing edges on the contact surface has been streamlined (it uses VTK functionality for drawinghidden line objects instead of creating multiple meshes).
Relates #15796
This change is