workspace: Upgrade libsdformat to latest release 9.2.0

[jwnimmer-tri]

Improve traceability comments on Converter cherry-pick patch, and make it look more like what git format-patch would show.

---

This change is 

[jwnimmer-tri]

@EricCousineau-TRI As of libsdformat 9.2, calling HasElement("friction") on a <surface/> now returns false (used to be true), I assume because sdf::Surface got added as of 9.2. Can you push a patch to fix Drake (or ask OSRC to)?

Doing bazel test //multibody/parsing:common_parser_test is a quick way to see the failure.

[EricCousineau-TRI]

Gotcha, will try it out.
Also a good point to see how we can avoid silent errors like this in the future. Thanks!

[EricCousineau-TRI]

Code in question, I believe:

drake/multibody/parsing/detail_scene_graph.cc

Lines 434 to 449 in 13e0024

	if (surface_element) {
	const sdf::Element* friction_element =
	MaybeGetChildElement(*surface_element, "friction");
	if (friction_element) {
	const sdf::Element* ode_element =
	MaybeGetChildElement(*friction_element, "ode");
	if (MaybeGetChildElement(*ode_element, "mu") ||
	MaybeGetChildElement(*ode_element, "mu2")) {
	logging::Warn one_time(
	"When drake contact parameters are fully specified in the "
	"<drake:proximity_properties> tag, the <surface><friction><ode>"
	"<mu*> tags are ignored. While parsing, there was at least one "
	"instance where friction coefficients were defined in both "
	"locations.");
	}
	}

[jwnimmer-tri]

Sorry, I should have hyperlinked it. For me, it was:

drake/multibody/parsing/detail_scene_graph.cc

Line 472 in 13e0024

GetChildElementOrThrow(*surface_element, "friction");

Catchpoint 1 (exception thrown), 0x00007ffff7aded1d in __cxa_throw ()
   from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
(gdb) bt
#0  0x00007ffff7aded1d in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00005555556173e5 in drake::multibody::internal::(anonymous namespace)::GetChildElementOrThrow(sdf::v9::Element const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#2  0x000055555561840e in drake::multibody::internal::MakeCoulombFrictionFromSdfCollisionOde(sdf::v9::Collision const&) ()
#3  0x000055555561aa5b in drake::multibody::internal::MakeProximityPropertiesForCollision(sdf::v9::Collision const&) ()
#4  0x0000555555615880 in drake::multibody::internal::(anonymous namespace)::AddModelFromSpecification(sdf::v9::Model const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, drake::multibody::MultibodyPlant<double>*, drake::multibody::PackageMap const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
...

But probably, both sites are broken anyway.

[scpeters]

I think the assumption in the following comment is incorrect:

drake/multibody/parsing/detail_scene_graph.cc

Lines 470 to 474 in 13e0024

	// Once <surface> is found, <friction> and <ode> are required.
	const sdf::Element& friction_element =
	GetChildElementOrThrow(*surface_element, "friction");
	const sdf::Element& ode_element =
	GetChildElementOrThrow(friction_element, "ode");

A <surface> element can hold a number of different types of parameters but is not required to contain the //surface/friction/ode/mu* parameters.

[scpeters]

I think the behavior change is caused by the following line of code that adds an empty <surface/> element to any <collision> elements that don't yet have a <surface>. I'll go ahead an add a HasElement check before calling that Load function, but I think the code I linked above in detail_scene_graph is brittle and should improve its logic.

[scpeters]

reverting the behavior change in gazebosim/sdformat#268, and available for discussion about how to reduce the brittleness of the afore-mentioned code

[EricCousineau-TRI]

A element can hold a number of different types of parameters but is not required to contain the //surface/friction/ode/mu* parameters.

Gotcha. It's stated in the spec (1.7) as being optional:
http://sdformat.org/spec?ver=1.7&elem=collision#friction_ode

So in this case, just change the logic to not require them?

Also, for solving this acute PR, I will push (fast-forward) onto Jeremy's current branch, but with your PR's commit to see if it solves the issue without changes to Drake.

After that, will file an additional PR to Drake that makes it less brittle, without the workspace bump.

Both should pass, and should be able to be merged independently.

EDIT: Example failure: //multibody/parsing:detail_scene_graph_test
https://drake-cdash.csail.mit.edu/testDetails.php?test=37586969&build=1288907

[EricCousineau-TRI]

Steve's PR fixes the failure in detail_scene_graph_test, and I'd assume elsewhere:
jwnimmer-tri#5

[jwnimmer-tri]

Thanks for the investigation and fixes, @scpeters and @EricCousineau-TRI. I'm not surprised Drake's parser was brittle.

It's possible that @amcastro-tri's intent was to subset the domain of valid inputs -- that Drake would only accept a subset of the files that sdformat specifies. I agree that doing so is suboptimal in this case, even though for other cases it's fine in general for Drake to only support a subset of sdformat.

[amcastro-tri]

You are correct @jwnimmer-tri. And I do agree my solution was brittle. I believe that comes from the time when we were not able to extend the specification with custom tags, and therefore I "hacked" a solution for which, as you pointed out, I only allow a subset of what the specification allows (or used to).
Double-checking with you guys, are the default values for when those tags are not present documented? I remember having a hard time finding those defaults back in the day. Though sdformat improved a lot!

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

[jwnimmer-tri]

Yes the "1.0" defaults are documented http://sdformat.org/spec?ver=1.7&elem=collision#ode_mu

[EricCousineau-TRI]

Er, for simplicity, I'm going to close this in lieu of #13206.
I will make sure that this upgrade is called out in the commit text.

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

---

tools/workspace/sdformat/3bbd303.patch, line 32 at r1 (raw file):

index 4e5abcbf..4dc66a1d 100644
--- src/Converter.cc
+++ src/Converter.cc

nit The commits here did not state how this patch was generated, and it was not a simple one-liner.
Can the exact process used to generate this patch be documented in the commit message?

I had to futz around with it; best I came up with:

cd sdformat/
ls
git checkout 3bbd303
git format-patch -p HEAD~
# Manually strip leading `a/` and `b/`

[EricCousineau-TRI]

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r1 (raw file):

 #include "sdf/Types.hh"
 
 #include "Converter.hh"

Also, this is not the exact same patch?
Seem my version here:
aa80320

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @EricCousineau-TRI)

---

tools/workspace/sdformat/3bbd303.patch, line 32 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit The commits here did not state how this patch was generated, and it was not a simple one-liner.
Can the exact process used to generate this patch be documented in the commit message?

I had to futz around with it; best I came up with:

cd sdformat/
ls
git checkout 3bbd303
git format-patch -p HEAD~
# Manually strip leading `a/` and `b/`

Done. I added a comment atop the file, and put back the a/ b/.

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Also, this is not the exact same patch?
Seem my version here:
aa80320

The patch in this PR matches upstream (other than excluding some files) exactly.

The patch currently on Drake master is not identical to upstream, because it did not apply cleanly to 9.1.0.

[EricCousineau-TRI]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

a discussion (no related file):
Requires merge of #13206

---

---

tools/workspace/sdformat/3bbd303.patch, line 32 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done. I added a comment atop the file, and put back the a/ b/.

OK Thanks!

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The patch in this PR matches upstream (other than excluding some files) exactly.

The patch currently on Drake master is not identical to upstream, because it did not apply cleanly to 9.1.0.

OK I don't agree with removing "unnecessary files" as that ultimately "corrupts" the patch, but meh for now. At least it's mentioned in the above text.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @EricCousineau-TRI)

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r1 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

OK I don't agree with removing "unnecessary files" as that ultimately "corrupts" the patch, but meh for now. At least it's mentioned in the above text.

My recollection is that the CMakeLists.txt had a ton of merge conflicts when applied against 9.1.0.

[EricCousineau-TRI]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @EricCousineau-TRI)

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My recollection is that the CMakeLists.txt had a ton of merge conflicts when applied against 9.1.0.

OK Ah, I misread. I though it was that the patch was massaged for merge conflicts, then other irrelevant files were moved.

[jwnimmer-tri]

Dismissed @EricCousineau-TRI from a discussion.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[jwnimmer-tri]

+@sherm1 for both reviews per schedule (tomorrow), please.

[sherm1]

x 2 pending answer to one question

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r3 (raw file):

index 4e5abcbf..4dc66a1d 100644
--- a/src/Converter.cc
+++ b/src/Converter.cc

BTW its not clear to me (as an infrequent patch user) what was gained by adding the a/ and b/ prefixes and then removing them with -p1. Consider saying something about that in the opening comment.

---

tools/workspace/sdformat/repository.bzl, line 12 at r3 (raw file):

        repository = "osrf/sdformat",
        commit = "sdformat9_9.2.0",
        sha256 = "0e42001d92aa2c089c7d0c4ea6a30db2beeff0af3a9a357e7ccd0a4e1131cae7",  # noqa

Where did this sha come from? I thrashed around on osrf/sdformat github and managed to locate the 9.1.0 one but failed to locate this one.

[jwnimmer-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignee sherm1(platform)

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW its not clear to me (as an infrequent patch user) what was gained by adding the a/ and b/ prefixes and then removing them with -p1. Consider saying something about that in the opening comment.

It's just to make Eric happy. It's not better or worse.

---

tools/workspace/sdformat/repository.bzl, line 12 at r3 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Where did this sha come from? I thrashed around on osrf/sdformat github and managed to locate the 9.1.0 one but failed to locate this one.

For everything under tools/workspace/.... the answer for "what should the sha256 be" is always "leave it blank, run a build, get an error, and paste the suggestion here"..

https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/README.md#finalizing-github_archive-changes

[EricCousineau-TRI]

---

tools/workspace/sdformat/3bbd303.patch, line 36 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It's just to make Eric happy. It's not better or worse.

OK I kinda agree with Sherm in that there should be raw command line for the starting bits. But meh overall.

[sherm1]

---

tools/workspace/sdformat/repository.bzl, line 12 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

For everything under tools/workspace/.... the answer for "what should the sha256 be" is always "leave it blank, run a build, get an error, and paste the suggestion here"..

https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/README.md#finalizing-github_archive-changes

Oh, that's a checksum rather than a commit id! No wonder I couldn't find it.