[geometry] Record animation of hydroelastic contact patches #21279

DamrongGuoy · 2024-04-09T00:35:10Z

Towards

[visualization] Support animation playback of hydroelastic contact patches in MeshCat #19142

This change is

DamrongGuoy

+(status: do not review)

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @DamrongGuoy)

RussTedrake

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @DamrongGuoy)

geometry/meshcat.h line 334 at r2 (raw file):

  @throws std::exception if `time_in_recording` corresponds to an earlier
   frame than the last frame.  */
  void SetTriangleColorMeshWithTime(

btw -- drive by comment: none of the other methods add "WithTime" to the method name... they simple add an overload of the original method name with time as an argument. Why deviate here?

DamrongGuoy

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @DamrongGuoy)

geometry/meshcat.h line 334 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- drive by comment: none of the other methods add "WithTime" to the method name... they simple add an overload of the original method name with time as an argument. Why deviate here?

Thanks for asking. Later I'll change to the overload (no "WithTime"). For the current draft, it's easier this way because I don't have to deal with overload_cast in Python binding yet.

DamrongGuoy

-(status: do not review)

+@rpoyner-tri for feature review, please.

You could run the tutorial:

 bazel run //tutorials:hydroelastic_contact_nonconvex_mesh

In the MeshCat tab, toggle the proximity checkbox, so you can see the contact patches clearer.
Playback with timeScale = 0.1 to slow down and see a movie like this:

Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers, missing label for release notes

DamrongGuoy

+(release notes: fix)

Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers

rpoyner-tri

one style point

Reviewed 7 of 7 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers

geometry/test/meshcat_test.cc line 471 at r3 (raw file):

  vertices << 0, 0.5, 0.5, 0,
              0, 0,   0.5, 0.5,
              0, 0,   0,   0.5;

This initializer will be more style-conforming and a bit more readable if it follows https://drake.mit.edu/styleguide/cppguide.html#Floating_Literals .

Suggestion:

  vertices << 0.0, 0.5, 0.5, 0.0,
              0.0, 0.0, 0.5, 0.5,
              0.0, 0.0, 0.0, 0.5;

jwnimmer-tri · 2024-04-24T18:26:48Z

I have a particular set of Meshcat expertise, so I'll sign up +@jwnimmer-tri for a bonus feature review to cross-check a few things.

(Depending on how far I go, I'll post back whether to count myself as platform review, or if we still need the on-call once I'm finished.)

jwnimmer-tri · 2024-04-25T14:52:20Z

geometry/test/meshcat_test.cc line 471 at r3 (raw file):
Just for utmost clarity, the current spelling is not outlawed by GSG, which says:

It is fine to initialize a floating-point variable with an integer literal (assuming the variable type can exactly represent that integer).

It's fine to say "the mixture of ints and doubles here is hard on the eyes" and request uniformity, but it's not a style violation de-jure.

DamrongGuoy

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/test/meshcat_test.cc line 471 at r3 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

This initializer will be more style-conforming and a bit more readable if it follows https://drake.mit.edu/styleguide/cppguide.html#Floating_Literals .

Done. Thanks.

jwnimmer-tri · 2024-04-25T15:13:47Z

geometry/meshcat.h line 314 at r3 (raw file):

  @experimental

  This is an experimental API that extends `SetTriangleColorMesh` to

Why is it experimental?

What criteria will we use to graduate it out of being experimental?

jwnimmer-tri · 2024-04-25T16:36:00Z

tutorials/hydroelastic_contact_nonconvex_mesh.ipynb line 267 at r3 (raw file):

    "        builder=builder, meshcat=meshcat,\n",
    "        config=VisualizationConfig(\n",
    "                 default_proximity_color=Rgba(r=0.8, g=0, b=0, a=0.5),\n",

FYI Originally I was going to say that hard-coding to 50% is strictly worse than just doing enable_alpha_sliders=True (which defaults to 50% but then would let the user slide to other values). However, when trying that locally TIL that the sliders don't work interact well with recordings (as opposed to live sims). This change is the best we can do here, for now.

DamrongGuoy

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/meshcat.h line 314 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why is it experimental?

What criteria will we use to graduate it out of being experimental?

Thanks for asking. I'm not sure how users will use it yet. If there are no complaints after a month, I'll be happy to remove @experimental.

tutorials/hydroelastic_contact_nonconvex_mesh.ipynb line 267 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Originally I was going to say that hard-coding to 50% is strictly worse than just doing enable_alpha_sliders=True (which defaults to 50% but then would let the user slide to other values). However, when trying that locally TIL that the sliders don't work interact well with recordings (as opposed to live sims). This change is the best we can do here, for now.

Thanks for the explanation.

