RobotLocomotion / drake

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

Diagnostics for unsupported SDFormat and URDF stanzas #16229

Closed jwnimmer-tri closed 2 years ago

jwnimmer-tri commented 2 years ago

When Drake's multibody Parser is loading an SDFormat or URDF input file, if the file calls for features that Drake does not implement yet, we should not silently ignore that input. Doing so leads to user confusion, where the user gave us a robot or scene that says one thing, but our code does something else.

One specific example is the failure mode in #16181.

We should fix Drake so that at minimum we log a console warning about the bad data. Ideally, the Parser object would offer a structured feedback channel or option to promote these warnings into errors or exceptions, or other such user-informed customization of the error handling.

There might be specific elements where silently ignoring them would not be a surprise to a user (in a case where the element's effect was obviously out-of-scope for Drake's feature set). We can deal with those on a case-by-case basis.

jwnimmer-tri commented 2 years ago

A related case is that the XML input is valid and supported, but resource files that it refers to (images or meshes) are not supported by our geometry back-end. That would also be useful to warn about, but I'd like to scope this issue to only the XML data.

DamrongGuoy commented 2 years ago

I assume this issue includes tags from drake too? For example, I'm working on #16224 that will change the tag drake:soft_hydroelastic to drake:compliant_hydroelastic.

I'm testing my code on a client code (anzu) before merging my code into drake master, and I observe the following phenomena. If I keep the old tag in SDF file instead of updating to the new tag, it will ignore hydroelastic contact models and crash in point-contact models with the following error message. The message gave no hint about the SDF tag. Technically the message is correct because it did crash in MultibodyPlant; however, the root cause is the wrong SDF tag that tricked MbP into the wrong setup.

C++ exception with description "MultibodyPlant's discrete update solver failed to converge at simulation time =    3.11 with discrete update period =   0.001. This usually means that the plant's discrete update period is too large to resolve the system's dynamics for the given simulation conditions. This is often the case during abrupt collisions or during complex and fast changing contact configurations. Another common cause is the use of high gains in the simulation of closed loop systems. These might cause numerical instabilities given our discrete solver uses an explicit treatment of actuation inputs. Possible solutions include:
  1. reduce the discrete update period set at construction,
  2. decrease the high gains in your controller whenever possible,
  3. switch to a continuous model (discrete update period is zero),      though this might affect the simulation run time." thrown in the test body.

If I didn't know what I was doing, seeing this error message, I won't have a clue that it's related to SDF tags.

DamrongGuoy commented 2 years ago

Just to be sure: the diagnostics will include the SDF/URDF file names, right? I imagine a project uses so many SDF/URDF files, so identifying the problematic file could be challenging.

rpoyner-tri commented 2 years ago

proposal: open a new ticket for mesh types and related diagnostics. Scope this ticket to just XML parsing.

rpoyner-tri commented 2 years ago

notes from f2f with @jwnimmer-tri.

cases: completely wrong text, messed-up text (maybe stale spelling?), correct but unsupported (mimic for example).

approximate goal: total parse. caveat: ignition output makes that tricky. Additional case: foreign namespaces maybe we want to ignore silently. sdformat has some elements that we may want to also ignore silently. geometry properties may have some special fun.

Some old code prototype: https://reviewable.io/reviews/jwnimmer-tri/drake/7 esp. diagnostic policy

tactics for totality: re-look at tests, maybe reorg, add new negative tests. Given current code structure, probably have to do a bunch of manual reasoning.

warn-once? maybe nice, but probably not necessary.

sdformat errors should be redir through drake log asap. @jwnimmer-tri will file and fix (#16344).

jwnimmer-tri commented 2 years ago

The sdformat::Error API might contain some useful subject matter background / ideas.

rpoyner-tri commented 2 years ago

Summary of thoughts toward a first PR:

rpoyner-tri commented 2 years ago

draft PR for design discussion: #16560

rpoyner-tri commented 2 years ago

This issue needs splitting. SDF and URDF are being worked as separate tasks, and diagnostic plumbing can be separated from semantic upgrades (more warnings).

jwnimmer-tri commented 2 years ago

It's a bit difficult to find within github's commit message timeline spam, but there is a checklist immediately above where this issue has been split into five separate, more specific topics.

I'll claim that those five sub-issues are collectively exhaustive, so that there is no need to keep this meta-issue open anymore. Please correct me if I've missed anything.