Inconsistencies between SDF tests, SDF docs, and sdformat/sdf/**/*.sdf files

[FirefoxMetzger]

I apologize for the slightly generic title. I figured that I will create this summative issue instead of creating a new issue for each problem to avoid flooding the issue list (any more than I already have).

There are multiple places where the sdf files used for CI, the SDF spec, and the sdf used to generate XSD bindings disagree. The problem with that is that I can't resolve these issues myself; I don't know which of these is authorative. Likely it should be decided on a case-by-case basis; however, I don't have the authority to make this decision.

Here is the list of mismatches that I am aware currently off:

1. [x] SDF (all versions) /link/sensor specifies @required=0 (meaning 0 or 1 occurrences iirc), but the test world_with_state.sdf (v1.6 SDF) (which uses a PR2 model) defines two different sensors in several .//links. I would change @required=* in the next SDF version, and remove the additional sensor elements from the test (since 1.6 doesn't allow multiple sensors). The same problem as above occurs in sensors.sdf, which appears to specify every possible sensor inside a single link element.. sensors.sdf is SDF v1.7 Note: This is fixed in the scikit-bot schema but not yet fixed in the spec.
2. [x] sdformat/sdf/**/schema/types.xsd defines <time> as a restriction on xsd:double; however, the spec specifies it as [{seconds} {nanoseconds}] and /**/state.sdf specify uses the default "0 0". This is a duplicate of https://github.com/ignitionrobotics/sdformat/issues/642

[FirefoxMetzger]

4. [x] The spec uses "0"/"1" or 'true'/'false' interchangeably when denoting the default of boolean values.

This are the places where it uses 0/1:

• /collision/**/friction/torsional/use_patch_radius
• /collision/**/collide_without_contact
• /collision/**/split_impulse
• /joint/**/must_be_loop_joint
• /physics/**/split_impulse
• /physics/**/thread_position_correction
• /physics/**/use_dynamic_moi_rescaling

These are the places where true/false is used:

• actor/**/loop
• actor/**/auto_start
• link/**/gravity
• plugin/copy_data
• scene/shadows@enabled
• scene/grid@enabled
• visual@cast_shadows

Both lists are for each SDF version that defines the mentioned elements/attributes. It's possible to parse both, but for consistency I'd vote to use one or the other through the entire spec. Both are used about equally often, C++ users default to 0/1 and XSD uses true/false for booleans, so there is no preference for either that is immediately obvious.

[FirefoxMetzger]

5. physics/max_contacts has/(should have) no effect.

