parsing: Warn non-URI subresource paths

[EricCousineau-TRI]

Edit: Original issue title was "parsing: Determine best semantics for resource URIs for tooling compatibility.

---

Follow-up from comment in #10156: https://reviewable.io/reviews/robotlocomotion/drake/10156#-LTZ2ckB7Q2kSxueKNC3

At present, we use a shy mix of supporting package map URIs, while also supporting URIs that are relative to the given SDF or URDF file.

First, we should confirm that the relative path approach is actually valid within SDF and URDF. Second: If it is not valid, we should consider deprecating this behavior in favor of compatibility. If it is valid, then yeah, this issue gets closed easily.

\cc @jwnimmer-tri

[EricCousineau-TRI]

Following up to @edrumwri's comment in #10156:

While it's not part of the URI spec, we're clearly able to parse this cleanly and it's less brittle than typical URI applications (IMO), since the expected case will be a file (and perhaps a URL in the future).

My ideal is that we adhere to the specification strictly so that other tools can parse our files without jumping through extra hoops. If existing ROS tooling (or say MoveIt!) can parse our URDFs / SDFs with relative paths, then sure, let's plan to keep non-URI paths. If not, we should plan to phase it out.

[jwnimmer-tri]

\CC @IanTheEngineer @cottsay FYI this seems related to #10531 (possibly even a duplicate?)

[jwnimmer-tri]

In #10531, we repaired Drake to use only URIs (not relative paths) in our provided models.

What remains here is to at least issue warnings when using relative filenames (a non-standard behavior).

Possibly we could also deprecate and remove that feature, but simply issuing a warning seems like it would be good enough, for my taste.

\CC @rpoyner-tri

[jwnimmer-tri]

@EricCousineau-TRI we might tackle this soon (@rpoyner-tri or myself).

I assume you would not object to Drake issuing parser warnings when parsing encounters a relative filename? (The warning could explain that relative filenames are a Drake extension to the file formats.)

Then, if we do add those warnings are you OK if we close this ticket as sufficiently resolved?

I could maybe still see us also deprecating and removing the filename feature entirely. The reason I'd support that is if it made our codebase easier to maintain, not really because of "non-standard" though -- I think the warning is enough for the standardization angle. Maybe @rpoyner-tri wants to weigh in on how onerous it is for our code to support the relative filenames.

[rpoyner-tri]

It's not so much filename support itself, as it is the combination of yet-another feature with a legacy system whose original implementation was almost all free functions. Try $ git grep -i root.*dir multibody/ | wc -l to get a sense of the splatter. #16677 made things a hair better than previously.

I wouldn't mind getting rid of it, but it's just one of many parsing warts at this point.

[EricCousineau-TRI]

No objection here! I think a warning would be great, and have no objection to deprecating and removing the feature.

[...] to get a sense of the splatter [...]

Eep! But fwiw, if you remove tests (and dev), it at least halves it - so slightly better?

$ git log -n 1 --oneline --no-decorate
4cfbb2e03d Resolves issue #17246 by fixing order of state in quadrotor_dynamics_test.cc (#17254)
$ find multibody/ -type f -a ! -wholename '*test*' -a ! -wholename '*/dev/*' | xargs grep -ni 'root.*dir' | wc -l
45

That being said, I think it's fine to defer as well!

[EricCousineau-TRI]

And yes, I think having the warning would be sufficient grounds to close the issue.

[jwnimmer-tri]

BTW I tagged this "bug" under the premise of "Drake fails to diagnose malformed input and tell the user about it".