-
Notifications
You must be signed in to change notification settings - Fork 95
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reference local schema files #535
base: sdf9
Are you sure you want to change the base?
Conversation
Signed-off-by: Jenn Nguyen <[email protected]>
sdf/1.7/light_state.sdf
Outdated
@@ -1,5 +1,5 @@ | |||
<!-- State information for a light --> | |||
<element name="light" required="*"> | |||
<element name="light_state" required="*"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should be changing these names because they are actually used by Gazebo-classic. If I remember correctly, keeping the original names causes an issue with the generated xsd. I was hoping we can fix that issue without renaming these elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed them back in aef3ab8
tools/xmlschema.rb
Outdated
@@ -130,8 +135,7 @@ def printIncludeRef(_file, _spaces, _inc) | |||
|
|||
################################################# | |||
def printInclude(_file, _spaces, _attr) | |||
loc = "http://sdformat.org/schemas/" | |||
loc += _attr.attributes['filename'].sub("\.sdf","\.xsd") | |||
loc = _attr.attributes['filename'].sub("\.sdf","\.xsd") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have been more clear in the issue description in that we do still want to be able to have xsd files that can be stored on sdformat.org/schemas. At a glance, this looks like it will embed the absolute paths of the local machine in to the generated schemas. That's useful for tests, but I think we do still want this script to generate xsd that can be uploaded to the website.
I wonder if instead of using <xsd:include>
, we just paste the contents and generate one xsd file that can be used for both testing and for the website. This might also solve the problem I mentioned in https://github.com/osrf/sdformat/pull/535/files#r609021282
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip! How's this? aef3ab8
Signed-off-by: Jenn Nguyen <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf9 #535 +/- ##
=======================================
Coverage 86.67% 86.67%
=======================================
Files 61 61
Lines 9531 9531
=======================================
Hits 8261 8261
Misses 1270 1270 Continue to review full report at Codecov.
|
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good so far. Just some minor comments to make pose
work properly.
sdf/1.7/schema/types.xsd
Outdated
@@ -26,12 +26,13 @@ | |||
|
|||
<xsd:simpleType name="pose"> | |||
<xsd:restriction base="xsd:string"> | |||
<xsd:pattern value="(\s*(-|\+)?(\d+(\.\d*)?|\.\d+|\d+\.\d+[eE][-\+]?[0-9]+)\s+){5}((-|\+)?(\d+(\.\d*)?|\.\d+|\d+\.\d+[eE][-\+]?[0-9]+))\s*"/> | |||
<xsd:pattern value="(\s*(-|\+)?(\d+(\.\d*)?|\.\d+|\d+\.\d+)([eE][-\+]?[0-9]+)?\s+){5}(-|\+)?(\d+(\.\d*)?|\.\d+|\d+\.\d+)([eE][-\+]?[0-9]+)?\s*"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update this for 1.7 and up because <pose/>
is now valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 1.7 to accept <pose/>
: 75b33b6
Also updated 1.5 & 1.6 regex for pose
(does not accept <pose/>
) and vector3d
tools/xmlschema.rb
Outdated
doc = REXML::Document.new File.new(loc) | ||
|
||
doc.elements.each_with_index("element") do |elem, i| | ||
printXSD(_file, _spaces+2, elem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we should use printElem
instead of printXSD
here so that the pose
element gets the pose
type. Right now this is what it generates
<xsd:element name='pose'>
<xsd:annotation>
<xsd:documentation xml:lang='en'>
<![CDATA[A position(x,y,z) and orientation(roll, pitch yaw) with respect to the frame named in the relative_to attribute.]]>
</xsd:documentation>
</xsd:annotation>
<xsd:complexType mixed='true'>
<xsd:attribute name='relative_to' type='xsd:string' >
<xsd:annotation>
<xsd:documentation xml:lang='en'>
<![CDATA[
Name of frame relative to which the pose is applied.
]]>
</xsd:documentation>
</xsd:annotation>
</xsd:attribute>
</xsd:complexType>
</xsd:element>
(1) We have <xsd:complexType mixed='true'>
which indicate that <pose>
can have child elements, which is not currently true
(2). The xsd type pose
is not used at all so it is not validating based on the regex. You can try this by trying to validate 1.0` in an sdf file. It passes currently.
When using printElem
, it generates:
<xsd:element name='pose'>
<xsd:annotation>
<xsd:documentation xml:lang='en'>
<![CDATA[A position(x,y,z) and orientation(roll, pitch yaw) with respect to the frame named in the relative_to attribute.]]>
</xsd:documentation>
</xsd:annotation>
<xsd:complexType>
<xsd:simpleContent>
<xsd:extension base='pose'>
<xsd:attribute name='relative_to' type='xsd:string' >
<xsd:annotation>
<xsd:documentation xml:lang='en'>
<![CDATA[
Name of frame relative to which the pose is applied.
]]>
</xsd:documentation>
</xsd:annotation>
</xsd:attribute>
</xsd:extension>
</xsd:simpleContent>
</xsd:complexType>
</xsd:element>
This extends the pose
type, so validation works properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 75b33b6
foreach(FIL ${sdfs}) | ||
get_filename_component(ABS_FIL ${FIL} ABSOLUTE) | ||
get_filename_component(FIL_WE ${FIL} NAME_WE) | ||
set(root root.sdf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the same changes to the all the sdf/*/CMakeLists.txt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
75b33b6 I've made the changes for until 1.5 since 1.4 and older do not print out the schemas
tools/xmlschema.rb
Outdated
@@ -72,13 +73,22 @@ def printElem(_file, _spaces, _elem) | |||
printDocumentation(_file, _spaces+2, _elem.elements["description"].text) | |||
end | |||
|
|||
_file.printf("%*s<xsd:complexType>\n", _spaces+2, "") | |||
if _elem.attributes['name'] == "pose" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't encounter pose
here because it has type='pose'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed 75b33b6
tools/xmlschema.rb
Outdated
printDocumentation(_file, _spaces+2, _elem.elements["description"].text) | ||
end | ||
|
||
if _elem.attributes['name'] == "pose" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use printElem
as I suggested, I'm not sure if we need a special treatment of pose
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed 75b33b6
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Unfortunately, this is not working for nested models 😞.. currently working on a solution |
We are going to table this work for now and focus on higher priority tasks since we don't have an easy solution that works for nested models. |
Signed-off-by: Jenn Nguyen <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
#1455 might help with this, but we'll need to make several updates to this PR to take advantage of that. I don't think we'll be able to get to it with the limited time we have before the code freeze, so I'll go ahead and remove the |
Signed-off-by: Jenn Nguyen [email protected]
🦟 Bug fix
Fixes #530
Summary
The xmlschema.rb script converts .sdf description files to xml schema (.xsd files) and now prints the contents of
<xsd:include>
instead of referencing files from http://sdformat.org/schemas/Before the fix, running the integration test required internet connection. With the fix, the internet is no longer required.
sdf/1.7
folderINTEGRATION_schema_test
while disconnected to the internetChecklist
sh tools/code_check.sh
)test coverage)
another open pull request
to support the maintainers
Note to maintainers: Remember to use Squash-Merge