jwnimmer-tri

Reviewed 4 of 7 files at r3, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

a discussion (no related file):
@DamrongGuoy for a lot of my review comments, it's going to be much easier to provide a (sometimes large) patch file alongside the discussion, than try to explain everything only with prose in review comments. For those patches, I could either (1) push to this branch, or (2) open a PR against this branch. Which would you prefer?

For option (1), the upsides are that the we (and Rico) would see the code change right away, with the discussion right beside it on the same page, and that it needs less button-clicking in GitHub to merge my fix-PR manually. The caveats are that so long as I'm working on patches, you'd need to locally use a "pull and merge" workflow (never any force-pushes or rebase), and that if you decide against any of my changes we'd need to push a "revert" commit atop the branch, instead of having me rework and resubmit.

For option (2), the pro/con are swapped -- harder to discuss & ponder the patches, but possibly easier git operations.

My vote is for (1) but it's up to you.

geometry/meshcat.h line 314 at r3 (raw file):
See https://drake.mit.edu/stable.html#exemptions:

Any new API will have these requirements waived for the first version where that API appears. (In other words, the second release of the API is the one where the Stable guarantees come into effect.)

All newly-added functions are already unstable for a little while so that we can adjust if need be. I don't yet understand why this particular API is differs in this respect from any other new function we might add. What makes it particularly risky?

multibody/meshcat/hydroelastic_contact_visualizer.cc line 179 at r3 (raw file):

  // Start with it being invisible, to prevent flickering at the origin.
  iter = path_visibility_status_.insert({path, {false, false}}).first;
  meshcat_->SetProperty(path, "visible", false, 0);

Working

Interesting that this is "time==0" instead of "time==nullopt". That's not new in this PR, but it might relate to the overall feature. My guess is that was broken for any simulation that didn't start at context time == 0.

I'll investigate.

DamrongGuoy

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@DamrongGuoy for a lot of my review comments, it's going to be much easier to provide a (sometimes large) patch file alongside the discussion, than try to explain everything only with prose in review comments. For those patches, I could either (1) push to this branch, or (2) open a PR against this branch. Which would you prefer?

For option (1), the upsides are that the we (and Rico) would see the code change right away, with the discussion right beside it on the same page, and that it needs less button-clicking in GitHub to merge my fix-PR manually. The caveats are that so long as I'm working on patches, you'd need to locally use a "pull and merge" workflow (never any force-pushes or rebase), and that if you decide against any of my changes we'd need to push a "revert" commit atop the branch, instead of having me rework and resubmit.

For option (2), the pro/con are swapped -- harder to discuss & ponder the patches, but possibly easier git operations.

My vote is for (1) but it's up to you.

Please do (1). I'll "pull and merge", no force-pushes, no rebase. I used that workflow before. Thanks!

DamrongGuoy

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/meshcat.h line 314 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

See https://drake.mit.edu/stable.html#exemptions:

Any new API will have these requirements waived for the first version where that API appears. (In other words, the second release of the API is the one where the Stable guarantees come into effect.)

All newly-added functions are already unstable for a little while so that we can adjust if need be. I don't yet understand why this particular API is differs in this respect from any other new function we might add. What makes it particularly risky?

Thank you for pointing that out. I agree it should not be @experimental then. There is no risk.

Hoist object-animation-timing doc to class overview; the caveats are going to apply to a whole bunch of functions, so we should explain it in one place and xref from each function. Remove timed-untimed overloading. The timing must be a variable, not a static dispatch. Some call sites will themselves be given optional time and need to forward it along, i.e., timed-vs-untimed is not always statically known at 100% of callers. Remove "experimental" marking. We agree it's not necessary.

jwnimmer-tri

I'm starting to push a few fixes. I'll comment inline about them.

There might be small typos or something in my pushes, I'm not trying to be 100% precise yet until all of the pushes are done (there are more coming). Feel free to just nit any problems and I'll follow up eventually.

Reviewed all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

geometry/meshcat.h line 314 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Thank you for pointing that out. I agree it should not be @experimental then. There is no risk.

BTW My latest push has removed the "experimental" markings. If you agree with the code change, you can resolve this discussion thread.

geometry/meshcat.h line 348 at r3 (raw file):

                            const Eigen::Ref<const Eigen::Matrix3Xi>& faces,
                            const Eigen::Ref<const Eigen::Matrix3Xd>& colors,
                            double time_in_recording, bool wireframe = false,

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

The canonical spelling for passing time_in_recording is std::optional<double> time_in_recording, i.e., using an optional; we must not overload on timed vs untimed. It's important that the user can easily pass in a conditional time=foobar without changing which overload they are using, i.e., they don't know at compile-time if it's a number or nullopt, because they are just forwarding along something that someone else told them.

