[parsing] Add parsing of custom drake:mimic tag for SDFormat

[joemasterjohn]

Adds a custom drake:mimic tag for SDFormat with semantics analogous to the already supported <mimic> tag in URDF.

---

This change is 

[joemasterjohn]

+@rpoyner-tri for feature review, please.

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

[joemasterjohn]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

    }
  }

FYI: I had to pull out the <mimic> tag parsing to after all of the joints were parsed because the parser would fail if a <mimic> tag specified a joint that had yet to be created in the plant.

[rpoyner-tri]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

FYI: I had to pull out the <mimic> tag parsing to after all of the joints were parsed because the parser would fail if a <mimic> tag specified a joint that had yet to be created in the plant.

I'm looking for evidence that this behavior is part of the URDF spec, and so far, I'm not finding any. Granted, the URDF spec is pretty loose, but I'm not sure it's a win to encourage people to write URDF that is not portable.

Is there a reason we actually need forward reference here?

[joemasterjohn]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

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

I'm looking for evidence that this behavior is part of the URDF spec, and so far, I'm not finding any. Granted, the URDF spec is pretty loose, but I'm not sure it's a win to encourage people to write URDF that is not portable.

Is there a reason we actually need forward reference here?

The coupler constraint cannot be created in the plant (the last step of the parsing) until both joints exist in the plant. The current flow of the parser goes node by node (ordered by where they're defined in the file) creating joints and parsing all of its elements (including ) . So if a joint exists earlier in the file and in its tag references a joint defined a few lines below the current parser blows up. I don't think that should be an error to forward reference a joint defined later in the file, right?

[joemasterjohn]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

The coupler constraint cannot be created in the plant (the last step of the parsing) until both joints exist in the plant. The current flow of the parser goes node by node (ordered by where they're defined in the file) creating joints and parsing all of its elements (including ) . So if a joint exists earlier in the file and in its tag references a joint defined a few lines below the current parser blows up. I don't think that should be an error to forward reference a joint defined later in the file, right?

Sorry I didn't code escape the tags I was talking about. (I meanto to say: including <mimic>).

[rpoyner-tri]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Sorry I didn't code escape the tags I was talking about. (I meanto to say: including <mimic>).

I've been looking for existing URDF files in the world that use forward reference for mimic, and I can't find any. IIRC, joint parent and child don't support forward reference. Why should mimic be any different in that regard? I don't think there's any particular mathematical or robot design reason to support forward reference.

[joemasterjohn]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

IIRC, joint parent and child don't support forward reference.

Do you mean that the URDF spec specifies that forward reference is not supported? Drake's URDF parser supports forward reference of parent or child (all link elements are parsed before any joint or drake:joint elements.)

[rpoyner-tri]

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

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

IIRC, joint parent and child don't support forward reference.

Do you mean that the URDF spec specifies that forward reference is not supported? Drake's URDF parser supports forward reference of parent or child (all link elements are parsed before any joint or drake:joint elements.)

The URDF "spec" doesn't say much. My concern is that we encourage people to write non-portable files. However, if someone does that, the fixes are easy: either swap the joint definitions, or invert the mimic tag and its math. I suppose it is fine to support forward reference for mimic, but I can't really find any evidence for calling non-support of it a "bug".

[rpoyner-tri]

-(release notes: fix)

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

---

-- commits line 5 at r1:
nit consider dropping the "bug" language here, and instead say "extends the parser" or similar.

---

multibody/parsing/test/detail_sdf_parser_test.cc line 993 at r1 (raw file):

}

TEST_F(SdfParserTest, MimicSuccessfulParsing) {

minor consider adding a test case that successfully parses a mimic with backward reference, which is by far the more common case in models found in the wild.

---

multibody/parsing/test/urdf_parser_test/joint_parsing_test.urdf line 141 at r1 (raw file):

    <parent link="link10"/>
    <child link="link11"/>
  </joint>

minor consider adding test data for a mimic with backward reference, which is by far the more common case in models found in the wild.

[joemasterjohn]

Thanks, @rpoyner-tri. +@sammy-tri for platform review, please.

Reviewable status: LGTM missing from assignee sammy-tri(platform)

---

-- commits line 5 at r1:

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

nit consider dropping the "bug" language here, and instead say "extends the parser" or similar.

Done.

---

multibody/parsing/detail_urdf_parser.cc line 1064 at r1 (raw file):

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

The URDF "spec" doesn't say much. My concern is that we encourage people to write non-portable files. However, if someone does that, the fixes are easy: either swap the joint definitions, or invert the mimic tag and its math. I suppose it is fine to support forward reference for mimic, but I can't really find any evidence for calling non-support of it a "bug".

OK, I'll remove the language calling this a bug fix.

---

multibody/parsing/test/detail_sdf_parser_test.cc line 993 at r1 (raw file):

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

minor consider adding a test case that successfully parses a mimic with backward reference, which is by far the more common case in models found in the wild.

Done.

---

multibody/parsing/test/urdf_parser_test/joint_parsing_test.urdf line 141 at r1 (raw file):

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

minor consider adding test data for a mimic with backward reference, which is by far the more common case in models found in the wild.

Done.

[rpoyner-tri]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee sammy-tri(platform)

[sammy-tri]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

---

multibody/parsing/detail_sdf_parser.cc line 678 at r2 (raw file):

    "drake:gear_ratio",
    "drake:controller_gains",
    "drake:mimic",

I think this custom element deserves a mention in parsing_doxygen.h https://github.com/RobotLocomotion/drake/blob/master/multibody/parsing/parsing_doxygen.h#L136 (it might also be worth documenting that joints can only mimic another joint in the same model instance)

[joemasterjohn]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

---

multibody/parsing/detail_sdf_parser.cc line 678 at r2 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

I think this custom element deserves a mention in parsing_doxygen.h https://github.com/RobotLocomotion/drake/blob/master/multibody/parsing/parsing_doxygen.h#L136 (it might also be worth documenting that joints can only mimic another joint in the same model instance)

Working. I though I already commited the doxygen addition, but I must have lost it in a rebase or something 🤦

[joemasterjohn]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

---

multibody/parsing/detail_sdf_parser.cc line 678 at r2 (raw file):

Previously, joemasterjohn (Joe Masterjohn) wrote…

Working. I though I already commited the doxygen addition, but I must have lost it in a rebase or something 🤦

Done.

[rpoyner-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee sammy-tri(platform)

[sammy-tri]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),sammy-tri(platform)

[scpeters]

sorry I didn't chime in sooner (I just noticed this after it was merged), but you might consider using the //joint/axis/mimic / //joint/axis2/mimic syntax that has recently been added to SDFormat 1.11 in libsdformat14:

• http://sdformat.org/spec?ver=1.11&elem=joint#axis_mimic
• http://sdformat.org/spec?ver=1.11&elem=joint#axis2_mimic
• http://sdformat.org/tutorials?tut=mimic_proposal&cat=pose_semantics_docs&
• Joint axis mimic constraints: add sdf element gazebosim/sdformat#1166
• URDF parser: use SDFormat 1.11, parse joint mimic gazebosim/sdformat#1333