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

Add notification bus to spawner #800

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

arturkamieniecki
Copy link
Contributor

What does this PR do?

This PR adds a bus to the spawner component that notifies other components about spawns.

How was this PR tested?

By spawning and despawning entities and listening on the bus with a custom component.

@arturkamieniecki arturkamieniecki requested review from a team as code owners November 29, 2024 12:08
@arturkamieniecki arturkamieniecki marked this pull request as draft November 29, 2024 12:12
@byrcolin byrcolin added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Dec 3, 2024
@jhanca-robotecai
Copy link
Contributor

Is this PR ready for the review?

@jhanca-robotecai jhanca-robotecai added sig/simulation Categorizes an issue or PR as relevant to SIG Simulation and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 19, 2024
@arturkamieniecki arturkamieniecki marked this pull request as ready for review December 19, 2024 13:41
@arturkamieniecki
Copy link
Contributor Author

Is this PR ready for the review?

Yes. This code was created to allow notifications about spawning to be sent to other components. Currently, no ROS2Gem components use this feature.

@arturkamieniecki arturkamieniecki changed the title Add notification bus to spawner [draft] Add notification bus to spawner Dec 20, 2024
@arturkamieniecki arturkamieniecki marked this pull request as draft December 20, 2024 13:12
@arturkamieniecki
Copy link
Contributor Author

arturkamieniecki commented Dec 20, 2024

Drafted as new functionality is being added.

@arturkamieniecki arturkamieniecki force-pushed the ak/SpawnerNotificationBus branch from d7359a9 to 7d44c30 Compare December 20, 2024 13:37
@arturkamieniecki arturkamieniecki marked this pull request as ready for review December 20, 2024 13:50
@arturkamieniecki arturkamieniecki changed the title [draft] Add notification bus to spawner Add notification bus to spawner Dec 20, 2024
@arturkamieniecki
Copy link
Contributor Author

I've added the notification handlers to the behavior context.

Copy link
Contributor

@jhanca-robotecai jhanca-robotecai left a comment

Choose a reason for hiding this comment

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

Tested with ScriptCanvas and the notifications work correctly (both spawning and despawning).

A design question: what is the reasoning behind the passing three data fields (spawnableName, rootEntityId, ticketName) when spawning and only one (ticketName) when despawning? It looks somewhat inconsistent.

@arturkamieniecki
Copy link
Contributor Author

A design question: what is the reasoning behind the passing three data fields (spawnableName, rootEntityId, ticketName) when spawning and only one (ticketName) when despawning? It looks somewhat inconsistent.

I wanted to send as much identifying information as possible. The spawnable name identifies the object type, entity id its place in O3DE, and the ticket name is the unique key that identifies it. There is no need to resend the spawnable name and entity id for the despawn notification as the ticket name uniquely identifies it.

Signed-off-by: Artur Kamieniecki <[email protected]>
Signed-off-by: Artur Kamieniecki <[email protected]>
@jhanca-robotecai jhanca-robotecai force-pushed the ak/SpawnerNotificationBus branch from c406cec to ccdaa99 Compare January 10, 2025 13:28
@jhanca-robotecai
Copy link
Contributor

Rebased and force-pushed on top of the development due to failing tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/simulation Categorizes an issue or PR as relevant to SIG Simulation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants