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

Fix whitespace preservation behavior with TinyXML2 #359

Merged
merged 7 commits into from
Sep 10, 2020
Merged

Conversation

mjcarroll
Copy link
Contributor

This addresses a change in how whitespace is preserved/collapsed between
tinyxml and tinyxml2. The change primarily impacts SDF files that have
newlines, which is frequently done for aesthetics with include uris.

In this case, we use an alternate constructor for TinyXML2 that will
collapse whitespace by default.

Note that there is a performance impact of this behavior, which causes
the parsing to run essentially twice.

More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/

Closes #322

Signed-off-by: Michael Carroll michael@openrobotics.org

This addresses a change in how whitespace is preserved/collapsed between
tinyxml and tinyxml2. The change primarily impacts SDF files that have
newlines, which is frequently done for aesthetics with include uris.

In this case, we use an alternate constructor for TinyXML2 that will
collapse whitespace by default.

Note that there is a performance impact of this behavior, which causes
the parsing to run essentially twice.

More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/

Closes #322

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll mjcarroll linked an issue Sep 4, 2020 that may be closed by this pull request
@mjcarroll mjcarroll requested review from scpeters and azeey September 4, 2020 17:03
@scpeters scpeters added this to the libsdformat10 milestone Sep 4, 2020
@chapulina chapulina added the 🔮 dome Ignition Dome label Sep 4, 2020
Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2020

Codecov Report

Merging #359 into sdf10 will increase coverage by 0.00%.
The diff coverage is 93.33%.

Impacted file tree graph

@@           Coverage Diff           @@
##            sdf10     #359   +/-   ##
=======================================
  Coverage   87.32%   87.32%           
=======================================
  Files          60       60           
  Lines        9214     9216    +2     
=======================================
+ Hits         8046     8048    +2     
  Misses       1168     1168           
Impacted Files Coverage Δ
src/parser.cc 78.55% <93.33%> (+0.04%) ⬆️

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 095d88a...544fa8a. Read the comment docs.

@scpeters
Copy link
Member

scpeters commented Sep 4, 2020

I'm going to test a narrower approach that calls sdf::trim on the contents of <uri> tags

scpeters added a commit to scpeters/sdformat that referenced this pull request Sep 4, 2020
This copies the whitespace test from gazebosim#359
but takes a narrower approach by only trimming
whitespace from `<uri>` element text.

Closes gazebosim#322.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters
Copy link
Member

scpeters commented Sep 4, 2020

I'm going to test a narrower approach that calls sdf::trim on the contents of <uri> tags

narrower approach in #365

@chapulina chapulina added the beta Targeting beta release of upcoming collection label Sep 8, 2020
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I think this is the right approach. I'll close #365

@scpeters
Copy link
Member

scpeters commented Sep 9, 2020

@osrf-jenkins run tests please

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.

Do we need to make the same changes in the URDF parser (src/parser_urdf.cc)?

@mjcarroll
Copy link
Contributor Author

Do we need to make the same changes in the URDF parser (src/parser_urdf.cc)?

I don't believe so, because my understanding was that URDF only uses attributes in tags, versus the actual content of tags:

<pose rpy="0 0 0" xyz="0 0 0" />
vs
<pose>0 0 0 0 0 0</pose>

Is that correct?

@azeey
Copy link
Collaborator

azeey commented Sep 9, 2020

Do we need to make the same changes in the URDF parser (src/parser_urdf.cc)?

I don't believe so, because my understanding was that URDF only uses attributes in tags, versus the actual content of tags:

<pose rpy="0 0 0" xyz="0 0 0" />
vs
<pose>0 0 0 0 0 0</pose>

Is that correct?

I think you're right about that, but if the URDF contains Gazebo extensions (eg. https://answers.gazebosim.org//question/7082/using-sdf-tags-in-urdf-gazebo-extension-directly/), it could have values in tags.

@mjcarroll
Copy link
Contributor Author

URDF contains Gazebo extensions

Failed to account for that. I will update and add a test for it.

Signed-off-by: Michael Carroll <michael@openrobotics.org>
@mjcarroll
Copy link
Contributor Author

I added a test case in 2721714, it actually didn't require any adjustment for me in the actual URDF parser.

