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

Consolidate entity creation. #2452

Merged
merged 5 commits into from
Jul 3, 2024
Merged

Conversation

nkoenig
Copy link
Contributor

@nkoenig nkoenig commented Jun 24, 2024

🎉 New feature

Summary

I'm slowly splitting up #1669. This PR breaks out the consolidation of entity creation into a separate PR. The LevelManager had entity creation logic, which mimicked (mostly) the functionality in SdfEntityCreator.

The SdfEntityCreator now contains the logic to create entities, and the level manager's logic has been removed. The SimulationRunner loads the level manager, plugins, and calls SdfEntityCreator::CreateEntities.

Notes

  1. Requires a new release of SDF13 (Added World::ActorByName: sdformat#1436).
  2. The LogicalAudioSensorPlugin and PosePublisher systems used topicFromScopedName with false for the _excludeWorld parameter. Prior to this PR, this result was a model scoped name (/mode/blah). This was because the model wasn't attached to the world when the plugin's Configure function was called. With this PR, the model is now properly attached. In order to maintain the expected behavior, I've changed the false to true when using the topicFromScopedName function.
  3. The Wind plugin was loaded by the LevelManager. I've propagated this to SdfEntityCreator. I don't think this should effect simulation behavior.

Test it

The tests should pass.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

Signed-off-by: Nate Koenig <[email protected]>
@nkoenig nkoenig requested a review from mjcarroll as a code owner June 24, 2024 13:09
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jun 24, 2024
Signed-off-by: Nate Koenig <[email protected]>
@ahcorde ahcorde added the needs upstream release Blocked by a release of an upstream library label Jun 24, 2024
@nkoenig nkoenig changed the title Resolve failing tests Consolidate entity creation. Jun 25, 2024
@nkoenig
Copy link
Contributor Author

nkoenig commented Jun 26, 2024

@osrf-jenkins run tests

@azeey azeey self-requested a review June 26, 2024 19:07
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

I see this error msg when running the levels.sdf world but performers + levels do seem to work though:

[Err] [LevelManager.cc:110] Could not find a plugin tag with name gz::sim. Levels and distributed simulation will not work.

src/EntityComponentManager.cc Outdated Show resolved Hide resolved
nkoenig and others added 2 commits June 28, 2024 05:18
Co-authored-by: Ian Chen <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
Signed-off-by: Nate Koenig <[email protected]>
@nkoenig
Copy link
Contributor Author

nkoenig commented Jun 28, 2024

I see this error msg when running the levels.sdf world but performers + levels do seem to work though:

[Err] [LevelManager.cc:110] Could not find a plugin tag with name gz::sim. Levels and distributed simulation will not work.

Thanks. It looks like I had a incorrect else. Should be fixed in 4c1a186

@nkoenig nkoenig merged commit 517a73a into gz-sim7 Jul 3, 2024
9 checks passed
@nkoenig nkoenig deleted the nkoenig/consolidate-create-entities branch July 3, 2024 12:17
@scpeters
Copy link
Member

I just tried merging this forward to gz-sim8, but there are conflicts, and I don't have enough context on these changes to resolve them myself. Help is needed!

@azeey azeey mentioned this pull request Aug 12, 2024
@azeey
Copy link
Contributor

azeey commented Aug 13, 2024

@nkoenig The change in plugin loading behavior would be considered a breaking change evidenced by the way topicFromScopedName had to be called with different arguments. Is it possible to undo that change here and add it to main?

@azeey azeey mentioned this pull request Aug 14, 2024
@azeey
Copy link
Contributor

azeey commented Aug 14, 2024

Related issue: #1786

@iche033
Copy link
Contributor

iche033 commented Aug 16, 2024

looks like we made a gz-sim7 release with this change on 2024-07-22 (changelog entry). If we undo the change in behavior for plugin loading, we should do another release to keep the no. of affected users small.

azeey added a commit to azeey/gz-sim that referenced this pull request Aug 16, 2024
PR gazebosim#2452 changed the loading order of plugins attached to worlds and models. While the change is desirable, it changed the behavior of functions like `topicFromScopedName`. This reverts the behavior and the changes in associated tests.

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey
Copy link
Contributor

azeey commented Aug 16, 2024

#2527

azeey added a commit that referenced this pull request Aug 20, 2024
Restores the behavior where the `Configure` function of model plugins are called before the model's parent entity is set.
---------

Signed-off-by: Addisu Z. Taddese <[email protected]>
@azeey azeey mentioned this pull request Sep 10, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden needs upstream release Blocked by a release of an upstream library
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants