From db92dffb14447e0da60cfbc7388c25f30074d10c Mon Sep 17 00:00:00 2001 From: Mike Balfour <82224783+mbalfour-amzn@users.noreply.github.com> Date: Tue, 22 Jun 2021 15:20:28 -0500 Subject: [PATCH] Fix Vegetation Modifier behavior when in-game (#1441) * Removed a bit of dead legacy code * Fixed entity references during spawning Entities that had references to other entities that hadn't been spawned yet weren't getting their IDs remapped correctly, since the new ID wasn't available yet. By pre-generating the full set of IDs, the references now remap correctly. * Fixed up Entity References to work across multiple SpawnEntities calls With SpawnEntities, entity references need to forward-reference to the *first* entity spawned, then from that point on backwards-reference to the *last* entity spawned. Added that logic, along with some initial unit tests for SpawnAllEntities. * Added more unit tests for SpawnEntities / SpawnAllEntities --- .../Spawnable/SpawnableEntitiesInterface.h | 12 +- .../Spawnable/SpawnableEntitiesManager.cpp | 109 ++++-- .../Spawnable/SpawnableEntitiesManager.h | 25 ++ .../SpawnableEntitiesManagerTests.cpp | 359 ++++++++++++++++++ .../Editor/EditorVegetationComponentBase.h | 7 - .../Editor/EditorVegetationComponentBase.inl | 20 - 6 files changed, 471 insertions(+), 61 deletions(-) diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h index 8236683163..d50e239f9a 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesInterface.h @@ -198,11 +198,13 @@ namespace AzFramework AZ::SerializeContext* m_serializeContext{ nullptr }; //! The priority at which this call will be executed. SpawnablePriority m_priority{ SpawnablePriority_Default }; - //! Entity references are resolved by referring to the last entity spawned from a template entity in the spawnable. If this - //! is set to false entities from previous spawn calls are not taken into account. If set to true entity references may be - //! resolved to a previously spawned entity. A lookup table has to be constructed when true, which may negatively impact - //! performance, especially if a large number of entities are present on a ticket. - bool m_referencePreviouslySpawnedEntities{ false }; + //! Entity references are resolved by referring to the most recent entity spawned from a template entity in the spawnable. + //! If the entity referred to hasn't been spawned yet, the reference will be resolved to the first one that *will* be spawned. + //! If this flag is set to "true", the id mappings will persist across SpawnEntites calls, and the entity references will resolve + //! correctly across them. + //! When "false", the entity id mappings will be reset on this call, so entity references will only work within this call, or + //! potentially with any subsequent SpawnEntities call where the flag is true once again. + bool m_referencePreviouslySpawnedEntities{ true }; }; struct DespawnAllEntitiesOptionalArgs final diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp index d6d005a944..8cbc60d94e 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.cpp @@ -250,10 +250,47 @@ namespace AzFramework AZ::Entity* SpawnableEntitiesManager::CloneSingleEntity(const AZ::Entity& entityTemplate, EntityIdMap& templateToCloneMap, AZ::SerializeContext& serializeContext) { - return AZ::IdUtils::Remapper::CloneObjectAndGenerateNewIdsAndFixRefs( + // If the same ID gets remapped more than once, preserve the original remapping instead of overwriting it. + constexpr bool allowDuplicateIds = false; + + return AZ::IdUtils::Remapper::CloneObjectAndGenerateNewIdsAndFixRefs( &entityTemplate, templateToCloneMap, &serializeContext); } + void SpawnableEntitiesManager::InitializeEntityIdMappings( + const Spawnable::EntityList& entities, EntityIdMap& idMap, AZStd::unordered_set& previouslySpawned) + { + // Make sure we don't have any previous data lingering around. + idMap.clear(); + previouslySpawned.clear(); + + idMap.reserve(entities.size()); + previouslySpawned.reserve(entities.size()); + + for (auto& entity : entities) + { + idMap.emplace(entity->GetId(), AZ::Entity::MakeId()); + } + } + + void SpawnableEntitiesManager::RefreshEntityIdMapping( + const AZ::EntityId& entityId, EntityIdMap& idMap, AZStd::unordered_set& previouslySpawned) + { + if (previouslySpawned.contains(entityId)) + { + // This entity has already been spawned at least once before, so we need to generate a new id for it and + // preserve the new id to fix up any future entity references to this entity. + idMap[entityId] = AZ::Entity::MakeId(); + } + else + { + // This entity hasn't been spawned yet, so use the first id we've already generated for this entity and mark + // it as spawned so we know not to reuse this id next time. + previouslySpawned.emplace(entityId); + } + } + + bool SpawnableEntitiesManager::ProcessRequest(SpawnAllEntitiesCommand& request) { Ticket& ticket = *request.m_ticket; @@ -269,18 +306,24 @@ namespace AzFramework const Spawnable::EntityList& entitiesToSpawn = ticket.m_spawnable->GetEntities(); size_t entitiesToSpawnSize = entitiesToSpawn.size(); - // Map keeps track of ids from template (spawnable) to clone (instance) - // Allowing patch ups of fields referring to entityIds outside of a given entity - EntityIdMap templateToCloneEntityIdMap; - // Reserve buffers spawnedEntities.reserve(spawnedEntities.size() + entitiesToSpawnSize); spawnedEntityIndices.reserve(spawnedEntityIndices.size() + entitiesToSpawnSize); - templateToCloneEntityIdMap.reserve(entitiesToSpawnSize); + + // Pre-generate the full set of entity id to new entity id mappings, so that during the clone operation below, + // any entity references that point to a not-yet-cloned entity will still get their ids remapped correctly. + // We clear out and regenerate the set of IDs on every SpawnAllEntities call, because presumably every entity reference + // in every entity we're about to instantiate is intended to point to an entity in our newly-instantiated batch, regardless + // of spawn order. If we didn't clear out the map, it would be possible for some entities here to have references to + // previously-spawned entities from a previous SpawnEntities or SpawnAllEntities call. + InitializeEntityIdMappings(entitiesToSpawn, ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); for (size_t i = 0; i < entitiesToSpawnSize; ++i) { - AZ::Entity* clone = CloneSingleEntity(*entitiesToSpawn[i], templateToCloneEntityIdMap, *request.m_serializeContext); + // If this entity has previously been spawned, give it a new id in the reference map + RefreshEntityIdMapping(entitiesToSpawn[i].get()->GetId(), ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); + + AZ::Entity* clone = CloneSingleEntity(*entitiesToSpawn[i], ticket.m_entityIdReferenceMap, *request.m_serializeContext); AZ_Assert(clone != nullptr, "Failed to clone spawnable entity."); spawnedEntities.emplace_back(clone); @@ -337,21 +380,17 @@ namespace AzFramework const Spawnable::EntityList& entitiesToSpawn = ticket.m_spawnable->GetEntities(); size_t entitiesToSpawnSize = request.m_entityIndices.size(); - // Reconstruct the template to entity mapping. - EntityIdMap templateToCloneEntityIdMap; - if (!request.m_referencePreviouslySpawnedEntities) - { - templateToCloneEntityIdMap.reserve(entitiesToSpawnSize); - } - else + if (ticket.m_entityIdReferenceMap.empty() || !request.m_referencePreviouslySpawnedEntities) { - templateToCloneEntityIdMap.reserve(spawnedEntitiesInitialCount + entitiesToSpawnSize); - SpawnableConstIndexEntityContainerView indexEntityView( - spawnedEntities.begin(), spawnedEntityIndices.begin(), spawnedEntities.size()); - for (auto& entry : indexEntityView) - { - templateToCloneEntityIdMap.insert_or_assign(entitiesToSpawn[entry.GetIndex()]->GetId(), entry.GetEntity()->GetId()); - } + // This map keeps track of ids from template (spawnable) to clone (instance) allowing patch ups of fields referring + // to entityIds outside of a given entity. + // We pre-generate the full set of entity id to new entity id mappings, so that during the clone operation below, + // any entity references that point to a not-yet-cloned entity will still get their ids remapped correctly. + // By default, we only initialize this map once because it needs to persist across multiple SpawnEntities calls, so + // that reference fixups work even when the entity being referenced is spawned in a different SpawnEntities + // (or SpawnAllEntities) call. + // However, the caller can also choose to reset the map by passing in "m_referencePreviouslySpawnedEntities = false". + InitializeEntityIdMappings(entitiesToSpawn, ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); } spawnedEntities.reserve(spawnedEntities.size() + entitiesToSpawnSize); @@ -361,7 +400,12 @@ namespace AzFramework { if (index < entitiesToSpawn.size()) { - AZ::Entity* clone = CloneSingleEntity(*entitiesToSpawn[index], templateToCloneEntityIdMap, *request.m_serializeContext); + // If this entity has previously been spawned, give it a new id in the reference map + RefreshEntityIdMapping( + entitiesToSpawn[index].get()->GetId(), ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); + + AZ::Entity* clone = + CloneSingleEntity(*entitiesToSpawn[index], ticket.m_entityIdReferenceMap, *request.m_serializeContext); AZ_Assert(clone != nullptr, "Failed to clone spawnable entity."); spawnedEntities.push_back(clone); @@ -451,9 +495,11 @@ namespace AzFramework ticket.m_spawnedEntities.clear(); const Spawnable::EntityList& entities = request.m_spawnable->GetEntities(); - // Map keeps track of ids from template (spawnable) to clone (instance) - // Allowing patch ups of fields referring to entityIds outside of a given entity - EntityIdMap templateToCloneEntityIdMap; + // Pre-generate the full set of entity id to new entity id mappings, so that during the clone operation below, + // any entity references that point to a not-yet-cloned entity will still get their ids remapped correctly. + // This map is intentionally cleared out and regenerated here to ensure that we're starting fresh with mappings that + // match the new set of template entities getting spawned. + InitializeEntityIdMappings(entities, ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); if (ticket.m_loadAll) { @@ -461,11 +507,13 @@ namespace AzFramework // to spawn every entity, simply start over. ticket.m_spawnedEntityIndices.clear(); size_t entitiesToSpawnSize = entities.size(); - templateToCloneEntityIdMap.reserve(entitiesToSpawnSize); for (size_t i = 0; i < entitiesToSpawnSize; ++i) { - AZ::Entity* clone = CloneSingleEntity(*entities[i], templateToCloneEntityIdMap, *request.m_serializeContext); + // If this entity has previously been spawned, give it a new id in the reference map + RefreshEntityIdMapping(entities[i].get()->GetId(), ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); + + AZ::Entity* clone = CloneSingleEntity(*entities[i], ticket.m_entityIdReferenceMap, *request.m_serializeContext); AZ_Assert(clone != nullptr, "Failed to clone spawnable entity."); ticket.m_spawnedEntities.push_back(clone); @@ -475,7 +523,7 @@ namespace AzFramework else { size_t entitiesSize = entities.size(); - templateToCloneEntityIdMap.reserve(entitiesSize); + for (size_t index : ticket.m_spawnedEntityIndices) { // It's possible for the new spawnable to have a different number of entities, so guard against this. @@ -483,7 +531,10 @@ namespace AzFramework // detected and will result in the incorrect entities being spawned. if (index < entitiesSize) { - AZ::Entity* clone = CloneSingleEntity(*entities[index], templateToCloneEntityIdMap, *request.m_serializeContext); + // If this entity has previously been spawned, give it a new id in the reference map + RefreshEntityIdMapping(entities[index].get()->GetId(), ticket.m_entityIdReferenceMap, ticket.m_previouslySpawned); + + AZ::Entity* clone = CloneSingleEntity(*entities[index], ticket.m_entityIdReferenceMap, *request.m_serializeContext); AZ_Assert(clone != nullptr, "Failed to clone spawnable entity."); ticket.m_spawnedEntities.push_back(clone); } diff --git a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h index 5f659182d3..0c3f1c9ef5 100644 --- a/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h +++ b/Code/Framework/AzFramework/AzFramework/Spawnable/SpawnableEntitiesManager.h @@ -85,6 +85,22 @@ namespace AzFramework AZ_CLASS_ALLOCATOR(Ticket, AZ::ThreadPoolAllocator, 0); static constexpr uint32_t Processing = AZStd::numeric_limits::max(); + //! Map of template entity ids to their associated instance ids. + //! Tickets can be used to spawn the same template entities multiple times, in any order, across multiple calls. + //! Since template entities can reference other entities, this map is used to fix up those references across calls + //! using the following policy: + //! - Entities referencing an entity that hasn't been spawned yet will get a reference to the id that *will* be used + //! the first time that entity will be spawned. The reference will be invalid until that entity is spawned, but + //! will be valid if/when it gets spawned. + //! - Entities referencing an entity that *has* been spawned will get a reference to the id that was *last* used to + //! spawn the entity. + //! Note that this implies a certain level of non-determinism when spawning across calls, because the entity references + //! will be based on the order in which the SpawnEntity calls occur, which can be affected by things like priority. + EntityIdMap m_entityIdReferenceMap; + //! For this to work, we also need to keep track of whether or not each entity has been spawned at least once, so we know + //! whether or not to replace the id in the map when spawning a new instance of that entity. + AZStd::unordered_set m_previouslySpawned; + AZStd::vector m_spawnedEntities; AZStd::vector m_spawnedEntityIndices; AZ::Data::Asset m_spawnable; @@ -194,6 +210,15 @@ namespace AzFramework bool ProcessRequest(BarrierCommand& request); bool ProcessRequest(DestroyTicketCommand& request); + //! Generate a base set of original-to-new entity ID mappings to use during spawning. + //! Since Entity references get fixed up on an entity-by-entity basis while spawning, it's important to have the complete + //! set of new IDs available right at the start. This way, entities that refer to other entities that haven't spawned yet + //! will still get their references remapped correctly. + void InitializeEntityIdMappings( + const Spawnable::EntityList& entities, EntityIdMap& idMap, AZStd::unordered_set& previouslySpawned); + void RefreshEntityIdMapping( + const AZ::EntityId& entityId, EntityIdMap& idMap, AZStd::unordered_set& previouslySpawned); + Queue m_highPriorityQueue; Queue m_regularPriorityQueue; diff --git a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp index f0614aa13b..462cf949b0 100644 --- a/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp +++ b/Code/Framework/Tests/Spawnable/SpawnableEntitiesManagerTests.cpp @@ -32,6 +32,33 @@ namespace UnitTest } }; + // Test component that has a reference to a different entity for use in validating per-instance entity id fixups. + class ComponentWithEntityReference : public AZ::Component + { + public: + AZ_COMPONENT(ComponentWithEntityReference, "{CF5FDE59-86E5-40B6-9272-BBC1C4AFD061}"); + + void Activate() override + { + } + + void Deactivate() override + { + } + + static void Reflect(AZ::ReflectContext* reflection) + { + if (auto* serializeContext = azrtti_cast(reflection)) + { + serializeContext->Class() + ->Field("EntityReference", &ComponentWithEntityReference::m_entityReference) + ; + } + } + + AZ::EntityId m_entityReference; + }; + class SpawnableEntitiesManagerTest : public AllocatorsFixture { public: @@ -42,6 +69,8 @@ namespace UnitTest m_application = new TestApplication(); AZ::ComponentApplication::Descriptor descriptor; m_application->Start(descriptor); + m_application->RegisterComponentDescriptor(ComponentWithEntityReference::CreateDescriptor()); + // Without this, the user settings component would attempt to save on finalize/shutdown. Since the file is // shared across the whole engine, if multiple tests are run in parallel, the saving could cause a crash // in the unit tests. @@ -80,6 +109,7 @@ namespace UnitTest void FillSpawnable(size_t numElements) { AzFramework::Spawnable::EntityList& entities = m_spawnable->GetEntities(); + entities.clear(); entities.reserve(numElements); for (size_t i=0; iGetEntities(); + size_t numElements = entities.size(); + for (size_t i = 0; i < numElements; ++i) + { + AZStd::unique_ptr& entity = entities[i]; + auto component = entity->CreateComponent(); + switch (refScheme) + { + case EntityReferenceScheme::AllReferenceFirst : + component->m_entityReference = entities[0]->GetId(); + break; + case EntityReferenceScheme::AllReferenceLast: + component->m_entityReference = entities[numElements - 1]->GetId(); + break; + case EntityReferenceScheme::AllReferenceThemselves: + component->m_entityReference = entities[i]->GetId(); + break; + case EntityReferenceScheme::AllReferenceNextCircular: + component->m_entityReference = entities[(i + 1) % numElements]->GetId(); + break; + case EntityReferenceScheme::AllReferencePreviousCircular: + component->m_entityReference = entities[(i + numElements - 1) % numElements]->GetId(); + break; + } + } + } + + // Verify that the entity references are pointing to the correct other entities within the same spawn batch. + // A "spawn batch" is the set of entities produced for each SpawnAllEntities command. + void ValidateEntityReferences( + EntityReferenceScheme refScheme, size_t entitiesPerBatch, AzFramework::SpawnableConstEntityContainerView entities) + { + size_t numElements = entities.size(); + + for (size_t i = 0; i < numElements; ++i) + { + // Calculate the element offset that's the start of each batch of entities spawned. + size_t curSpawnBatch = i / entitiesPerBatch; + size_t curBatchOffset = curSpawnBatch * entitiesPerBatch; + size_t curBatchIndex = i - curBatchOffset; + + const AZ::Entity* const entity = *(entities.begin() + i); + + auto component = entity->FindComponent(); + ASSERT_NE(nullptr, component); + AZ::EntityId comparisonId; + // Ids should be local to a batch, so each of these will be compared within a batch of entities, not globally across + // the entire set. + switch (refScheme) + { + case EntityReferenceScheme::AllReferenceFirst: + // Compare against the first entity in each batch + comparisonId = (*(entities.begin() + curBatchOffset))->GetId(); + break; + case EntityReferenceScheme::AllReferenceLast: + // Compare against the last entity in each batch + comparisonId = (*(entities.begin() + curBatchOffset + (entitiesPerBatch - 1)))->GetId(); + break; + case EntityReferenceScheme::AllReferenceThemselves: + // Compare against itself + comparisonId = entity->GetId(); + break; + case EntityReferenceScheme::AllReferenceNextCircular: + // Compare against the next entity in each batch, looping around so that the last entity in the batch should refer + // to the first entity in the batch. + comparisonId = (*(entities.begin() + curBatchOffset + ((curBatchIndex + 1) % entitiesPerBatch)))->GetId(); + break; + case EntityReferenceScheme::AllReferencePreviousCircular: + // Compare against the previous entity in each batch, looping around so that the first entity in the batch should refer + // to the last entity in the batch. + comparisonId = (*(entities.begin() + curBatchOffset + ((curBatchIndex + numElements - 1) % entitiesPerBatch)))->GetId(); + break; + } + EXPECT_EQ(comparisonId, component->m_entityReference); + } + }; + protected: AZ::Data::Asset* m_spawnableAsset { nullptr }; AzFramework::SpawnableEntitiesManager* m_manager { nullptr }; @@ -185,6 +303,73 @@ namespace UnitTest m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } + TEST_F(SpawnableEntitiesManagerTest, SpawnAllEntities_AllEntitiesReferenceOtherEntities_EntityIdsAreMappedCorrectly) + { + // This tests that entity id references get mapped correctly in a SpawnAllEntities call whether they're forward referencing + // in the list, backwards referencing, or self-referencing. The circular tests are to ensure the implementation works regardless + // of entity ordering. + for (EntityReferenceScheme refScheme : { + EntityReferenceScheme::AllReferenceFirst, EntityReferenceScheme::AllReferenceLast, + EntityReferenceScheme::AllReferenceThemselves, EntityReferenceScheme::AllReferenceNextCircular, + EntityReferenceScheme::AllReferencePreviousCircular }) + { + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = [this, refScheme, NumEntities] + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + ValidateEntityReferences(refScheme, NumEntities, entities); + }; + AzFramework::SpawnAllEntitiesOptionalArgs optionalArgs; + optionalArgs.m_completionCallback = AZStd::move(callback); + m_manager->SpawnAllEntities(*m_ticket, AZStd::move(optionalArgs)); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + } + + TEST_F(SpawnableEntitiesManagerTest, SpawnAllEntities_AllEntitiesReferenceOtherEntities_EntityIdsOnlyReferWithinASingleCall) + { + // This tests that entity id references get mapped correctly with multiple SpawnAllEntities calls. Each call should only map + // the entities to other entities within the same call, regardless of forward or backward mapping. + // For example, suppose entities 1, 2, and 3 refer to 4. In the first SpawnAllEntities call, entities 1-3 will refer to 4. + // In the second SpawnAllEntities call, entities 1-3 will refer to the second 4, not the previously-spawned 4. + for (EntityReferenceScheme refScheme : + { EntityReferenceScheme::AllReferenceFirst, EntityReferenceScheme::AllReferenceLast, + EntityReferenceScheme::AllReferenceThemselves, EntityReferenceScheme::AllReferenceNextCircular, + EntityReferenceScheme::AllReferencePreviousCircular + }) + { + // Make sure we start with a fresh ticket each time, or else each iteration through this loop would continue to build up + // more and more entities. + delete m_ticket; + m_ticket = new AzFramework::EntitySpawnTicket(*m_spawnableAsset); + + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = [this, refScheme, NumEntities] + (AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + ValidateEntityReferences(refScheme, NumEntities, entities); + }; + + // Spawn twice. + constexpr size_t NumSpawnAllCalls = 2; + for (int spawns = 0; spawns < NumSpawnAllCalls; spawns++) + { + AzFramework::SpawnAllEntitiesOptionalArgs optionalArgs; + optionalArgs.m_completionCallback = AZStd::move(callback); + m_manager->SpawnAllEntities(*m_ticket, AZStd::move(optionalArgs)); + } + + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + } + TEST_F(SpawnableEntitiesManagerTest, SpawnAllEntities_DeleteTicketBeforeCall_NoCrash) { { @@ -363,6 +548,180 @@ namespace UnitTest m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); } + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_AllEntitiesReferenceOtherEntities_ForwardReferencesWorkInSingleCall) + { + constexpr EntityReferenceScheme refScheme = EntityReferenceScheme::AllReferenceNextCircular; + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = + [this, refScheme, NumEntities](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + ValidateEntityReferences(refScheme, NumEntities, entities); + }; + + // Verify that by default, entities that refer to other entities that haven't been spawned yet have the correct references + // when the spawning all occurs in the same call + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3 }); + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_AllEntitiesReferenceOtherEntities_ForwardReferencesWorkAcrossCalls) + { + constexpr EntityReferenceScheme refScheme = EntityReferenceScheme::AllReferenceNextCircular; + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = + [this, refScheme, NumEntities](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + ValidateEntityReferences(refScheme, NumEntities, entities); + }; + + // Verify that by default, entities that refer to other entities that haven't been spawned yet have the correct references + // even when the spawning is across multiple calls + m_manager->SpawnEntities(*m_ticket, { 0 }); + m_manager->SpawnEntities(*m_ticket, { 1 }); + m_manager->SpawnEntities(*m_ticket, { 2 }); + m_manager->SpawnEntities(*m_ticket, { 3 }); + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_AllEntitiesReferenceOtherEntities_ReferencesPointToFirstOrLatest) + { + // With SpawnEntities, entity references should either refer to the first entity that *will* be spawned, or the last entity + // that *has* been spawned. This test will create entities 0 1 2 3 that all refer to entity 3, and it will create two batches + // of those. In the first batch, they'll forward-reference. In the second batch, they should backward-reference, except for + // the second entity 3, which will now refer to itself as the last one that's been spawned. + constexpr EntityReferenceScheme refScheme = EntityReferenceScheme::AllReferenceLast; + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = + [this, refScheme, NumEntities](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + size_t numElements = entities.size(); + + for (size_t i = 0; i < numElements; ++i) + { + const AZ::Entity* const entity = *(entities.begin() + i); + + auto component = entity->FindComponent(); + ASSERT_NE(nullptr, component); + AZ::EntityId comparisonId; + if (i < (numElements - 1)) + { + // There are two batches of NumEntities elements. Every entity should either forward-reference or backward-reference + // to the last entity of the first batch, except for the very last entity of the second batch, which should reference + // itself. + comparisonId = (*(entities.begin() + (NumEntities- 1)))->GetId(); + } + else + { + // The very last entity of the second batch should reference itself because it's now the latest instance of that + // entity to be spawned. + comparisonId = entity->GetId(); + } + + EXPECT_EQ(comparisonId, component->m_entityReference); + } + }; + + // Create 2 batches of forward references. In the first batch, entities 0 1 2 will point forward to 3. In the second batch, + // entities 0 1 2 will point *backward* to the first 3, and the second entity 3 will point to itself. + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3 }); + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3 }); + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_AllEntitiesReferenceOtherEntities_MultipleSpawnsInSameCallReferenceCorrectly) + { + // With SpawnEntities, entity references should either refer to the first entity that *will* be spawned, or the last entity + // that *has* been spawned. This test will create entities 0 1 2 3 that all refer to entity 3, and it will create three sets + // of those in the same call, with the following results: + // - The first 0 1 2 will forward-reference to the first 3 + // - The first 3 will reference itself + // - The second 0 1 2 will backwards-reference to the first 3 + // - The second 3 will reference itself + // - The third 0 1 2 will backwards-reference to the second 3 + // - The third 3 will reference itself + constexpr EntityReferenceScheme refScheme = EntityReferenceScheme::AllReferenceLast; + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = + [this, refScheme, NumEntities](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + size_t numElements = entities.size(); + + for (size_t i = 0; i < numElements; ++i) + { + const AZ::Entity* const entity = *(entities.begin() + i); + + auto component = entity->FindComponent(); + ASSERT_NE(nullptr, component); + AZ::EntityId comparisonId; + + if (i < ((NumEntities * 2) - 1)) + { + // The first 7 entities (0 1 2 3 0 1 2) will all refer to the 4th one (1st '3'). + comparisonId = (*(entities.begin() + (NumEntities - 1)))->GetId(); + } + else if (i < (numElements - 1)) + { + // The next 4 entities (3 0 1 2) will all refer to the 8th one (2nd '3'). + comparisonId = (*(entities.begin() + ((NumEntities * 2) - 1)))->GetId(); + } + else + { + // The very last entity (3) will reference itself (3rd '3'). + comparisonId = entity->GetId(); + } + + EXPECT_EQ(comparisonId, component->m_entityReference); + } + }; + + // Create the 3 batches of entities 0, 1, 2, 3. The entity references should work as described at the top of the test. + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3, 0, 1, 2, 3, 0, 1, 2, 3 }); + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_AllEntitiesReferenceOtherEntities_OptionalFlagClearsReferenceMap) + { + constexpr EntityReferenceScheme refScheme = EntityReferenceScheme::AllReferenceLast; + constexpr size_t NumEntities = 4; + FillSpawnable(NumEntities); + CreateEntityReferences(refScheme); + + auto callback = + [this, refScheme, NumEntities](AzFramework::EntitySpawnTicket::Id, AzFramework::SpawnableConstEntityContainerView entities) + { + ValidateEntityReferences(refScheme, NumEntities, entities); + }; + + // By setting the "referencePreviouslySpawnedEntities" flag to false, the map will get cleared on each call, so in both batches + // the entities will forward-reference to the last entity in the batch. If the flag were true, entities 0 1 2 in the second + // batch would refer backwards to the first entity 3. + + AzFramework::SpawnEntitiesOptionalArgs optionalArgsSecondBatch; + optionalArgsSecondBatch.m_completionCallback = AZStd::move(callback); + optionalArgsSecondBatch.m_referencePreviouslySpawnedEntities = false; + + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3 }, optionalArgsSecondBatch); + m_manager->SpawnEntities(*m_ticket, { 0, 1, 2, 3 }, AZStd::move(optionalArgsSecondBatch)); + m_manager->ListEntities(*m_ticket, callback); + m_manager->ProcessQueue(AzFramework::SpawnableEntitiesManager::CommandQueuePriority::Regular); + } + TEST_F(SpawnableEntitiesManagerTest, SpawnEntities_DeleteTicketBeforeCall_NoCrash) { { diff --git a/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.h b/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.h index 3ad20b8f6a..cbf41f6bf8 100644 --- a/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.h +++ b/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.h @@ -17,7 +17,6 @@ #include #include #include -#include namespace Vegetation { @@ -28,7 +27,6 @@ namespace Vegetation template class EditorVegetationComponentBase : public LmbrCentral::EditorWrappedComponentBase - , private CrySystemEventBus::Handler { public: using BaseClassType = LmbrCentral::EditorWrappedComponentBase; @@ -48,11 +46,6 @@ namespace Vegetation static void Reflect(AZ::ReflectContext* context); - //////////////////////////////////////////////////////////////////////////// - // CrySystemEvents - void OnCryEditorBeginLevelExport() override; - void OnCryEditorEndLevelExport(bool /*success*/) override; - protected: using BaseClassType::m_configuration; using BaseClassType::m_component; diff --git a/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.inl b/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.inl index 3a5afd1c27..93c253f212 100644 --- a/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.inl +++ b/Gems/Vegetation/Code/Include/Vegetation/Editor/EditorVegetationComponentBase.inl @@ -12,24 +12,6 @@ namespace Vegetation { - template - void EditorVegetationComponentBase::OnCryEditorEndLevelExport(bool /*success*/) - { - // Restore the activation state of our components after the export is complete. - if (m_visible) - { - m_component.Activate(); - } - } - - template - void EditorVegetationComponentBase::OnCryEditorBeginLevelExport() - { - // We need to deactivate our game components at the start of level exports because any vegetation meshes that are loaded - // or instances that are spawned can end up in our static vegetation level data. - m_component.Deactivate(); - } - template AZ::u32 EditorVegetationComponentBase::ConfigurationChanged() { @@ -69,13 +51,11 @@ namespace Vegetation { GradientSignal::SetSamplerOwnerEntity(m_configuration, GetEntityId()); BaseClassType::Activate(); - CrySystemEventBus::Handler::BusConnect(); } template void EditorVegetationComponentBase::Deactivate() { - CrySystemEventBus::Handler::BusDisconnect(); BaseClassType::Deactivate(); }