Migrate to using TinyXML2 #264

mjcarroll · 2020-04-30T14:17:11Z

This is the forward port of #241 for the current master branch sdf10.

scpeters · 2020-05-11T19:04:30Z

there are some conflicts here, partially due to deprecation of the API's in parser_urdf.hh in the following bitbucket PR:

https://osrf-migration.github.io/sdformat-gh-pages/#!/osrf/sdformat/pull-requests/658/page/1

@azeey, we marked it as deprecated as of 9.2, and I think we are planning to remove it in 10.0? let's make a separate PR to move that header file from include to src, and then it will clarify the API impact of this PR

scpeters · 2020-05-12T01:07:18Z

PR to not install parser_urdf.hh: #276

scpeters · 2020-05-15T21:26:45Z

I just merged #276, which makes parser_urdf.hh a private header in the src/ folder, so I recommend rebasing on top of master

EricCousineau-TRI · 2020-05-28T19:02:59Z

@mjcarroll Do you know if you need a stable release for Ignition Dome (libsdformat10), or will y'all be fine with using master?

Asking in the context of #278, whether we should target libsdformat11 instead of libsdformat10.

mjcarroll · 2020-06-01T15:20:15Z

@EricCousineau-TRI I would like to target dome.

EricCousineau-TRI · 2020-06-01T15:24:09Z

OK, thanks!

scpeters · 2020-06-02T03:34:39Z

do you mind rebasing on top of sdf10 and changing the target of this PR?

https://github.com/osrf/sdformat/commits/sdf10

mjcarroll · 2020-06-02T21:01:12Z

do you mind rebasing on top of sdf10 and changing the target of this PR?

Done! Thanks for getting that set up.

scpeters · 2020-06-02T22:02:07Z

this PR is huge, but I'm not sure if there's a way to split up the changes. once you start parsing with tinyxml2, all the functions are going to need to read those data structures

scpeters

I'd like to see unit tests for XmlUtils

mjcarroll · 2020-06-03T13:17:24Z

this PR is huge, but I'm not sure if there's a way to split up the changes.

One thing to reduce the size could be to do the conversion, and then the removal of the vendored TinyXML in a followup PR

azeey

Looks good on first pass. I'll look into failing tests.

src/XmlUtils.hh

src/parser.cc

src/parser_urdf.cc

src/parser_urdf.hh

src/CMakeLists.txt

src/parser_urdf_TEST.cc

src/parser.cc

mjcarroll · 2020-06-11T15:59:17Z

@scpeters How do I go about updating sdformat-ci-pr_any-ubuntu_auto-amd64 to install tinyxml2?

codecov-commenter · 2020-06-11T16:01:08Z

Codecov Report

Merging #264 into sdf10 will decrease coverage by 0.19%.
The diff coverage is 78.74%.

@@            Coverage Diff             @@
##            sdf10     #264      +/-   ##
==========================================
- Coverage   87.69%   87.50%   -0.20%     
==========================================
  Files          59       60       +1     
  Lines        9048     9085      +37     
==========================================
+ Hits         7935     7950      +15     
- Misses       1113     1135      +22

Impacted Files	Coverage Δ
src/SDFExtension.hh	`100.00% <ø> (ø)`
src/XmlUtils.cc	`62.50% <62.50%> (ø)`
src/parser_urdf.cc	`79.43% <74.13%> (-0.78%)`	⬇️
src/parser.cc	`78.48% <83.82%> (-0.05%)`	⬇️
src/Converter.cc	`91.57% <92.94%> (+0.11%)`	⬆️
src/Types.cc	`100.00% <100.00%> (ø)`

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 bd119e5...7155942. Read the comment docs.

mjcarroll · 2020-06-11T16:31:25Z

It appears that the example is failing due to a trailing quote in the simple.sdf file: https://github.com/osrf/sdformat/blob/master/examples/simple.sdf#L6

Is this intentional, or is it that TinyXML2 happens to be a bit more strict?

scpeters · 2020-06-11T17:38:49Z

How do I go about updating sdformat-ci-pr_any-ubuntu_auto-amd64 to install tinyxml2?

the logic for the ubuntu build is in release-tools:

https://github.com/ignition-tooling/release-tools/blob/master/jenkins-scripts/lib/dependencies_archive.sh#L55-L107

for homebrew, we'll need a new formula with different dependencies; I'll work on that

scpeters · 2020-06-11T17:47:53Z

for homebrew, we'll need a new formula with different dependencies; I'll work on that

osrf/homebrew-simulation#1054

Signed-off-by: Michael Carroll <[email protected]>

src/XmlUtils.cc

src/XmlUtils.hh

src/urdf/urdf_parser/joint.cpp

src/urdf/urdf_parser/link.cpp

src/parser_urdf.cc

Signed-off-by: Michael Carroll <[email protected]>

scpeters · 2020-07-14T17:15:47Z

@osrf-jenkins run tests please

scpeters · 2020-07-14T19:20:07Z

the build farm is backed up, but I expect it to pass, so I'll approve

let's wait to merge until the last job finishes

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey

LGTM!

scpeters · 2020-07-15T18:57:31Z

ok, it looks it just needs an update to the migration guide and changelog, and then we can squash and merge

@mjcarroll do you want me to update these?

Signed-off-by: Steve Peters <[email protected]>

scpeters · 2020-07-16T23:46:17Z

ok, it looks it just needs an update to the migration guide and changelog, and then we can squash and merge

@mjcarroll do you want me to update these?

updated in 7155942

* Remove vendored TinyXML * Add FindTinyXML2 CMake Logic * Add XmlUtils with test * Update CI dependencies * Allow trim to remove newlines as well * Remove trailing quote in example sdf * Make internal URDF parser use tinyxml2 Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>

This has changed recently: gazebosim/sdformat#264

scpeters · 2020-07-18T05:44:12Z

updating dependency in release repo: gazebo-release/sdformat10-release#1

* Remove vendored TinyXML * Add FindTinyXML2 CMake Logic * Add XmlUtils with test * Update CI dependencies * Allow trim to remove newlines as well * Remove trailing quote in example sdf * Make internal URDF parser use tinyxml2 Signed-off-by: Michael Carroll <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>

There is special handling in the parser_urdf code to update plugin fields when links are merged together during fixed joint reduction. A test for this was added to sdf6 in #500. A portion of this test is applied directly to the sdf10 branch to illustrate a problem with ReduceSDFExtensionPluginFrameReplace in parser_urdf.cc. The original migration to use tinyxml2 in #264 changed the data structure used in SDFExtension to store XMLDocuments instead of XMLPointers, which requires an extra call to FirstChildElement, but the ReduceSDFExtension*FrameReplace functions did not receive this update. The fix here refactors the function arguments to pass the first child element directly, which simplifies the helper function implementation. Signed-off-by: Steve Peters <[email protected]>

mjcarroll mentioned this pull request Apr 30, 2020

Port to TinyXML2 #241

Closed

mjcarroll force-pushed the tinyxml2_sdf10 branch from a200269 to 3829a40 Compare April 30, 2020 15:04

mjcarroll requested a review from scpeters June 1, 2020 14:12

mjcarroll marked this pull request as ready for review June 1, 2020 14:12

mjcarroll requested a review from azeey June 1, 2020 18:18

mjcarroll force-pushed the tinyxml2_sdf10 branch from e65a00d to ddeb6c2 Compare June 1, 2020 18:47

mjcarroll force-pushed the tinyxml2_sdf10 branch from 4c160e7 to 4841176 Compare June 2, 2020 20:58

mjcarroll changed the base branch from master to sdf10 June 2, 2020 20:58

mjcarroll force-pushed the tinyxml2_sdf10 branch from 4841176 to adcf7dc Compare June 2, 2020 21:00

scpeters requested changes Jun 2, 2020

View reviewed changes

azeey reviewed Jun 10, 2020

View reviewed changes

src/parser.cc Outdated Show resolved Hide resolved

mjcarroll force-pushed the tinyxml2_sdf10 branch from 8086293 to a6c4612 Compare June 11, 2020 15:49

Don't add dllexport on Windows

b8af137

Signed-off-by: Michael Carroll <[email protected]>

scpeters reviewed Jul 13, 2020

View reviewed changes

src/XmlUtils.cc Outdated Show resolved Hide resolved

src/XmlUtils.hh Outdated Show resolved Hide resolved

src/urdf/urdf_parser/joint.cpp Outdated Show resolved Hide resolved

scpeters reviewed Jul 14, 2020

View reviewed changes

src/urdf/urdf_parser/link.cpp Outdated Show resolved Hide resolved

src/parser_urdf.cc Outdated Show resolved Hide resolved

src/parser_urdf.cc Show resolved Hide resolved

src/parser_urdf.cc Show resolved Hide resolved

src/parser_urdf.cc Outdated Show resolved Hide resolved

mjcarroll added 4 commits July 14, 2020 09:34

Address reviewer feedback about documentation

f21871e

Signed-off-by: Michael Carroll <[email protected]>

Change ValueStr -> Name for consistency

9c9b74a

Signed-off-by: Michael Carroll <[email protected]>

Remove unneeded return

47817f0

Signed-off-by: Michael Carroll <[email protected]>

Fix linting

afe61c8

Signed-off-by: Michael Carroll <[email protected]>

scpeters approved these changes Jul 14, 2020

View reviewed changes

Style

4af3373

Signed-off-by: Addisu Z. Taddese <[email protected]>

azeey approved these changes Jul 15, 2020

View reviewed changes

Migration guide and changelog

7155942

Signed-off-by: Steve Peters <[email protected]>

scpeters merged commit db28ef2 into sdf10 Jul 17, 2020

scpeters deleted the tinyxml2_sdf10 branch July 17, 2020 17:11

scpeters mentioned this pull request Jul 17, 2020

Cherry pick from sdf10 to master #317

Merged

scpeters added a commit to gazebo-release/sdformat10-release that referenced this pull request Jul 18, 2020

Depend on tinyxml2

1a1e1c3

This has changed recently: gazebosim/sdformat#264

scpeters mentioned this pull request Jul 18, 2020

Depend on tinyxml2 gazebo-release/sdformat10-release#1

Merged

chapulina mentioned this pull request Jul 23, 2020

Bump to SDFormat 10 for Dome gazebosim/gz-physics#85

Merged

This was referenced Nov 9, 2021

9 to 10 20211108 #741

Closed

parser_urdf: ReduceSDFExtension*FrameReplace functions are broken in libsdformat10+ #746

Open

Fix URDF fixed joint reduction of plugins #745

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to using TinyXML2 #264

Migrate to using TinyXML2 #264

mjcarroll commented Apr 30, 2020

scpeters commented May 11, 2020

scpeters commented May 12, 2020

scpeters commented May 15, 2020

EricCousineau-TRI commented May 28, 2020

mjcarroll commented Jun 1, 2020

EricCousineau-TRI commented Jun 1, 2020

scpeters commented Jun 2, 2020

mjcarroll commented Jun 2, 2020

scpeters commented Jun 2, 2020

scpeters left a comment

mjcarroll commented Jun 3, 2020

azeey left a comment

mjcarroll commented Jun 11, 2020

codecov-commenter commented Jun 11, 2020 •

edited

Loading

mjcarroll commented Jun 11, 2020

scpeters commented Jun 11, 2020

scpeters commented Jun 11, 2020

scpeters commented Jul 14, 2020

scpeters commented Jul 14, 2020

azeey left a comment

scpeters commented Jul 15, 2020

scpeters commented Jul 16, 2020

scpeters commented Jul 18, 2020

Migrate to using TinyXML2 #264

Migrate to using TinyXML2 #264

Conversation

mjcarroll commented Apr 30, 2020

scpeters commented May 11, 2020

scpeters commented May 12, 2020

scpeters commented May 15, 2020

EricCousineau-TRI commented May 28, 2020

mjcarroll commented Jun 1, 2020

EricCousineau-TRI commented Jun 1, 2020

scpeters commented Jun 2, 2020

mjcarroll commented Jun 2, 2020

scpeters commented Jun 2, 2020

scpeters left a comment

Choose a reason for hiding this comment

mjcarroll commented Jun 3, 2020

azeey left a comment

Choose a reason for hiding this comment

mjcarroll commented Jun 11, 2020

codecov-commenter commented Jun 11, 2020 • edited Loading

Codecov Report

mjcarroll commented Jun 11, 2020

scpeters commented Jun 11, 2020

scpeters commented Jun 11, 2020

scpeters commented Jul 14, 2020

scpeters commented Jul 14, 2020

azeey left a comment

Choose a reason for hiding this comment

scpeters commented Jul 15, 2020

scpeters commented Jul 16, 2020

scpeters commented Jul 18, 2020

codecov-commenter commented Jun 11, 2020 •

edited

Loading