Skip to content
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

Draft
wants to merge 9 commits into
base: sdf9
Choose a base branch
from
Draft

Reference local schema files #535

wants to merge 9 commits into from

Conversation

jennuine
Copy link
Collaborator

@jennuine jennuine commented Apr 7, 2021

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.

  1. In the build directory, delete the sdf/1.7 folder
  2. Recompile
  3. Run INTEGRATION_schema_test while disconnected to the internet

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Code check passed (In source directory, run sh tools/code_check.sh)
  • 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

Signed-off-by: Jenn Nguyen <[email protected]>
@jennuine jennuine requested a review from azeey April 7, 2021 00:44
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Apr 7, 2021
@@ -1,5 +1,5 @@
<!-- State information for a light -->
<element name="light" required="*">
<element name="light_state" required="*">
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

@@ -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")
Copy link
Collaborator

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

Copy link
Collaborator Author

@jennuine jennuine Apr 23, 2021

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

@codecov-commenter
Copy link

codecov-commenter commented Apr 23, 2021

Codecov Report

Merging #535 (aef3ab8) into sdf9 (066b2fe) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 066b2fe...aef3ab8. Read the comment docs.

@jennuine jennuine requested a review from azeey June 10, 2021 21:11
Copy link
Collaborator

@azeey azeey left a 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.

@@ -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*"/>
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

doc = REXML::Document.new File.new(loc)

doc.elements.each_with_index("element") do |elem, i|
printXSD(_file, _spaces+2, elem)
Copy link
Collaborator

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.

Copy link
Collaborator Author

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)
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

@@ -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"
Copy link
Collaborator

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed 75b33b6

printDocumentation(_file, _spaces+2, _elem.elements["description"].text)
end

if _elem.attributes['name'] == "pose"
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed 75b33b6

@jennuine
Copy link
Collaborator Author

Unfortunately, this is not working for nested models 😞.. currently working on a solution

@azeey
Copy link
Collaborator

azeey commented Jun 22, 2021

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.

@chapulina chapulina added the enhancement New feature or request label Jul 23, 2022
@azeey azeey added beta Targeting beta release of upcoming collection and removed beta Targeting beta release of upcoming collection labels Jul 31, 2023
@azeey azeey added the beta Targeting beta release of upcoming collection label Jul 29, 2024
@azeey
Copy link
Collaborator

azeey commented Aug 19, 2024

#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 beta label.

@azeey azeey removed the beta Targeting beta release of upcoming collection label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel enhancement New feature or request
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

4 participants