SystemLoader ignores XML content when loading plugins through LoadPlugin(...)

[g-arjones]

Environment

• OS Version: Ubuntu 22.04
• Source or binary build? Binary (version 8.1.0)

Description

• Expected behavior:
• Actual behavior:

Steps to reproduce

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::DummyPlugin", "<test>foo</test>");
  auto mockSystemPlugin = systemLoader.LoadPlugin(sdfPlugin);
  EXPECT_TRUE(*server.AddSystem(mockSystemPlugin.value()));

Output

You should get on your console:

<plugin filename='__default__'/>

When you should actually be getting something like:

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

[azeey]

It looks like we need a new API for adding systems programmatically with XML content. The current System::AddSystem API calls SimulationRunner::AddSystem with the default arguments, which has sdf::ElementPtr set to std::nullopt.

gz-sim/src/SimulationRunner.hh

Lines 117 to 120 in 8a56f02

	public: void AddSystem(const SystemPluginPtr &_system,
	std::optional<Entity> _entity = std::nullopt,
	std::optional<std::shared_ptr<const sdf::Element>> _sdf =
	std::nullopt);

[g-arjones]

Unfortunately, both SystemManager and SimulationRunner are private classes so I don't see any way around this. Is there a reason why this wasn't implemented? Would you guys accept a patch?

[azeey]

Yes, I think it would be great to have a Server::AddSystem that mirrors SimulationRunner::AddSystem but has the additional _worldIndex parameter. A patch for that would be awesome!

Another thought is to extend ServerConfig to allow adding plugins in addition to the ones loaded by default or listed in the SDFormat file. This would be useful for gazebosim/ros_gz#500

[g-arjones]

Another thought is to extend ServerConfig to allow adding plugins in addition to the ones loaded by default or listed in the SDFormat file. This would be useful for gazebosim/ros_gz#500

Not sure I get this one... Isn't this possible already through these interfaces:

gz-sim/include/gz/sim/ServerConfig.hh

Lines 391 to 398 in 633ce72

	/// \brief Instruct simulation to attach a plugin to a specific
	/// entity when simulation starts.
	/// \param[in] _info Information about the plugin to load.
	public: void AddPlugin(const PluginInfo &_info);
	/// \brief Add multiple plugins to the simulation
	/// \param[in] _plugins List of Information about the plugin to load.
	public: void AddPlugins(const std::list<PluginInfo> &_plugins);

[azeey]

Unfortunately, those APIs don't work if the SDF file doesn't have any plugins and instead wants to just use the default set. When calling ServerConfig::AddPlugin, just the plugins in the ServerConfig get loaded, but the default set of plugins doesn't. I haven't actually what happens if the SDF file actually has plugins listed and we call ServerConfig::AddPlugin.

[g-arjones]

I see. OK, I will give it a try.

[g-arjones]

@azeey I have submitted a patch to fix the original issue. The feature you proposed seemed a bit more involved and is worth a separate PR IMO. I will work on the other one if we can merge #2324 .