Document Entity/Component Usage for Fortress and beyond

[adlarkin]

As mentioned in #628, documentation for how to use the EntityComponentManager (and consequentially, how to use Entities/Components) is lacking. While #628 proposes a lot of good things to document, it's likely that some of the documentation will change starting in Fortress once #711 is addressed.

At the time of this writing, the following items should be addressed (as more items come to mind, we can either add to this comment or create additional comments in this issue):

• [ ] Deprecate Component::Data(). This may also require deprecation of Component::operator= so that users don't use this instead
• [ ] Extend classes like Model so that more common/beginner-friendly use cases don't require dealing with the ECM
• [ ] Stick to either adding/removing components that aren't needed each iteration, or to "zeroing them out", but not both (perhaps we can make better use of EntityComponentManager::ComponentState)

[diegoferigo]

Deprecate Component::Data(). This may also require deprecation of Component::operator= so that users don't use this instead

I'm a bit worried by this change. If a component stores a big chunk of data (say, an image), being able to directly access the component's storage in r/w (i.e. for setting the value) could be relevant for performance. Is there any other way to prevent allocating a temporary object like what could happen when using Component::SetData?

The use case I have in mind is when the data type of the component differs from the data type of the caller. It's kind of related to https://github.com/ignitionrobotics/ign-gazebo/issues/494.

[chapulina]

Good point, there may be use cases when the caller wants to modify the data without necessarily creating a new object. Like how you could now do:

poseComp->Data().SetX(10);

We haven't been doing that a lot internally, but it is a good pattern for performance.

For context, the reason we'd like to force users to go through SetData is to make it easier to set the changed state. But maybe we can recommend SetData for intermediate users, while advanced users can use Data() and be sure to use SetChangedState correctly afterwards.

[diegoferigo]

For context, the reason we'd like to force users to go through SetData is to make it easier to set the changed state. But maybe we can recommend SetData for intermediate users, while advanced users can use Data() and be sure to use SetChangedState correctly afterwards.

Thanks for providing this piece of information, now it makes even more sense. I like a lot the idea of automatically updating the state of the component when calling Component::SetData, especially after the last changes that optimized performance. I recently faced some problem due to that, and I found out that manually calling EntityComponentManager::SetChanged was necessary. If this approach goes through, keeping this advanced usage available would be great.

[chapulina]

Deprecate Component::Data().

Given the discussion above, I suggest we don't deprecate that, and instead document that SetData is recommended, but if one must use Data, they should remember to also call SetChanged.

Extend classes like Model

This is captured in https://github.com/ignitionrobotics/ign-gazebo/issues/325

Stick to either adding/removing components that aren't needed each iteration, or to "zeroing them out", but not both

With the new view implementation in #856, removing components is not very costly. So we could use that. I'm reluctant to updating the components that are currently zeroed out though, because that will force downstream users to update their code when migrating.

[adlarkin]

Given the discussion above, I suggest we don't deprecate that, and instead document that SetData is recommended, but if one must use Data, they should remember to also call SetChanged.

Sounds good to me :+1: are we currently expecting for users to call SetChanged after calling Data, or will this be a new expectation starting in Fortress? I believe there are places scattered across the codebase that call Data without calling SetChanged. I'm assuming this change would take place in Fortress since one could argue that it's a behavioral change.

With the new view implementation in #856, removing components is not very costly. So we could use that. I'm reluctant to updating the components that are currently zeroed out though, because that will force downstream users to update their code when migrating.

Agreed :+1: once we merge #856, we can tell users that removing/re-adding components is okay.

[chapulina]

Tasks from #787:

• [ ] Documenting on the Physics system that it won't function correctly unless entities are created in the proper order. That is, parents must be created before their children.
• [ ] Write a tutorial with examples of how to create entities using the create service (recommended way) or the SdfEntityCreator / EntityComponentManager APIs (advanced usage, and users should be careful about the order in which entities are created).