Offer an API option to display new warnings on problems with URDF to SDF conversions

[j-rivero]

🎉 New feature

Should help providing an API option to have explicit warnings for #199 and #1007.

Summary

The PR tries to address the lack of the explicit information on decisions made that end up ignoring some link definitions in the user input which can lead to many problems see #199 and #1007.

For keeping backwards compatibility, no new warning is raised unless the consumer of sdformat readFile

The PR implements a new API function:

bool readFile(const std::string &_filename, SDFPtr _sdf, bool _enable_new_warnings);

that allows users of sdf to enable new extra warnings.

If the parameter is enabled, the problems with links being ignored silently (not really, but the issues were stored in the logs) are promoted to warnings using gzwarn in the CreateSDF function.

Test it

Should be fairly trivial to change Gazebo or other consumer of the readFile API to use the new parameter. Examples of code that triggers problems can be found in #199 and #1007. Simple patch for gazebo9:

diff --git a/tools/gz.cc b/tools/gz.cc
index 7a8f3d6b16..116c94541f 100644
--- a/tools/gz.cc
+++ b/tools/gz.cc
@@ -1070,6 +1070,10 @@ bool SDFCommand::RunImpl()
     }
   }
 
+  bool new_warnings = true;
+  if (common::getEnv("GAZEBO11_BACKWARDS_COMPAT_WARNINGS_ERRORS"))
+    new_warnings = false;
+
   if (!sdf::init(sdf))
   {
     std::cerr << "ERROR: SDF parsing the xml failed" << std::endl;
@@ -1124,7 +1128,7 @@ bool SDFCommand::RunImpl()
     if (!boost::filesystem::exists(path))
       std::cerr << "Error: File doesn't exist[" << path.string() << "]\n";
 
-    if (!sdf::readFile(path.string(), sdf))
+    if (!sdf::readFile(path.string(), sdf, new_warnings))
     {
       std::cerr << "Error: SDF parsing the xml failed\n";
       return false;

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #1171 (afbd18b) into sdf9 (b7f0cba) will increase coverage by 0.00%.
The diff coverage is 76.19%.

❗ Current head afbd18b differs from pull request most recent head 1d755ae. Consider uploading reports for the commit 1d755ae to get more accurate results

@@           Coverage Diff           @@
##             sdf9    #1171   +/-   ##
=======================================
  Coverage   87.82%   87.82%           
=======================================
  Files          64       65    +1     
  Lines       10095    10112   +17     
=======================================
+ Hits         8866     8881   +15     
- Misses       1229     1231    +2     

Impacted Files	Coverage Δ
src/parser_urdf.cc	84.47% <72.41%> (+0.10%)	⬆️
src/parser.cc	78.88% <77.77%> (-0.18%)	⬇️
include/sdf/parser_urdf.hh	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[azeey]

While I think this is super helpful, I'm not sure adding a new overload is the best option. In Citadel, we only have two existing overloads, so this adds a third. In Garden, we have 6 overloads already! and how this will interact with all of them when forward ported is not clear. Starting from Fortress, these functions take a ParserConfig object, so adding it to that makes sense, but for citadel, I would prefer if we could use a different approach. Perhaps an env variable? Or backport ParserConfig?

[scpeters]

before handling sdf9, I'd like to review a pull request that targets sdf12 and uses ParserConfig. Once we have completed that review, we can decide how we want to backport it to sdf9. Does that sound ok?

[j-rivero]

sure, I'll try to create it in the next days.

[aaronchongth]

Hello @azeey, @j-rivero, @scpeters! Circling back to this discussion, I've implemented the ideas for sdf12, #1238,

• changing sdfdbg to sdfwarn whenever links are ignored during urdf->sdf conversion
• adding a setting in ParserConfig to convert urdf links with no inertia or masses smaller than 1e-6 to frames

I found that using warnings directly do not trigger any regressions, users will however start seeing them in their workflows without any change in behavior (might be a good thing, since these links are most likely mistakes)

Let me know what yall think!

Edit: I've previously opened a similar PR targeting sdf9 accidentally, and it seems to pass all the CIs as well. #1230

[azeey]

I think converting the debug messages to warnings (without any config flags) is reasonable since my expectation is that users generally do not rely on this behavior being the normal, i.e, we want to avoid spamming users who expect links to be ignored and they're okay with it, but I don't think that's most users.

[aaronchongth]

We just got #1238 merged into sdf12, changing debug messages to warnings, with more explicit information if links or joints were ignored, and suggestions to resolve those issues. There's no behavior change (but with 1 bug fix), and ParserConfig was not used. If it looks ok, we can probably try to cherry pick that back to sdf9 for gazebo11 and citadel?

[aaronchongth]

@j-rivero @azeey #1258 is the cherry-pick towards sdf9, it looks like the new warnings are not causing any regressions. Do yall think we still need an option to display newer warnings?

[azeey]

Closing since #1238 and #1258 have been merged.