Skip to content
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

Use statically-typed views for better performance #856

Merged
merged 30 commits into from
Aug 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
046210a
Use statically-typed views for better performance
adlarkin May 27, 2021
e4a6568
Merge branch 'main' into adlarkin/view_restructure
iche033 Jun 18, 2021
ee9b6c5
clearer deprecation messages
adlarkin Jun 18, 2021
39b499e
review feedback
adlarkin Jun 18, 2021
cd635dd
use a removed flag on components instead of a ignore flag
adlarkin Jun 28, 2021
f8204de
add nullptr checks for CreateComponent and RemoveComponent in ECM uni…
adlarkin Jun 29, 2021
52f8c7e
updated documentation and other review nits
adlarkin Jun 29, 2021
023bc7f
Merge branch 'main' into adlarkin/view_restructure
adlarkin Jun 30, 2021
40101a8
added ComponentStorage tests
adlarkin Jul 1, 2021
ee3d3d0
added tests for View and BaseView classes
adlarkin Jul 2, 2021
4a90e86
remove ComponentStorage from detail namespace
adlarkin Jul 13, 2021
782844a
re-construct ComponentStorage instead of using Reset()
adlarkin Jul 13, 2021
ea7d9e9
separate function declarations from implementation for View class
adlarkin Jul 13, 2021
7e17a61
Merge branch 'main' into adlarkin/view_restructure
adlarkin Jul 13, 2021
d3e5a25
renamed ComponentStorage to EntityComponentStorage
adlarkin Jul 13, 2021
a177b19
Proposal for #856: Deprecations instead of removals (#873)
chapulina Jul 14, 2021
b63a428
Merge branch 'main' into adlarkin/view_restructure
adlarkin Aug 2, 2021
d02d72a
Fix crash on GUI entity removal with levels (#913)
adlarkin Aug 4, 2021
d2752ca
Merge branch 'main' into adlarkin/view_restructure
adlarkin Aug 4, 2021
86e1dd3
Simplifications to storage (#943)
chapulina Aug 6, 2021
f0a0fe8
update migration guide with deprecations and method/type modifications
adlarkin Aug 6, 2021
d871df3
review feedback
adlarkin Aug 10, 2021
660268e
Merge branch 'main' into adlarkin/view_restructure
adlarkin Aug 10, 2021
9baa1a8
set visibility for template specializations
chapulina Aug 12, 2021
66bb2a2
Try the ign-gui plugins approach
chapulina Aug 12, 2021
640df12
merged from main
chapulina Aug 12, 2021
d9a49e6
Make Server test not write to /home/chapulina, attempt more Windows s…
chapulina Aug 12, 2021
772a5f8
Remove visibility from templated class and methods
j-rivero Aug 12, 2021
2dadec6
merged from main
chapulina Aug 13, 2021
abf9d23
Merge branch 'main' into adlarkin/view_restructure
chapulina Aug 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions Migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,33 @@ release will remove the deprecated code.
* `dynamic_pose/info` topic is removed from `LogRecord` and `LogPlayback`
since pose information is being logged in the `changed_state` topic.

* The internal management of entities and components in the
`EntityComponentManager` has been updated to improve runtime performance. As a
result, several methods have been deprecated, and a few types have changed.
* **Deprecated**:
+ All `EntityComponentManager` methods that use `ComponentKey` as an input
parameter.
+ The `EntityComponentManager::First` method.
+ The `ComponentId` and `ComponentKey` types are now deprecated. A
combination of `Entity` and `ComponentTypeId` should be used instead.
+ The `components::StorageDescriptorBase` and
`components::StorageDescriptor<ComponentTypeT>` classes.
+ Methods in `components::Factory` that have deprecated input parameter
types and/or deprecated return types.
- The version of `components::Factory::Register` which has a
`StorageDescriptorBase *` input parameter.
- `components::Factory::NewStorage`
+ The `ComponentStorageBase` and `ComponentStorage<ComponentTypeT>` classes.
* **Modified**:
+ `EntityComponentManager::CreateComponent` now returns a pointer to the
created component instead of a `ComponentKey`.
+ `ComponentKey` has been modified to be a
`std::pair<ComponentTypeId, Entity>` (it used to be a
`std::pair<ComponentTypeId, ComponentId>`) since the `ComponentId` type
is now deprecated. `ComponentKey` has also been deprecated, so usage of
this type is discouraged (see the **Deprecated** section above for more
information about how to replace usage of `ComponentKey`).

## Ignition Gazebo 4.x to 5.x

* Use `cli` component of `ignition-utils1`.
Expand Down
126 changes: 34 additions & 92 deletions include/ignition/gazebo/EntityComponentManager.hh
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ namespace ignition
/// \param[in] _entity The entity to check.
/// \param[in] _key The component to check.
/// \return True if the component key belongs to the entity.
public: bool EntityHasComponent(const Entity _entity,
public: bool IGN_DEPRECATED(6) EntityHasComponent(const Entity _entity,
const ComponentKey &_key) const;

/// \brief Check whether an entity has a specific component type.
Expand All @@ -152,7 +152,7 @@ namespace ignition
/// \param[in] _key A key that uniquely identifies a component.
/// \return True if the entity and component existed and the component was
/// removed.
public: bool RemoveComponent(
public: bool IGN_DEPRECATED(6) RemoveComponent(
const Entity _entity, const ComponentKey &_key);

/// \brief Remove a component from an entity based on a type id.
Expand Down Expand Up @@ -180,9 +180,12 @@ namespace ignition
/// \param[in] _entity The entity that will be associated with
/// the component.
/// \param[in] _data Data used to construct the component.
/// \return Key that uniquely identifies the component.
/// \return A pointer to the component that was created. nullptr is
/// returned if the component was not able to be created. If _entity
/// does not exist, nullptr will be returned.
public: template<typename ComponentTypeT>
ComponentKey CreateComponent(const Entity _entity,
ComponentTypeT *CreateComponent(
const Entity _entity,
const ComponentTypeT &_data);

/// \brief Get a component assigned to an entity based on a
Expand All @@ -206,14 +209,16 @@ namespace ignition
/// \return The component associated with the key, or nullptr if the
/// component could not be found.
public: template<typename ComponentTypeT>
const ComponentTypeT *Component(const ComponentKey &_key) const;
const ComponentTypeT IGN_DEPRECATED(6) * Component(
const ComponentKey &_key) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows is not happy about something here:

C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-gazebo\include\ignition/gazebo/EntityComponentManager.hh(212,37): error C2059: syntax error: '' (compiling source file C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-gazebo\src\EntityComponentManager.cc) [C:\Jenkins\workspace\ign_gazebo-pr-win\ws\build\ignition-gazebo6\src\ignition-gazebo6.vcxproj]

Maybe there's some weird character hiding in there? 🤷‍♀️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it has something to do with using IGN_DEPRECATED in combination with methods that return a pointer. I am guessing that the * in the method signature is not being understood correctly when parsing occurs on windows. I removed all of the IGN_DEPRECATED macros for methods that return pointers in 576efc7 (but left the IGN_DEPRECATED macros for methods that do not return pointers), and these build errors no longer appear. However, I'm now seeing errors with the function definitions for the templated View class (https://build.osrfoundation.org/job/ign_gazebo-pr-win/2858/consoleFull#-11549300628ff58640-3599-4406-a210-216932f1748c):

C:\Jenkins\workspace\ign_gazebo-pr-win\ws\ign-gazebo\include\ignition/gazebo/detail/View.hh(150,27): error C2491: 'ignition::gazebo::v6::detail::View<ComponentTypeTs...>::View': definition of dllimport function not allowed (compiling source file C:\Jenkins\workspace\ign_gazebo-pr-win\ws\build\ignition-gazebo6\src\gui\ignition-gazebo6-gui_autogen\mocs_compilation.cpp) [C:\Jenkins\workspace\ign_gazebo-pr-win\ws\build\ignition-gazebo6\src\gui\ignition-gazebo6-gui.vcxproj]

@j-rivero do you have any thoughts about what's going on with the IGN_DEPRECATED macro, and also how to resolve the definition of dllimport function not allowed error above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so it sounds like the IGN_DEPRECATED issue has been resolved in #856 (comment). However, I still believe that the definition of dllimport function not allowed error remains. Does anyone have any thoughts about how to handle this other build error?


/// \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<typename ComponentTypeT>
ComponentTypeT *Component(const ComponentKey &_key);
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
Expand All @@ -222,7 +227,7 @@ namespace ignition
/// \param[in] _default The value that should be used to construct
/// the component in case the component doesn't exist.
/// \return The component of the specified type assigned to the specified
/// entity.
/// entity. If _entity does not exist, nullptr is returned.
public: template<typename ComponentTypeT>
ComponentTypeT *ComponentDefault(Entity _entity,
const typename ComponentTypeT::Type &_default =
Expand Down Expand Up @@ -261,20 +266,20 @@ namespace ignition
Entity _entity) const;

/// \brief The first component instance of the specified type.
/// \return First component instance of the specified type, or nullptr
/// if the type does not exist.
/// This function is now deprecated, and will always return nullptr.
/// \return nullptr.
public: template<typename ComponentTypeT>
const ComponentTypeT *First() const;
const ComponentTypeT IGN_DEPRECATED(6) * First() const;

/// \brief The first component instance of the specified type.
/// \return First component instance of the specified type, or nullptr
/// if the type does not exist.
/// This function is now deprecated, and will always return nullptr.
/// \return nullptr.
public: template<typename ComponentTypeT>
ComponentTypeT *First();
ComponentTypeT IGN_DEPRECATED(6) * First();

/// \brief Get an entity which matches the value of all the given
/// components. For example, the following will return the entity which
/// has an name component equal to "name" and has a model component:
/// has a name component equal to "name" and has a model component:
///
/// auto entity = EntityByComponents(components::Name("name"),
/// components::Model());
Expand Down Expand Up @@ -523,6 +528,7 @@ namespace ignition
/// and components.
/// \detail The header of the message will not be populated, it is the
/// responsibility of the caller to timestamp it before use.
/// \param[out] _state The serialized state message to populate.
/// \param[in] _entities Entities to be serialized. Leave empty to get
/// all entities.
/// \param[in] _types Type ID of components to be serialized. Leave empty
Expand Down Expand Up @@ -611,24 +617,14 @@ namespace ignition
/// \return True if the Entity has been marked to be removed.
private: bool IsMarkedForRemoval(const Entity _entity) const;

/// \brief Delete an existing Entity.
/// \param[in] _entity The entity to remove.
/// \returns True if the Entity existed and was deleted.
private: bool RemoveEntity(const Entity _entity);

/// \brief The first component instance of the specified type.
/// \return First component instance of the specified type, or nullptr
/// if the type does not exist.
private: components::BaseComponent *First(
const ComponentTypeId _componentTypeId);

/// \brief Implementation of CreateComponent.
/// \param[in] _entity The entity that will be associated with
/// the component.
/// \param[in] _componentTypeId Id of the component type.
/// \param[in] _data Data used to construct the component.
/// \return Key that uniquely identifies the component.
private: ComponentKey CreateComponentImplementation(
/// \return True if the component's data needs to be set externally; false
/// otherwise.
private: bool CreateComponentImplementation(
const Entity _entity,
const ComponentTypeId _componentTypeId,
const components::BaseComponent *_data);
Expand All @@ -651,79 +647,29 @@ namespace ignition
const Entity _entity,
const ComponentTypeId _type);

/// \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.
private: const components::BaseComponent *ComponentImplementation(
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.
private: components::BaseComponent *ComponentImplementation(
const ComponentKey &_key);

/// \brief End of the AddComponentToView recursion. This function is
/// called when Rest is empty.
/// \param[in, out] _view The FirstComponent will be added to the
/// _view.
/// \param[in] _entity The entity.
private: template<typename FirstComponent,
typename ...RemainingComponents,
typename std::enable_if<
sizeof...(RemainingComponents) == 0, int>::type = 0>
void AddComponentsToView(detail::View &_view,
const Entity _entity) const;

/// \brief Recursively add components to a view. This function is
/// called when Rest is NOT empty.
/// \param[in, out] _view The FirstComponent will be added to the
/// _view.
/// \param[in] _entity The entity.
private: template<typename FirstComponent,
typename ...RemainingComponents,
typename std::enable_if<
sizeof...(RemainingComponents) != 0, int>::type = 0>
void AddComponentsToView(detail::View &_view,
const Entity _entity) const;

/// \brief Find a View that matches the set of ComponentTypeIds. If
/// a match is not found, then a new view is created.
/// \tparam ComponentTypeTs All the component types that define a view.
/// \return A reference to the view.
/// \return A pointer to the view.
private: template<typename ...ComponentTypeTs>
detail::View &FindView() const;
detail::View<ComponentTypeTs...> *FindView() const;

/// \brief Find a view based on the provided component type ids.
/// \param[in] _types The component type ids that serve as a key into
/// a map of views.
/// \param[out] _iter Iterator to the found element in the view map.
/// Check the return value to see if this iterator is valid.
/// \return True if the view was found, false otherwise.
private: bool FindView(const std::set<ComponentTypeId> &_types,
std::map<detail::ComponentTypeKey,
detail::View>::iterator &_iter) const; // NOLINT
/// \return A pointer to the view. nullptr is returned if the view wasn't
/// found.
private: detail::BaseView *FindView(
const std::vector<ComponentTypeId> &_types) const;

/// \brief Add a new view to the set of stored views.
/// \param[in] _types The set of component type ids that is the key
/// \param[in] _types The set of component type ids that act as the key
/// for the view.
/// \param[in] _view The view to add.
/// \return An iterator to the view.
private: std::map<detail::ComponentTypeKey, detail::View>::iterator
AddView(const std::set<ComponentTypeId> &_types,
detail::View &&_view) const;

/// \brief Update views that contain the provided entity.
/// \param[in] _entity The entity.
private: void UpdateViews(const Entity _entity);

/// \brief Get a component ID based on an entity and the component's type.
/// \param[in] _entity The entity.
/// \param[in] _type Component type ID.
private: ComponentId EntityComponentIdFromType(
const Entity _entity, const ComponentTypeId _type) const;
/// \return A pointer to the view.
private: detail::BaseView *AddView(
const detail::ComponentTypeKey &_types,
std::unique_ptr<detail::BaseView> _view) const;

/// \brief Add an entity and its components to a serialized state message.
/// \param[out] _msg The state message.
Expand Down Expand Up @@ -761,10 +707,6 @@ namespace ignition
// states. Like the runners, the managers are internal.
friend class NetworkManagerPrimary;
friend class NetworkManagerSecondary;

// Make View a friend so that it can access components.
// This should be safe since View is internal to Gazebo.
friend class detail::View;
};
}
}
Expand Down
9 changes: 8 additions & 1 deletion include/ignition/gazebo/Types.hh
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include <functional>
#include <utility>

#include "ignition/gazebo/Entity.hh"

namespace ignition
{
namespace gazebo
Expand Down Expand Up @@ -78,6 +80,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.
/// \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
Expand All @@ -87,7 +91,10 @@ namespace ignition

/// \brief A key that uniquely identifies, at the global scope, a component
/// instance
using ComponentKey = std::pair<ComponentTypeId, ComponentId>;
/// \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<ComponentTypeId, Entity>;

/// \brief typedef for query callbacks
using EntityQueryCallback = std::function<void (const UpdateInfo,
Expand Down
Loading