BUG: resolve XSD aliasing and parse missed elements
🦟 Bug fix
Fixes https://github.com/ignitionrobotics/sdformat/issues/642 https://github.com/ignitionrobotics/sdformat/issues/637 https://github.com/ignitionrobotics/sdformat/issues/633 https://github.com/ignitionrobotics/sdformat/issues/632
Summary
This PR adds a new generator for XSD from the current SDFormat. It is currently set up to convert SDFormat v1.8, but can easily be backported to the other versions by updating the corresponding CMakeLists.
It fixes multiple issues with the old parser:
- parse all sdf elements nested deeper than 1 level below root (previous:
and tags were ignored, see https://github.com/ignitionrobotics/sdformat/issues/633) - resolve name aliasing of elements defined in X.sdf and X_shape.sdf (see https://github.com/ignitionrobotics/sdformat/issues/632). This is done by introducing a unique namespace for each file and correctly referencing the desired type in the namespace in question.
- change
types:typeintypes.xsdfrom double to string and add the respective restriction (see: https://github.com/ignitionrobotics/sdformat/issues/632)
It also handles nested models and nested model states. This was apparently a blocking issue in previous attempts to refactor this script. I didn't encounter this problem directly; however, I had to guess the desired meaning of the ref=X attribute used in this case. I left a comment in the appropriate place.
On a high level, the generator is based around named types. It creates two files for each filename.sdf: one filename.xsd and one filenameType.xsd (the actual workhorse), e.g. model.xsd and modelType.xsd. FilenameType.xsd holds the complex type used by the root element of filename.sdf and filename.xsd is a shim to help parsers to make sense of partial sdf.
Checklist
- [ ] Signed all commits for DCO
- [ ] Added tests
- [ ] Updated documentation (as needed)
- [ ] Updated migration guide (as needed)
- [ ]
codecheckpassed (See contributing) - [ ] All tests passed (See test coverage)
- [ ] While waiting for a review on your PR, please help review another open pull request to support the maintainers
Note to maintainers: Remember to use Squash-Merge
@azeey I partially addressed #649 by replacing the <xs:choice> with a <xs:all> whenever all child elements are bounded (all child elements have required=0 or required=1.
As a result, two validation tests were breaking:
The double_pendulum.sdf didn't declare some required keyword arguments. I added them (with arbitrary values); you may wish to double-check if this is the preferred fix, or if it should be fixed by changing the spec. In particular the <use_parent_model_frame> element looks like it should be optional and not required.
The other test is pr2.sdf, which fails because it is SDF v1.4, but the oldest schema that has been generated thus far is v1.5. Naturally, there are a few problems with parsing it; starting with the non-backwards compatibility of SDF (<dynamics> requires a <spring_reference> and <spring_stiffness>) and <axis> requires above mentioned <use_parent_model_frame>. I could again fix it in the pr2.sdf; however, before I do this I need to know if this is the correct place to do so, or if it is a problem in SDF.
Please advise :D
Unfortunately, we didn't have the resources to review this PR in October or November and it looks like we we may not be able to in the coming couple of months. My apologies for the delay, but I wanted to let you know it's still in our radar.
@FirefoxMetzger
Can you please rebase this on the head of the current sdf branch? The changes for #642 have already been merged via #662 (with a proper <xsd:whiteSpace value="collapse"/>) but are also listed as changes here (without <xsd:whiteSpace value="collapse"/>).
Thanks for the review @Bi0T1N . I won't get around to looking at this for another couple of days, because I have a deadline coming up and am tied up elsewhere.
That said, I have a more up-to-date version of this script that lives here. I never pushed the changes upstream, because it uses features from a more recent version of python (dataclasses), and schema generation currently doesn't seem to have high priority here (which is completely fine by me). You might also be interested in the generated schemata (currently up to v1.8).
Hi @FirefoxMetzger , unfortunately we haven't had time to go over this PR yet. Edifice (sdf11) will EOL next week. How about retargeting this at Fortress (sdf12)?
Hi @FirefoxMetzger , unfortunately we haven't had time to go over this PR yet. Edifice (sdf11) will EOL next week. How about retargeting this at Fortress (sdf12)?
@chapulina Sure that can be done; however, I don't feel very motivated to add more work to this PR just for it to keep sitting around waiting for a reviewer. Once someone from the team has the time to review and we can resolve the action items that need a decision for this PR I'm happy to pick it up again.
For reference these are the open points:
- Should we use XSD 1.1 instead of XSD 1.0? Biggest pro: better representation of SDF that avoids false negatives (accepts invalid SDF) when validating (see: https://github.com/ignitionrobotics/sdformat/pull/643#discussion_r839390021 for details). Biggest Con: xmllint (which is currently used by the ign command line tool) doesn't support XSD 1.1
- Does an elements
requiredvalue take priority over a default value or vice versa? (see: https://github.com/ignitionrobotics/sdformat/issues/637 for details)
Sure that can be done; however, I don't feel very motivated to add more work to this PR just for it to keep sitting around waiting for a reviewer.
That's very fair 👍🏽 We'll let you know when we can put some time into this. Thanks for the patience
Contributor
If we found:
- a linter that could parse XSD 1.1 to replace XMLLint
- It can be integrated with
colcontest - It can be run from VSCode Would that help the situation?
There is a recent-ish list here of options: https://softwarerecs.stackexchange.com/questions/55262/xml-validator-using-xsd-1-1
Maybe. https://github.com/gazebosim/sdformat/pull/1377 is also another direction--basically rename the tags that are causing problems.