Check World joints in checkJointParentChildNames #1189
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test that parent and child frames do not resolve to the same link for //world/joint. Signed-off-by: Steve Peters <[email protected]>
This refactors the checkModelJointParentChildNames lambda function into a template function that works for both sdf::Model and sdf::World objects, after adding the World::NameExistsInFrameAttachedToGraph API. Signed-off-by: Steve Peters <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf13 #1189 +/- ##
=======================================
Coverage 87.25% 87.26%
=======================================
Files 125 125
Lines 16114 16111 -3
=======================================
- Hits 14061 14059 -2
+ Misses 2053 2052 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Steve Peters <[email protected]>
ahcorde
previously requested changes
Oct 18, 2022
python/test/pyModel_TEST.py
Outdated
| @@ -145,6 +145,10 @@ def test_default_construction(self): | |||
| self.assertEqual(errors[1].message(), | |||
| "PoseRelativeToGraph error: scope does not point to a valid graph.") | |||
|
|
|||
| # model dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| # model dosn't have graphs, so no names should exist in graphs | |
| # model doesn't have graphs, so no names should exist in graphs |
python/test/pyWorld_TEST.py
Outdated
| @@ -86,6 +86,10 @@ def test_default_construction(self): | |||
| self.assertEqual(errors[1].message(), | |||
| "PoseRelativeToGraph error: scope does not point to a valid graph.") | |||
|
|
|||
| # world dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| # world dosn't have graphs, so no names should exist in graphs | |
| # world doesn't have graphs, so no names should exist in graphs |
src/Model_TEST.cc
Outdated
| @@ -154,6 +154,10 @@ TEST(DOMModel, Construction) | |||
| errors[1].Message().find( | |||
| "PoseRelativeToGraph error: scope does not point to a valid graph")) | |||
| << errors[1]; | |||
|
|
|||
| // model dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| // model dosn't have graphs, so no names should exist in graphs | |
| // model doesn't have graphs, so no names should exist in graphs |
src/Model_TEST.cc
Outdated
| @@ -229,6 +233,9 @@ TEST(DOMModel, AddLink) | |||
| EXPECT_EQ(1u, model.LinkCount()); | |||
| EXPECT_FALSE(model.AddLink(link)); | |||
| EXPECT_EQ(1u, model.LinkCount()); | |||
| // model dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| // model dosn't have graphs, so no names should exist in graphs | |
| // model doesn't have graphs, so no names should exist in graphs |
src/World_TEST.cc
Outdated
| @@ -93,6 +93,10 @@ TEST(DOMWorld, Construction) | |||
| EXPECT_NE(std::string::npos, | |||
| errors[1].Message().find( | |||
| "PoseRelativeToGraph error: scope does not point to a valid graph")); | |||
|
|
|||
| // world dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| // world dosn't have graphs, so no names should exist in graphs | |
| // world doesn't have graphs, so no names should exist in graphs |
src/World_TEST.cc
Outdated
| @@ -384,6 +388,9 @@ TEST(DOMWorld, AddModel) | |||
| EXPECT_EQ(1u, world.ModelCount()); | |||
| EXPECT_FALSE(world.AddModel(model)); | |||
| EXPECT_EQ(1u, world.ModelCount()); | |||
| // world dosn't have graphs, so no names should exist in graphs | |||
There was a problem hiding this comment.
Suggested change
| // world dosn't have graphs, so no names should exist in graphs | |
| // world doesn't have graphs, so no names should exist in graphs |
Signed-off-by: Steve Peters <[email protected]>
azeey
approved these changes
Oct 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎉 New feature
Follow-up to #1117
Summary
Joints were added in the world scope in #1117, but the
checkJointParentChildNamesmethod inparser.hhwas not updated to check these joints. A failing test is added in 4e86c3e, and then thecheckModelJointParentChildNameslambda function into a template function that works for bothsdf::Modelandsdf::Worldobjects, after adding theWorld::NameExistsInFrameAttachedToGraphAPI.I recommend reviewing the changes in parser.cc without whitespace in order to more easily see how the lambda function was refactored into a templated function.
Test it
Run
INTEGRATION_joint_domorgz sdf --check test/sdf/world_joint_invalid_resolved_parent_same_as_child.sdfChecklist
codecheckpassed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-bymessages.