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.

Sign up for GitHub

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

Recommend usage of SdfEntityCreator in Fortress #787

Closed
wants to merge 1 commit into from
Closed

Recommend usage of SdfEntityCreator in Fortress #787

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 6 additions & 0 deletions Migration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ Deprecated code produces compile-time warnings. These warning serve as
notification to users that their code should be upgraded. The next major
release will remove the deprecated code.

## Ignition Gazebo 5.x to 6.x

* Entity creation is now required to be done through the
`ignition::gazebo::SdfEntityCreator`. In earlier versions of Ignition Gazebo,
usage of this class for entity creation is optional, but strongly encouraged.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@adlarkin adlarkin May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@adlarkin adlarkin May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, SGTM 👍


## Ignition Gazebo 4.x to 5.x

* Use `cli` component of `ignition-utils1`.
Expand Down