-
Notifications
You must be signed in to change notification settings - Fork 95
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 support for merge-includes in worlds #1233
Conversation
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
1842f0c
to
d979739
Compare
Codecov Report
@@ Coverage Diff @@
## sdf13 #1233 +/- ##
=======================================
Coverage 87.51% 87.51%
=======================================
Files 126 126
Lines 16239 16248 +9
=======================================
+ Hits 14211 14219 +8
- Misses 2028 2029 +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: Addisu Z. Taddese <addisu@openrobotics.org>
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.
This is awesome! I only have 2 small comments, but otherwise LGTM!
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
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.
LGTM!
Migration.md
Outdated
@@ -558,6 +558,7 @@ ABI was broken for `sdf::Element`, and restored on version 11.2.1. | |||
### Additions | |||
|
|||
1. **world.sdf**: A joint can be specified directly in a world. | |||
1. **world.sdf**: Merge-includes are now allowed in worlds. The included models must not contain links or grippers. |
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.
nit Consider qualifying:
"The included models must not contain top-level links or grippers."
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.
Done in f6a9752
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.
Thanks!
{ | ||
Error unsupportedError( | ||
ErrorCode::MERGE_INCLUDE_UNSUPPORTED, | ||
"Merge-include for <world> does not support element of type " + |
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.
weird that coverage doesn't detect this because I see that it is tested
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 coverage struggles with multiline code.
Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>
I think the homebrew CI errors will be fixed by the gz-math7 bottle rebuild in osrf/homebrew-simulation#2213 |
rerunning homebrew CI since osrf/homebrew-simulation#2213 has been merged |
green CI! I just merged with upstream and will leave it to @azeey to merge |
🎉 New feature
Summary
Implements merge-includes for worlds as discussed in gazebosim/sdf_tutorials#86. This required very few changes since it uses the same merge mechanism used for models.
Test it
Two tests were added in
test/integration/includes.cc
.IncludeTest.WorldMergeInclude
: Checks the proper merging of elements and their poses.`IncludeTest.WorldMergeIncludeInvalidElements. Checks that merging models that contain links into the world emits an error.
Checklist
[ ] Consider updating Python bindings (if the library has them)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.