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

Add World::ValidateGraphs method (sdf9) #602

Merged
merged 5 commits into from
Jul 1, 2021

Conversation

scpeters
Copy link
Member

🎉 New feature

I noticed a bug in ign-physics that needs a new API in libsdformat to properly fix it. This is similar to #601 but is targeted at sdf9.

Summary

I noticed a test in ign-physics in which some models do not have properly loaded graphs, but it's hard to detect since the graph data is private. This creates a World::ValidateGraphs() method to call the hidden validate*Graph functions in FrameSemantics.cc with the private graph data. It also requires an extra check in the validate*Graph functions that the scope points to a valid graph.

Test it

Run the UNIT_World_TEST and INTEGRATION_world_dom tests and see that they pass.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • 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

The graph data is private, so expose a method for
checking that the graphs are valid.

Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters requested review from azeey and jennuine June 25, 2021 07:50
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jun 25, 2021
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, just have a request for more testing.

I also noticed that codecov may not be correctly setup for this repo. Let's fix this next week.

src/World_TEST.cc Show resolved Hide resolved
Load an sdf::World instead of an sdf::Root to
see the graph errors.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@scpeters scpeters force-pushed the world_validate_graphs_9 branch from 82e2909 to 8a4fc3c Compare June 29, 2021 00:21
@scpeters
Copy link
Member Author

there were some copy/paste issues with 82e2909 so I replaced it with a5985ab

@scpeters
Copy link
Member Author

I added Model::ValidateGraphs as well per #601 (comment)

@scpeters scpeters merged commit 18f908b into gazebosim:sdf9 Jul 1, 2021
@scpeters scpeters deleted the world_validate_graphs_9 branch July 1, 2021 00:10
@scpeters scpeters mentioned this pull request Jul 1, 2021
jennuine pushed a commit that referenced this pull request Jul 29, 2021
The graph data is private, so expose methods for
checking that the graphs are valid.

* Add tests showing graph errors

Load an sdf::World or sdf::Model instead of an sdf::Root to
see the graph errors.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Jenn Nguyen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants