-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[tutorials] Hydroelastic Contact: Nonconvex Mesh #20774
[tutorials] Hydroelastic Contact: Nonconvex Mesh #20774
Conversation
f59564d
to
f67de05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
a discussion (no related file):
Working temporary SHA1 until the models PR 29 is merged.
8bbbe66
to
c3492b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the tutorials test_timeout
is moderate
(300s) instead of long
(900s). The optimized build could have been short
(60s) but the debug build needs moderate
.
The improvement came from the draft CalcDistanceToSurfaceMeshWithBvh()
. It speeds up loading VTK tetrahedral meshes, which calls MakeVolumeMeshPressureField()
, about 10X using BVH (O(mn) becomes O(n log n + m log n), m = number of tetrahedral vertices, n = number of surface triangles).
CC: @joemasterjohn @xuchenhan-tri
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Working. CalcDistanceToSurfaceMeshWithBvh()
will come from a prequel PR of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Working.
CalcDistanceToSurfaceMeshWithBvh()
will come from a prequel PR of this PR.
The prequel PR is #20798.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-debug/5633/consoleFull
[10:17:09 AM] //tutorials:py/hydroelastic_contact_nonconvex_debug_load_test PASSED in 14.7s
[10:17:09 AM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 140.5s
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-release/8202/consoleFull
[10:04:31 AM] //tutorials:py/hydroelastic_contact_nonconvex_debug_load_test PASSED in 2.5s
[10:04:31 AM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 4.8s
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
49d6172
to
25f73aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
I challenged myself by returning to the original bowl mesh (44k tetrahedra) from the coarsen one (4k tetrahedra). The optimized CI is fine with a slight increase in running time. Unfortunately, the debug CI timeout again.
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-release/8258/consoleFull
[1:55:35 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 5.5s
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-debug/5691/consoleFull
[2:20:05 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test TIMEOUT in 3 out of 3 in 300.0s
Set to Working.
25fa21c
to
3e06843
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I challenged myself by returning to the original bowl mesh (44k tetrahedra) from the coarsen one (4k tetrahedra). The optimized CI is fine with a slight increase in running time. Unfortunately, the debug CI timeout again.
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-release/8258/consoleFull
[1:55:35 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 5.5s
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-debug/5691/consoleFull
[2:20:05 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test TIMEOUT in 3 out of 3 in 300.0s
Set to Working.
I ran a debug build (--compilation_mode=dbg
) on my Puget and found that it took around 8 minutes, about 100 times slower than the optimized build.
Intel VTune showed that the debug build spent 20% in RotationMatrix::ThrowIfNotValid
called by RotationMatrix::set()
. The other significant part is in IsInTriangle()
(calc_distance_to_surface_mesh.cc) calling M.colPivHouseholderQr().solve(Q)
. Speeding up these codes is out of the scope of this PR.
Sherm suggested that I should switch between a hi-resolution mesh and a low-resolution mesh for a typical run and a CI test, respectively. It's the same philosophy that we use very short simulated time in CI tests.
I'll store both the 44k-tetrahedron mesh and the 4k-tetrahedron mesh in the model repo PR 29. I think this is a balanced solution.
3e06843
to
46e9c20
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
CI flake https://drake-jenkins.csail.mit.edu/job/linux-focal-gcc-bazel-experimental-everything-release/11107/
@drake-jenkins-bot linux-focal-gcc-bazel-experimental-everything-release please.
@drake-jenkins-bot linux-focal-gcc-bazel-experimental-everything-release please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
I ran a debug build (
--compilation_mode=dbg
) on my Puget and found that it took around 8 minutes, about 100 times slower than the optimized build.Intel VTune showed that the debug build spent 20% in
RotationMatrix::ThrowIfNotValid
called byRotationMatrix::set()
. The other significant part is inIsInTriangle()
(calc_distance_to_surface_mesh.cc) callingM.colPivHouseholderQr().solve(Q)
. Speeding up these codes is out of the scope of this PR.Sherm suggested that I should switch between a hi-resolution mesh and a low-resolution mesh for a typical run and a CI test, respectively. It's the same philosophy that we use very short simulated time in CI tests.
I'll store both the 44k-tetrahedron mesh and the 4k-tetrahedron mesh in the model repo PR 29. I think this is a balanced solution.
Done.
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-release/8292/consoleFull
[1:32:03 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 3.0s
https://drake-jenkins.csail.mit.edu/job/linux-jammy-gcc-bazel-experimental-debug/5726/consoleFull
[1:44:52 PM] //tutorials:py/hydroelastic_contact_nonconvex_mesh_test PASSED in 31.0s
ff5932a
to
c315a53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
The prequel PR is #20798.
Done. #20798 was merged into master.
5656285
to
c45d8be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-(status: do not merge) -(status: do not review)
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@rpoyner-tri for feature review, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers
@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typos, some model file problem we need to fix
Reviewed 9 of 10 files at r14, 3 of 3 files at r15, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 71 at r15 (raw file):
"We will specify the inertia matrix in the `<inertial>` block. See [mesh_to_model](https://drake.mit.edu/pydrake/pydrake.multibody.mesh_to_model.html) to compute the inertia matrix.\n", "\n", "We will create a visual geometry using a triangle surface mesh from an OBJ file and a collisiong geometry using a tetrahedral volume mesh from a VTK file.\n",
typo
Suggestion:
collision
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 111 at r15 (raw file):
" <geometry>\n", " <mesh>\n", " <uri>package://drake_models/veggies/yellow_bell_pepper_no_stem_low.obj</uri>\n",
When I try to run the tutorial, either locally, or remotely, I get this message:
WARNING:drake:Meshcat: Failed http request for unknown URL /bell_pepper_no_stem_normal.png
Apparently, this is a problem with mtl parsing. h/t @jwnimmer-tri for helping debug this. Below is copied from a chat log:
The Meshcat mtl parser can't handle the bump map syntax in that mtl file:
bump bell_pepper_no_stem_normal.png -bm 0.001
In drake/geometry/meshcat.cc
// Scan .mtl file for map_ lines. For each, load the file and add
// the contents to geometry.resources.
// The syntax (http://paulbourke.net/dataformats/mtl/) is e.g.
// map_Ka -options args filename
// Here we ignore the options and only extract the filename (by
// extracting the last word before the end of line/string).
// - "map_.+" matches the map_ plus any options,
// - "\s" matches one whitespace (before the filename),
// - "[^\s]+" matches the filename, and
// - "[$\r\n]" matches the end of string or end of line.
// TODO(russt): This parsing could still be more robust.
std::regex map_regex(R"""(map_.+\s([^\s]+)\s*[$\r\n])""");
According to the spec, options are supposed to precede the texture filename. However, at least one vendor generates files with options at the end.
-- https://en.wikipedia.org/wiki/Wavefront_.obj_file
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 151 at r15 (raw file):
"We will specify the inertia matrix in the `<inertial>` block. It was calculated by [mesh_to_model](https://drake.mit.edu/pydrake/pydrake.multibody.mesh_to_model.html).\n", "\n", "We will create a visual geometry using a triangle surface mesh from an OBJ file and a collisiong geometry using a tetrahedral volume mesh from a VTK file.\n",
typo
Suggestion:
collision
c45d8be
to
2a1749a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 71 at r15 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
typo
Done. Thanks.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 111 at r15 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
When I try to run the tutorial, either locally, or remotely, I get this message:
WARNING:drake:Meshcat: Failed http request for unknown URL /bell_pepper_no_stem_normal.png
Apparently, this is a problem with mtl parsing. h/t @jwnimmer-tri for helping debug this. Below is copied from a chat log:
The Meshcat mtl parser can't handle the bump map syntax in that mtl file: bump bell_pepper_no_stem_normal.png -bm 0.001 In drake/geometry/meshcat.cc // Scan .mtl file for map_ lines. For each, load the file and add // the contents to geometry.resources. // The syntax (http://paulbourke.net/dataformats/mtl/) is e.g. // map_Ka -options args filename // Here we ignore the options and only extract the filename (by // extracting the last word before the end of line/string). // - "map_.+" matches the map_ plus any options, // - "\s" matches one whitespace (before the filename), // - "[^\s]+" matches the filename, and // - "[$\r\n]" matches the end of string or end of line. // TODO(russt): This parsing could still be more robust. std::regex map_regex(R"""(map_.+\s([^\s]+)\s*[$\r\n])"""); According to the spec, options are supposed to precede the texture filename. However, at least one vendor generates files with options at the end. -- https://en.wikipedia.org/wiki/Wavefront_.obj_file
Done. Please test it again. In drake_models PR 29, the mtl line became:
bump -bm 0.001 bell_pepper_no_stem_normal.png
map_bump -bm 0.001 bell_pepper_no_stem_normal.png
to support both implementations of "bump" and "map_bump". I've just realized that's how models/ur3e/*.mtl
do it.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 151 at r15 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
typo
Done. Thanks.
2a1749a
to
4c07cc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@jwnimmer-tri for Monday's platform review, please.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 10 files at r14, 1 of 3 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status: 10 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
tutorials/BUILD.bazel
line 125 at r17 (raw file):
"@drake_models//:veggies/yellow_bell_pepper_no_stem_low.vtk", ], test_timeout = "short",
nit The default is short
already. This line is dead code, so should be removed.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 19 at r17 (raw file):
nit Saying "compliant-hydroelastic" every other word makes it difficult for me to survive this sentence until the end. There should some way to not hammer the audience so hard. Maybe:
This tutorial shows you how to set up simulations using compliant-hydroelastic nonconvex meshes. We'll use a simple example of a bell pepper dropped onto a bowl on a table top, with all three objects represented by compliant-hydroelastic meshes. Contact forces are calculated and visualized.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 36 at r17 (raw file):
"from pydrake.systems.analysis import Simulator\n", "from pydrake.systems.framework import DiagramBuilder\n", "from pydrake.visualization import ModelVisualizer, ApplyVisualizationConfig, VisualizationConfig"
nit Alpha-sort the imports ("Apply" before "Model", etc.)
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 136 at r17 (raw file):
"\n", "# Visualize the SDFormat string you just defined.\n", "visualizer = ModelVisualizer(meshcat=meshcat, visualize_frames=True)\n",
nit The frame is just the identity. For my taste, the preview of just the single module at the origin is worse with any frames added.
Ditto throughout.
Code quote:
visualize_frames=True
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 140 at r17 (raw file):
"visualizer.Run(loop_once=True)" ] },
BTW I'm not sure where you want to put it within the tutorial flow, but surely somewhere we should tell the user to open on the mesh control panel and look at the "proximity" geometry instead of the "illustration" geometry? If the main thrust of this tutorial is the contact, they should be looking at the proximity shapes, not the illustration shapes. Or at least, they should be aware of the option to do so. It's a very common problem that users try to debug unexpected collisions by looking at illustration geometry, and I think it's important to inform of that potential misunderstanding.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 213 at r17 (raw file):
"metadata": {}, "source": [ "## Create compliant-hydroelastic table top in URDF\n",
BTW Why are we using URDF files? Is it just to demonstrate how the extra hydro syntax is spelled in the two different XML flavors?
I guess that's a fine reason, but probably in the text we should explain that idea to users. Drake strongly disprefers URDF and we should not encourage anyone to use it for any newly-created assets. We should explicitly tell them to use SDFormat for their assets. (Or MJCF. Ideally, we'd have a MJCF asset in this tutorial some day.)
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 288 at r17 (raw file):
" parser = Parser(plant)\n", "\n", " # Load the table top and the box we created.\n",
nit Should this comment mention the bowl, also? (It's not confusing without the mention, it just seems like it might be an oversight.)
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 349 at r17 (raw file):
"We will run the simulation. In MeshCat, the red arrow will represent the force `f`, and the blue arrow will represent the torque `tau`. You should see the contact patch moving around together with the force and torque vectors.\n", "\n", "After running the code below, playback with `timeScale` = 0.1 to appreciate the contact dynamics. You should see the force and torque vectors oscillate synchronously with the rocking bell pepper and bowl.\n",
nit I think a typical user reading this text will not understand that "timeScale" is something in the meshcat control panel, nor that "playback" refers to an action then can take after unfolding a bunch of the control panel options that are not visible by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r16, all commit messages.
Reviewable status: 9 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 73 at r17 (raw file):
"We will create a visual geometry using a triangle surface mesh from an OBJ file and a collision geometry using a tetrahedral volume mesh from a VTK file.\n", "\n", "The `<drake:proximity_properties>` block will control hydroelastic contacts. We will set `<drake:hydroelastic_modulus>` to 1e6 Pascals to mimic low-density polyethylene (LDPE). Hydroelastic modulus is not a physical property but rather a numerical parameter to tune our contact model. As a rule of thumb, set hydroelastic modulus to about 1/100 of Young's modulus.\n",
I don't think this is true in generall @DamrongGuoy, or at least I would never recommend doing this.
I tell you what I personally do:
- If I want to model a very stiff (rigid) object with compliant-hydro, I start with 1e6 to 1e7 Pa.
- For something that is complieant, like a soft rubber, using Young's modulus as a starting point is a good idea.
So in general, I'd say that in practice our practice is something more like E_hydro = min(E_young, 1e7)
Code quote:
s to about 1/100 of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 10 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Working temporary SHA1 until the models PR 29 is merged.
Done in r18.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r18, all commit messages.
Reviewable status: 9 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
tutorials/BUILD.bazel
line 125 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit The default is
short
already. This line is dead code, so should be removed.
Done. Removed. Thanks.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 19 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Saying "compliant-hydroelastic" every other word makes it difficult for me to survive this sentence until the end. There should some way to not hammer the audience so hard. Maybe:
This tutorial shows you how to set up simulations using compliant-hydroelastic nonconvex meshes. We'll use a simple example of a bell pepper dropped onto a bowl on a table top, with all three objects represented by compliant-hydroelastic meshes. Contact forces are calculated and visualized.
Done. Thanks. I appreciate the paragraph that you wrote.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 36 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Alpha-sort the imports ("Apply" before "Model", etc.)
Done. Thanks.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 73 at r17 (raw file):
Previously, amcastro-tri (Alejandro Castro) wrote…
I don't think this is true in generall @DamrongGuoy, or at least I would never recommend doing this.
I tell you what I personally do:
- If I want to model a very stiff (rigid) object with compliant-hydro, I start with 1e6 to 1e7 Pa.
- For something that is complieant, like a soft rubber, using Young's modulus as a starting point is a good idea.
So in general, I'd say that in practice our practice is something more like
E_hydro = min(E_young, 1e7)
Done. I have removed the rule of thumb. I have removed mention of LDPE or HDPE in all assets.
When the Hydro User Guide (HUG) has the recommendation or table of hydro modulus, we can refer to HUG.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 136 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit The frame is just the identity. For my taste, the preview of just the single module at the origin is worse with any frames added.
Ditto throughout.
Done. Thanks. Use the default now for all assets. I appreciate your picture explaining the issue.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 140 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW I'm not sure where you want to put it within the tutorial flow, but surely somewhere we should tell the user to open on the mesh control panel and look at the "proximity" geometry instead of the "illustration" geometry? If the main thrust of this tutorial is the contact, they should be looking at the proximity shapes, not the illustration shapes. Or at least, they should be aware of the option to do so. It's a very common problem that users try to debug unexpected collisions by looking at illustration geometry, and I think it's important to inform of that potential misunderstanding.
Done. In the previous paragraph, I added a short description to toggle "proximity" and referred to authoring_multibody_simulation.ipynb for more details.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 213 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Why are we using URDF files? Is it just to demonstrate how the extra hydro syntax is spelled in the two different XML flavors?
I guess that's a fine reason, but probably in the text we should explain that idea to users. Drake strongly disprefers URDF and we should not encourage anyone to use it for any newly-created assets. We should explicitly tell them to use SDFormat for their assets. (Or MJCF. Ideally, we'd have a MJCF asset in this tutorial some day.)
Done. I added an explanation below.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 288 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Should this comment mention the bowl, also? (It's not confusing without the mention, it just seems like it might be an oversight.)
Done. It's copy-paste errors. Thanks.
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 349 at r17 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit I think a typical user reading this text will not understand that "timeScale" is something in the meshcat control panel, nor that "playback" refers to an action then can take after unfolding a bunch of the control panel options that are not visible by default.
Done. I refer users to the other tutorial for details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes seem plausible. I'll run it locally one more time.
Reviewed 2 of 2 files at r19, all commit messages.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)
a discussion (no related file):
nit Please add this new file to index.ipynb
list of tutorials.
bab5763
to
6d91498
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 unresolved discussions
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Please add this new file to
index.ipynb
list of tutorials.
Thank you! I completely forgot about that.
I also reword "Meshes" to "Mesh", so the title and the file name match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dismissed @amcastro-tri from a discussion.
Reviewable status: 1 unresolved discussion
tutorials/hydroelastic_contact_nonconvex_mesh.ipynb
line 73 at r17 (raw file):
Previously, DamrongGuoy (Damrong Guoy) wrote…
Done. I have removed the rule of thumb. I have removed mention of LDPE or HDPE in all assets.
When the Hydro User Guide (HUG) has the recommendation or table of hydro modulus, we can refer to HUG.
Per f2f discussion, Alejandro let me dismiss this, so I can merge it now. I have removed the rule of thumb.
This change is