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

Check joint parent link names in Model::Load #726

Merged
merged 3 commits into from
Oct 15, 2021

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Oct 5, 2021

🦟 Bug fix

Fixes #719 on sdf10 (a separate fix is needed for sdf11 and later)

Summary

As noted in #710, several example files in the test/sdf/ folder specify non-existent links in the //joint/parent element, which violates the spec. I added an ign sdf --check test/sdf/joint_axis_infinite_limits.sdf test case in b47f9bf, which properly notes the failure, though the sdf::Root::Load call in INTEGRATION_joint_axis_dom does not report an error. To detect the error in sdf::Root::Load, I added a check in Model::Load that //joint/parent contains a valid link name and fixed the test/sdf/ files in ce8c032.

A different fix will be needed in sdf11 to account for changes in the spec that allow any frame to be named in //joint/parent.

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

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

Codecov Report

Merging #726 (fa37f00) into sdf10 (a55f9e3) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head fa37f00 differs from pull request most recent head ce8c032. Consider uploading reports for the commit ce8c032 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            sdf10     #726      +/-   ##
==========================================
- Coverage   87.81%   87.76%   -0.06%     
==========================================
  Files          65       65              
  Lines       10403    10409       +6     
==========================================
  Hits         9135     9135              
- Misses       1268     1274       +6     
Impacted Files Coverage Δ
src/Model.cc 86.60% <100.00%> (+0.26%) ⬆️
src/ign.cc 70.90% <0.00%> (-1.82%) ⬇️
src/parser.cc 79.22% <0.00%> (-0.56%) ⬇️

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 a55f9e3...ce8c032. Read the comment docs.

@scpeters
Copy link
Member Author

scpeters commented Oct 5, 2021

code coverage is dropping because there is redundant error checking

// Check an SDF file with the infinite values for joint axis limits.
// This is a valid file.
{
std::string path = pathBase +"/joint_axis_infinite_limits.sdf";
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: same comment as the sdf11 PR, not sure if this is necessary since the error is now caught in Model and other tests should already have this file covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

as I wrote in #727 (comment), I would keep the test

@scpeters scpeters merged commit aa9362b into gazebosim:sdf10 Oct 15, 2021
@scpeters scpeters deleted the issue_719 branch October 15, 2021 20:55
@osrf-triage
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants