[2409] Csv Spawner Notification Bus #109

w-czerski · 2025-01-28T15:59:38Z

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".

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

arturkamieniecki

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 · 2025-01-28T16:26:13Z

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.

patrykantosz

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 · 2025-01-29T08:48:16Z

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.

w-czerski · 2025-01-29T12:56:47Z

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.

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerUtils.h

jhanca-robotecai · 2025-02-11T17:29:09Z

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h

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


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

Overall, I thought - spawn can still be successful even when some errors / issues occur. Like partialy some entites are spawned, some are spawned but not in desired place etc.

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h

jhanca-robotecai · 2025-02-14T17:24:32Z

Rethining the problem... Could we include a behavioral context for LUA/ScriptCanvas in this PR? I can imagine a script reacting to spawned objects.

w-czerski · 2025-02-17T08:37:56Z

Sure. I'll add also similar notify bus for Geo Json Spawner.

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

norbertprokopiuk

I checked the code and I found some key issues that need your attention:

Unused Variable: The presence of an unused lambda capture relating to tickets is causing compilation issues. This needs to be either utilized or removed to get the code to compile.
Dangling References: There are dangling references, which can lead to unexpected behavior.
Logical Issues: The conditional logic involving totalTicket and completedTickets isn't set up correctly. This results in conditions that can never be met due to how the variables are managed.
Missing Notifications: It's important to include notification calls to alert users about any failures.

I will be happy to check refined version

norbertprokopiuk · 2025-07-02T12:35:21Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

            };
            optionalArgs.m_completionCallback =
-                [parentId, &spawnStatusCode](
+                [parentId, &spawnStatusCode, &broadcastSpawnInfo, &tickets, totalTickets, &completedTickets](


It appears that the four references in question are currently dangling. This arises because local variables are being passed to the completion callback, which is executed after the SpawnEntities function has ended. Once the function finishes, the references to the callback become invalid since the variables they point to no longer exist. It is important to address this issue to ensure the code functions as intended.

norbertprokopiuk · 2025-07-02T12:37:15Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

            };
            optionalArgs.m_completionCallback =
-                [parentId, &spawnStatusCode](
+                [parentId, &spawnStatusCode, &broadcastSpawnInfo, &tickets, totalTickets, &completedTickets](


The reference to tickets is not only dangling but also remains unused, which consequently interferes with building O3DE in the profile configuration.

norbertprokopiuk · 2025-07-02T12:57:38Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

+                completedTickets++;
+                if (completedTickets == totalTickets)
+                {
+                    // Call CsvSpawner EBus notification - Finished
+                    CsvSpawnerNotificationBus::Broadcast(&CsvSpawnerInterface::OnEntitiesSpawnFinished, broadcastSpawnInfo, spawnStatusCode);
+                }


Even if we assume that completedTickets is not a dangling reference, the current condition will not function as intended. The issue arises because totalTicket is passed as a copy and incremented after the callback is defined. Consequently, completedTickets will always be one greater than totalTickets, resulting in the condition never being met. If you move the totalTickets++ before defining the callback, it would trigger with every callback. Therefore, it is advisable to explore a different approach to address this condition.

norbertprokopiuk · 2025-07-02T13:08:36Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

+                [parentId, &spawnStatusCode, &broadcastSpawnInfo, &tickets, totalTickets, &completedTickets](
                    [[maybe_unused]] AzFramework::EntitySpawnTicket::Id ticketId, AzFramework::SpawnableConstEntityContainerView view)
            {
                if (view.empty())


The code within this if statement exhibits undefined behavior because spawnStatusCode is a dangling reference. This may be the reason why you have not encountered any issues, as the code possibly has not executed as expected. Additionally, the code is missing a notification call to indicate a failure. A similar situation is present in the preinsertionCallback.

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

…lamdas Signed-off-by: Wojciech Czerski <[email protected]>

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

norbertprokopiuk

I checked your fix and undefined behaviour are removed, which is nice. Good Job. Although there are some issues still present.

there is invalid logic with decrementing of pendingSpawns
I think that notifcation bus definition is a bit wrong. We can add multiple CSVComponents on scene, so we should either send one notification when all components finished their jobs or each of them should send individual notification. Currently all notifications are broadcasted what is not optimal solution

Could you also explain why you decide to solve sending notification in the current way? I was thinking if wouldn't it be cleaner to define internal event, only in this Gem, which would be trigger everytime single completion callback is executed. It would take status, and cmopleted ticketId and would trigger the event on entityId of the componentOwner. Each component already has map of spawnedTickets. you would need to add unordered_set, which will gather all completedTickets inside mentioned event callback. If all tickets are completed, then you can send main notification that spawning is finished. I think it could potentially limit amount of data passed between threads and it might be a bit cleaner. But I don't want to force you to change the implementation right now. I would like to know your point of view on this problem.

norbertprokopiuk · 2025-07-03T05:41:07Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp


 #include "CsvSpawnerUtils.h"

+#include "AzCore/std/smart_ptr/make_shared.h"


Suggested change

#include "AzCore/std/smart_ptr/make_shared.h"

#include <AzCore/std/smart_ptr/make_shared.h>

norbertprokopiuk · 2025-07-03T05:44:07Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

        SpawnInfo broadcastSpawnInfo =
            SpawnInfo{ entitiesToSpawn, physicsSceneName, parentId }; // Spawn Info used in CsvSpawner EBus notify.
-        SpawnStatus spawnStatusCode = SpawnStatus::Success; // Spawn Status Code used for CsvSpawner EBus notify - OnEntitiesSpawnFinished.
+        // SpawnStatus spawnStatusCode = SpawnStatus::Success; // Spawn Status Code used for CsvSpawner EBus notify -


I recommend to remove this code if it is commented

norbertprokopiuk · 2025-07-03T05:50:36Z

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h

+        virtual void OnEntitiesSpawnBegin(CsvSpawnerUtils::SpawnInfo m_spawnInfo) = 0;

        /**
         * @brief Called when entity spawning finishes.
         * @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(CsvSpawnerUtils::SpawnInfo& m_spawnInfo, CsvSpawnerUtils::SpawnStatus m_statusCode) = 0;
+        virtual void OnEntitiesSpawnFinished(CsvSpawnerUtils::SpawnInfo m_spawnInfo, CsvSpawnerUtils::SpawnStatus m_statusCode) = 0;


Is there a reason for removing those references?

norbertprokopiuk · 2025-07-03T06:25:06Z

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h

I think that created notification methods are correct, but I've noticed that you set AddressPolicy as Single. We can have multiple CSVSpawner components on the scene. It means that we should be send notification either when all components finishes their jobs (you would need system component for that), or you can change this notification bus AddressPolicy to ById to allow each component to send their own notification. Then it would be client call to decide on which entity/component notification they want to connect with.

norbertprokopiuk · 2025-07-03T06:48:20Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

        auto spawner = AZ::Interface<AzFramework::SpawnableEntitiesDefinition>::Get();
        AZ_Assert(spawner, "Unable to get spawnable entities definition");

+        auto pendingSpawns = AZStd::make_shared<AZStd::atomic_int>(static_cast<int>(entitiesToSpawn.size()));


Inside for loop through entitiesToSpawn you have some conditions which cause iteration skip without calling spawning interface. This causes lack of decremention of pendingSpawns what leads to situation where condition in line 349 is never met if as least one entity failed to spawn.

norbertprokopiuk · 2025-07-03T07:17:16Z

Gems/CsvSpawner/Code/Source/CsvSpawner/CsvSpawnerUtils.cpp

+                    const AZ::Entity* root = *view.begin();
+                    AZ::TransformBus::Event(root->GetId(), &AZ::TransformBus::Events::SetParent, parentId);


Suggested change

const AZ::Entity* root = *view.begin();

AZ::TransformBus::Event(root->GetId(), &AZ::TransformBus::Events::SetParent, parentId);

const AZ::Entity* root = *view.begin();

AZ_Assert(root, "Invalid root entity");

AZ::TransformBus::Event(root->GetId(), &AZ::TransformBus::Events::SetParent, parentId);

w-czerski added 7 commits January 28, 2025 10:36

init spline subscriber

6470d8a

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

clang format

6ccf97e

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

fix | get spline and convert into vector of vertex

915d029

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

remove auto

1b9affd

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

apply review changes

57322f8

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

remove comments

ee5c565

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

use emplace | format | apply reviews

399c2d1

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

w-czerski mentioned this pull request Jan 28, 2025

Add Spline Path Publisher Component #108

Merged

arturkamieniecki reviewed Jan 28, 2025

View reviewed changes

patrykantosz reviewed Jan 29, 2025

View reviewed changes

w-czerski requested review from patrykantosz and arturkamieniecki January 29, 2025 12:28

patrykantosz approved these changes Feb 3, 2025

View reviewed changes

jhanca-robotecai requested changes Feb 11, 2025

View reviewed changes

pijaro requested changes Feb 12, 2025

View reviewed changes

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h Outdated Show resolved Hide resolved

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h Outdated Show resolved Hide resolved

Gems/CsvSpawner/Code/Include/CsvSpawner/CsvSpawnerInterface.h Outdated Show resolved Hide resolved

w-czerski added 7 commits March 18, 2025 12:13

change ros2 -> ROS 2

5b7dd26

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

review resolved | topic changed to spline

3fc175d

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

review resolved | use proper ROS 2 naming

9cf218a

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

get ros2frame outside tick

4651355

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

add update frequency

45bd34c

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

add readme info | add update frequency

ffa7f34

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

add readme info | add update frequency

23e9891

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

w-czerski requested review from pijaro and jhanca-robotecai March 18, 2025 12:21

w-czerski self-assigned this Mar 18, 2025

w-czerski added 14 commits July 2, 2025 14:40

remove const in func definition

51e1941

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

make interface clas final

476f2b4

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

init value definition | add spawn stop break points

26cb4bd

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

remove const& from struct definition

62358ef

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

remove final from interface

9e3cf1a

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

use constexpr

95b70d1

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

move struct to utils

b30aac2

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

add lua / script canvas support

75e3739

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

add info to readme

61fc9aa

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

fix headers | make interface func pure virtual

13671a4

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

fix reflections

2c17d49

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

revert 3rdParty changes

896f067

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

add counter to track if all entities are spawned

687aff9

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

clang format

b30dda3

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

w-czerski force-pushed the wc/csvspawner_notifications branch from 9ce6d49 to b30dda3 Compare July 2, 2025 12:40

w-czerski added 2 commits July 2, 2025 15:25

Merge branch 'main' into wc/csvspawner_notifications

3694333

remove unused directives

2e4d01a

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

norbertprokopiuk requested changes Jul 2, 2025

View reviewed changes

w-czerski added 3 commits July 2, 2025 15:36

clang format

3345918

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

remove dangling references and pointers | using shared ptr for async …

48cd8d2

…lamdas Signed-off-by: Wojciech Czerski <[email protected]>

add spacing

24c4e8d

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

norbertprokopiuk reviewed Jul 3, 2025

View reviewed changes

jhanca-robotecai deleted the branch o3de-2409 July 25, 2025 07:55

jhanca-robotecai closed this Jul 25, 2025

jhanca-robotecai reopened this Jul 25, 2025

jhanca-robotecai changed the base branch from main to 2409 July 25, 2025 08:50

jhanca-robotecai requested review from mateuszwasilewski, michalpelka and zakmat as code owners July 25, 2025 08:50

jhanca-robotecai changed the title ~~Csv Spawner Notification Bus~~ [2409] Csv Spawner Notification Bus Jul 25, 2025


		#include "CsvSpawnerUtils.h"

		#include "AzCore/std/smart_ptr/make_shared.h"

	#include "AzCore/std/smart_ptr/make_shared.h"
	#include <AzCore/std/smart_ptr/make_shared.h>

		const AZ::Entity* root = *view.begin();
		AZ::TransformBus::Event(root->GetId(), &AZ::TransformBus::Events::SetParent, parentId);

[2409] Csv Spawner Notification Bus #109

Are you sure you want to change the base?

[2409] Csv Spawner Notification Bus #109

Uh oh!

Conversation

w-czerski commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About:

Uh oh!

arturkamieniecki left a comment

Choose a reason for hiding this comment

Uh oh!

w-czerski commented Jan 28, 2025

Uh oh!

patrykantosz left a comment

Choose a reason for hiding this comment

Uh oh!

w-czerski commented Jan 29, 2025

Uh oh!

w-czerski commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhanca-robotecai commented Feb 14, 2025

Uh oh!

w-czerski commented Feb 17, 2025

Uh oh!

norbertprokopiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

norbertprokopiuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

w-czerski commented Jan 28, 2025 •

edited

Loading

w-czerski commented Jan 29, 2025 •

edited

Loading