[geometry] Enable drake visualizer to consume new hydro lcm fields

[SeanCurtis-TRI]

This gives the drake_visualizer plugin the power to consume the new per-body fields in the lcm message: model and geometry names.

Previously, the _Contact class was synonymous with per-body-pair contact. That is no longer true. A body pair can have multiple contacts. This led to a refactoring where there is now _BodyContact (all of the contacts between a pair of bodies) and _Contact (a single contact between body pairs).

The names displayed in the object model for bodies and contact data now depend on a GUI configuration. Default behavior is the legacy behavior (compact, yet possibly ambiguous). Turning it on provides verbose but unambiguous names all the time.

With this change, the visualizer can now handle multiple contacts per body pair and distinguish between two bodies with the same name, but in different model instances.

This incidentally extends simple_contact_surface_vis.cc to populate all of the new fields in the lcm message to test the visualizer.

relates #15555

---

This change is 

[SeanCurtis-TRI]

+@xuchenhan-tri for feature review, please.

Reviewable status: LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @xuchenhan-tri)

[xuchenhan-tri]

checkpoint before meeting.

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @xuchenhan-tri)

a discussion (no related file):

file:///home/xuchanhan/Pictures/Screenshot%20from%202021-09-02%2015-57-52.png

This is what I got from running the simple_contact_surface_viz example, and it's not what I expected to see. For one, I can't distinguish the contact between can 1 and 2. Perhaps I'm missing something. Will come back to this comment once I read more of the code.

---

---

examples/scene_graph/simple_contact_surface_vis.cc, line 364 at r1 (raw file):

  // We provide two cylinders to more robustly exercise the logic. Ultimately,
  // because they are affixed to the same dynamic frame, we are exploring how
  // the visualizer supports multiple contacts between two bodies.

[xuchenhan-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @xuchenhan-tri)

a discussion (no related file):

Previously, xuchenhan-tri wrote…

file:///home/xuchanhan/Pictures/Screenshot%20from%202021-09-02%2015-57-52.png

This is what I got from running the simple_contact_surface_viz example, and it's not what I expected to see. For one, I can't distinguish the contact between can 1 and 2. Perhaps I'm missing something. Will come back to this comment once I read more of the code.

Ok, I see there's a box I need to check. Retracted.

---

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI and @xuchenhan-tri)

a discussion (no related file):

Previously, xuchenhan-tri wrote…

Ok, I see there's a box I need to check. Retracted.

You got it. The reasoning is that this version can get incredibly verbose. So, to avoid visual spam, I felt it was better to opt in. (Not all users would even notice.) The follow up PR will remove the option and always use "globally minimum names" all the time.

---

[xuchenhan-tri]

Feature review done. A few minor and nits.
A bigger picture question: Where is drake_visualizer heading with all the meshcat capabilities? Are we introducing all these nice features only to be thrown out when meshcat takes the center stage?

Reviewed 1 of 2 files at r1.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

You got it. The reasoning is that this version can get incredibly verbose. So, to avoid visual spam, I felt it was better to opt in. (Not all users would even notice.) The follow up PR will remove the option and always use "globally minimum names" all the time.

I agree about the opt-in plan (vs. opt-out) and that the "globally minimum name" would be a further improvement.

---

---

examples/scene_graph/simple_contact_surface_vis.cc, line 364 at r1 (raw file):
My own comments after the quoted texts isn't showing for some reason. Could be due to my poor internet connection the other day.

The sentence starting with "because" is a bit confusing, as it doesn't convey any causal relationship. How about

// We provide two cylinders that are affixed to the same dynamic frame to explore how
// the visualizer supports multiple contacts between two bodies.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 512 at r1 (raw file):

        """Creates the key for this contact surface (based on the
        geometries involved). We assume that the bodies referenced in this
        `surface` are consistent with this instance."""

nit: I'm not sure what you mean by "are consistent with this instance". Could you clarify?

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 518 at r1 (raw file):

    def _contact_label_suffix(key, use_full_name: bool):
        """Creates a contact label based on the contact key. If we are not
        using the "full" name, the prefix is empty."""

nit: prefix-->suffix/postfix.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 529 at r1 (raw file):

        suffix = self._contact_label_suffix(self._key, state)
        for label, vis_item in self.items.items():
            vis_item.item.rename(label + suffix, False)

nit: I dug through director's API, but can only infer that this False refers to not renaming the the object's children with moderate confidence. Perhaps a short comment would help.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 532 at r1 (raw file):

    def clear(self):
        """Clears all the contat data for this contact."""

