Avoid duplicate warnings in multibody XML parsers

[RussTedrake]

The multibody parsers, in particular, had a habit of being very noisy, since if an unsupported element or attribute is used once it is likely used many times. This actually made it hard to visually inspect the output to see the unique set of unsupported tags. This change significantly improves the output for examples like in #20444.

+@rpoyner-tri for feature review, please.

---

This change is 

[rpoyner-tri]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers

[rpoyner-tri]

+@jwnimmer-tri for Monday platform review.

Reviewable status: LGTM missing from assignee jwnimmer-tri(platform)

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):
When the user calls parser.AddModels() on a DMD file that cites the same URDF file multiple times with different model instance names (e.g., left arm vs right arm) -- what is our desired behavior for warning de-duplication, and does this PR do it that way?

---

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):
The heuristic used here is exact string matching. Does that mean that changing a warning message text from "A link has specified the youngs_modulus tag ..." to "Link '{}' has specified the youngs_modulus tag ..." should be defective now, since we will no longer suppress multiple copies of that warning (each one will have a unique name)?

Is it defective in this PR to have messages like "Joint '{}' specifies a mimic element that will be ignored ..." which are not being de-duplicated?

---

[RussTedrake]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

When the user calls parser.AddModels() on a DMD file that cites the same URDF file multiple times with different model instance names (e.g., left arm vs right arm) -- what is our desired behavior for warning de-duplication, and does this PR do it that way?

Honestly, I would be ok with either solution, and would favor less duplication always (so that the user can actually read the output and see the warnings more clearly), even when multiple model files are involved. My reading of the code is that ProcessModelDirectives does indeed make a workspace and pass it around to the subparses, so one diagnostic policy / warn once cache is used for all of the parses. I could cover that with a test if you think it's important.

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The heuristic used here is exact string matching. Does that mean that changing a warning message text from "A link has specified the youngs_modulus tag ..." to "Link '{}' has specified the youngs_modulus tag ..." should be defective now, since we will no longer suppress multiple copies of that warning (each one will have a unique name)?

Is it defective in this PR to have messages like "Joint '{}' specifies a mimic element that will be ignored ..." which are not being de-duplicated?

It's a fair question. My thinking was coming from the other direction... if the message that the parser is emitting is an exact duplicate (except for the location/line number), then it's pretty easy to convince myself that we risk little by suppressing the duplicate message. We could further clean up the warnings printed from the code if they are still producing too much spam, but I might stop short of actively requiring people to print less information in the warning messages.

---

[jwnimmer-tri]

I probably could have provided more context for my questions. They were just the first steps on a journey. The over-arching question is what our design intention is as a team, and how we put that into the code and comments so that it remains intact as this code is co-maintained over time.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):

Previously, RussTedrake (Russ Tedrake) wrote…

Honestly, I would be ok with either solution, and would favor less duplication always (so that the user can actually read the output and see the warnings more clearly), even when multiple model files are involved. My reading of the code is that ProcessModelDirectives does indeed make a workspace and pass it around to the subparses, so one diagnostic policy / warn once cache is used for all of the parses. I could cover that with a test if you think it's important.

If the answer is "we don't care", then it would suffice to add comments to the tinyxml2 helper explaining that it is only silencing duplicates per model instance instead of per call to "Add...". So only a small problem of clarity.

If the answer is "we should also silence those", then there is more code and tests to write.

---

a discussion (no related file):

Previously, RussTedrake (Russ Tedrake) wrote…

It's a fair question. My thinking was coming from the other direction... if the message that the parser is emitting is an exact duplicate (except for the location/line number), then it's pretty easy to convince myself that we risk little by suppressing the duplicate message. We could further clean up the warnings printed from the code if they are still producing too much spam, but I might stop short of actively requiring people to print less information in the warning messages.

Imagine this hypothetical -- a future developer adds more detail to a warning message, and now some of the tests here fail (because they will now be insisting on only a single warning). How will the developer know whether their code is wrong, or the test expectation is wrong? At the minimum, some comments in the test could mitigate that -- if the test is just intended to highlight current behavior, instead of prevent regressions, then we could comment it as "the number of warnings here doesn't matter, just change the test to match whatever the code does". But that would seem to me to defeat your purpose here.

Stepping back, I don't think the implementation strategy used here is compatible with any cohesive and durable design. It's plausible and probably even desirable for any given warning message to contain pattern-substituted text (i.e., fmt::format). If this design doesn't support warning strings with fmt messages, then it's probably not a design we should use.

---

[rpoyner-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform)

a discussion (no related file):
Having read the discussion so far, I'm having second thoughts.

• In my workflow, I care almost exclusively about the file and line number, and only secondarily about the contents. Following the general robotics misapprehension that XML files are user editable, I use the messages in my "IDE" (emacs) to bring me to the point that needs editing. We could land this patch, but I would probably end up making it configurable or working around it to maintain my workflow.
• As a general matter in handling diagnostics, it's a good idea to separate generation of them from the their filtering. In particular, the filtering and presentation should be configurable, but the generation should not be. We have some separation in this patch, but we don't (yet) have any configurability.

There are other designs we could consider:

• just limit the total number of warnings from one Parse(), similar to compilers that clam up after 100 messages or so.
• expose (maybe with Experimental or Advanced doc labels) the diagnostic policy object from Parser, so that users can implement whatever filtering they want.

I have some sympathy for the second solution, since there are some one-off workflows that might come in handy when doing parser development and maintenance.

---