sdformat icon indicating copy to clipboard operation
sdformat copied to clipboard

BUG: resolve XSD aliasing and parse missed elements

Open FirefoxMetzger opened this issue 4 years ago • 9 comments

🦟 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:type in types.xsd from 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)
  • [ ] codecheck passed (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

FirefoxMetzger avatar Jul 28 '21 13:07 FirefoxMetzger

@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

FirefoxMetzger avatar Aug 02 '21 04:08 FirefoxMetzger

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.

azeey avatar Nov 29 '21 16:11 azeey

@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"/>).

Bi0T1N avatar Mar 12 '22 17:03 Bi0T1N

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).

FirefoxMetzger avatar Mar 13 '22 21:03 FirefoxMetzger

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 avatar Mar 25 '22 04:03 chapulina

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 required value take priority over a default value or vice versa? (see: https://github.com/ignitionrobotics/sdformat/issues/637 for details)

FirefoxMetzger avatar Mar 31 '22 10:03 FirefoxMetzger

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

chapulina avatar Mar 31 '22 14:03 chapulina

Contributor

If we found:

  • a linter that could parse XSD 1.1 to replace XMLLint
  • It can be integrated with colcon test
  • 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

Ryanf55 avatar Apr 20 '24 16:04 Ryanf55

Maybe. https://github.com/gazebosim/sdformat/pull/1377 is also another direction--basically rename the tags that are causing problems.

azeey avatar Apr 22 '24 15:04 azeey