From 817d8428b879323dfd5d5f2c71322bba2617c714 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 4 Apr 2022 14:23:23 -0300 Subject: [PATCH 1/4] Add interface for a system that declares parameters Signed-off-by: Ivan Santiago Paunovic Pass ecm as parameter to systems implementing the ConfigureParameters interface Signed-off-by: Ivan Santiago Paunovic Fix parameter registry namespace Signed-off-by: Ivan Santiago Paunovic Fixes after rebasing Signed-off-by: Ivan Santiago Paunovic --- CMakeLists.txt | 2 +- include/ignition/gazebo/System.hh | 15 +++++++++++++++ src/SimulationRunner.cc | 6 +++++- src/SimulationRunner.hh | 5 +++++ src/SystemInternal.hh | 9 +++++++++ src/SystemManager.cc | 30 +++++++++++++++++++++++++----- src/SystemManager.hh | 22 ++++++++++++++++++---- 7 files changed, 78 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d2cb82ef15..97faae2a09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -70,7 +70,7 @@ set(IGN_PLUGIN_VER ${ignition-plugin1_VERSION_MAJOR}) #-------------------------------------- # Find ignition-transport -ign_find_package(ignition-transport11 REQUIRED COMPONENTS log) +ign_find_package(ignition-transport11 REQUIRED COMPONENTS log parameters) set(IGN_TRANSPORT_VER ${ignition-transport11_VERSION_MAJOR}) #-------------------------------------- diff --git a/include/ignition/gazebo/System.hh b/include/ignition/gazebo/System.hh index 67d7f1a00b..5b0f97728b 100644 --- a/include/ignition/gazebo/System.hh +++ b/include/ignition/gazebo/System.hh @@ -25,6 +25,8 @@ #include #include +#include + #include namespace ignition @@ -100,6 +102,19 @@ namespace ignition EventManager &_eventMgr) = 0; }; + /// \class ISystemConfigureParameters ISystem.hh ignition/gazebo/System.hh + /// \brief Interface for a system that declares parameters. + /// + /// ISystemConfigureParameters::ConfigureParameters is called after + /// ISystemConfigure::Configure. + class ISystemConfigureParameters { + /// \brief Configure the parameters of the system. + /// \param[in] _registry The parameter registry. + public: virtual void ConfigureParameters( + ignition::transport::parameters::ParametersRegistry & _registry, + EntityComponentManager &_ecm) = 0; + }; + /// \class ISystemPreUpdate ISystem.hh ignition/gazebo/System.hh /// \brief Interface for a system that uses the PreUpdate phase class ISystemPreUpdate { diff --git a/src/SimulationRunner.cc b/src/SimulationRunner.cc index 3fdf7725d5..fcc04887e6 100644 --- a/src/SimulationRunner.cc +++ b/src/SimulationRunner.cc @@ -61,6 +61,10 @@ SimulationRunner::SimulationRunner(const sdf::World *_world, // Keep world name this->worldName = _world->Name(); + this->parametersRegistry = std::make_unique< + ignition::transport::parameters::ParametersRegistry>( + std::string{"world/"} + this->worldName); + // Get the physics profile // TODO(luca): remove duplicated logic in SdfEntityCreator and LevelManager auto physics = _world->PhysicsByIndex(0); @@ -133,7 +137,7 @@ SimulationRunner::SimulationRunner(const sdf::World *_world, // Create the system manager this->systemMgr = std::make_unique(_systemLoader, - &this->entityCompMgr, &this->eventMgr, validNs); + &this->entityCompMgr, &this->eventMgr, validNs, this->parametersRegistry.get()); this->pauseConn = this->eventMgr.Connect( std::bind(&SimulationRunner::SetPaused, this, std::placeholders::_1)); diff --git a/src/SimulationRunner.hh b/src/SimulationRunner.hh index 99b66c1c95..f36ce6e4ba 100644 --- a/src/SimulationRunner.hh +++ b/src/SimulationRunner.hh @@ -396,6 +396,11 @@ namespace ignition /// Note: must be before EntityComponentManager private: EventManager eventMgr; + /// \brief Manager all parameters + private: std::unique_ptr< + ignition::transport::parameters::ParametersRegistry + > parametersRegistry; + /// \brief Manager of all components. private: EntityComponentManager entityCompMgr; diff --git a/src/SystemInternal.hh b/src/SystemInternal.hh index 66277cf026..0f0e6ff4d7 100644 --- a/src/SystemInternal.hh +++ b/src/SystemInternal.hh @@ -45,6 +45,8 @@ namespace ignition : systemPlugin(std::move(_systemPlugin)), system(systemPlugin->QueryInterface()), configure(systemPlugin->QueryInterface()), + configureParameters( + systemPlugin->QueryInterface()), preupdate(systemPlugin->QueryInterface()), update(systemPlugin->QueryInterface()), postupdate(systemPlugin->QueryInterface()), @@ -60,6 +62,8 @@ namespace ignition : systemShared(_system), system(_system.get()), configure(dynamic_cast(_system.get())), + configureParameters( + dynamic_cast(_system.get())), preupdate(dynamic_cast(_system.get())), update(dynamic_cast(_system.get())), postupdate(dynamic_cast(_system.get())), @@ -83,6 +87,11 @@ namespace ignition /// Will be nullptr if the System doesn't implement this interface. public: ISystemConfigure *configure = nullptr; + /// \brief Access this system via the ISystemConfigureParameters + /// interface. + /// Will be nullptr if the System doesn't implement this interface. + public: ISystemConfigureParameters *configureParameters = nullptr; + /// \brief Access this system via the ISystemPreUpdate interface /// Will be nullptr if the System doesn't implement this interface. public: ISystemPreUpdate *preupdate = nullptr; diff --git a/src/SystemManager.cc b/src/SystemManager.cc index ff3bc2f65b..8371fd7e3b 100644 --- a/src/SystemManager.cc +++ b/src/SystemManager.cc @@ -28,13 +28,16 @@ using namespace ignition; using namespace gazebo; ////////////////////////////////////////////////// -SystemManager::SystemManager(const SystemLoaderPtr &_systemLoader, - EntityComponentManager *_entityCompMgr, - EventManager *_eventMgr, - const std::string &_namespace) +SystemManager::SystemManager( + const SystemLoaderPtr &_systemLoader, + EntityComponentManager *_entityCompMgr, + EventManager *_eventMgr, + const std::string &_namespace, + ignition::transport::parameters::ParametersRegistry *_parametersRegistry) : systemLoader(_systemLoader), entityCompMgr(_entityCompMgr), - eventMgr(_eventMgr) + eventMgr(_eventMgr), + parametersRegistry(_parametersRegistry) { transport::NodeOptions opts; opts.SetNameSpace(_namespace); @@ -101,6 +104,9 @@ size_t SystemManager::ActivatePendingSystems() if (system.configure) this->systemsConfigure.push_back(system.configure); + + if (system.configureParameters) + this->systemsConfigureParameters.push_back(system.configureParameters); if (system.preupdate) this->systemsPreupdate.push_back(system.preupdate); @@ -169,6 +175,14 @@ void SystemManager::AddSystemImpl( *this->eventMgr); } + // Configure the system parameters, if necessary + if (_system.configureParameters && this->entityCompMgr && this->parametersRegistry) + { + _system.configureParameters->ConfigureParameters( + *this->parametersRegistry, + *this->entityCompMgr); + } + // Update callbacks will be handled later, add to queue std::lock_guard lock(this->pendingSystemsMutex); this->pendingSystems.push_back(_system); @@ -180,6 +194,12 @@ const std::vector& SystemManager::SystemsConfigure() return this->systemsConfigure; } +////////////////////////////////////////////////// +const std::vector& SystemManager::SystemsConfigureParameters() +{ + return this->systemsConfigureParameters; +} + ////////////////////////////////////////////////// const std::vector& SystemManager::SystemsPreUpdate() { diff --git a/src/SystemManager.hh b/src/SystemManager.hh index a7bf1484ee..d4a84dd11b 100644 --- a/src/SystemManager.hh +++ b/src/SystemManager.hh @@ -52,10 +52,13 @@ namespace ignition /// \param[in] _eventMgr Pointer to the event manager to be used when /// configuring new systems /// \param[in] _namespace Namespace to use for the transport node - public: explicit SystemManager(const SystemLoaderPtr &_systemLoader, - EntityComponentManager *_entityCompMgr = nullptr, - EventManager *_eventMgr = nullptr, - const std::string &_namespace = std::string()); + public: explicit SystemManager( + const SystemLoaderPtr &_systemLoader, + EntityComponentManager *_entityCompMgr = nullptr, + EventManager *_eventMgr = nullptr, + const std::string &_namespace = std::string(), + ignition::transport::parameters::ParametersRegistry * + _parametersRegistry = nullptr); /// \brief Load system plugin for a given entity. /// \param[in] _entity Entity @@ -99,6 +102,11 @@ namespace ignition /// \return Vector of systems's configure interfaces. public: const std::vector& SystemsConfigure(); + /// \brief Get an vector of all active systems implementing + /// "ConfigureParameters" + /// \return Vector of systems's configure interfaces. + public: const std::vector& SystemsConfigureParameters(); + /// \brief Get an vector of all active systems implementing "PreUpdate" /// \return Vector of systems's pre-update interfaces. public: const std::vector& SystemsPreUpdate(); @@ -162,6 +170,9 @@ namespace ignition /// \brief Systems implementing Configure private: std::vector systemsConfigure; + /// \brief Systems implementing ConfigureParameters + private: std::vector systemsConfigureParameters; + /// \brief Systems implementing PreUpdate private: std::vector systemsPreupdate; @@ -191,6 +202,9 @@ namespace ignition /// \brief Node for communication. private: std::unique_ptr node{nullptr}; + + /// \brief Pointer to associated parameters registry + private: ignition::transport::parameters::ParametersRegistry *parametersRegistry; }; } } // namespace gazebo From caf05807c505452b82afda78f4fb5cd481f5f54d Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 9 Nov 2022 17:10:27 -0300 Subject: [PATCH 2/4] Fix linters Signed-off-by: Ivan Santiago Paunovic --- include/ignition/gazebo/System.hh | 3 ++- src/SimulationRunner.cc | 5 +++-- src/SystemManager.cc | 9 ++++++--- src/SystemManager.hh | 9 ++++++--- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/include/ignition/gazebo/System.hh b/include/ignition/gazebo/System.hh index 5b0f97728b..a4c4601d17 100644 --- a/include/ignition/gazebo/System.hh +++ b/include/ignition/gazebo/System.hh @@ -111,7 +111,8 @@ namespace ignition /// \brief Configure the parameters of the system. /// \param[in] _registry The parameter registry. public: virtual void ConfigureParameters( - ignition::transport::parameters::ParametersRegistry & _registry, + ignition::transport::parameters::ParametersRegistry & + _registry, EntityComponentManager &_ecm) = 0; }; diff --git a/src/SimulationRunner.cc b/src/SimulationRunner.cc index fcc04887e6..5fbcd38c29 100644 --- a/src/SimulationRunner.cc +++ b/src/SimulationRunner.cc @@ -136,8 +136,9 @@ SimulationRunner::SimulationRunner(const sdf::World *_world, this->node = std::make_unique(opts); // Create the system manager - this->systemMgr = std::make_unique(_systemLoader, - &this->entityCompMgr, &this->eventMgr, validNs, this->parametersRegistry.get()); + this->systemMgr = std::make_unique( + _systemLoader, &this->entityCompMgr, &this->eventMgr, validNs, + this->parametersRegistry.get()); this->pauseConn = this->eventMgr.Connect( std::bind(&SimulationRunner::SetPaused, this, std::placeholders::_1)); diff --git a/src/SystemManager.cc b/src/SystemManager.cc index 8371fd7e3b..1397fa9354 100644 --- a/src/SystemManager.cc +++ b/src/SystemManager.cc @@ -104,7 +104,7 @@ size_t SystemManager::ActivatePendingSystems() if (system.configure) this->systemsConfigure.push_back(system.configure); - + if (system.configureParameters) this->systemsConfigureParameters.push_back(system.configureParameters); @@ -176,7 +176,9 @@ void SystemManager::AddSystemImpl( } // Configure the system parameters, if necessary - if (_system.configureParameters && this->entityCompMgr && this->parametersRegistry) + if ( + _system.configureParameters && this->entityCompMgr && + this->parametersRegistry) { _system.configureParameters->ConfigureParameters( *this->parametersRegistry, @@ -195,7 +197,8 @@ const std::vector& SystemManager::SystemsConfigure() } ////////////////////////////////////////////////// -const std::vector& SystemManager::SystemsConfigureParameters() +const std::vector& +SystemManager::SystemsConfigureParameters() { return this->systemsConfigureParameters; } diff --git a/src/SystemManager.hh b/src/SystemManager.hh index d4a84dd11b..f6883da4de 100644 --- a/src/SystemManager.hh +++ b/src/SystemManager.hh @@ -105,7 +105,8 @@ namespace ignition /// \brief Get an vector of all active systems implementing /// "ConfigureParameters" /// \return Vector of systems's configure interfaces. - public: const std::vector& SystemsConfigureParameters(); + public: const std::vector& + SystemsConfigureParameters(); /// \brief Get an vector of all active systems implementing "PreUpdate" /// \return Vector of systems's pre-update interfaces. @@ -171,7 +172,8 @@ namespace ignition private: std::vector systemsConfigure; /// \brief Systems implementing ConfigureParameters - private: std::vector systemsConfigureParameters; + private: std::vector + systemsConfigureParameters; /// \brief Systems implementing PreUpdate private: std::vector systemsPreupdate; @@ -204,7 +206,8 @@ namespace ignition private: std::unique_ptr node{nullptr}; /// \brief Pointer to associated parameters registry - private: ignition::transport::parameters::ParametersRegistry *parametersRegistry; + private: ignition::transport::parameters::ParametersRegistry * + parametersRegistry; }; } } // namespace gazebo From 0c8d710d6184d3c5443995a6da3ebac63938183a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 6 Dec 2022 14:15:59 -0300 Subject: [PATCH 3/4] Add test case Signed-off-by: Ivan Santiago Paunovic --- src/SystemManager_TEST.cc | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/SystemManager_TEST.cc b/src/SystemManager_TEST.cc index b53d63d24c..776adde0b1 100644 --- a/src/SystemManager_TEST.cc +++ b/src/SystemManager_TEST.cc @@ -31,7 +31,8 @@ using namespace ignition::gazebo; ///////////////////////////////////////////////// class SystemWithConfigure: public System, - public ISystemConfigure + public ISystemConfigure, + public ISystemConfigureParameters { // Documentation inherited public: void Configure( @@ -39,8 +40,13 @@ class SystemWithConfigure: const std::shared_ptr &, EntityComponentManager &, EventManager &) override { configured++; }; + + public: void ConfigureParameters( + ignition::transport::parameters::ParametersRegistry &, + EntityComponentManager &) override { configuredParameters++; } public: int configured = 0; + public: int configuredParameters = 0; }; ///////////////////////////////////////////////// @@ -99,6 +105,7 @@ TEST(SystemManager, AddSystemNoEcm) // SystemManager without an ECM/EventmManager will mean no config occurs EXPECT_EQ(0, configSystem->configured); + EXPECT_EQ(0, configSystem->configuredParameters); EXPECT_EQ(0u, systemMgr.ActiveCount()); EXPECT_EQ(1u, systemMgr.PendingCount()); @@ -150,7 +157,10 @@ TEST(SystemManager, AddSystemEcm) auto ecm = EntityComponentManager(); auto eventManager = EventManager(); - SystemManager systemMgr(loader, &ecm, &eventManager); + auto paramRegistry = std::make_unique< + ignition::transport::parameters::ParametersRegistry>("SystemManager_TEST"); + SystemManager systemMgr( + loader, &ecm, &eventManager, std::string(), paramRegistry.get()); EXPECT_EQ(0u, systemMgr.ActiveCount()); EXPECT_EQ(0u, systemMgr.PendingCount()); @@ -165,6 +175,7 @@ TEST(SystemManager, AddSystemEcm) // Configure called during AddSystem EXPECT_EQ(1, configSystem->configured); + EXPECT_EQ(1, configSystem->configuredParameters); EXPECT_EQ(0u, systemMgr.ActiveCount()); EXPECT_EQ(1u, systemMgr.PendingCount()); From 154972f473d2d41243a1dde57ea2ac9170289c6a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 6 Dec 2022 14:30:51 -0300 Subject: [PATCH 4/4] remove whitespace Signed-off-by: Ivan Santiago Paunovic --- src/SystemManager_TEST.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/SystemManager_TEST.cc b/src/SystemManager_TEST.cc index 776adde0b1..63bec9b571 100644 --- a/src/SystemManager_TEST.cc +++ b/src/SystemManager_TEST.cc @@ -40,7 +40,7 @@ class SystemWithConfigure: const std::shared_ptr &, EntityComponentManager &, EventManager &) override { configured++; }; - + public: void ConfigureParameters( ignition::transport::parameters::ParametersRegistry &, EntityComponentManager &) override { configuredParameters++; }