-
Notifications
You must be signed in to change notification settings - Fork 98
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
Added Root mutable accessors, and Root::Clone function #841
Conversation
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf12 #841 +/- ##
==========================================
+ Coverage 90.80% 90.85% +0.05%
==========================================
Files 78 78
Lines 12555 12583 +28
==========================================
+ Hits 11400 11432 +32
+ Misses 1155 1151 -4
Continue to review full report at Codecov.
|
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
I've merged in the work done in #843, and extended the tests. |
Signed-off-by: Nate Koenig <[email protected]>
test/integration/frame.cc
Outdated
EXPECT_EQ("M1::L", body); | ||
}); | ||
|
||
testFunc(root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, even though it might cause test code duplication, a test for sdf::Root::Clone
and sdf::Root::UpdateGraphs
belongs in it's own test case in test/integration/root_dom.cc
. Putting these expectations here limits their discoverability as this test is meant for checking the loading of <frame>
tags in by the sdf::World
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this test modification in ab3d64d
test/integration/frame.cc
Outdated
@@ -1332,3 +1348,104 @@ TEST(Frame, IncludeFrameWithSubmodel) | |||
EXPECT_EQ(submodelPose.Inverse(), | |||
ignition::math::Pose3d(5, 5, 0, 0, 0, 0)); | |||
} | |||
|
|||
///////////////////////////////////////////////// | |||
TEST(DOMFrame, CreateMulipleWorlds) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this test belongs in test/integraiton/root_dom
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved in ab3d64d
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
This pull request has been mentioned on Gazebo Community. There might be relevant details there: https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1 |
🎉 New feature
Summary
Similar to #838. This adds non-const accessors to the Root class.
Also adds a
Root::Clone
function so that onesdf::Root
can copy anothersdf::Root
Test it
Run the tests
Checklist
codecheck
passed (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-by
messages.