RobotLocomotion / drake

Model-based design and verification for robotics.
https://drake.mit.edu
Other
3.27k stars 1.26k forks source link

SDFormat parser: support first-class feedback channel #16784

Closed rpoyner-tri closed 2 years ago

rpoyner-tri commented 2 years ago

Split from https://github.com/RobotLocomotion/drake/issues/16229. Proposal is to plumb the full SDF parser via common/diagnostic_policy, so that handling of errors and warnings can be configured.

Victory condition: all instances of throw, drake::log->warn(), and log_once reachable from AddModelsFromSdf() are replaced by diagnostic policy usage. There may be specific exceptions for difficult cases.

TODOs:

jwnimmer-tri commented 2 years ago

Given the implementation tactic uncertainty in #16785, I'm going to change the victory condition here. Re-spelling the plumbing to use "diagnostic.Error" vs "throw" is not terribly useful. We are not certain that the existing call paths are were we're going to detect new kinds of errors. If we find it necessary, we can add new plumbing as-needed.

The new victory conditions are:

EricCousineau-TRI commented 2 years ago

Per Slack DM, in Anzu PR 8769 (r1), I could see super useful filenames + lineno. for URDF files, but not for SDFormat files. Any chance that could be part of this work? (and might you want me to work w/ OSRC on this?)

Most notably, this error message appeared w/o direct trace back to files (which were SDFormat): https://github.com/RobotLocomotion/drake/blob/da38019a96d7015628be1f620c0fb00aa97307c0/multibody/parsing/detail_common.cc#L79-L83

jwnimmer-tri commented 2 years ago

Our original intention for the scope of this issue was to have the parsing code use a diagnostic object at all to report messages, instead of bespoke throw or drake::log() or etc. -- as kind of a baseline of infrastructure. (And I've even down-scoped it since that point.)

We didn't intend to try to update all of the diagnostic calls to also add in the filename or line number. Doing so would definitely be useful, but probably worth its own separate issue. If OSRC wanted to jump in and work on that, I think it would be very helpful.

EricCousineau-TRI commented 2 years ago

Sounds good! --> #17053