Not sure if this point should be in this list, or if it should be a separate point. This is a point where sdformat/sdf/*/.sdf is inconsistent with itself instead of being inconsistent with another aspect of SDFormat (tests/docs). I will change the title of the issue.

The spec is inconsistent in its use of required=0/required=* combined with default within elements. If an optional element has a default specified, then - in most cases - this seems to mean "if not present, it is set to this particular default value". An example of this is //pose, which should be 0 0 0 0 0 0 if not present, since None/Null doesn't seem to make sense in this case.

In the case of physics/max_contacts and collision/max_contacts this leads to an interesting interaction. Since collision/max_contacts overwrites physics/max_contacts, and collision/max_contacts sets a default of 10 (if unspecified), the value set in physics will never have any effect. Here None/Null seems to have a special meaning, so I would vote for removing the default from collision/max_contacts.

[FirefoxMetzger]

6. SDFormat v1.8 description says that only one of world/model/light is allowed, but required=* indicates 0-N.

I've noticed that the description of the sdf v1.8 tag was changed to

SDFormat base element that can include one model, actor, light, or worlds. A user of multiple worlds could run parallel instances of simulation, or offer selection of a world at runtime.

form the SDFormat v1.7 description

SDFormat base element that can include 0-N models, actors, lights, and/or worlds. A user of multiple worlds could run parallel instances of simulation, or offer selection of a world at runtime.

but the respective @required=* attributes have remained the same. I assume this is meant to be @required=0?

[azeey]

SDF (all versions) /link/sensor specifies @required=0 (meaning 0 or 1 occurrences iirc), but the test world_with_state.sdf (v1.6 SDF) (which uses a PR2 model) defines two different sensors in several .//links. I would change @required=* in the next SDF version, and remove the additional sensor elements from the test (since 1.6 doesn't allow multiple sensors)

I think the issue is that in link.sdf we have https://github.com/ignitionrobotics/sdformat/blob/22a968e3789440f142212d0ed74493445c109744/sdf/1.6/link.sdf#L44 and that overrides what in sensor.sdf. I believe it's always been possible to have multiple sensors inside //link, so I think this is correct.

sdformat/sdf//schema/types.xsd defines as a restriction on xsd:double; however, the spec specifies it as [{seconds} {nanoseconds}] and //state.sdf specify uses the default "0 0". This is a duplicate of Invalid default value for elements of type #642

Commented on #642.

The spec uses "0"/"1" or 'true'/'false' interchangeably when denoting the default of boolean values.

I'd vote for true/false.

physics/max_contacts has/(should have) no effect.

Yes, I see the inconsistency there. How default values are handled in libsdformat can be a little bit confusing. Currently, if an element with a default and @required=1 or @required=+ is missing, libsdformat actually inserts the default value for the element and the downstream application will get a true when calling sdf::Element::HasElement. And sdf::Element::GetExplicitlySetInFile has to be called to find out if the element was explicitly set by the user. If the element is not required, i.e @required != 1 and @required != + and the element is missing, the downstream application will get a false when calling sdf::Element::HasElement.

All of this to say that, the documentation is probably is correct in describing the actual behavior in Gazebo classic because since both physics/max_contacts and collision/max_contacts have @required=0, it's up to the downstream application (Gazebo classic in this case) to implement the correct override behavior. I don't think it's possible to encode the dependency between the two parameters in the SDFormat description files.

SDFormat v1.8 description says that only one of world/model/light is allowed, but required=* indicates 0-N.

sdf/1.8/root.sdf has the correct required values. The values in the individual files (i.e. model.sdf, actor.sdf) would not change because they can be 0-N when they are inside //world. When they are directly inside //sdf only one of them is allowed. BTW, this does not apply to worlds as it is possible to have multiple //world inside //sdf.

[FirefoxMetzger]

and that overrides what in sensor.sdf

I see. This overwrite doesn't seem to make it to the spec online: http://sdformat.org/spec?ver=1.8&elem=link#link_sensor

Edit: This also explains point 6.

I'll check if the XSD schema generator respects the overwrite.

Edit2: This is now fixed in my generator script downstream and part of the schema there as of v0.3.3. (The schema lives here: https://github.com/FirefoxMetzger/ropy/tree/main/skbot/ignition/sdformat/schema). It may still have to be addressed here to update the website and/or in the upstream generator script once the other points there are resolved.

And sdf::Element::GetExplicitlySetInFile has to be called to find out if the element was explicitly set by the user

This smells a bit funny. Is there ever a need/desire to know if a value has been explicitly set or comes from the spec itself? And, if so, why isn't element.value != default sufficient? I.e., what is the use-case of knowing that a value (that is exactly the default value of the spec) has been set by the user like this or not?

I'm asking, because my python SDFormat serializer will explicitly write default values into the file instead of omitting them. in XSD there is no difference between the two, so I didn't care too much. As a result, if you read a file that doesn't specify defaults and write it straight back to disk with my current python parser those default values will be created in the file. If this means different things and it is important to differentiate, I will need to think about how to make this happen with the XSD (as my bindings are just a straight up bind from the XSD).

If the element is not required, i.e @required != 1 and @required != + and the element is missing, the downstream application will get a false when calling sdf::Element::HasElement.

This seems weird. I mean the logic of having @required=1/+ populate defaults and @required=0/* set the default to None/NULL is perfectly reasonable and valid. What confuses me is that in the case of @required=0/* any @default= will have no effect ... ever ... so why specify it?

On top, there are a few elements where the default is actually really meaningful and important, despite it being set to @required=0/*. To name a few (though there are more):

//model/static: http://sdformat.org/spec?ver=1.8&elem=model#model_static //model/self_collide: http://sdformat.org/spec?ver=1.8&elem=model#model_self_collide //pose (!): http://sdformat.org/spec?ver=1.8&elem=model#frame_pose

I don't think it's possible to encode the dependency between the two parameters in the SDFormat description files.

We could remove the default from //collision/max_contacts and keep it as required=0 (making it's absence meaningful) and change required=1 for //physics/max_contacts. This way, if no value is given, the max_contacts are set to the default of //physics/max_contacts. If //physics/max_contacts is set then this value is used in the absence of max_contacts, and if //collision/max_contacts is present it overwrites the physics/max_contacts regardless of it being explicitly set or left at default.

[FirefoxMetzger]

Update (to keep track of things): Except for the default/required discussion around //collision/max_contacts and //physics/max_contacts every other problem in this issue has been resolved.