[parsing] Package map manipulation is untested

[SeanCurtis-TRI]

In multibody::Parser() we have two lines that do the following:

package_map_.PopulateUpstreamToDrake(file_name);

Once in [Parser::AddAllModelsFromFile]() and once in [Parser::AddModelFromFile](). Deleting those two lines has no impact on the unit tests.

In the unit tests there is a test that feels like it should relate to those removed lines, however, it doesn't require those lines to exist to pass. And, generally, it's unclear what aspect of Parser it is testing other than a general dependency on the quality of the data in the PackageMap.

At the very least, the two invocations of PopulateUpstreamToDrake should be properly tested (and the misleading PackageMap-related test should possibly be removed).

[SeanCurtis-TRI]

I've linked a PR against this issue that deletes the two lines to see if they have an effect beyond the unit tests.

[SeanCurtis-TRI]

The code in question was added in #10156.

[jwnimmer-tri]

If there's any uncertainty with respect to how the PackageMap and Parser should inter-operate, I'm happy to try to explain (and/or improve the design documentation).

[SeanCurtis-TRI]

In this case, there's some behavior in the parser that isn't tested in its unit test. But the ManipulationStation definitely depends on it. So, I think the real fix is to make sure a unit test gets added to parser_test.cc that actually is sensitive to the code in parser.cc. :)

This came up because I was adding a "parse from string" API to the parser and I hit the lines in question and found I was unable to truly understand the intended effect of the code in question.

[SeanCurtis-TRI]

So, having looked through the code and the test, I infer the following behavior:

With a new Parser (where no packages have been explicitly added via PopulateFromFolder()) I have two different behaviors upon loading an arbitrary urdf/sdf:

1. If the model is located in Drake's tree, then all valid package.xml that can be found in directories between Drake root and model directory get added to the package map implicitly.
2. For a model located in a different tree, no searching takes place. Even if the parent directory has a package specificaiton, it is not searched for nor found. The only way to resolve that package would be to explicitly call PopulateFromFolder().

@jwnimmer-tri Can I get confirmation that this is the desired behavior?

[jwnimmer-tri]

With a new Parser, if the model is located in Drake's tree, then all valid package.xml that can be found in directories between Drake root and model directory get added to the package map implicitly. For a model located in a different tree, no searching takes place. Even if the parent directory has a package specification, it is not searched for nor found.

Correct.

The only way to resolve that package would be to explicitly call PopulateFromFolder().

Well, any of the Add or Poplate methods on PackageMap could be used, not just that one.

Also of note: per #10531 and #9308, all of the SDF and URDF files in Drake should be using package URIs for subresources (obj, mtl, etc), never using relative paths. Most are actually wrong right now (which is why so few unit tests failed when you nixed the two lines). One we make that change, having package.xml files be implicitly loaded within Drake will become even more relevant.

[SeanCurtis-TRI]

Closed by #12871