I am not 100% sure whether time should come before wireframe or after side. If we put it before wireframe then we'll need a tiny deprecation shim to bump back the other arguments.

I'm slightly leaning towards putting it last. As we add the time to all of the other functions (SetObject, SetTriangleMesh, etc.) it seems like it will go most smoothly to always just tack it on the end. Most calls that need to pass non-null time will be inside of Drake, not from users, so it's the least impactful to users at the end.

I'm open to discussion, though.

C++ platform review -- when maps are looked up by string_view, always use string_container.h for efficiency.

jwnimmer-tri

Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

geometry/meshcat.cc line 2489 at r3 (raw file):

  }

  if (last_frame_.contains(std::string(path)) &&

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

The temp string(path) creation here just for a map lookup is wasteful. We should be using string_unordered_map<int> for a so-called "transparent" map.

https://drake.mit.edu/doxygen_cxx/namespacedrake.html#a3d336683e20c4bce752ae362db010b32

geometry/meshcat.cc line 2490 at r3 (raw file):

  if (last_frame_.contains(std::string(path)) &&
      last_frame_[std::string(path)] > animation_->frame(time_in_recording)) {

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

This is the second of four times we look up exactly the same std::string(path) key in the last_frame_ map; that's unnecessarily wasteful. We should look up the value once and re-use it thereafter.

DamrongGuoy

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

geometry/meshcat.h line 314 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW My latest push has removed the "experimental" markings. If you agree with the code change, you can resolve this discussion thread.

Agree and resolve.

geometry/meshcat.h line 348 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

The canonical spelling for passing time_in_recording is std::optional<double> time_in_recording, i.e., using an optional; we must not overload on timed vs untimed. It's important that the user can easily pass in a conditional time=foobar without changing which overload they are using, i.e., they don't know at compile-time if it's a number or nullopt, because they are just forwarding along something that someone else told them.

I am not 100% sure whether time should come before wireframe or after side. If we put it before wireframe then we'll need a tiny deprecation shim to bump back the other arguments.

I'm slightly leaning towards putting it last. As we add the time to all of the other functions (SetObject, SetTriangleMesh, etc.) it seems like it will go most smoothly to always just tack it on the end. Most calls that need to pass non-null time will be inside of Drake, not from users, so it's the least impactful to users at the end.

I'm open to discussion, though.

I agree to have time_in_recording as the last parameter.

geometry/meshcat.cc line 2489 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

The temp string(path) creation here just for a map lookup is wasteful. We should be using string_unordered_map<int> for a so-called "transparent" map.

https://drake.mit.edu/doxygen_cxx/namespacedrake.html#a3d336683e20c4bce752ae362db010b32

Agree. Thanks for telling me about string_unordered_map.

geometry/meshcat.cc line 2490 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW My latest push has resolved the problem I mention below. If you agree with the code change, you can resolve this discussion thread.

This is the second of four times we look up exactly the same std::string(path) key in the last_frame_ map; that's unnecessarily wasteful. We should look up the value once and re-use it thereafter.

Agree.

rpoyner-tri

Reviewed 2 of 6 files at r6, 1 of 3 files at r8, 4 of 4 files at r9, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

This is a very rough cut at moving the current code into the new class. Do not review yet, further changes forthcoming.

jwnimmer-tri

Reviewed 3 of 6 files at r6, 1 of 3 files at r7, 2 of 3 files at r8, 1 of 4 files at r9, 3 of 6 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

geometry/meshcat.h line 571 at r10 (raw file):

  /** Deletes the object at the given `path` as well as all of its children.
  See @ref meshcat_path for the detailed semantics of deletion. */
  void Delete(std::string_view path = "");

Working

We're probably not handling Deletion during recording correctly.

geometry/meshcat_recording_internal.cc line 32 at r10 (raw file):

template <typename T>
bool MeshcatRecording::SetProperty(std::string_view path,

Working

I anticipate that with our path respellings, we should be remapping some SetProperty calls also? Doing SetProperty on opacity is probably a good smoke test. Unlike "visible" or "scale", I don't think it's inherited down the scene tree.

jwnimmer-tri

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @DamrongGuoy)

geometry/meshcat_recording_internal.cc line 133 at r11 (raw file):

      .path = std::move(animation_path),
      .time = time,
      .visible = true,

Working

If the user option set_visualizations_while_recording is false, we should not be showing the object live -- it should remain invisible.

DamrongGuoy added the status: do not review label Apr 9, 2024

DamrongGuoy commented Apr 9, 2024

View reviewed changes

DamrongGuoy mentioned this pull request Apr 9, 2024

WIP [geometry] Add time parameter to Meshcat::SetObject() of Shape. #20959

Closed

DamrongGuoy force-pushed the draft2_SetTriangleColorMeshWithTime branch from 3fe6698 to 6475374 Compare April 18, 2024 02:43

RussTedrake reviewed Apr 18, 2024

View reviewed changes

DamrongGuoy commented Apr 18, 2024

View reviewed changes

DamrongGuoy force-pushed the draft2_SetTriangleColorMeshWithTime branch 2 times, most recently from 720adf1 to c484d93 Compare April 24, 2024 00:09

DamrongGuoy changed the title ~~Add Meshcat::SetTriangleColorMeshWithTime~~ [geometry] Record animation of hydroelastic contact patches Apr 24, 2024

[geometry] Record animation of hydroelastic contact patches

8ba0460

DamrongGuoy force-pushed the draft2_SetTriangleColorMeshWithTime branch from c484d93 to 8ba0460 Compare April 24, 2024 01:00

DamrongGuoy removed the status: do not review label Apr 24, 2024

DamrongGuoy assigned rpoyner-tri Apr 24, 2024

DamrongGuoy commented Apr 24, 2024

View reviewed changes

DamrongGuoy marked this pull request as ready for review April 24, 2024 01:07

DamrongGuoy added the release notes: fix This pull request contains fixes (no new features) label Apr 24, 2024

DamrongGuoy commented Apr 24, 2024

View reviewed changes

rpoyner-tri reviewed Apr 24, 2024

View reviewed changes

jwnimmer-tri self-assigned this Apr 24, 2024

jwnimmer-tri mentioned this pull request Apr 25, 2024

[WIP] Playback hydroelatic contact surface #20866

Closed

Floating-point literals

a916b48

DamrongGuoy commented Apr 25, 2024

View reviewed changes

jwnimmer-tri mentioned this pull request Apr 25, 2024

[geometry] Fix pass-by-value style for POD optionals #21356

Merged

DamrongGuoy commented Apr 25, 2024

View reviewed changes

jwnimmer-tri reviewed Apr 25, 2024

View reviewed changes

DamrongGuoy commented Apr 25, 2024

View reviewed changes

jwnimmer-tri mentioned this pull request Apr 25, 2024

[geometry] Port MeshcatAnimation to string_view #21358

Merged

jwnimmer-tri added 2 commits April 25, 2024 13:57

merge

44f90b5

jwnimmer-tri reviewed Apr 25, 2024

View reviewed changes

Use string_unordered_map

bf8c46b

C++ platform review -- when maps are looked up by string_view, always use string_container.h for efficiency.

jwnimmer-tri reviewed Apr 25, 2024

View reviewed changes

jwnimmer-tri mentioned this pull request Apr 25, 2024

[geometry] Meshcat has a default recording #21360

Merged

merge

42beddb

DamrongGuoy commented Apr 26, 2024

View reviewed changes

merge

0b33b3e

rpoyner-tri reviewed Apr 27, 2024

View reviewed changes

jwnimmer-tri mentioned this pull request May 7, 2024

[geometry] Factor recording implementation out of Meshcat #21362

Merged

jwnimmer-tri added 2 commits May 8, 2024 06:56

Merge remote-tracking branch 'upstream/master' into 21279

50ef4e0

This is a very rough cut at moving the current code into the new class. Do not review yet, further changes forthcoming.

tidy up

1e0ef99

jwnimmer-tri reviewed May 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[geometry] Record animation of hydroelastic contact patches #21279

[geometry] Record animation of hydroelastic contact patches #21279

DamrongGuoy commented Apr 9, 2024 •

edited

DamrongGuoy left a comment

RussTedrake left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

rpoyner-tri left a comment

jwnimmer-tri commented Apr 24, 2024

jwnimmer-tri commented Apr 25, 2024

DamrongGuoy left a comment

jwnimmer-tri commented Apr 25, 2024

jwnimmer-tri commented Apr 25, 2024

DamrongGuoy left a comment

jwnimmer-tri left a comment

DamrongGuoy left a comment

DamrongGuoy left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

DamrongGuoy left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

jwnimmer-tri left a comment

[geometry] Record animation of hydroelastic contact patches #21279

Are you sure you want to change the base?

[geometry] Record animation of hydroelastic contact patches #21279

Conversation

DamrongGuoy commented Apr 9, 2024 • edited

DamrongGuoy left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Apr 24, 2024

jwnimmer-tri commented Apr 25, 2024

DamrongGuoy left a comment

Choose a reason for hiding this comment

jwnimmer-tri commented Apr 25, 2024

jwnimmer-tri commented Apr 25, 2024

DamrongGuoy left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

DamrongGuoy left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

DamrongGuoy commented Apr 9, 2024 •

edited