I think this is because we trim the values that we parse from URDF in the process of changing to SDF.

In the case of this test, the tinyxml2::XMLDocument doc (parsed URDF) is:

<robot name="test">
    <link name="link">
        <inertial>
            <mass value="0.1"/>
            <origin rpy="1.570796326794895 0 0" xyz="0.123456789123456 0 0.0"/>
            <inertia ixx="0.01" ixy="0" ixz="0" iyy="0.01" iyz="0" izz="0.01"/>
        </inertial>
        <visual>
            <geometry>
                <sphere radius="1.0"/>
            </geometry>
        </visual>
        <collision>
            <geometry>
                <sphere radius="1.0"/>
            </geometry>
        </collision>
    </link>
    <gazebo reference="link">
        <material>
      Gazebo/Orange
    </material>
        <mu1>
      100
    </mu1>
        <mu2>

      1000

    </mu2>
    </gazebo>
</robot>

And then sdfXml (the converted SDF) is

<sdf version="1.7">
    <model name="test">
        <link name="link">
            <inertial>
                <pose>0.123456789123456 0 0 1.570796326794895 0 0</pose>
                <mass>0.1</mass>
                <inertia>
                    <ixx>0.01</ixx>
                    <ixy>0</ixy>
                    <ixz>0</ixz>
                    <iyy>0.01</iyy>
                    <iyz>0</iyz>
                    <izz>0.01</izz>
                </inertia>
            </inertial>
            <collision name="link_collision">
                <pose>0 0 0 0 0 0</pose>
                <geometry>
                    <sphere>
                        <radius>1</radius>
                    </sphere>
                </geometry>
                <surface>
                    <contact>
                        <ode/>
                    </contact>
                    <friction>
                        <ode>
                            <mu>100</mu>
                            <mu2>1000</mu2>
                        </ode>
                    </friction>
                </surface>
            </collision>
            <visual name="link_visual">
                <pose>0 0 0 0 0 0</pose>
                <geometry>
                    <sphere>
                        <radius>1</radius>
                    </sphere>
                </geometry>
                <material>
                    <script>
                        <name>Gazebo/Orange</name>
                        <uri>file://media/materials/scripts/gazebo.material</uri>
                    </script>
                </material>
            </visual>
            <gravity>true</gravity>
            <velocity_decay/>
        </link>
    </model>
</sdf>

Signed-off-by: Michael Carroll <michael@openrobotics.org>
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 good! And thanks for adding the URDF Test.

@scpeters scpeters merged commit 784eb19 into sdf10 Sep 10, 2020
@scpeters scpeters deleted the squash_whitespace branch September 10, 2020 08:09
azeey pushed a commit to azeey/sdformat that referenced this pull request Dec 9, 2020
This addresses a change in how whitespace is preserved/collapsed between
tinyxml and tinyxml2. The change primarily impacts SDF files that have
newlines, which is frequently done for aesthetics with include uris.

In this case, we use an alternate constructor for TinyXML2 that will
collapse whitespace by default.

Note that there is a performance impact of this behavior, which causes
the parsing to run essentially twice.

More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/

Closes gazebosim#322

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
brawner pushed a commit that referenced this pull request Dec 11, 2020
This addresses a change in how whitespace is preserved/collapsed between
tinyxml and tinyxml2. The change primarily impacts SDF files that have
newlines, which is frequently done for aesthetics with include uris.

In this case, we use an alternate constructor for TinyXML2 that will
collapse whitespace by default.

Note that there is a performance impact of this behavior, which causes
the parsing to run essentially twice.

More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/

Closes #322

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
brawner pushed a commit that referenced this pull request Jan 9, 2021
This addresses a change in how whitespace is preserved/collapsed between
tinyxml and tinyxml2. The change primarily impacts SDF files that have
newlines, which is frequently done for aesthetics with include uris.

In this case, we use an alternate constructor for TinyXML2 that will
collapse whitespace by default.

Note that there is a performance impact of this behavior, which causes
the parsing to run essentially twice.

More information on the behavior may be found here: https://leethomason.github.io/tinyxml2/

Closes #322

Signed-off-by: Michael Carroll <michael@openrobotics.org>

Co-authored-by: Steve Peters <scpeters@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Whitespace preservation inconsistency
5 participants