From c946d579282340877d8e11cba758a82b38e37cb8 Mon Sep 17 00:00:00 2001 From: AMZN-koppersr <82230785+AMZN-koppersr@users.noreply.github.com> Date: Thu, 27 May 2021 11:14:21 -0700 Subject: [PATCH] Addressed PR feedback. --- .../Spawnable/SpawnableEntitiesContainer.cpp | 12 +++---- .../Spawnable/SpawnableEntitiesInterface.cpp | 2 +- .../Spawnable/SpawnableEntitiesInterface.h | 16 ++++----- .../Spawnable/SpawnableEntitiesManager.h | 4 +++ .../Spawnable/SpawnableSystemComponent.cpp | 4 +-- .../SpawnableEntitiesManagerTests.cpp | 34 +++++++++---------- 6 files changed, 38 insertions(+), 34 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp index 9b06eb1f20..808de74e71 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesContainer.cpp @@ -38,20 +38,20 @@ namespace AzFramework void SpawnableEntitiesContainer::SpawnAllEntities() { AZ_Assert(m_threadData, "Calling SpawnAllEntities on a Spawnable container that's not set."); - SpawnableEntitiesInterface::Get()->SpawnAllEntities(m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default); + SpawnableEntitiesInterface::Get()->SpawnAllEntities(m_threadData->m_spawnedEntitiesTicket, SpawnablePriority_Default); } void SpawnableEntitiesContainer::SpawnEntities(AZStd::vector entityIndices) { AZ_Assert(m_threadData, "Calling SpawnEntities on a Spawnable container that's not set."); SpawnableEntitiesInterface::Get()->SpawnEntities( - m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default, AZStd::move(entityIndices)); + m_threadData->m_spawnedEntitiesTicket, SpawnablePriority_Default, AZStd::move(entityIndices)); } void SpawnableEntitiesContainer::DespawnAllEntities() { AZ_Assert(m_threadData, "Calling DespawnEntities on a Spawnable container that's not set."); - SpawnableEntitiesInterface::Get()->DespawnAllEntities(m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default); + SpawnableEntitiesInterface::Get()->DespawnAllEntities(m_threadData->m_spawnedEntitiesTicket, SpawnablePriority_Default); } void SpawnableEntitiesContainer::Reset(AZ::Data::Asset spawnable) @@ -69,7 +69,7 @@ namespace AzFramework SpawnableEntitiesInterface::Get()->Barrier( m_threadData->m_spawnedEntitiesTicket, - SpawnablePriorty_Default, + SpawnablePriority_Default, [threadData = m_threadData](EntitySpawnTicket::Id) mutable { threadData.reset(); @@ -88,7 +88,7 @@ namespace AzFramework AZ_Assert(m_threadData, "Calling DespawnEntities on a Spawnable container that's not set."); SpawnableEntitiesInterface::Get()->Barrier( m_threadData->m_spawnedEntitiesTicket, - SpawnablePriorty_Default, + SpawnablePriority_Default, [generation = m_threadData->m_generation, callback = AZStd::move(callback)](EntitySpawnTicket::Id) { callback(generation); @@ -116,6 +116,6 @@ namespace AzFramework AZ_TracePrintf("Spawnables", "Reloading spawnable '%s'.\n", replacementAsset.GetHint().c_str()); SpawnableEntitiesInterface::Get()->ReloadSpawnable( - m_threadData->m_spawnedEntitiesTicket, SpawnablePriorty_Default, AZStd::move(replacementAsset)); + m_threadData->m_spawnedEntitiesTicket, SpawnablePriority_Default, AZStd::move(replacementAsset)); } } // namespace AzFramework diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp index 26a10933b5..ad1bf032c8 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.cpp @@ -275,7 +275,7 @@ namespace AzFramework return *this; } - uint64_t EntitySpawnTicket::GetId() const + auto EntitySpawnTicket::GetId() const -> Id { return m_id; } diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h index b40136def7..27f45064b6 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h @@ -27,11 +27,11 @@ namespace AzFramework { AZ_TYPE_SAFE_INTEGRAL(SpawnablePriority, uint8_t); - inline static constexpr SpawnablePriority SpawnablePriorty_Highest { 0 }; - inline static constexpr SpawnablePriority SpawnablePriorty_High { 32 }; - inline static constexpr SpawnablePriority SpawnablePriorty_Default { 128 }; - inline static constexpr SpawnablePriority SpawnablePriorty_Low { 192 }; - inline static constexpr SpawnablePriority SpawnablePriorty_Lowest { 255 }; + inline static constexpr SpawnablePriority SpawnablePriority_Highest { 0 }; + inline static constexpr SpawnablePriority SpawnablePriority_High { 32 }; + inline static constexpr SpawnablePriority SpawnablePriority_Default { 128 }; + inline static constexpr SpawnablePriority SpawnablePriority_Low { 192 }; + inline static constexpr SpawnablePriority SpawnablePriority_Lowest { 255 }; class SpawnableEntityContainerView { @@ -154,7 +154,7 @@ namespace AzFramework EntitySpawnTicket& operator=(const EntitySpawnTicket&) = delete; EntitySpawnTicket& operator=(EntitySpawnTicket&& rhs); - uint64_t GetId() const; + Id GetId() const; bool IsValid() const; private: @@ -179,11 +179,11 @@ namespace AzFramework //! Calls on the same ticket are guaranteed to be executed in the order they are issued. Note that when issuing requests from //! multiple threads on the same ticket the order in which the requests are assigned to the ticket is not guaranteed. //! - //! Most calls have a priority where values closer to 0 mean higher priority than values closer to 255. The implementation of this + //! Most calls have a priority with values that range from 0 (highest priority) to 255 (lowest priority). The implementation of this //! interface may choose to use priority lanes which doesn't guarantee that higher priority requests happen before lower priority //! requests if they don't pass the priority lane threshold. Priority lanes and their thresholds are implementation specific and may //! differ between platforms. Note that if a call happened on a ticket with lower priority followed by a one with a higher priority - //! the first lower priority call will still needs to complete before the second higher priority call can be executed and the priority + //! the first lower priority call will still need to complete before the second higher priority call can be executed and the priority //! of the first call will not be updated. class SpawnableEntitiesDefinition { diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h index 373a4db9cb..afffdab8b5 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h @@ -207,6 +207,10 @@ namespace AzFramework AZ::Event> m_onSpawnedEvent; AZ::Event> m_onDespawnedEvent; + //! The threshold used to determine if a request goes in the regular (if bigger than the value) or high priority queue (if smaller + //! or equal to this value). The starting value of 64 is chosen as it's between default values SpawnablePriority_High and + //! SpawnablePriority_Default which gives users a bit of room to fine tune the priorities as this value can be configured + //! through the Settings Registry under the key "/O3DE/AzFramework/Spawnables/HighPriorityThreshold". SpawnablePriority m_highPriorityThreshold { 64 }; }; diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableSystemComponent.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableSystemComponent.cpp index 32cc61914e..300ff1441e 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableSystemComponent.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableSystemComponent.cpp @@ -60,8 +60,8 @@ namespace AzFramework void SpawnableSystemComponent::OnSystemTick() { - // Handle only high priority spawning events such as those created from network. These need to happen even if the server - // doesn't have focus to avoid + // Handle only high priority spawning events such as those created from network. These need to happen even if the client + // doesn't have focus to avoid time-out issues for instance. m_entitiesManager.ProcessQueue(SpawnableEntitiesManager::CommandQueuePriority::High); } diff --git a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp index 320e2ec435..484b7f46d7 100644 --- a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp +++ b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp @@ -104,7 +104,7 @@ namespace UnitTest { spawnedEntitiesCount += entities.size(); }; - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, {}, AZStd::move(callback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_Default, {}, AZStd::move(callback)); m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); EXPECT_EQ(NumEntities, spawnedEntitiesCount); @@ -114,7 +114,7 @@ namespace UnitTest { { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->SpawnAllEntities(ticket, AzFramework::SpawnablePriorty_Default); + m_manager->SpawnAllEntities(ticket, AzFramework::SpawnablePriority_Default); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -128,7 +128,7 @@ namespace UnitTest { { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->SpawnEntities(ticket, AzFramework::SpawnablePriorty_Default, {}); + m_manager->SpawnEntities(ticket, AzFramework::SpawnablePriority_Default, {}); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -142,7 +142,7 @@ namespace UnitTest { { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->DespawnAllEntities(ticket, AzFramework::SpawnablePriorty_Default); + m_manager->DespawnAllEntities(ticket, AzFramework::SpawnablePriority_Default); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -156,7 +156,7 @@ namespace UnitTest { { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->ReloadSpawnable(ticket, AzFramework::SpawnablePriorty_Default, *m_spawnableAsset); + m_manager->ReloadSpawnable(ticket, AzFramework::SpawnablePriority_Default, *m_spawnableAsset); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -183,8 +183,8 @@ namespace UnitTest spawnedEntitiesCount += entities.size(); }; - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default); - m_manager->ListEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_Default); + m_manager->ListEntities(*m_ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); EXPECT_TRUE(allValidEntityIds); @@ -197,7 +197,7 @@ namespace UnitTest { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->ListEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->ListEntities(ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -228,8 +228,8 @@ namespace UnitTest } }; - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default); - m_manager->ListIndicesAndEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_Default); + m_manager->ListIndicesAndEntities(*m_ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); EXPECT_TRUE(allValidEntityIds); @@ -242,7 +242,7 @@ namespace UnitTest { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->ListIndicesAndEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->ListIndicesAndEntities(ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -258,7 +258,7 @@ namespace UnitTest { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->ClaimEntities(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->ClaimEntities(ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -274,7 +274,7 @@ namespace UnitTest { AzFramework::EntitySpawnTicket ticket(*m_spawnableAsset); - m_manager->Barrier(ticket, AzFramework::SpawnablePriorty_Default, AZStd::move(callback)); + m_manager->Barrier(ticket, AzFramework::SpawnablePriority_Default, AZStd::move(callback)); } m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } @@ -305,8 +305,8 @@ namespace UnitTest defaultPriorityCallId = callCounter++; }; - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, {}, AZStd::move(defaultCallback)); - m_manager->SpawnAllEntities(highPriorityTicket, AzFramework::SpawnablePriorty_High, {}, AZStd::move(highCallback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_Default, {}, AZStd::move(defaultCallback)); + m_manager->SpawnAllEntities(highPriorityTicket, AzFramework::SpawnablePriority_High, {}, AZStd::move(highCallback)); m_manager->ProcessQueue( AzFramework::SpawnableEntitiesManager::CommandQueuePriority::High | AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); @@ -333,8 +333,8 @@ namespace UnitTest defaultPriorityCallId = callCounter++; }; - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_Default, {}, AZStd::move(defaultCallback)); - m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriorty_High, {}, AZStd::move(highCallback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_Default, {}, AZStd::move(defaultCallback)); + m_manager->SpawnAllEntities(*m_ticket, AzFramework::SpawnablePriority_High, {}, AZStd::move(highCallback)); m_manager->ProcessQueue( AzFramework::SpawnableEntitiesManager::CommandQueuePriority::High | AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular);