From e0948a26bc1cd169de8ae5a2f82bc8d69794aa1e Mon Sep 17 00:00:00 2001 From: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> Date: Wed, 26 May 2021 14:26:16 -0700 Subject: [PATCH] Fixed early ticket delete crash This commit fixes a crash that could happen when a spawnable ticket was deleted before all requests in the queue had completed. Because of this crash the requests now only hold on to the payload of the ticket but not the ticket itself. As a side effect, callbacks can no longer provide the ticket itself so instead a unique id for the ticket is returned. --- .../Spawnable/SpawnableEntitiesContainer.cpp | 4 +- .../Spawnable/SpawnableEntitiesInterface.cpp | 13 +- .../Spawnable/SpawnableEntitiesInterface.h | 22 +-- .../Spawnable/SpawnableEntitiesManager.cpp | 101 ++++++------ .../Spawnable/SpawnableEntitiesManager.h | 48 +++--- .../SpawnableEntitiesManagerTests.cpp | 150 ++++++++++++++++-- 6 files changed, 242 insertions(+), 96 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp index e9a78dccde..9b06eb1f20 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp @@ -70,7 +70,7 @@ namespace AzFramework SpawnableEntitiesInterface::Get()->Barrier( m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default, - [threadData = m_threadData](EntitySpawnTicket&) mutable + [threadData = m_threadData](EntitySpawnTicket::Id) mutable { threadData.reset(); }); @@ -89,7 +89,7 @@ namespace AzFramework SpawnableEntitiesInterface::Get()->Barrier( m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default, - [generation = m_threadData->m_generation, callback = AZStd::move(callback)](EntitySpawnTicket&) + [generation = m_threadData->m_generation, callback = AZStd::move(callback)](EntitySpawnTicket::Id) { callback(generation); }); diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp index 97169ebeb3..26a10933b5 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp @@ -239,7 +239,9 @@ namespace AzFramework { auto manager = SpawnableEntitiesInterface::Get(); AZ_Assert(manager, "Attempting to create an entity spawn ticket while the SpawnableEntitiesInterface has no implementation."); - m_payload = manager->CreateTicket(AZStd::move(spawnable)); + AZStd::pair result = manager->CreateTicket(AZStd::move(spawnable)); + m_id = result.first; + m_payload = result.second; } EntitySpawnTicket::~EntitySpawnTicket() @@ -250,6 +252,7 @@ namespace AzFramework AZ_Assert(manager, "Attempting to destroy an entity spawn ticket while the SpawnableEntitiesInterface has no implementation."); manager->DestroyTicket(m_payload); m_payload = nullptr; + m_id = 0; } } @@ -263,12 +266,20 @@ namespace AzFramework AZ_Assert(manager, "Attempting to destroy an entity spawn ticket while the SpawnableEntitiesInterface has no implementation."); manager->DestroyTicket(m_payload); } + m_id = rhs.m_id; + rhs.m_id = 0; + m_payload = rhs.m_payload; rhs.m_payload = nullptr; } return *this; } + uint64_t EntitySpawnTicket::GetId() const + { + return m_id; + } + bool EntitySpawnTicket::IsValid() const { return m_payload != nullptr; diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h index 97d06e1f37..b40136def7 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h @@ -143,6 +143,8 @@ namespace AzFramework public: friend class SpawnableEntitiesDefinition; + using Id = uint64_t; + EntitySpawnTicket() = default; EntitySpawnTicket(const EntitySpawnTicket&) = delete; EntitySpawnTicket(EntitySpawnTicket&& rhs); @@ -152,20 +154,22 @@ namespace AzFramework EntitySpawnTicket& operator=(const EntitySpawnTicket&) = delete; EntitySpawnTicket& operator=(EntitySpawnTicket&& rhs); + uint64_t GetId() const; bool IsValid() const; private: void* m_payload{ nullptr }; + Id m_id { 0 }; //!< An id that uniquely identifies a ticket. }; - using EntitySpawnCallback = AZStd::function; - using EntityPreInsertionCallback = AZStd::function; - using EntityDespawnCallback = AZStd::function; - using ReloadSpawnableCallback = AZStd::function; - using ListEntitiesCallback = AZStd::function; - using ListIndicesEntitiesCallback = AZStd::function; - using ClaimEntitiesCallback = AZStd::function; - using BarrierCallback = AZStd::function; + using EntitySpawnCallback = AZStd::function; + using EntityPreInsertionCallback = AZStd::function; + using EntityDespawnCallback = AZStd::function; + using ReloadSpawnableCallback = AZStd::function; + using ListEntitiesCallback = AZStd::function; + using ListIndicesEntitiesCallback = AZStd::function; + using ClaimEntitiesCallback = AZStd::function; + using BarrierCallback = AZStd::function; //! Interface definition to (de)spawn entities from a spawnable into the game world. //! @@ -265,7 +269,7 @@ namespace AzFramework virtual void AddOnDespawnedHandler(AZ::Event>::Handler& handler) = 0; protected: - [[nodiscard]] virtual void* CreateTicket(AZ::Data::Asset&& spawnable) = 0; + [[nodiscard]] virtual AZStd::pair CreateTicket(AZ::Data::Asset&& spawnable) = 0; virtual void DestroyTicket(void* ticket) = 0; template diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp index 959d2ab64f..caf1112e9b 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp @@ -25,10 +25,11 @@ namespace AzFramework template void SpawnableEntitiesManager::QueueRequest(EntitySpawnTicket& ticket, SpawnablePriority priority, T&& request) { + request.m_ticket = &GetTicketPayload(ticket); Queue& queue = priority <= HighPriorityThreshold ? m_highPriorityQueue : m_regularPriorityQueue; { AZStd::scoped_lock queueLock(queue.m_pendingRequestMutex); - request.m_ticketId = GetTicketPayload(ticket).m_nextTicketId++; + request.m_requestId = GetTicketPayload(ticket).m_nextRequestId++; queue.m_pendingRequest.push(AZStd::move(request)); } } @@ -40,7 +41,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to SpawnAllEntities hasn't been initialized."); SpawnAllEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_completionCallback = AZStd::move(completionCallback); queueEntry.m_preInsertionCallback = AZStd::move(preInsertionCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); @@ -53,7 +54,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to SpawnEntities hasn't been initialized."); SpawnEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_entityIndices = AZStd::move(entityIndices); queueEntry.m_completionCallback = AZStd::move(completionCallback); queueEntry.m_preInsertionCallback = AZStd::move(preInsertionCallback); @@ -66,7 +67,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to DespawnAllEntities hasn't been initialized."); DespawnAllEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_completionCallback = AZStd::move(completionCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); } @@ -78,7 +79,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to ReloadSpawnable hasn't been initialized."); ReloadSpawnableCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_spawnable = AZStd::move(spawnable); queueEntry.m_completionCallback = AZStd::move(completionCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); @@ -90,7 +91,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to ListEntities hasn't been initialized."); ListEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_listCallback = AZStd::move(listCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); } @@ -102,7 +103,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to ListEntities hasn't been initialized."); ListIndicesEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_listCallback = AZStd::move(listCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); } @@ -113,7 +114,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to ClaimEntities hasn't been initialized."); ClaimEntitiesCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_listCallback = AZStd::move(listCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); } @@ -124,7 +125,7 @@ namespace AzFramework AZ_Assert(ticket.IsValid(), "Ticket provided to Barrier hasn't been initialized."); BarrierCommand queueEntry; - queueEntry.m_ticket = &ticket; + queueEntry.m_ticketId = ticket.GetId(); queueEntry.m_completionCallback = AZStd::move(completionCallback); QueueRequest(ticket, priority, AZStd::move(queueEntry)); } @@ -217,11 +218,13 @@ namespace AzFramework return queue.m_delayed.empty() ? CommandQueueStatus::NoCommandsLeft : CommandQueueStatus::HasCommandsLeft; } - void* SpawnableEntitiesManager::CreateTicket(AZ::Data::Asset&& spawnable) + AZStd::pair SpawnableEntitiesManager::CreateTicket(AZ::Data::Asset&& spawnable) { + static AZStd::atomic_uint64_t idCounter { 1 }; + auto result = aznew Ticket(); result->m_spawnable = AZStd::move(spawnable); - return result; + return AZStd::make_pair(idCounter++, result); } void SpawnableEntitiesManager::DestroyTicket(void* ticket) @@ -230,7 +233,7 @@ namespace AzFramework queueEntry.m_ticket = reinterpret_cast(ticket); { AZStd::scoped_lock queueLock(m_regularPriorityQueue.m_pendingRequestMutex); - queueEntry.m_ticketId = reinterpret_cast(ticket)->m_nextTicketId++; + queueEntry.m_requestId = reinterpret_cast(ticket)->m_nextRequestId++; m_regularPriorityQueue.m_pendingRequest.push(AZStd::move(queueEntry)); } } @@ -254,8 +257,8 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(SpawnAllEntitiesCommand& request, AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (ticket.m_spawnable.IsReady() && request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (ticket.m_spawnable.IsReady() && request.m_requestId == ticket.m_currentRequestId) { AZStd::vector& spawnedEntities = ticket.m_spawnedEntities; AZStd::vector& spawnedEntityIndices = ticket.m_spawnedEntityIndices; @@ -303,7 +306,7 @@ namespace AzFramework // Let other systems know about newly spawned entities for any pre-processing before adding to the scene/game context. if (request.m_preInsertionCallback) { - request.m_preInsertionCallback(*request.m_ticket, SpawnableEntityContainerView( + request.m_preInsertionCallback(request.m_ticketId, SpawnableEntityContainerView( ticket.m_spawnedEntities.begin() + spawnedEntitiesInitialCount, ticket.m_spawnedEntities.end())); } @@ -317,13 +320,13 @@ namespace AzFramework // Let other systems know about newly spawned entities for any post-processing after adding to the scene/game context. if (request.m_completionCallback) { - request.m_completionCallback(*request.m_ticket, SpawnableConstEntityContainerView( + request.m_completionCallback(request.m_ticketId, SpawnableConstEntityContainerView( ticket.m_spawnedEntities.begin() + spawnedEntitiesInitialCount, ticket.m_spawnedEntities.end())); } m_onSpawnedEvent.Signal(ticket.m_spawnable); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -334,8 +337,8 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(SpawnEntitiesCommand& request, AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (ticket.m_spawnable.IsReady() && request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (ticket.m_spawnable.IsReady() && request.m_requestId == ticket.m_currentRequestId) { AZStd::vector& spawnedEntities = ticket.m_spawnedEntities; AZStd::vector& spawnedEntityIndices = ticket.m_spawnedEntityIndices; @@ -370,9 +373,7 @@ namespace AzFramework // Let other systems know about newly spawned entities for any pre-processing before adding to the scene/game context. if (request.m_preInsertionCallback) { - request.m_preInsertionCallback( - *request.m_ticket, - SpawnableEntityContainerView( + request.m_preInsertionCallback(request.m_ticketId, SpawnableEntityContainerView( ticket.m_spawnedEntities.begin() + spawnedEntitiesInitialCount, ticket.m_spawnedEntities.end())); } @@ -385,13 +386,13 @@ namespace AzFramework if (request.m_completionCallback) { - request.m_completionCallback(*request.m_ticket, SpawnableConstEntityContainerView( + request.m_completionCallback(request.m_ticketId, SpawnableConstEntityContainerView( ticket.m_spawnedEntities.begin() + spawnedEntitiesInitialCount, ticket.m_spawnedEntities.end())); } m_onSpawnedEvent.Signal(ticket.m_spawnable); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -403,8 +404,8 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(DespawnAllEntitiesCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (request.m_requestId == ticket.m_currentRequestId) { for (AZ::Entity* entity : ticket.m_spawnedEntities) { @@ -420,12 +421,12 @@ namespace AzFramework if (request.m_completionCallback) { - request.m_completionCallback(*request.m_ticket); + request.m_completionCallback(request.m_ticketId); } m_onDespawnedEvent.Signal(ticket.m_spawnable); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -436,11 +437,11 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(ReloadSpawnableCommand& request, AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); + Ticket& ticket = *request.m_ticket; AZ_Assert(ticket.m_spawnable.GetId() == request.m_spawnable.GetId(), "Spawnable is being reloaded, but the provided spawnable has a different asset id. " "This will likely result in unexpected entities being created."); - if (ticket.m_spawnable.IsReady() && request.m_ticketId == ticket.m_currentTicketId) + if (ticket.m_spawnable.IsReady() && request.m_requestId == ticket.m_currentRequestId) { // Delete the original entities. for (AZ::Entity* entity : ticket.m_spawnedEntities) @@ -496,11 +497,11 @@ namespace AzFramework if (request.m_completionCallback) { - request.m_completionCallback(*request.m_ticket, SpawnableConstEntityContainerView( + request.m_completionCallback(request.m_ticketId, SpawnableConstEntityContainerView( ticket.m_spawnedEntities.begin(), ticket.m_spawnedEntities.end())); } - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; m_onSpawnedEvent.Signal(ticket.m_spawnable); @@ -514,12 +515,12 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(ListEntitiesCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (request.m_requestId == ticket.m_currentRequestId) { - request.m_listCallback(*request.m_ticket, SpawnableConstEntityContainerView( + request.m_listCallback(request.m_ticketId, SpawnableConstEntityContainerView( ticket.m_spawnedEntities.begin(), ticket.m_spawnedEntities.end())); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -530,17 +531,15 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(ListIndicesEntitiesCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (request.m_requestId == ticket.m_currentRequestId) { AZ_Assert( ticket.m_spawnedEntities.size() == ticket.m_spawnedEntityIndices.size(), "Entities and indices on spawnable ticket have gone out of sync."); - request.m_listCallback( - *request.m_ticket, - SpawnableConstIndexEntityContainerView( + request.m_listCallback(request.m_ticketId, SpawnableConstIndexEntityContainerView( ticket.m_spawnedEntities.begin(), ticket.m_spawnedEntityIndices.begin(), ticket.m_spawnedEntities.size())); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -551,16 +550,16 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(ClaimEntitiesCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (request.m_requestId == ticket.m_currentRequestId) { - request.m_listCallback(*request.m_ticket, SpawnableEntityContainerView( + request.m_listCallback(request.m_ticketId, SpawnableEntityContainerView( ticket.m_spawnedEntities.begin(), ticket.m_spawnedEntities.end())); ticket.m_spawnedEntities.clear(); ticket.m_spawnedEntityIndices.clear(); - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -571,15 +570,15 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(BarrierCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - Ticket& ticket = GetTicketPayload(*request.m_ticket); - if (request.m_ticketId == ticket.m_currentTicketId) + Ticket& ticket = *request.m_ticket; + if (request.m_requestId == ticket.m_currentRequestId) { if (request.m_completionCallback) { - request.m_completionCallback(*request.m_ticket); + request.m_completionCallback(request.m_ticketId); } - ticket.m_currentTicketId++; + ticket.m_currentRequestId++; return true; } else @@ -590,7 +589,7 @@ namespace AzFramework bool SpawnableEntitiesManager::ProcessRequest(DestroyTicketCommand& request, [[maybe_unused]] AZ::SerializeContext& serializeContext) { - if (request.m_ticketId == request.m_ticket->m_currentTicketId) + if (request.m_requestId == request.m_ticket->m_currentRequestId) { for (AZ::Entity* entity : request.m_ticket->m_spawnedEntities) { diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h index 137376ac7b..b98be60145 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h @@ -97,8 +97,8 @@ namespace AzFramework AZStd::vector m_spawnedEntities; AZStd::vector m_spawnedEntityIndices; AZ::Data::Asset m_spawnable; - uint32_t m_nextTicketId{ 0 }; //!< Next id for this ticket. - uint32_t m_currentTicketId{ 0 }; //!< The id for the command that should be executed. + uint32_t m_nextRequestId{ 0 }; //!< Next id for this ticket. + uint32_t m_currentRequestId { 0 }; //!< The id for the command that should be executed. bool m_loadAll{ true }; }; @@ -106,58 +106,66 @@ namespace AzFramework { EntitySpawnCallback m_completionCallback; EntityPreInsertionCallback m_preInsertionCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct SpawnEntitiesCommand { AZStd::vector m_entityIndices; EntitySpawnCallback m_completionCallback; EntityPreInsertionCallback m_preInsertionCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct DespawnAllEntitiesCommand { EntityDespawnCallback m_completionCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct ReloadSpawnableCommand { AZ::Data::Asset m_spawnable; ReloadSpawnableCallback m_completionCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct ListEntitiesCommand { ListEntitiesCallback m_listCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct ListIndicesEntitiesCommand { ListIndicesEntitiesCallback m_listCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct ClaimEntitiesCommand { ClaimEntitiesCallback m_listCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct BarrierCommand { BarrierCallback m_completionCallback; - EntitySpawnTicket* m_ticket; - uint32_t m_ticketId; + Ticket* m_ticket; + EntitySpawnTicket::Id m_ticketId; + uint32_t m_requestId; }; struct DestroyTicketCommand { Ticket* m_ticket; - uint32_t m_ticketId; + uint32_t m_requestId; }; using Requests = AZStd::variant< @@ -173,7 +181,7 @@ namespace AzFramework template void QueueRequest(EntitySpawnTicket& ticket, SpawnablePriority priority, T&& request); - void* CreateTicket(AZ::Data::Asset&& spawnable) override; + AZStd::pair CreateTicket(AZ::Data::Asset&& spawnable) override; void DestroyTicket(void* ticket) override; CommandQueueStatus ProcessQueue(Queue& queue); diff --git a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp index 191b14c31d..320e2ec435 100644 --- a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp +++ b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp @@ -89,6 +89,10 @@ namespace UnitTest TestApplication* m_application { nullptr }; }; + // + // SpawnAllEntitities + // + TEST_F(SpawnableEntitiesManagerTest, SpawnAllEntities_Call_AllEntitiesSpawned) { static constexpr size_t NumEntities = 4; @@ -96,7 +100,7 @@ namespace UnitTest size_t spawnedEntitiesCount = 0; auto callback = - [&spawnedEntitiesCount](AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView entities) + [&spawnedEntitiesCount](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) { spawnedEntitiesCount += entities.size(); }; @@ -106,6 +110,62 @@ namespace UnitTest EXPECT_EQ(NumEntities, spawnedEntitiesCount); } + TEST_F(SpawnableEntitiesManagerTest, SpawnAllEntities_DeleteTicketBeforeCall_NoCrash) + { + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->SpawnAllEntities(ticket, AzFramework::SpawnablePriorty_Default); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // SpawnEntities + // + + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_DeleteTicketBeforeCall_NoCrash) + { + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->SpawnEntities(ticket, AzFramework::SpawnablePriorty_Default, {}); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // DespawnAllEntities + // + + TEST_F(SpawnableEntitiesManagerTest, DespawnAllEntities_DeleteTicketBeforeCall_NoCrash) + { + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->DespawnAllEntities(ticket, AzFramework::SpawnablePriorty_Default); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // ReloadSpawnable + // + + TEST_F(SpawnableEntitiesManagerTest, ReloadSpawnable_DeleteTicketBeforeCall_NoCrash) + { + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->ReloadSpawnable(ticket, AzFramework::SpawnablePriorty_Default, *m_spawnableAsset); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // ListEntitities + // + TEST_F(SpawnableEntitiesManagerTest, ListEntities_Call_AllEntitiesAreReported) { static constexpr size_t NumEntities = 4; @@ -114,7 +174,7 @@ namespace UnitTest bool allValidEntityIds = true; size_t spawnedEntitiesCount = 0; auto callback = [&allValidEntityIds, &spawnedEntitiesCount] - (AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView entities) + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) { for (auto&& entity : entities) { @@ -131,6 +191,22 @@ namespace UnitTest EXPECT_EQ(NumEntities, spawnedEntitiesCount); } + TEST_F(SpawnableEntitiesManagerTest, ListEntities_DeleteTicketBeforeCall_NoCrash) + { + auto callback = [](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView) {}; + + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->ListEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // ListIndicesAndEntities + // + TEST_F(SpawnableEntitiesManagerTest, ListIndicesAndEntities_Call_AllEntitiesAreReportedAndIncrementByOne) { static constexpr size_t NumEntities = 4; @@ -139,7 +215,7 @@ namespace UnitTest bool allValidEntityIds = true; size_t spawnedEntitiesCount = 0; auto callback = [&allValidEntityIds, &spawnedEntitiesCount] - (AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstIndexEntityContainerView entities) + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstIndexEntityContainerView entities) { for (auto&& indexEntityPair : entities) { @@ -160,6 +236,54 @@ namespace UnitTest EXPECT_EQ(NumEntities, spawnedEntitiesCount); } + TEST_F(SpawnableEntitiesManagerTest, ListIndicesAndEntities_DeleteTicketBeforeCall_NoCrash) + { + auto callback = [](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstIndexEntityContainerView) {}; + + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->ListIndicesAndEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // ClaimEntities + // + + TEST_F(SpawnableEntitiesManagerTest, ClaimEntities_DeleteTicketBeforeCall_NoCrash) + { + auto callback = [](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableEntityContainerView) {}; + + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->ClaimEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // Barrier + // + + TEST_F(SpawnableEntitiesManagerTest, Barrier_DeleteTicketBeforeCall_NoCrash) + { + auto callback = [](AzFramework::EntitySpawnTicket::Id) {}; + + { + AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); + m_manager->Barrier(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + } + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + + // + // Misc. - Priority tests + // + TEST_F(SpawnableEntitiesManagerTest, Priority_HighBeforeDefault_HigherPriorityCallHappensBeforeDefaultPriorityEvenWhenQueuedLater) { static constexpr size_t NumEntities = 4; @@ -171,12 +295,12 @@ namespace UnitTest size_t highPriorityCallId = 0; size_t defaultPriorityCallId = 0; auto highCallback = [&callCounter, &highPriorityCallId] - (AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView) + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView) { highPriorityCallId = callCounter++; }; auto defaultCallback = [&callCounter, &defaultPriorityCallId] - (AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView) + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView) { defaultPriorityCallId = callCounter++; }; @@ -199,15 +323,15 @@ namespace UnitTest size_t highPriorityCallId = 0; size_t defaultPriorityCallId = 0; auto highCallback = - [&callCounter, &highPriorityCallId](AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView) - { - highPriorityCallId = callCounter++; - }; + [&callCounter, &highPriorityCallId](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView) + { + highPriorityCallId = callCounter++; + }; auto defaultCallback = - [&callCounter, &defaultPriorityCallId](AzFramework::EntitySpawnTicket&, AzFramework::SpawnableConstEntityContainerView) - { - defaultPriorityCallId = callCounter++; - }; + [&callCounter, &defaultPriorityCallId](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView) + { + defaultPriorityCallId = callCounter++; + }; m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, {}, AZStd::move(defaultCallback)); m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_High, {}, AZStd::move(highCallback));