-
Notifications
You must be signed in to change notification settings - Fork 100
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 plugin to SDF DOM #788
Conversation
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf12 #788 +/- ##
==========================================
+ Coverage 89.56% 89.60% +0.04%
==========================================
Files 76 78 +2
Lines 12603 12691 +88
==========================================
+ Hits 11288 11372 +84
- Misses 1315 1319 +4
Continue to review full report at Codecov.
|
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
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.
Just a couple of minor comments. Otherwise, LGTM!
/// \brief Copy constructor. This is here so that we can copy the | ||
/// plugin contents. | ||
/// \param[in] _plugin Plugin to copy. | ||
public: Plugin(const Plugin &_plugin); |
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.
We'll need to add move constructor and move assignment as well per the rule of five.
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.
was there a reason we didn't use ImplPtr here?
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.
is it because we need to take care to clone the sdf::ElementPtr
contents instead of just copying the shared pointer?
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.
Yes. Here's more context: https://github.com/ignitionrobotics/sdformat/pull/788/files#r770912499
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 |
Signed-off-by: Nate Koenig [email protected]
🎉 New feature
Summary
Added a
Plugin
object to the SDF DOM and utilized thePlugin
in the GUI DOM object. I'll addPlugin
capabilities to the other DOM objects in future PRs.Test it
Run the tests.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge