From baa12ce57a7c53fdce29fa2338706675da957286 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Mon, 21 Jun 2021 21:40:11 -0700 Subject: [PATCH 1/5] Proposal for #865: Deprecations instead of removals Signed-off-by: Louise Poubel --- .../ignition/gazebo/EntityComponentManager.hh | 16 +++++ include/ignition/gazebo/Types.hh | 9 +-- .../ignition/gazebo/components/Component.hh | 6 +- include/ignition/gazebo/components/Factory.hh | 58 +++++++++++++++ .../gazebo/detail/ComponentStorageBase.hh | 55 ++++++++++++++ .../gazebo/detail/EntityComponentManager.hh | 17 +++++ .../{ComponentStorage.hh => EntityStorage.hh} | 4 +- src/CMakeLists.txt | 4 +- src/EntityComponentManager.cc | 24 +++---- src/EntityComponentManager_TEST.cc | 72 +++++++++++++++++++ src/{ComponentStorage.cc => EntityStorage.cc} | 22 +++--- ...tStorage_TEST.cc => EntityStorage_TEST.cc} | 30 ++++---- test/integration/view.cc | 10 +-- 13 files changed, 270 insertions(+), 57 deletions(-) create mode 100644 include/ignition/gazebo/detail/ComponentStorageBase.hh rename include/ignition/gazebo/detail/{ComponentStorage.hh => EntityStorage.hh} (98%) rename src/{ComponentStorage.cc => EntityStorage.cc} (88%) rename src/{ComponentStorage_TEST.cc => EntityStorage_TEST.cc} (93%) diff --git a/include/ignition/gazebo/EntityComponentManager.hh b/include/ignition/gazebo/EntityComponentManager.hh index e0ddcca297..88ddb947ef 100644 --- a/include/ignition/gazebo/EntityComponentManager.hh +++ b/include/ignition/gazebo/EntityComponentManager.hh @@ -203,6 +203,22 @@ namespace ignition public: template ComponentTypeT *Component(const Entity _entity); + /// \brief Get a component based on a key. + /// \param[in] _key A key that uniquely identifies a component. + /// \return The component associated with the key, or nullptr if the + /// component could not be found. + public: template + const ComponentTypeT *IGN_DEPRECATED(6) Component( + const ComponentKey &_key) const; + + /// \brief Get a mutable component based on a key. + /// \param[in] _key A key that uniquely identifies a component. + /// \return The component associated with the key, or nullptr if the + /// component could not be found. + public: template + ComponentTypeT *IGN_DEPRECATED(6) Component( + const ComponentKey &_key); + /// \brief Get a mutable component assigned to an entity based on a /// component type. If the component doesn't exist, create it and /// initialize with the given default value. diff --git a/include/ignition/gazebo/Types.hh b/include/ignition/gazebo/Types.hh index 67ae2cf367..ee0a75e106 100644 --- a/include/ignition/gazebo/Types.hh +++ b/include/ignition/gazebo/Types.hh @@ -78,8 +78,8 @@ namespace ignition /// \brief A unique identifier for a component instance. The uniqueness /// of a ComponentId is scoped to the component's type. /// \sa ComponentKey. - // TODO(anyone) remove this in ign-gazebo7. This is no longer used in - // ign-gazebo6 + /// \deprecated Deprecated on version 6, removed on version 7. Use + /// ComponentTypeId + Entity instead. using ComponentId = int; /// \brief A unique identifier for a component type. A component type @@ -89,8 +89,9 @@ namespace ignition /// \brief A key that uniquely identifies, at the global scope, a component /// instance - // TODO(anyone) remove this in ign-gazebo7. This is no longer used in - // ign-gazebo6 + /// \note On version 6, the 2nd element equals the entity ID. + /// \deprecated Deprecated on version 6, removed on version 7. Use + /// ComponentTypeId + Entity instead. using ComponentKey = std::pair; /// \brief typedef for query callbacks diff --git a/include/ignition/gazebo/components/Component.hh b/include/ignition/gazebo/components/Component.hh index 6646fd1ff7..b1c822892a 100644 --- a/include/ignition/gazebo/components/Component.hh +++ b/include/ignition/gazebo/components/Component.hh @@ -226,7 +226,7 @@ namespace serializers namespace detail { - class ComponentStorage; + class EntityStorage; } namespace components @@ -296,10 +296,10 @@ namespace components /// users interact with components. private: bool removed{false}; - // Make ComponentStorage and EntityComponentManager a friend of the + // Make EntityStorage and EntityComponentManager a friend of the // BaseComponent class so that they have access to a component's "removed" // flag. - friend class ignition::gazebo::detail::ComponentStorage; + friend class ignition::gazebo::detail::EntityStorage; friend class ignition::gazebo::EntityComponentManager; }; diff --git a/include/ignition/gazebo/components/Factory.hh b/include/ignition/gazebo/components/Factory.hh index 3d9d5e013f..a91fc9745c 100644 --- a/include/ignition/gazebo/components/Factory.hh +++ b/include/ignition/gazebo/components/Factory.hh @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -77,10 +78,57 @@ namespace components } }; + /// \brief A base class for an object responsible for creating storages. + class StorageDescriptorBase + { + /// \brief Constructor + public: IGN_DEPRECATED(6) StorageDescriptorBase() = default; + + /// \brief Destructor + public: virtual ~StorageDescriptorBase() = default; + + /// \brief Create an instance of a storage. + /// \return Pointer to a storage. + public: virtual std::unique_ptr Create() const = 0; + }; + + /// \brief A class for an object responsible for creating storages. + /// \tparam ComponentTypeT type of component that the storage will hold. + template + class StorageDescriptor + : public StorageDescriptorBase + { + /// \brief Constructor + public: IGN_DEPRECATED(6) StorageDescriptor() = default; + + /// \brief Create an instance of a storage that holds ComponentTypeT + /// components. + /// \return Pointer to a component. + public: std::unique_ptr Create() const override + { + return std::make_unique>(); + } + }; + /// \brief A factory that generates a component based on a string type. class Factory : public ignition::common::SingletonT { + /// \brief Register a component so that the factory can create instances + /// of the component and its storage based on an ID. + /// \param[in] _type Type of component to register. + /// \param[in] _compDesc Object to manage the creation of ComponentTypeT + /// objects. + /// \param[in] _storageDesc Ignored. + /// \tparam ComponentTypeT Type of component to register. + /// \deprecated See function that doesn't accept a storage + public: template + void IGN_DEPRECATED(6) Register(const std::string &_type, + ComponentDescriptorBase *_compDesc, StorageDescriptorBase *) + { + this->Register(_type, _compDesc); + } + /// \brief Register a component so that the factory can create instances /// of the component based on an ID. /// \param[in] _type Type of component to register. @@ -236,6 +284,16 @@ namespace components return comp; } + /// \brief Create a new instance of a component storage. + /// \param[in] _typeId Type of component which the storage will hold. + /// \return Always returns nullptr. + /// \deprecated Storages aren't necessary anymore. + public: std::unique_ptr IGN_DEPRECATED(6) NewStorage( + const ComponentTypeId &) + { + return nullptr; + } + /// \brief Get all the registered component types by ID. /// return Vector of component IDs. public: std::vector TypeIds() const diff --git a/include/ignition/gazebo/detail/ComponentStorageBase.hh b/include/ignition/gazebo/detail/ComponentStorageBase.hh new file mode 100644 index 0000000000..cb4e0edcc2 --- /dev/null +++ b/include/ignition/gazebo/detail/ComponentStorageBase.hh @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2018 Open Source Robotics Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * +*/ +#ifndef IGNITION_GAZEBO_DETAIL_COMPONENTSTORAGEBASE_HH_ +#define IGNITION_GAZEBO_DETAIL_COMPONENTSTORAGEBASE_HH_ + +#include "ignition/gazebo/Export.hh" + +namespace ignition +{ + namespace gazebo + { + // Inline bracket to help doxygen filtering. + inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { + // + /// \brief All component instances of the same type are stored + /// squentially in memory. This is a base class for storing components + /// of a particular type. + class IGNITION_GAZEBO_HIDDEN ComponentStorageBase + { + /// \brief Constructor + public: IGN_DEPRECATED(6) ComponentStorageBase() = default; + + /// \brief Destructor + public: virtual ~ComponentStorageBase() = default; + }; + + /// \brief Templated implementation of component storage. + template + class IGNITION_GAZEBO_HIDDEN ComponentStorage : public ComponentStorageBase + { + /// \brief Constructor + public: explicit IGN_DEPRECATED(6) ComponentStorage() + : ComponentStorageBase() + { + } + }; + } + } +} + +#endif diff --git a/include/ignition/gazebo/detail/EntityComponentManager.hh b/include/ignition/gazebo/detail/EntityComponentManager.hh index 7289679a92..d07f0bbba4 100644 --- a/include/ignition/gazebo/detail/EntityComponentManager.hh +++ b/include/ignition/gazebo/detail/EntityComponentManager.hh @@ -127,6 +127,23 @@ ComponentTypeT *EntityComponentManager::Component(const Entity _entity) this->ComponentImplementation(_entity, typeId)); } +////////////////////////////////////////////////// +template +const ComponentTypeT *EntityComponentManager::Component( + const ComponentKey &_key) const +{ + return static_cast( + this->ComponentImplementation(_key.second, _key.first)); +} + +////////////////////////////////////////////////// +template +ComponentTypeT *EntityComponentManager::Component(const ComponentKey &_key) +{ + return static_cast( + this->ComponentImplementation(_key.second, _key.first)); +} + ////////////////////////////////////////////////// template ComponentTypeT *EntityComponentManager::ComponentDefault(Entity _entity, diff --git a/include/ignition/gazebo/detail/ComponentStorage.hh b/include/ignition/gazebo/detail/EntityStorage.hh similarity index 98% rename from include/ignition/gazebo/detail/ComponentStorage.hh rename to include/ignition/gazebo/detail/EntityStorage.hh index daf003dbd5..1428613e76 100644 --- a/include/ignition/gazebo/detail/ComponentStorage.hh +++ b/include/ignition/gazebo/detail/EntityStorage.hh @@ -35,7 +35,7 @@ namespace gazebo inline namespace IGNITION_GAZEBO_VERSION_NAMESPACE { namespace detail { -/// \brief Enum class that lets external users of ComponentStorage know what +/// \brief Enum class that lets external users of EntityStorage know what /// type of action took place when a component was added to an entity. enum class ComponentAdditionResult { @@ -59,7 +59,7 @@ enum class ComponentAdditionResult /// /// \note The symbols are visible so that unit tests can be written for this /// class. -class IGNITION_GAZEBO_VISIBLE ComponentStorage +class IGNITION_GAZEBO_VISIBLE EntityStorage { /// \brief Clear all of the entity and component data that has been tracked. /// This "resets" the storage to its initial, empty state. diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 7a48220efd..3e01ffd724 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -43,9 +43,9 @@ target_link_libraries(${ign_lib_target} set (sources Barrier.cc BaseView.cc - ComponentStorage.cc Conversions.cc EntityComponentManager.cc + EntityStorage.cc LevelManager.cc Link.cc Model.cc @@ -68,7 +68,7 @@ set (gtest_sources BaseView_TEST.cc Component_TEST.cc ComponentFactory_TEST.cc - ComponentStorage_TEST.cc + EntityStorage_TEST.cc Conversions_TEST.cc EntityComponentManager_TEST.cc EventManager_TEST.cc diff --git a/src/EntityComponentManager.cc b/src/EntityComponentManager.cc index 65fc8c3204..df2bd1e2f1 100644 --- a/src/EntityComponentManager.cc +++ b/src/EntityComponentManager.cc @@ -27,7 +27,7 @@ #include #include "ignition/gazebo/components/Component.hh" #include "ignition/gazebo/components/Factory.hh" -#include "ignition/gazebo/detail/ComponentStorage.hh" +#include "ignition/gazebo/detail/EntityStorage.hh" #include "ignition/gazebo/EntityComponentManager.hh" using namespace ignition; @@ -78,7 +78,7 @@ class ignition::gazebo::EntityComponentManagerPrivate /// \brief A class that stores all components and maps entities to their /// component types - public: detail::ComponentStorage componentStorage; + public: detail::EntityStorage entityStorage; /// \brief All component types that have ever been created. public: std::unordered_set createdCompTypes; @@ -211,7 +211,7 @@ Entity EntityComponentManagerPrivate::CreateEntityImplementation(Entity _entity) // Reset descendants cache this->descendantCache.clear(); - if (!this->componentStorage.AddEntity(_entity)) + if (!this->entityStorage.AddEntity(_entity)) { ignwarn << "Attempted to add entity [" << _entity << "] to component storage, but this entity is already in component " @@ -307,7 +307,7 @@ void EntityComponentManager::ProcessRemoveEntityRequests() this->dataPtr->toRemoveEntities.clear(); this->dataPtr->entityComponentsDirty = true; - this->dataPtr->componentStorage.Reset(); + this->dataPtr->entityStorage.Reset(); // All views are now invalid. this->dataPtr->views.clear(); @@ -329,7 +329,7 @@ void EntityComponentManager::ProcessRemoveEntityRequests() // Remove the components, if any. if (entityIter != this->dataPtr->entityComponents.end()) { - this->dataPtr->componentStorage.RemoveEntity(entity); + this->dataPtr->entityStorage.RemoveEntity(entity); // Remove the entry in the entityComponent map this->dataPtr->entityComponents.erase(entity); @@ -379,7 +379,7 @@ bool EntityComponentManager::RemoveComponent( } auto removedComp = - this->dataPtr->componentStorage.RemoveComponent(_entity, _typeId); + this->dataPtr->entityStorage.RemoveComponent(_entity, _typeId); if (removedComp) { // update views to reflect the component removal @@ -402,9 +402,6 @@ bool EntityComponentManager::RemoveComponent( bool EntityComponentManager::RemoveComponent( const Entity _entity, const ComponentKey &_key) { - ignwarn << "EntityComponentManager::RemoveComponent is deprecated. Please " - << "use the RemoveComponent method with a ComponentTypeId parameter type " - << "instead." << std::endl; return this->RemoveComponent(_entity, _key.first); } @@ -412,9 +409,6 @@ bool EntityComponentManager::RemoveComponent( bool EntityComponentManager::EntityHasComponent(const Entity _entity, const ComponentKey &_key) const { - ignwarn << "EntityComponentManager::EntityHasComponent is deprecated. Please " - << "use EntityComponentManager::EntityHasComponentType instead." - << std::endl; if (!this->HasEntity(_entity)) return false; auto &compSet = this->dataPtr->entityComponents[_entity]; @@ -593,7 +587,7 @@ bool EntityComponentManager::CreateComponentImplementation( auto newComp = components::Factory::Instance()->New(_componentTypeId, _data); auto compAddResult = - this->dataPtr->componentStorage.AddComponent(_entity, std::move(newComp)); + this->dataPtr->entityStorage.AddComponent(_entity, std::move(newComp)); switch (compAddResult) { case detail::ComponentAdditionResult::FAILED_ADDITION: @@ -661,14 +655,14 @@ const components::BaseComponent const Entity _entity, const ComponentTypeId _type) const { IGN_PROFILE("EntityComponentManager::ComponentImplementation"); - return this->dataPtr->componentStorage.ValidComponent(_entity, _type); + return this->dataPtr->entityStorage.ValidComponent(_entity, _type); } ///////////////////////////////////////////////// components::BaseComponent *EntityComponentManager::ComponentImplementation( const Entity _entity, const ComponentTypeId _type) { - return this->dataPtr->componentStorage.ValidComponent(_entity, _type); + return this->dataPtr->entityStorage.ValidComponent(_entity, _type); } ///////////////////////////////////////////////// diff --git a/src/EntityComponentManager_TEST.cc b/src/EntityComponentManager_TEST.cc index 4d878ee92c..a7322993b7 100644 --- a/src/EntityComponentManager_TEST.cc +++ b/src/EntityComponentManager_TEST.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include "ignition/gazebo/components/Factory.hh" #include "ignition/gazebo/components/Pose.hh" @@ -2561,6 +2562,77 @@ TEST_P(EntityComponentManagerFixture, RemovedComponentsSyncBetweenServerAndGUI) } } +///////////////////////////////////////////////// +// Check that some widely used deprecated APIs still work +TEST_P(EntityComponentManagerFixture, Deprecated) +{ + IGN_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + + // Create some entities + auto eInt = manager.CreateEntity(); + auto eDouble = manager.CreateEntity(); + auto eIntDouble = manager.CreateEntity(); + EXPECT_EQ(3u, manager.EntityCount()); + + // Add components and keep their unique ComponentKeys + EXPECT_NE(nullptr, manager.CreateComponent(eInt, + IntComponent(123))); + ComponentKey cIntEInt = {IntComponent::typeId, eInt}; + + EXPECT_NE(nullptr, manager.CreateComponent(eDouble, + DoubleComponent(0.123))); + ComponentKey cDoubleEDouble = {DoubleComponent::typeId, eDouble}; + + EXPECT_NE(nullptr, manager.CreateComponent(eIntDouble, + IntComponent(456))); + ComponentKey cIntEIntDouble = {IntComponent::typeId, eIntDouble}; + + EXPECT_NE(nullptr, manager.CreateComponent(eIntDouble, + DoubleComponent(0.456))); + ComponentKey cDoubleEIntDouble = {DoubleComponent::typeId, eIntDouble}; + + // Check entities have the components + EXPECT_TRUE(manager.EntityHasComponent(eInt, cIntEInt)); + EXPECT_EQ(1u, manager.ComponentTypes(eInt).size()); + EXPECT_EQ(IntComponent::typeId, *manager.ComponentTypes(eInt).begin()); + + EXPECT_TRUE(manager.EntityHasComponent(eDouble, cDoubleEDouble)); + EXPECT_EQ(1u, manager.ComponentTypes(eDouble).size()); + EXPECT_EQ(DoubleComponent::typeId, *manager.ComponentTypes(eDouble).begin()); + + EXPECT_TRUE(manager.EntityHasComponent(eIntDouble, cIntEIntDouble)); + EXPECT_TRUE(manager.EntityHasComponent(eIntDouble, cDoubleEIntDouble)); + EXPECT_EQ(2u, manager.ComponentTypes(eIntDouble).size()); + auto types = manager.ComponentTypes(eIntDouble); + EXPECT_NE(types.end(), types.find(IntComponent::typeId)); + EXPECT_NE(types.end(), types.find(DoubleComponent::typeId)); + + // Remove component by key + EXPECT_TRUE(manager.RemoveComponent(eInt, cIntEInt)); + EXPECT_FALSE(manager.EntityHasComponent(eInt, cIntEInt)); + EXPECT_TRUE(manager.ComponentTypes(eInt).empty()); + + // Remove component by type id + auto typeDouble = DoubleComponent::typeId; + + EXPECT_TRUE(manager.RemoveComponent(eDouble, typeDouble)); + EXPECT_FALSE(manager.EntityHasComponent(eDouble, cDoubleEDouble)); + EXPECT_TRUE(manager.ComponentTypes(eDouble).empty()); + + // Remove component by type + EXPECT_TRUE(manager.RemoveComponent(eIntDouble)); + EXPECT_FALSE(manager.EntityHasComponent(eIntDouble, cIntEIntDouble)); + EXPECT_TRUE(manager.EntityHasComponent(eIntDouble, cDoubleEIntDouble)); + EXPECT_EQ(1u, manager.ComponentTypes(eIntDouble).size()); + + EXPECT_TRUE(manager.RemoveComponent(eIntDouble)); + EXPECT_FALSE(manager.EntityHasComponent(eIntDouble, cIntEIntDouble)); + EXPECT_FALSE(manager.EntityHasComponent(eIntDouble, cDoubleEIntDouble)); + EXPECT_EQ(0u, manager.ComponentTypes(eIntDouble).size()); + + IGN_UTILS_WARN_RESUME__DEPRECATED_DECLARATION +} + // Run multiple times. We want to make sure that static globals don't cause // problems. INSTANTIATE_TEST_SUITE_P(EntityComponentManagerRepeat, diff --git a/src/ComponentStorage.cc b/src/EntityStorage.cc similarity index 88% rename from src/ComponentStorage.cc rename to src/EntityStorage.cc index 39c77e1297..51c694eff2 100644 --- a/src/ComponentStorage.cc +++ b/src/EntityStorage.cc @@ -14,7 +14,7 @@ * limitations under the License. * */ -#include "ignition/gazebo/detail/ComponentStorage.hh" +#include "ignition/gazebo/detail/EntityStorage.hh" #include #include @@ -30,14 +30,14 @@ using namespace gazebo; using namespace detail; ////////////////////////////////////////////////// -void ComponentStorage::Reset() +void EntityStorage::Reset() { this->entityComponents.clear(); this->componentTypeIndex.clear(); } ////////////////////////////////////////////////// -bool ComponentStorage::AddEntity(const Entity _entity) +bool EntityStorage::AddEntity(const Entity _entity) { const auto [it, success] = this->entityComponents.insert({_entity, std::vector>()}); @@ -51,7 +51,7 @@ bool ComponentStorage::AddEntity(const Entity _entity) } ////////////////////////////////////////////////// -bool ComponentStorage::RemoveEntity(const Entity _entity) +bool EntityStorage::RemoveEntity(const Entity _entity) { const auto removedComponents = this->entityComponents.erase(_entity); const auto removedTypeIdxMap = this->componentTypeIndex.erase(_entity); @@ -59,7 +59,7 @@ bool ComponentStorage::RemoveEntity(const Entity _entity) } ////////////////////////////////////////////////// -ComponentAdditionResult ComponentStorage::AddComponent( +ComponentAdditionResult EntityStorage::AddComponent( const Entity _entity, std::unique_ptr _component) { @@ -101,7 +101,7 @@ ComponentAdditionResult ComponentStorage::AddComponent( } ////////////////////////////////////////////////// -bool ComponentStorage::RemoveComponent(const Entity _entity, +bool EntityStorage::RemoveComponent(const Entity _entity, const ComponentTypeId _typeId) { auto compPtr = this->Component(_entity, _typeId); @@ -116,7 +116,7 @@ bool ComponentStorage::RemoveComponent(const Entity _entity, } ////////////////////////////////////////////////// -const components::BaseComponent *ComponentStorage::Component( +const components::BaseComponent *EntityStorage::Component( const Entity _entity, const ComponentTypeId _typeId) const { @@ -144,15 +144,15 @@ const components::BaseComponent *ComponentStorage::Component( } ////////////////////////////////////////////////// -components::BaseComponent *ComponentStorage::Component(const Entity _entity, +components::BaseComponent *EntityStorage::Component(const Entity _entity, const ComponentTypeId _typeId) { return const_cast( - static_cast(*this).Component(_entity, _typeId)); + static_cast(*this).Component(_entity, _typeId)); } ////////////////////////////////////////////////// -const components::BaseComponent *ComponentStorage::ValidComponent( +const components::BaseComponent *EntityStorage::ValidComponent( const Entity _entity, const ComponentTypeId _typeId) const { auto compPtr = this->Component(_entity, _typeId); @@ -162,7 +162,7 @@ const components::BaseComponent *ComponentStorage::ValidComponent( } ////////////////////////////////////////////////// -components::BaseComponent *ComponentStorage::ValidComponent( +components::BaseComponent *EntityStorage::ValidComponent( const Entity _entity, const ComponentTypeId _typeId) { auto compPtr = this->Component(_entity, _typeId); diff --git a/src/ComponentStorage_TEST.cc b/src/EntityStorage_TEST.cc similarity index 93% rename from src/ComponentStorage_TEST.cc rename to src/EntityStorage_TEST.cc index 49f125f031..0f4d8c12e8 100644 --- a/src/ComponentStorage_TEST.cc +++ b/src/EntityStorage_TEST.cc @@ -26,7 +26,7 @@ #include "ignition/gazebo/components/Factory.hh" #include "ignition/gazebo/components/LinearVelocity.hh" #include "ignition/gazebo/components/Pose.hh" -#include "ignition/gazebo/detail/ComponentStorage.hh" +#include "ignition/gazebo/detail/EntityStorage.hh" using namespace ignition; using namespace gazebo; @@ -42,7 +42,7 @@ const Entity entity3 = 3; const Entity entity4 = 4; ///////////////////////////////////////////////// -class ComponentStorageTest : public ::testing::Test +class EntityStorageTest : public ::testing::Test { // Documentation inherited protected: void SetUp() override @@ -65,7 +65,7 @@ class ComponentStorageTest : public ::testing::Test return this->storage.AddComponent(_entity, std::move(compPtr)); } - /// \brief Helper function that uses ComponentStorage::ValidComponent to get + /// \brief Helper function that uses EntityStorage::ValidComponent to get /// a component of a particular type that belongs to an entity. /// \param[in] _entity The entity /// \return A pointer to the component of the templated type. If no such @@ -87,7 +87,7 @@ class ComponentStorageTest : public ::testing::Test return static_cast(baseComp); } - public: detail::ComponentStorage storage; + public: detail::EntityStorage storage; public: const Entity e1{1}; @@ -95,7 +95,7 @@ class ComponentStorageTest : public ::testing::Test }; ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, AddEntity) +TEST_F(EntityStorageTest, AddEntity) { // Entities were already added to the storage in the test fixture's SetUp // method. So, try to add entities that are already in the storage. @@ -109,7 +109,7 @@ TEST_F(ComponentStorageTest, AddEntity) } ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, RemoveEntity) +TEST_F(EntityStorageTest, RemoveEntity) { // Try to remove entities that aren't in the storage EXPECT_FALSE(this->storage.RemoveEntity(3)); @@ -125,7 +125,7 @@ TEST_F(ComponentStorageTest, RemoveEntity) } ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, AddComponent) +TEST_F(EntityStorageTest, AddComponent) { // Add components to entities in the storage EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -166,8 +166,8 @@ TEST_F(ComponentStorageTest, AddComponent) &linVelComp2)); // We can't check if the modification actually took place since this requires - // functionality beyond the ComponentStorage API (see the comments in the - // ComponentStorage::AddComponent method definition for more details), but we + // functionality beyond the EntityStorage API (see the comments in the + // EntityStorage::AddComponent method definition for more details), but we // can at least check that the components still exist after modification ASSERT_NE(nullptr, this->Component(this->e1)); ASSERT_NE(nullptr, this->Component(this->e2)); @@ -195,7 +195,7 @@ TEST_F(ComponentStorageTest, AddComponent) } ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, RemoveComponent) +TEST_F(EntityStorageTest, RemoveComponent) { // Add components to entities EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -231,7 +231,7 @@ TEST_F(ComponentStorageTest, RemoveComponent) } ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, ValidComponent) +TEST_F(EntityStorageTest, ValidComponent) { // Attach a component to an entity EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -260,9 +260,9 @@ TEST_F(ComponentStorageTest, ValidComponent) ///////////////////////////////////////////////// // Similar to the ValidComponent test, but this test covers the const version of -// the ComponentStorage::ValidComponent method (the ValidComponent test covered -// the non-const version of the ComponentStorage::ValidComponent) -TEST_F(ComponentStorageTest, ValidComponentConst) +// the EntityStorage::ValidComponent method (the ValidComponent test covered +// the non-const version of the EntityStorage::ValidComponent) +TEST_F(EntityStorageTest, ValidComponentConst) { // Attach a component to an entity EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -285,7 +285,7 @@ TEST_F(ComponentStorageTest, ValidComponentConst) } ///////////////////////////////////////////////// -TEST_F(ComponentStorageTest, Reset) +TEST_F(EntityStorageTest, Reset) { // Add components to entities in the storage EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, diff --git a/test/integration/view.cc b/test/integration/view.cc index c45f4be560..d9661d939c 100644 --- a/test/integration/view.cc +++ b/test/integration/view.cc @@ -28,7 +28,7 @@ #include "ignition/gazebo/Entity.hh" #include "ignition/gazebo/Types.hh" #include "ignition/gazebo/detail/View.hh" -#include "ignition/gazebo/detail/ComponentStorage.hh" +#include "ignition/gazebo/detail/EntityStorage.hh" #include "ignition/gazebo/components/Component.hh" #include "ignition/gazebo/components/Factory.hh" #include "ignition/gazebo/components/Model.hh" @@ -62,7 +62,7 @@ class ViewTest : public::testing::Test } }; -/// \brief Helper class that wraps ComponentStorage to provide a simplified +/// \brief Helper class that wraps EntityStorage to provide a simplified /// API for working with entities and their components. This class also stores /// a set of user-defined views and handles updating views as needed whenever /// an entity's components are modified. @@ -187,7 +187,7 @@ class StorageViewWrapper EXPECT_EQ(entityComp->Data(), _component.Data()); } - /// \brief Helper function that uses ComponentStorage::ValidComponent to get + /// \brief Helper function that uses EntityStorage::ValidComponent to get /// a component of a particular type that belongs to an entity. /// \param[in] _entity The entity /// \return A pointer to the component of the templated type. If no such @@ -230,8 +230,8 @@ class StorageViewWrapper return this->storage.AddComponent(_entity, std::move(compPtr)); } - /// \brief The actual detail::ComponentStorage instance - private: detail::ComponentStorage storage; + /// \brief The actual detail::EntityStorage instance + private: detail::EntityStorage storage; /// \brief All of the views that will be updated as needed based on the /// changes being made to this->storage From 45a982a4f832adcc2f77a50c3e04dbf015d4727c Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Tue, 22 Jun 2021 10:28:10 -0700 Subject: [PATCH 2/5] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Louise Poubel Co-authored-by: Alejandro Hernández Cordero --- include/ignition/gazebo/components/Factory.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/ignition/gazebo/components/Factory.hh b/include/ignition/gazebo/components/Factory.hh index a91fc9745c..f7caee0c81 100644 --- a/include/ignition/gazebo/components/Factory.hh +++ b/include/ignition/gazebo/components/Factory.hh @@ -124,7 +124,7 @@ namespace components /// \deprecated See function that doesn't accept a storage public: template void IGN_DEPRECATED(6) Register(const std::string &_type, - ComponentDescriptorBase *_compDesc, StorageDescriptorBase *) + ComponentDescriptorBase *_compDesc, StorageDescriptorBase * /*_storageDesc*/) { this->Register(_type, _compDesc); } @@ -289,7 +289,7 @@ namespace components /// \return Always returns nullptr. /// \deprecated Storages aren't necessary anymore. public: std::unique_ptr IGN_DEPRECATED(6) NewStorage( - const ComponentTypeId &) + const ComponentTypeId & /*_typeId*/) { return nullptr; } From e9eeef00533478b7b29eefff6bf47a96868309c9 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Fri, 9 Jul 2021 15:14:27 -0700 Subject: [PATCH 3/5] Update ComponentKey, EntityComponentStorage Signed-off-by: Louise Poubel --- include/ignition/gazebo/Types.hh | 6 ++-- .../ignition/gazebo/components/Component.hh | 6 ++-- ...tyStorage.hh => EntityComponentStorage.hh} | 4 +-- src/CMakeLists.txt | 12 ++++---- src/EntityComponentManager.cc | 4 +-- ...tyStorage.cc => EntityComponentStorage.cc} | 25 ++++++++-------- ...TEST.cc => EntityComponentStorage_TEST.cc} | 30 +++++++++---------- test/integration/view.cc | 10 +++---- 8 files changed, 50 insertions(+), 47 deletions(-) rename include/ignition/gazebo/detail/{EntityStorage.hh => EntityComponentStorage.hh} (98%) rename src/{EntityStorage.cc => EntityComponentStorage.cc} (87%) rename src/{EntityStorage_TEST.cc => EntityComponentStorage_TEST.cc} (93%) diff --git a/include/ignition/gazebo/Types.hh b/include/ignition/gazebo/Types.hh index ee0a75e106..d30ae00d06 100644 --- a/include/ignition/gazebo/Types.hh +++ b/include/ignition/gazebo/Types.hh @@ -22,6 +22,8 @@ #include #include +#include "ignition/gazebo/Entity.hh" + namespace ignition { namespace gazebo @@ -89,10 +91,10 @@ namespace ignition /// \brief A key that uniquely identifies, at the global scope, a component /// instance - /// \note On version 6, the 2nd element equals the entity ID. + /// \note On version 6, the 2nd element was changed to the entity ID. /// \deprecated Deprecated on version 6, removed on version 7. Use /// ComponentTypeId + Entity instead. - using ComponentKey = std::pair; + using ComponentKey = std::pair; /// \brief typedef for query callbacks using EntityQueryCallback = std::function #include "ignition/gazebo/components/Component.hh" #include "ignition/gazebo/components/Factory.hh" -#include "ignition/gazebo/detail/EntityStorage.hh" +#include "ignition/gazebo/detail/EntityComponentStorage.hh" #include "ignition/gazebo/EntityComponentManager.hh" using namespace ignition; @@ -78,7 +78,7 @@ class ignition::gazebo::EntityComponentManagerPrivate /// \brief A class that stores all components and maps entities to their /// component types - public: detail::EntityStorage entityStorage; + public: detail::EntityComponentStorage entityStorage; /// \brief All component types that have ever been created. public: std::unordered_set createdCompTypes; diff --git a/src/EntityStorage.cc b/src/EntityComponentStorage.cc similarity index 87% rename from src/EntityStorage.cc rename to src/EntityComponentStorage.cc index 51c694eff2..495441342e 100644 --- a/src/EntityStorage.cc +++ b/src/EntityComponentStorage.cc @@ -14,7 +14,7 @@ * limitations under the License. * */ -#include "ignition/gazebo/detail/EntityStorage.hh" +#include "ignition/gazebo/detail/EntityComponentStorage.hh" #include #include @@ -30,14 +30,14 @@ using namespace gazebo; using namespace detail; ////////////////////////////////////////////////// -void EntityStorage::Reset() +void EntityComponentStorage::Reset() { this->entityComponents.clear(); this->componentTypeIndex.clear(); } ////////////////////////////////////////////////// -bool EntityStorage::AddEntity(const Entity _entity) +bool EntityComponentStorage::AddEntity(const Entity _entity) { const auto [it, success] = this->entityComponents.insert({_entity, std::vector>()}); @@ -51,7 +51,7 @@ bool EntityStorage::AddEntity(const Entity _entity) } ////////////////////////////////////////////////// -bool EntityStorage::RemoveEntity(const Entity _entity) +bool EntityComponentStorage::RemoveEntity(const Entity _entity) { const auto removedComponents = this->entityComponents.erase(_entity); const auto removedTypeIdxMap = this->componentTypeIndex.erase(_entity); @@ -59,7 +59,7 @@ bool EntityStorage::RemoveEntity(const Entity _entity) } ////////////////////////////////////////////////// -ComponentAdditionResult EntityStorage::AddComponent( +ComponentAdditionResult EntityComponentStorage::AddComponent( const Entity _entity, std::unique_ptr _component) { @@ -101,7 +101,7 @@ ComponentAdditionResult EntityStorage::AddComponent( } ////////////////////////////////////////////////// -bool EntityStorage::RemoveComponent(const Entity _entity, +bool EntityComponentStorage::RemoveComponent(const Entity _entity, const ComponentTypeId _typeId) { auto compPtr = this->Component(_entity, _typeId); @@ -116,7 +116,7 @@ bool EntityStorage::RemoveComponent(const Entity _entity, } ////////////////////////////////////////////////// -const components::BaseComponent *EntityStorage::Component( +const components::BaseComponent *EntityComponentStorage::Component( const Entity _entity, const ComponentTypeId _typeId) const { @@ -144,15 +144,16 @@ const components::BaseComponent *EntityStorage::Component( } ////////////////////////////////////////////////// -components::BaseComponent *EntityStorage::Component(const Entity _entity, - const ComponentTypeId _typeId) +components::BaseComponent *EntityComponentStorage::Component( + const Entity _entity, const ComponentTypeId _typeId) { return const_cast( - static_cast(*this).Component(_entity, _typeId)); + static_cast(*this).Component(_entity, + _typeId)); } ////////////////////////////////////////////////// -const components::BaseComponent *EntityStorage::ValidComponent( +const components::BaseComponent *EntityComponentStorage::ValidComponent( const Entity _entity, const ComponentTypeId _typeId) const { auto compPtr = this->Component(_entity, _typeId); @@ -162,7 +163,7 @@ const components::BaseComponent *EntityStorage::ValidComponent( } ////////////////////////////////////////////////// -components::BaseComponent *EntityStorage::ValidComponent( +components::BaseComponent *EntityComponentStorage::ValidComponent( const Entity _entity, const ComponentTypeId _typeId) { auto compPtr = this->Component(_entity, _typeId); diff --git a/src/EntityStorage_TEST.cc b/src/EntityComponentStorage_TEST.cc similarity index 93% rename from src/EntityStorage_TEST.cc rename to src/EntityComponentStorage_TEST.cc index 0f4d8c12e8..28a69f7c00 100644 --- a/src/EntityStorage_TEST.cc +++ b/src/EntityComponentStorage_TEST.cc @@ -26,7 +26,7 @@ #include "ignition/gazebo/components/Factory.hh" #include "ignition/gazebo/components/LinearVelocity.hh" #include "ignition/gazebo/components/Pose.hh" -#include "ignition/gazebo/detail/EntityStorage.hh" +#include "ignition/gazebo/detail/EntityComponentStorage.hh" using namespace ignition; using namespace gazebo; @@ -42,7 +42,7 @@ const Entity entity3 = 3; const Entity entity4 = 4; ///////////////////////////////////////////////// -class EntityStorageTest : public ::testing::Test +class EntityComponentStorageTest : public ::testing::Test { // Documentation inherited protected: void SetUp() override @@ -65,7 +65,7 @@ class EntityStorageTest : public ::testing::Test return this->storage.AddComponent(_entity, std::move(compPtr)); } - /// \brief Helper function that uses EntityStorage::ValidComponent to get + /// \brief Helper function that uses EntityComponentStorage::ValidComponent to get /// a component of a particular type that belongs to an entity. /// \param[in] _entity The entity /// \return A pointer to the component of the templated type. If no such @@ -87,7 +87,7 @@ class EntityStorageTest : public ::testing::Test return static_cast(baseComp); } - public: detail::EntityStorage storage; + public: detail::EntityComponentStorage storage; public: const Entity e1{1}; @@ -95,7 +95,7 @@ class EntityStorageTest : public ::testing::Test }; ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, AddEntity) +TEST_F(EntityComponentStorageTest, AddEntity) { // Entities were already added to the storage in the test fixture's SetUp // method. So, try to add entities that are already in the storage. @@ -109,7 +109,7 @@ TEST_F(EntityStorageTest, AddEntity) } ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, RemoveEntity) +TEST_F(EntityComponentStorageTest, RemoveEntity) { // Try to remove entities that aren't in the storage EXPECT_FALSE(this->storage.RemoveEntity(3)); @@ -125,7 +125,7 @@ TEST_F(EntityStorageTest, RemoveEntity) } ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, AddComponent) +TEST_F(EntityComponentStorageTest, AddComponent) { // Add components to entities in the storage EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -166,8 +166,8 @@ TEST_F(EntityStorageTest, AddComponent) &linVelComp2)); // We can't check if the modification actually took place since this requires - // functionality beyond the EntityStorage API (see the comments in the - // EntityStorage::AddComponent method definition for more details), but we + // functionality beyond the EntityComponentStorage API (see the comments in the + // EntityComponentStorage::AddComponent method definition for more details), but we // can at least check that the components still exist after modification ASSERT_NE(nullptr, this->Component(this->e1)); ASSERT_NE(nullptr, this->Component(this->e2)); @@ -195,7 +195,7 @@ TEST_F(EntityStorageTest, AddComponent) } ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, RemoveComponent) +TEST_F(EntityComponentStorageTest, RemoveComponent) { // Add components to entities EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -231,7 +231,7 @@ TEST_F(EntityStorageTest, RemoveComponent) } ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, ValidComponent) +TEST_F(EntityComponentStorageTest, ValidComponent) { // Attach a component to an entity EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -260,9 +260,9 @@ TEST_F(EntityStorageTest, ValidComponent) ///////////////////////////////////////////////// // Similar to the ValidComponent test, but this test covers the const version of -// the EntityStorage::ValidComponent method (the ValidComponent test covered -// the non-const version of the EntityStorage::ValidComponent) -TEST_F(EntityStorageTest, ValidComponentConst) +// the EntityComponentStorage::ValidComponent method (the ValidComponent test covered +// the non-const version of the EntityComponentStorage::ValidComponent) +TEST_F(EntityComponentStorageTest, ValidComponentConst) { // Attach a component to an entity EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, @@ -285,7 +285,7 @@ TEST_F(EntityStorageTest, ValidComponentConst) } ///////////////////////////////////////////////// -TEST_F(EntityStorageTest, Reset) +TEST_F(EntityComponentStorageTest, Reset) { // Add components to entities in the storage EXPECT_EQ(detail::ComponentAdditionResult::NEW_ADDITION, diff --git a/test/integration/view.cc b/test/integration/view.cc index d9661d939c..f01cf9e3d0 100644 --- a/test/integration/view.cc +++ b/test/integration/view.cc @@ -28,7 +28,7 @@ #include "ignition/gazebo/Entity.hh" #include "ignition/gazebo/Types.hh" #include "ignition/gazebo/detail/View.hh" -#include "ignition/gazebo/detail/EntityStorage.hh" +#include "ignition/gazebo/detail/EntityComponentStorage.hh" #include "ignition/gazebo/components/Component.hh" #include "ignition/gazebo/components/Factory.hh" #include "ignition/gazebo/components/Model.hh" @@ -62,7 +62,7 @@ class ViewTest : public::testing::Test } }; -/// \brief Helper class that wraps EntityStorage to provide a simplified +/// \brief Helper class that wraps EntityComponentStorage to provide a simplified /// API for working with entities and their components. This class also stores /// a set of user-defined views and handles updating views as needed whenever /// an entity's components are modified. @@ -187,7 +187,7 @@ class StorageViewWrapper EXPECT_EQ(entityComp->Data(), _component.Data()); } - /// \brief Helper function that uses EntityStorage::ValidComponent to get + /// \brief Helper function that uses EntityComponentStorage::ValidComponent to get /// a component of a particular type that belongs to an entity. /// \param[in] _entity The entity /// \return A pointer to the component of the templated type. If no such @@ -230,8 +230,8 @@ class StorageViewWrapper return this->storage.AddComponent(_entity, std::move(compPtr)); } - /// \brief The actual detail::EntityStorage instance - private: detail::EntityStorage storage; + /// \brief The actual detail::EntityComponentStorage instance + private: detail::EntityComponentStorage storage; /// \brief All of the views that will be updated as needed based on the /// changes being made to this->storage From b5b03d4e6d0a26b17d16bf780c8059cf2cfa9003 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Tue, 13 Jul 2021 16:43:52 -0700 Subject: [PATCH 4/5] Add test case from PR feedback Signed-off-by: Louise Poubel --- src/EntityComponentManager_TEST.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/EntityComponentManager_TEST.cc b/src/EntityComponentManager_TEST.cc index a7322993b7..54af688f4b 100644 --- a/src/EntityComponentManager_TEST.cc +++ b/src/EntityComponentManager_TEST.cc @@ -2568,6 +2568,10 @@ TEST_P(EntityComponentManagerFixture, Deprecated) { IGN_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + // Fail to create component for inexistent entity + EXPECT_EQ(nullptr, manager.CreateComponent(789, + IntComponent(123))); + // Create some entities auto eInt = manager.CreateEntity(); auto eDouble = manager.CreateEntity(); From 2e2252dd3a66ba1f5b91650ef59382707f37dee3 Mon Sep 17 00:00:00 2001 From: Louise Poubel Date: Tue, 13 Jul 2021 16:59:22 -0700 Subject: [PATCH 5/5] cppcheck Signed-off-by: Louise Poubel --- include/ignition/gazebo/components/Factory.hh | 3 ++- test/integration/view.cc | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/include/ignition/gazebo/components/Factory.hh b/include/ignition/gazebo/components/Factory.hh index f7caee0c81..25b3d2b1da 100644 --- a/include/ignition/gazebo/components/Factory.hh +++ b/include/ignition/gazebo/components/Factory.hh @@ -124,7 +124,8 @@ namespace components /// \deprecated See function that doesn't accept a storage public: template void IGN_DEPRECATED(6) Register(const std::string &_type, - ComponentDescriptorBase *_compDesc, StorageDescriptorBase * /*_storageDesc*/) + ComponentDescriptorBase *_compDesc, + StorageDescriptorBase * /*_storageDesc*/) { this->Register(_type, _compDesc); } diff --git a/test/integration/view.cc b/test/integration/view.cc index 3bfd3c7432..b7497e10f3 100644 --- a/test/integration/view.cc +++ b/test/integration/view.cc @@ -62,10 +62,10 @@ class ViewTest : public::testing::Test } }; -/// \brief Helper class that wraps EntityComponentStorage to provide a simplified -/// API for working with entities and their components. This class also stores -/// a set of user-defined views and handles updating views as needed whenever -/// an entity's components are modified. +/// \brief Helper class that wraps EntityComponentStorage to provide a +/// simplified API for working with entities and their components. This class +/// also stores a set of user-defined views and handles updating views as +/// needed whenever an entity's components are modified. class StorageViewWrapper { /// \brief Constructor @@ -187,8 +187,8 @@ class StorageViewWrapper EXPECT_EQ(entityComp->Data(), _component.Data()); } - /// \brief Helper function that uses EntityComponentStorage::ValidComponent to get - /// a component of a particular type that belongs to an entity. + /// \brief Helper function that uses EntityComponentStorage::ValidComponent + /// to get a component of a particular type that belongs to an entity. /// \param[in] _entity The entity /// \return A pointer to the component of the templated type. If no such /// component exists, nullptr is returned