Missing required elements not being reported

[marcoag]

Description

It seems that this if statement:

sdformat/src/parser.cc

Line 1928 in 3fdc0a1

if (_sdf->GetName() == "joint" &&

Is preventing sdformat from reporting missing elements that are required according to the specification.

Steps to reproduce

Create and load and sdformat that has a missing required element (i.e. a world without gravity) and nothing will be reported. Remove the above statement in the library and the missing element errors will arise.

Way forward

I opened this issue to start a discussion on the way forward. This seems to be an ancient bug that when fixed will probably prevent many sdf files from loading successfully. I guess the safest option would be to fix it in main and add a backport fix with a flag to current supported versions of sdformat.

As a sneak peek into the impact this fix might have: just removing the if yields 202 errors from the tests in the library itself.

[scpeters]

I don't think it's necessarily wrong to add a default element when required elements aren't present; it's primarily a matter of what is the expected behavior. I think it is a bug that ball joint elements are treated differently. I'm guessing it's because the //joint/axis/xyz elements are marked as required, but ball joints shouldn't need an axis to be specified. I think this code prevents a default //axis/xyz element to be added during parser. I think the minimum goal should be to remove the special case for ball joints. We could go further and decide to change behavior, but as you've noted it would be a disruptive change, so we should think carefully about it and make sure that it is worth the effort.

[marcoag]

Isn't the expected behavior in SDFormat to complain when a required element is not present? I'm saying this mainly because of consistency across the library: we do for missing required attributes, for top level elements and for certain elements that go through the check in other parts of the code.

I have the impression the intention of that code was just to not complain for ball joint axis that don't have an <xyz> and add a default instead.

I agree that ball joints shouldn't be a special case.

I guess an option could be to keep adding defaults and only add the complain to main. Although the effort of adding the complain even just to main can be important so don't know if it's worth it.

[scpeters]

I guess an option could be to keep adding defaults and only add the complain to main. Although the effort of adding the complain even just to main can be important so don't know if it's worth it.

We can add an option to ParserConfig in garden with the default set to the current behavior and then consider changing the default on main

[marcoag]

I like that idea. Still wanna confirm if changing the default on main is worth the effort (fixing at least 200 test errors plus whatever comes from the rest of gazebo.)... @azeey?

[azeey]

I think adding the strict mode option to ParserConfig makes sense. Making strict mode the default in main may not be feasible if we have to fix all the errors, but maybe our new tests can start using strict mode and we can gradually fix the errors in existing tests. I think some errors will be easy to fix by just adding the missing element, but some might be difficult because we've come to rely too much on the old behavior. So, the gradual fix approach seems more feasible to me.