nit: contat-->contact.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 682 at r1 (raw file):

            del self._contacts[key]
        if len(self) == 0:
            om.removeFromObjectModel(self._folder)

Looks like you could call the clear() method below and recycle the comment on removeFromObjectModel (which is useful IMO).

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 686 at r1 (raw file):

    def clear(self):
        """Clears this body contact."""
        # Recursively remove all of the model item.s

nit: item.s --> items.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 695 at r1 (raw file):

      item_data: DebugData, item_name: str, view, use_full_name: bool):
        key = _Contact.make_key(surface)
        self._contacts[key].set_debug_data(item_data, item_name, view,

nit: This feels dangerous to me. Any chance that the key doesn't exist? A comment that describes the prerequisite or a guarding predicate would ease my nerve.

Same comment for set_mesh_data and the corresponding methods in VisualModel.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 759 at r1 (raw file):

        # is the "body-pair key" (which later becomes a folder name for the
        # pair of contacting bodies).
        self._contacts = {}

BTW, perhaps rename this to _body_contacts to imply the type?

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 773 at r1 (raw file):

        if state != self._use_full_name:
            self._use_full_name = state
            for key, body_contact in self._contacts.items():

BTW, for body_contact in self._contacts.values():?

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 801 at r1 (raw file):

                                                          self._timestamp)
            else:
                body_contact = _BodyContact(surface, self._root_folder,

BTW, this explicit construction doesn't seem necessary since the add_contact method below already handles that.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 814 at r1 (raw file):

                folders_to_remove.append(key)
        for key in folders_to_remove:
            del self._contacts[key]

python question: does this destruct this value and removes the key-value pair in the dictionary? From googling, it seems like yes?

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 894 at r1 (raw file):

            use_full_name: If None, the configuration use_full_name value is
                used. Otherwise, the given boolean value."""
        if use_full_name is None:

is the use_full_name flag really needed? It looks like it only has one (temporary) usage where the flag is set to true (i.e. to display the spatial force).

[SeanCurtis-TRI]

Comments addressed, PTAL.

Bigger picture answer: Yep. All of this will eventually be thrown out. Two important related points:

1. This PR isn't about "nice features", it's about fixing a known visualization bug (and the presence of the configuration selection shows that the fix isn't particularly "nice"). It'll get nicer in the next PR.
2. We don't know what "eventually" means in concrete terms. So, for now, the code should be maintainable and clear. :) So, making sure the code is nice, even if the features aren't, is still important.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @xuchenhan-tri)

---

examples/scene_graph/simple_contact_surface_vis.cc, line 364 at r1 (raw file):

Previously, xuchenhan-tri wrote…

My own comments after the quoted texts isn't showing for some reason. Could be due to my poor internet connection the other day.

The sentence starting with "because" is a bit confusing, as it doesn't convey any causal relationship. How about

// We provide two cylinders that are affixed to the same dynamic frame to explore how
// the visualizer supports multiple contacts between two bodies.

Done

I opted for a complete rehash.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 512 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit: I'm not sure what you mean by "are consistent with this instance". Could you clarify?

Done

It's actually meaningless. This is a @staticmethod, so, there is no "this instance" in the context of executing this function.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 518 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit: prefix-->suffix/postfix.

Done

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 529 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit: I dug through director's API, but can only infer that this False refers to not renaming the the object's children with moderate confidence. Perhaps a short comment would help.

Done

Python's keyword arguments to the rescue!

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 532 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit: contat-->contact.

Done

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 682 at r1 (raw file):

Previously, xuchenhan-tri wrote…

Looks like you could call the clear() method below and recycle the comment on removeFromObjectModel (which is useful IMO).

Done

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 695 at r1 (raw file):

Previously, xuchenhan-tri wrote…

nit: This feels dangerous to me. Any chance that the key doesn't exist? A comment that describes the prerequisite or a guarding predicate would ease my nerve.

Same comment for set_mesh_data and the corresponding methods in VisualModel.

Done

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 759 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, perhaps rename this to _body_contacts to imply the type?

Done

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 773 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, for body_contact in self._contacts.values():?

Done (Another leftover relic.)

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 801 at r1 (raw file):

Previously, xuchenhan-tri wrote…

BTW, this explicit construction doesn't seem necessary since the add_contact method below already handles that.

Either a) I'm misunderstanding you, or b) you've not read the code carefully enough.

The only add_contact method I see (the following line) is a method on the instantiated BodyContact instance. I can hardly call the method on an instance that hasn't been created.

In this case, we're are instantiating a class to hold contacts between two bodies. And then the add_contact method adds a specific contact (due to two geometries) to that body-pair aggregator.

Is the add_contact() name too ambiguous? Should I've got classes _BodyContact and _Contact which probably contribute to that. Suggestions for alternative names?

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 814 at r1 (raw file):

Previously, xuchenhan-tri wrote…

python question: does this destruct this value and removes the key-value pair in the dictionary? From googling, it seems like yes?

Your googling skills have not let you down. However, it's sufficiently arcane that I've added a comment to help future readers.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 894 at r1 (raw file):

Previously, xuchenhan-tri wrote…

is the use_full_name flag really needed? It looks like it only has one (temporary) usage where the flag is set to true (i.e. to display the spatial force).

Done

[xuchenhan-tri]

a discussion (no related file):
@drake-jenkins-bot linux-bionic-gcc-bazel-experimental-everything-release please

---

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @xuchenhan-tri)

a discussion (no related file):

Previously, xuchenhan-tri wrote…

@drake-jenkins-bot linux-bionic-gcc-bazel-experimental-everything-release please

Thanks for restarting this test. The second time it got over the "incomplete download" problem that seems to be accelerating recently.

---

[xuchenhan-tri]

Thanks for the explanation for the bigger picture and looking forward to the day we fully embrace meshcat!

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 529 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done

Python's keyword arguments to the rescue!

Ah, this is nice.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 801 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Either a) I'm misunderstanding you, or b) you've not read the code carefully enough.

The only add_contact method I see (the following line) is a method on the instantiated BodyContact instance. I can hardly call the method on an instance that hasn't been created.

In this case, we're are instantiating a class to hold contacts between two bodies. And then the add_contact method adds a specific contact (due to two geometries) to that body-pair aggregator.

Is the add_contact() name too ambiguous? Should I've got classes _BodyContact and _Contact which probably contribute to that. Suggestions for alternative names?

You are right. I confused _BodyContact with _Contact. The shared method names in _BodyContact and _Contact are sometimes a tiny bit confusing (e.g. set_mesh_data), but I won't say it's a defect. I'd attribute my oversight here to a little bit too much context-switching while I was doing the review. Sorry about the mistake.

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 814 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Your googling skills have not let you down. However, it's sufficiently arcane that I've added a comment to help future readers.

Thanks!

[xuchenhan-tri]

+@jwnimmer-tri for platform review per rotation please, thank you.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @SeanCurtis-TRI)

---

examples/scene_graph/simple_contact_surface_vis.cc, line 89 at r2 (raw file):

/* To help simulate MultibodyPlant; we're going to assign frames "frame groups"
 that correlate with MBP's "model instance indices". We're defining the indices

nit MbP -- it's not a MultiBodyPlant, it's a MultibodyPlant.

Ditto throughout.

---

examples/scene_graph/simple_contact_surface_vis.cc, line 93 at r2 (raw file):

constexpr int kBallModelInstance = 1;
constexpr int kCylinderModelInstance = 2;
const std::unordered_map<int, std::string> kModelInstanceNames(

This map is a non-trivial static global; it needs to be function-local (can be never_destroyed, if necessary).

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 102 at r2 (raw file):

        self.use_full_name = QtGui.QCheckBox()
        self.use_full_name.setChecked(False)
        self.use_full_name.setToolTip(

FYI Slightly more idiomatic would be to use triplequoted strings and textwrap.dedent to strip the leading spaces on each line; but this is fine.

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri and @xuchenhan-tri)

---

examples/scene_graph/simple_contact_surface_vis.cc, line 89 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit MbP -- it's not a MultiBodyPlant, it's a MultibodyPlant.

Ditto throughout.

Done

---

examples/scene_graph/simple_contact_surface_vis.cc, line 93 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This map is a non-trivial static global; it needs to be function-local (can be never_destroyed, if necessary).

Done (now has auto storage duration).

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 102 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

FYI Slightly more idiomatic would be to use triplequoted strings and textwrap.dedent to strip the leading spaces on each line; but this is fine.

In this case, I don't want to insert line breaks. I want QT to be responsible for that. That is how all of the tool tips are handled (for better or worse).

[xuchenhan-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),xuchenhan-tri (waiting on @SeanCurtis-TRI)

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 102 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

In this case, I don't want to insert line breaks. I want QT to be responsible for that. That is how all of the tool tips are handled (for better or worse).

Right. The dedent removes the hard breaks.

[jwnimmer-tri]

---

tools/workspace/drake_visualizer/_drake_visualizer_builtin_scripts/show_hydroelastic_contact.py, line 102 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Right. The dedent removes the hard breaks.

Oh, no it doesn't? Anyway, there is a function to do it somewhere. Spraying ' all over the place is not idomatic, but I also think it's fine here.