Clean up PR2 model

[RussTedrake]

• Minor collision geometries (often described by seemingly arbitrary small boxes) have been commented out; they were causing self-collisions even in the default configuration.
• Additional Drake collision-filtering tags have been added to remove remaining self-collisions in the default position.
• The finger tip collision mesh geometry has been replaced by simple box geometry, with additional Drake tags to support hydroelastic contact.
• Long comments we reflowed to fit in Drake's preferred 80 character line limit.

+@cohnt for feature review, please?

Note that there is a version of the urdf (with the original mimic joints which have been restored) here: https://github.com/sehoonha/pydart2/blob/master/examples/data/urdf/PR2/pr2.urdf

---

This change is 

[cohnt]

Couple small comments/questions, but they aren't blocking.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 2154 at r1 (raw file):

      <drake:proximity_properties>
        <drake:compliant_hydroelastic/>
        <drake:hydroelastic_modulus value="1e6"/>

Is there a reason for this particular choice of modulus value? (Is it documented somewhere that this is a reasonable default value?)

(Similar comment for the other three places this value is used.)

---

examples/pr2/test/load_pr2_simplified_test.cc line 11 at r1 (raw file):

namespace pr2 {

// Tests if the model from pr2_simplified.urdf loads with no errors.

I don't know the Drake standards for this, but are there any other things that we test programmatically for models? I manually inspected the model, and also confirmed that it works for the mobile base IK problem we have for manipulation.

[RussTedrake]

+@jwnimmer-tri for platform review, please (since we discussed it). Feel free to punt to tomorrow's reviewer.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 2154 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Is there a reason for this particular choice of modulus value? (Is it documented somewhere that this is a reasonable default value?)

(Similar comment for the other three places this value is used.)

1e7 and 1e8 are relatively common; i chose 1e6 because the fingers should be a little squishy.

---

examples/pr2/test/load_pr2_simplified_test.cc line 11 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I don't know the Drake standards for this, but are there any other things that we test programmatically for models? I manually inspected the model, and also confirmed that it works for the mobile base IK problem we have for manipulation.

It's true that this is a bare minimum test coverage, and I'm not proud. But we adding comprehensive test coverage of the model is not a goal (especially since it's in the examples directory).

[jwnimmer-tri]

-(release notes: none) +(release notes: fix) This model is installed, so we should mention changes to it as part of the release notes. (It doesn't matter that its not part of the stable API.)

platform.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions

a discussion (no related file):
As is typical for most of our vendored models, the nearby README.md contains an inventory of our changes compared to the original. I see that the URDF file itself also contains an inventory. The two sources of truth is unfortunate, but that's a pre-existing condition so we can excuse it from the scope of this PR.

However, the README does still says this:

The description also contains fully actuated finger joints, instead of a single prismatic joint that closes and opens finger joints that mimic each other.

Is that still fully accurate? It seems at minimum we should make sure both places are all true.

If you wanted to combine the two lists, my vote would be for the URDF to have the inventory, and the README should just say a simple overview "refer to the list in the URDF comments for specific details".

---

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 8 at r1 (raw file):

<!-- 
  Note that this file has been edited by hand for Drake compatibility:

It seems to me like this PR also introduces new mimic constraints? That seems like one of the more important changes, and so should be part of the inventory here.

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 2372 at r1 (raw file):

    <mechanicalReduction>1.0</mechanicalReduction>
  </transmission>
  <transmission name="r_gripper_r_finger_trans" type="SimpleTransmission">

BTW We seem to have been pretty consistent about commenting out transmissions that we don't want, instead of deleting them entirely. (Both in prior PRs, and in other places in this one.) Is there a reason for this instance to use a different mechanism?

[RussTedrake]

Reviewable status: 3 unresolved discussions

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

As is typical for most of our vendored models, the nearby README.md contains an inventory of our changes compared to the original. I see that the URDF file itself also contains an inventory. The two sources of truth is unfortunate, but that's a pre-existing condition so we can excuse it from the scope of this PR.

However, the README does still says this:

The description also contains fully actuated finger joints, instead of a single prismatic joint that closes and opens finger joints that mimic each other.

Is that still fully accurate? It seems at minimum we should make sure both places are all true.

If you wanted to combine the two lists, my vote would be for the URDF to have the inventory, and the README should just say a simple overview "refer to the list in the URDF comments for specific details".

Done.

---

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 8 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It seems to me like this PR also introduces new mimic constraints? That seems like one of the more important changes, and so should be part of the inventory here.

The previous inventory listed the removal of the original mimic joints. I've simply put them back.

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 2372 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW We seem to have been pretty consistent about commenting out transmissions that we don't want, instead of deleting them entirely. (Both in prior PRs, and in other places in this one.) Is there a reason for this instance to use a different mechanism?

These transmissions were introduced by us, solely because we didn't have mimic joint support before.

[jwnimmer-tri]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions

---

examples/pr2/README.md line 10 at r2 (raw file):

In addition, some tags unsupported by Drake have been removed, to reduce the
burden of warning output.

This sentence is I think still true and important, but I don't see it in the URDF. I think we need another bullet in that file with this information.

---

examples/pr2/models/pr2_description/urdf/pr2_simplified.urdf line 8 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

The previous inventory listed the removal of the original mimic joints. I've simply put them back.

Yup. So the README was incorrect in r1, but is fine now that r2 has removed it.

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions

---

examples/pr2/README.md line 10 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In addition, some tags unsupported by Drake have been removed, to reduce the
burden of warning output.

This sentence is I think still true and important, but I don't see it in the URDF. I think we need another bullet in that file with this information.

(I don't find the second sentence with the URL particularly helpful, so I'm OK if you want to ditch it.)

[RussTedrake]

Reviewable status: 2 unresolved discussions

---

examples/pr2/README.md line 10 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(I don't find the second sentence with the URL particularly helpful, so I'm OK if you want to ditch it.)

Done.

[jwnimmer-tri]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions

[RussTedrake]

@cohnt said explicitly "not blocking" above. I'm going to push it through to make the nightly release.

Reviewable status: 2 unresolved discussions

[cohnt]

Regardless, Russ' answers make sense. Thanks! Glad to see we could get this merged quickly.