gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
621 stars 251 forks source link

Add entity and sdf parameters to Server's AddSystem interface #2324

Closed g-arjones closed 4 months ago

g-arjones commented 4 months ago

🦟 Bug fix

Fixes #2322

Summary

Entity and XML content information are lost when adding systems through the Server::AddSystem(system) API. To reproduce it:

  1. Create a dummy system plugin:
    class DummyPlugin : public gz::sim::System,
    public gz::sim::ISystemConfigure,
    {
    public:
    void Configure(
    const gz::sim::Entity &_entity,
    const std::shared_ptr<const sdf::Element> &_sdf,
    gz::sim::EntityComponentManager &_ecm,
    gz::sim::EventManager &_eventMgr) override
    {
    std::cout << _sdf->ToString("") << std::endl;
    }
    };
  2. Load it with the SystemLoader (extracted from here):
    sim::SystemLoader systemLoader;
    sdf::Plugin sdfPlugin("dummy_plugin", "gz::sim::systems::DummyPlugin", "<test>foo</test>");
    auto mockSystemPlugin = systemLoader.LoadPlugin(sdfPlugin);
    EXPECT_TRUE(*server.AddSystem(mockSystemPlugin.value()));

Without this patch, the code above will output:

<plugin filename='__default__'/>

After the patch, you will be able to use the extended Server::AddSystem(plugin, entity, sdf) API and get:

<plugin filename='dummy_plugin' name='gz::sim::DummyPlugin'>
  <test>foo</test>
</plugin>

Checklist

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.

g-arjones commented 4 months ago

Amended to fix linting issues

g-arjones commented 4 months ago

That would encapsulate loading the plugin and extracting the _sdf element from sdf::Plugin. Thoughts?

Sounds good to me. May I do it in this PR ?

azeey commented 4 months ago

That would encapsulate loading the plugin and extracting the _sdf element from sdf::Plugin. Thoughts?

Sounds good to me. May I do it in this PR ?

Go for it!

g-arjones commented 4 months ago

@azeey Comments have been addressed.

Thank you for the review!

g-arjones commented 4 months ago

@azeey Had to amend again because of another linter issue, sorry about that.

BTW, the contribution guide mentions a codecheck target but the current cmake configuration doesn't seem to be generating that:

$ make codecheck
make: *** No rule to make target 'codecheck'.  Stop.

How should I run cpplint and cppcheck on gz-sim properly?

azeey commented 4 months ago

@azeey Had to amend again because of another linter issue, sorry about that.

BTW, the contribution guide mentions a codecheck target but the current cmake configuration doesn't seem to be generating that:

$ make codecheck
make: *** No rule to make target 'codecheck'.  Stop.

How should I run cpplint and cppcheck on gz-sim properly?

You must install cppcheck first for the codecheck target to be available.

g-arjones commented 4 months ago

You must install cppcheck first for the codecheck target to be available.

Got it. I think we are good to go now.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.98%. Comparing base (633ce72) to head (4e739b8).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## gz-sim8 #2324 +/- ## =========================================== + Coverage 65.97% 65.98% +0.01% =========================================== Files 327 327 Lines 31328 31339 +11 =========================================== + Hits 20668 20680 +12 + Misses 10660 10659 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

g-arjones commented 4 months ago

No problem. Amended again.

g-arjones commented 4 months ago

@azeey @iche033 Any idea when this might be released?