Fix URDF fixed joint reduction of plugins #745

scpeters · 2021-11-11T00:35:32Z

🦟 Bug fix

Fixes bug in URDF conversion for libsdformat10+

Summary

While attempting to merge forward #500 from 9 -> 10 (follow-up to #741), I found that the test added in #500 was failing. The test passes on the sdf6 and sdf9 branches, which use tinyxml, but was failing in my first merge attempt to sdf10, which uses tinyxml2. To simplify matters, I ported a simplified form of the test from #500 directly to sdf10 in 787abbc to illustrate the failure in ReduceSDFExtensionPluginFrameReplace. The test is fixed in c02e63d by accounting for the extra root element ~~in the tinyxml2 document API~~ in the blobs data structure added in #264.

Note that there are many other ReduceSDFExtension*FrameReplace functions in parser_urdf.cc that are untested and likely suffer this same error. I will open an issue to document this.

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

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 gazebosim#500. A portion of this test is applied directly to the sdf10 branch to illustrate that it is broken. Signed-off-by: Steve Peters <[email protected]>

I believe the tinyxml2 API has an extra root element in documents compared to the tinyxml API, which broke the untested ReduceSDFExtension*FrameReplace functions. This fixes the Plugin function, as confirmed by the fixed test. Signed-off-by: Steve Peters <[email protected]>

codecov-commenter · 2021-11-11T01:25:17Z

Codecov Report

Merging #745 (854315c) into sdf10 (2905643) will increase coverage by 0.62%.
The diff coverage is 100.00%.

❗ Current head 854315c differs from pull request most recent head c02e63d. Consider uploading reports for the commit c02e63d to get more accurate results

@@            Coverage Diff             @@
##            sdf10     #745      +/-   ##
==========================================
+ Coverage   87.76%   88.38%   +0.62%     
==========================================
  Files          65       65              
  Lines       10409    10410       +1     
==========================================
+ Hits         9135     9201      +66     
+ Misses       1274     1209      -65

Impacted Files	Coverage Δ
src/parser_urdf.cc	`82.72% <100.00%> (+3.95%)`	⬆️

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 2905643...c02e63d. Read the comment docs.

scpeters · 2021-11-11T01:42:48Z

Note that there are many other ReduceSDFExtension*FrameReplace functions in parser_urdf.cc that are untested and likely suffer this same error. I will open an issue to document this.

#746

iche033

by accounting for the extra root element in the tinyxml2 document API.

ouch :/

Changes look good to me.

iche033 · 2021-11-11T18:49:42Z

src/parser_urdf.cc

@@ -3588,17 +3588,18 @@ void ReduceSDFExtensionPluginFrameReplace(
 {
  std::string linkName = _link->name;
  std::string parentLinkName = _link->getParent()->name;
-  if ((*_blobIt)->FirstChildElement()->Name() == _pluginName)
+  tinyxml2::XMLElement *rootElement = (*_blobIt)->FirstChildElement();


add a comment here that there is an extra root element in tinyxml2?

ok, I think it's not an issue with the tinyxml2 API but actually a change in how we used it from #264...

we changed what we store in the SDFExtension data structure here: 6dd6d07#diff-2bc5ca23bcfc66fe173f513399bec8065b1cb4e607d741566517d7368e6bce0fL1315-L1320

I'll update the PR description accordingly

I just refactored the fix in b98b827: I change the function signature so it now accepts an XMLElement pointer instead of an XMLDocument iterator. Then we can call FirstChildElement() once and pass the resulting pointer, which simplifies the implementation in ReduceSDFExtensionPluginFrameReplace

I'll fix the other functions in follow-up pull requests if you think this approach is ok

yep the approach looks good to me.

Pass an XMLElement pointer instead of an iterator to an XMLDocument to simplify the API calls in ReduceSDFExtensionPluginFrameReplace. Signed-off-by: Steve Peters <[email protected]>

osrf-triage · 2022-03-25T22:29:38Z

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-03-25-fortress-edifice-citadel/1343/1

Repeat fix from gazebosim#745 to account for the extra root element in documents created with tinyxml2. Signed-off-by: Steve Peters <[email protected]>

Fix ReduceSDFExtensionJointFrameReplace by repeating fix from #745 to account for the extra root element in documents created with tinyxml2. This includes a test case for a URDF with a chain of links with fixed joint reduction and an SDFormat joint embedded in a <gazebo> block whose parent and child are reduced links. This confirms that the parent and child link names are updated properly during fixed joint reduction. Signed-off-by: Steve Peters <[email protected]>

scpeters added 2 commits November 10, 2021 15:12

scpeters requested review from azeey and mjcarroll November 11, 2021 00:35

scpeters mentioned this pull request Nov 11, 2021

9 to 10 20211108 #741

Closed

github-actions bot added the 🔮 dome Ignition Dome label Nov 11, 2021

scpeters mentioned this pull request Nov 11, 2021

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

Open

5 tasks

iche033 approved these changes Nov 11, 2021

View reviewed changes

Refactor fix with different function argument

b98b827

Pass an XMLElement pointer instead of an iterator to an XMLDocument to simplify the API calls in ReduceSDFExtensionPluginFrameReplace. Signed-off-by: Steve Peters <[email protected]>

scpeters merged commit 424bf9c into gazebosim:sdf10 Nov 12, 2021

scpeters deleted the test_fixed_joint_reduction_plugin_10 branch November 12, 2021 01:37

scpeters added a commit to scpeters/sdformat that referenced this pull request Jul 22, 2022

Fix ReduceSDFExtensionJointFrameReplace

5aabef4

Repeat fix from gazebosim#745 to account for the extra root element in documents created with tinyxml2. Signed-off-by: Steve Peters <[email protected]>

scpeters mentioned this pull request Jul 22, 2022

Fix URDF fixed joint reduction of SDF joints #1089

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix URDF fixed joint reduction of plugins #745

Fix URDF fixed joint reduction of plugins #745

scpeters commented Nov 11, 2021 •

edited

Loading

codecov-commenter commented Nov 11, 2021

scpeters commented Nov 11, 2021

iche033 left a comment

iche033 Nov 11, 2021

scpeters Nov 11, 2021

scpeters Nov 11, 2021

scpeters Nov 11, 2021

iche033 Nov 12, 2021

osrf-triage commented Mar 25, 2022

Fix URDF fixed joint reduction of plugins #745

Fix URDF fixed joint reduction of plugins #745

Conversation

scpeters commented Nov 11, 2021 • edited Loading

🦟 Bug fix

Summary

Checklist

codecov-commenter commented Nov 11, 2021

Codecov Report

scpeters commented Nov 11, 2021

iche033 left a comment

Choose a reason for hiding this comment

iche033 Nov 11, 2021

Choose a reason for hiding this comment

scpeters Nov 11, 2021

Choose a reason for hiding this comment

scpeters Nov 11, 2021

Choose a reason for hiding this comment

scpeters Nov 11, 2021

Choose a reason for hiding this comment

iche033 Nov 12, 2021

Choose a reason for hiding this comment

osrf-triage commented Mar 25, 2022

scpeters commented Nov 11, 2021 •

edited

Loading