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

Csv Spawner Notification Bus #109

Open
wants to merge 12 commits into
base: wc/spline_publisher
Choose a base branch
from

Conversation

w-czerski
Copy link
Contributor

@w-czerski w-czerski commented Jan 28, 2025

About:

This PR introduces a notification bus for the CSV spawner.
It provides the ability to trigger actions when entities are spawned and to gather information when the spawning process begins.
This is particularly useful when there is a need to react to the moment "when entities are spawned".

Merge order:

This branch is a copy of wc/spline_publisher. I've tested it already with new changes.
#109 -> #108 -> main

Signed-off-by: Wojciech Czerski <[email protected]>
Copy link
Contributor

@arturkamieniecki arturkamieniecki left a comment

Choose a reason for hiding this comment

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

There is a lot of info passed in the bus calls. Verify if everything is needed. Consider creating a struct for this data.

Overall looks good!

@w-czerski
Copy link
Contributor Author

Sure, it’s just a quick fix that I needed. A lot of the information is indeed unnecessary. I’ll refactor it soon.

On further thought, it might be better to introduce some global robotec-o3de-tools notification buses to reduce dependencies? This way, we would only need to include the notification bus in CMake once, instead of creating dependencies for individual tools like spline tools, geo json, csv spawner, etc.

Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Copy link
Contributor

@patrykantosz patrykantosz left a comment

Choose a reason for hiding this comment

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

Code looks okay. I'm just curious if anything (beyond this PR, as it is obvious the implementation is not here) implements handlers for these two newly added functions? Or has the bus been created for some further work planned for the future?

@w-czerski
Copy link
Contributor Author

In current state, we need some explicit logic to handle is terrain created (please look at #102).
This is just a helpful functionality to prevent users to stacks tick frames, or queue functions. User can directly do something on spawn ready, and skip in most cases terrain handling (since this is not useful or needed for user).

Exactly this functionality is needed in one project I'm working on. I would reconsider what information should be passed down in the buses. Right now notification is only needed.

Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
Signed-off-by: Wojciech Czerski <[email protected]>
@w-czerski
Copy link
Contributor Author

w-czerski commented Jan 29, 2025

I've added:

  • more context with descriptions
  • default implementation of events (do nothing - we don't want to force override of each not implemented function)
  • introduced struct with info
  • added a enum result status code for the spawn finished process
    Let me know what you think. I've made it more generic for the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason for putting utilities in the public API?

Comment on lines +29 to +32
Success = 0, ///< Operation succeeded.
Fail = 1 << 0, ///< Generic failure.
SpawnStopped = 1 << 1, ///< Spawning was stopped prematurely but not necessarily a failure.
ErrorOccurred = 1 << 2, ///< An error occurred during spawning (potentially recoverable).
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the bitshifts needed here? Typically, the bitshifts are used when multiple flags can happen at once.

* @param m_spawnInfo Struct holding information about entities to be spawned.
* @param m_statusCode Status code indicating success, failure and warnings of the spawn.
*/
virtual void OnEntitiesSpawnFinished(const SpawnInfo& m_spawnInfo, const SpawnStatusCode& m_statusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

const has no use in definitions, hence it should be used only to increase the readability of the code. m_statusCode indicates success, failure and warnings, but the const indicator would suggest the value will not be changed by the method, hence the indication will not take any place.

* CsvSpawnerInterface is an Event Bus interface that notifies multiple
* listeners when entity spawning begins and finishes.
*/
class CsvSpawnerInterface : public AZ::EBusTraits
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the interface method stay purely virtual?

@@ -196,6 +196,13 @@ namespace CsvSpawner::CsvSpawnerUtils
const AZStd::string& physicsSceneName,
AZ::EntityId parentId)
{
SpawnInfo broadcastSpawnInfo =
SpawnInfo{ entitiesToSpawn, physicsSceneName, parentId }; // Spawn Info used in CsvSpawner EBus notify.
SpawnStatusCode spawnStatusCode; // Spawn Status Code Status used for CsvSpawner EBus notify - OnEntitiesSpawnFinished.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please implicitly set such flags to success (or failure) during initialization to avoid problems in the future.

* SpawnStatusCode provides various status indicators for entity spawning.
* These flags help track whether spawning was successful, stopped, or failed.
*/
enum class SpawnStatusCode : uint8_t
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for Code postfix.

SpawnStatus is enough.

Comment on lines +31 to +32
SpawnStopped = 1 << 1, ///< Spawning was stopped prematurely but not necessarily a failure.
ErrorOccurred = 1 << 2, ///< An error occurred during spawning (potentially recoverable).
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep it simpler:

Suggested change
SpawnStopped = 1 << 1, ///< Spawning was stopped prematurely but not necessarily a failure.
ErrorOccurred = 1 << 2, ///< An error occurred during spawning (potentially recoverable).
Stopped = 1 << 1, ///< Spawning was stopped prematurely but not necessarily a failure.
Error = 1 << 2, ///< An error occurred during spawning (potentially recoverable).

Comment on lines +46 to +48
const AZStd::vector<CsvSpawnerUtils::CsvSpawnableEntityInfo>& m_entitiesToSpawn; ///< List of entities to spawn.
const AZStd::string& m_physicsSceneName; ///< Name of the physics scene where entities will be spawned.
const AZ::EntityId& m_spawnerParentEntityId; ///< Parent entity ID managing the spawn process.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why const references in this struct? Huge dangling reference risk here (I can imagine someone waits a frame or two after acquiring the status - the referenced objects might be gone then).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants