Drake: Multibody plant SDF parsing / sdformat "preserve unknown elements" is broken

[jwnimmer-tri]

When I upgrade Drake's commit of SDFormat to the latest upstream default (aka master), the "preserve unknown elements" feature causes things to go haywire. In particular, the new unit test in #9220 fails.

Reverting #9221 will demonstrate this -- 9221 rolls back the SDFormat commit from one that has "preserve unknown", to a month-prior version that doesn't. If we push forward to a new commit, the unit test will break.

[amcastro-tri]

From conversation with @jwnimmer-tri, an example failure is:

==================== Test output for //multibody/multibody_tree/parsing:multibody_plant_sdf_parser_test:
Using drake_cc_googletest_main.cc

[==========] Running 8 tests from 3 test cases.
[----------] Global test environment set-up.
[----------] 2 tests from AcrobotModelTests
[ RUN      ] AcrobotModelTests.ModelBasics
[       OK ] AcrobotModelTests.ModelBasics (52 ms)
[ RUN      ] AcrobotModelTests.VerifyMassMatrixAgainstBenchmark
[       OK ] AcrobotModelTests.VerifyMassMatrixAgainstBenchmark (67 ms)
[----------] 2 tests from AcrobotModelTests (119 ms total)

[----------] 5 tests from MultibodyPlantSdfParser
[ RUN      ] MultibodyPlantSdfParser.LinkWithVisuals
No HOME defined in the environment. Will not log.
unknown file: Failure
C++ exception with description "From AddModelFromSdfFile():
Error: Plane geometry is missing a <size> child element. Using a size of 1, 1.
Error: Plane geometry is missing a <size> child element. Using a size of 1, 1.
Error: link with name[link3] already exists.
" thrown in the tesINFO: From Testing //multibody/multibody_tree/parsing:multibody_plant_sdf_parser_test:
t body.
[  FAILED  ] MultibodyPlantSdfParser.LinkWithVisuals (59 ms)

[jwnimmer-tri]

The links posted in #9220 show all of the failures (that the unit tests exposes).

[amcastro-tri]

@SeanCurtis-TRI was asking me why the "high priority" @jwnimmer-tri. Could you elaborate?

[jwnimmer-tri]

Yes, good question. When we pin an external to an older revision because their master is broken with respect to us, it means that we can _never_ upgrade to any newer version, for any bugfix, new feature, security hole, or any other cause where we'd want to be able to pull in new code from upstream. In short, we are "frozen" at an old revision of sdformat, and can't unfreeze until this issue is resolved. To me, that seems like a high-priority problem. Otherwise, all parsing-related development or bugfixes are blocked indefinitely.

[sherm1]

FYI @nkoenig (we're not sure what's wrong yet)

[nkoenig]

I'll take a look at this problem.

[jwnimmer-tri]

\CC @EricCousineau-TRI FYI

[scpeters]

I've been able to reproduce the problem, and I've been testing a branch from @nkoenig that appears to fix it:

https://github.com/RobotLocomotion/drake/compare/master...nkoenig:sdf_include_tags_update

I'll try rebasing it and submitting a pull request

[sherm1]

Thanks, @scpeters!

[amcastro-tri]

Excellent, thanks! keep us updated @scpeters

[scpeters]

I just finished rebasing the branch (it was a bit tricky), and I'll test it tomorrow.

• https://github.com/RobotLocomotion/drake/compare/master...scpeters:sdf_include_tags_update?expand=1

[scpeters]

Rebasing that branch didn't help, but one weird console error stuck out to me:

~
Error: link with name[link3] already exists.
~

There's a bug in the sdf parser that duplicates all sdf elements whenever an unknown element is encountered (starting at this line in parser.cc). Part of it is a bug in the copyChildren function, that I've fixed in the following branch:

• https://bitbucket.org/osrf/sdformat/branch/unknown_element_copy_children?w=1#diff

It still has a problem, as N unknown elements will be copied N times. That's fine if N == 1, but it should be easy to fix this bug too. I'll submit an sdformat pull request after I fix that part.

[scpeters]

upstream fix: https://bitbucket.org/osrf/sdformat/pull-requests/462/parser-dont-duplicate-elements-when/diff

[scpeters]

and it's merged upstream https://bitbucket.org/osrf/sdformat/commits/2fed80e6bc44afe6efc3ff71a67e14defb96cce5

[scpeters]

@jwnimmer-tri is that enough or do you want me to update the bazel workspace too?

[jwnimmer-tri]

Nah, I think @amcastro-tri or @SeanCurtis-TRI can upgrade Drake to use that commit.

[amcastro-tri]

cc'ing @EricCousineau-TRI, I believe you wanted to use this right away.

[EricCousineau-TRI]

Sorry for the late follow-up, but it's unclear who is responsible for the next step in closing this issue. I will briefly check to see if our current version is still affected by this, but if not, we will need to (a) lower the priority unless that's inappropriate, and (b) find one owner to be responsible for fixing this.