Recommend usage of SdfEntityCreator in Fortress

[adlarkin]

Signed-off-by: Ashton Larkin [email protected]

Summary

As discussed offline, the usage of the SdfEntityCreator should be required strongly encouraged beginning in Fortress in order to properly handle new features related to nested models.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

[codecov]

Codecov Report

Merging #787 (3a66849) into main (f4a3ea4) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #787      +/-   ##
==========================================
- Coverage   65.34%   65.31%   -0.03%     
==========================================
  Files         240      240              
  Lines       17602    17602              
==========================================
- Hits        11502    11497       -5     
- Misses       6100     6105       +5     

Impacted Files	Coverage Δ
src/SimulationRunner.cc	92.73% <0.00%> (-1.04%)	⬇️
src/Server.cc	83.43% <0.00%> (+0.63%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4a3ea4...3a66849. Read the comment docs.

[chapulina]

I think it's tough to make this statement without providing users the full context. Someone may come and ask what is the EntityComponentManager::CreateEntity function for then; should it be made private and a friend of the SdfEntityCreator so no system can use it, but the SdfEntityCreator can?

I think the message we'd like to convey is that there's functionality in the Physics plugin that relies on entities being created by the SdfEntityCreator, specifically because of their order of creation, and that the plugin may misbehave for entities created directly through the ECM's API.

We may even want to document that on the Physics system's header.

[adlarkin]

Someone may come and ask what is the EntityComponentManager::CreateEntity function for then; should it be made private and a friend of the SdfEntityCreator so no system can use it, but the SdfEntityCreator can?

As I look closer at the SdfEntityCreator API, I think that we would need to modify/expand its API a bit if we want to push users towards using it. There appear to be CreateEntities methods for various sdf tags (sensor, model, etc.), but I also think that we should have a method that creates an "empty" entity: something like this:

/// \brief Create an entity
/// \brief _parent The entity's parent
/// \return The newly created entity
Entity CreateEntity(const Entity _parent = kNullEntity);

That way, for things like system plugins, users can create additional entities as needed (I had to do something similar to this in the logical audio sensor plugin - I'm not sure if what I did in this plugin is actually "best practices" or not). In terms of how this method is implemented, perhaps we can do something like have it call EntityComponentManager::CreateEntity if it's a friend of the SdfEntityCreator. This method would also take care of setting the entity's parent, so users can use it like this:

// this is the way users may currently be creating entities
auto entity = ecm.CreateEntity();
sdfEntityCreator.SetParent(entity, parent);

// users should now do this instead
auto entity = sdfEntityCreator.CreateEntity(parent);

// they can also do something like this if the entity has no parent
auto entity = sdfEntityCreator.CreateEntity();

Thoughts?

[chapulina]

we should have a method that creates an "empty" entity

As the name says, the SdfEntityCreator is concerned with creating entities from SDF structures 😉 Empty entities should be created directly with the ECM. It's important to keep the separation of concerns among the classes, I'm not sure what's would be the advantage of creating empty entities from the SdfEntityCreator.

[adlarkin]

All good points, and you're right (again 🙂). It seems to me like for advanced use cases, users may need to create entities and components with the ECM (i.e., they can't just rely on the SdfEntityCreator).

So, with that being said, I don't think that we can require the SdfEntityCreator to be used since some scenarios require making entities that don't come from SDF. I think that the best thing to do is to encourage the usage of SdfEntityCreator whenever possible, including how to use the SdfEntityCreator (this is related to #628). As you mentioned in your original review comment, we can also provide more context to why using the SdfEntityCreator is encouraged (for example, the physics system).

Does simply re-wording the migration guide to discuss these items sound good enough to you? Or do we need to add more documentation elsewhere (and potentially modify/document source code) as well?

[adlarkin]

Also, I have updated the PR title and description to indicate that SdfEntityCreator usage is recommended, but not required.

[chapulina]

Empty entities should be created directly with the ECM.

As discussed during the meeting, we could also provide APIs to create entities from the Model, Link and World classes. We should be trying to add more functions to those classes so that beginners can use them, while advanced users can use the ECM for custom entities.

Does simply re-wording the migration guide to discuss these items sound good enough to you? Or do we need to add more documentation elsewhere (and potentially modify/document source code) as well?

I think that this, combined with the documentation that will come out of #628, should provide lots of pointers to users.

[adlarkin]

It seems to me like updating the migration guide here and merging this PR may be difficult to do before addressing #628 and #805, since SdfEntityCreator usage depends a lot on documentation pointed out by these issues, which is missing/lacking at the time of this writing. I think we should mark this PR as a draft, address these other issues first, and then update the migration guide accordingly based on the documentation created by these other issues. Does that sound good to you?

[chapulina]

Yup, SGTM 👍

[adlarkin]

I'm marking this as a draft for now (and moving this from in review to to do for the Ignition core development board) based on this conversation. I'm planning to re-visit this once #628 and #805 are addressed.

[chapulina]

I talked to @adlarkin offline, and we decided to close this PR in favor of:

• 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).