From c9301a8c947f3f21ca1789eed84ea28fb5976e4d Mon Sep 17 00:00:00 2001 From: srikappa-amzn <82230713+srikappa-amzn@users.noreply.github.com> Date: Thu, 29 Jul 2021 12:04:04 -0700 Subject: [PATCH] Fix a bug in patching templates and improve error messaging (#2394) * Fix a bug in patching templates and improve error messaging * Fixed incorrect order of function arguments * Fix build errors regarding c style strings and casting entity ids Signed-off-by: srikappa-amzn --- .../Instance/InstanceToTemplatePropagator.cpp | 2 +- .../Prefab/PrefabPublicHandler.cpp | 59 +++++++++++++------ .../Prefab/PrefabPublicHandler.h | 7 ++- 3 files changed, 46 insertions(+), 22 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp index c7bf72ff7b..8f93ebb6df 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/Instance/InstanceToTemplatePropagator.cpp @@ -177,7 +177,7 @@ namespace AzToolsFramework PrefabDomUtils::ApplyPatches(templateDomReference, templateDomReference.GetAllocator(), providedPatch); //trigger propagation - if (result.GetOutcome() != AZ::JsonSerializationResult::Outcomes::Success) + if (result.GetProcessing() != AZ::JsonSerializationResult::Processing::Completed) { AZ_Error("Prefab", false, "Patch was not successfully applied."); return false; diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index 16db933192..a03e48062c 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -90,10 +90,11 @@ namespace AzToolsFramework AZStd::unordered_map nestedInstanceLinkPatchesMap; // Retrieve all entities affected and identify Instances - if (!RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonRootEntityOwningInstance->get(), entities, instances)) + PrefabOperationResult retrieveEntitiesAndInstancesOutcome = RetrieveAndSortPrefabEntitiesAndInstances( + inputEntityList, commonRootEntityOwningInstance->get(), entities, instances); + if (!retrieveEntitiesAndInstancesOutcome.IsSuccess()) { - return AZ::Failure( - AZStd::string("Could not create a new prefab out of the entities provided - invalid selection.")); + return retrieveEntitiesAndInstancesOutcome; } AZStd::unordered_map oldEntityAliases; @@ -646,7 +647,12 @@ namespace AzToolsFramework { // Retrieve all nested instances that are part of the subtree under the current entity. EntityList entities; - RetrieveAndSortPrefabEntitiesAndInstances({ entity }, beforeOwningInstance->get(), entities, instancesInvolved); + PrefabOperationResult retrieveEntitiesAndInstancesOutcome = RetrieveAndSortPrefabEntitiesAndInstances( + { entity }, beforeOwningInstance->get(), entities, instancesInvolved); + if (!retrieveEntitiesAndInstancesOutcome.IsSuccess()) + { + return retrieveEntitiesAndInstancesOutcome; + } } for (Instance* instance : instancesInvolved) @@ -748,7 +754,9 @@ namespace AzToolsFramework AZStd::vector instances; // Retrieve all descendant entities and instances of this entity that belonged to the same owning instance. - RetrieveAndSortPrefabEntitiesAndInstances({ entity }, beforeOwningInstance->get(), entities, instances); + PrefabOperationResult retrieveEntitiesAndInstancesOutcome = RetrieveAndSortPrefabEntitiesAndInstances( + { entity }, beforeOwningInstance->get(), entities, instances); + AZ_Error("Prefab", retrieveEntitiesAndInstancesOutcome.IsSuccess(), retrieveEntitiesAndInstancesOutcome.GetError().data()); AZStd::vector> instanceUniquePtrs; AZStd::vector> instancePatches; @@ -981,11 +989,12 @@ namespace AzToolsFramework AZStd::vector instances; EntityList inputEntityList = EntityIdSetToEntityList(duplicationSet); - bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); + PrefabOperationResult retrieveEntitiesAndInstancesOutcome = + RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); - if (!success) + if (!retrieveEntitiesAndInstancesOutcome.IsSuccess()) { - return AZ::Failure(AZStd::string("Failed to retrieve entities and instances from the given list of entity ids for duplication")); + return AZStd::move(retrieveEntitiesAndInstancesOutcome); } // Take a snapshot of the instance DOM before we manipulate it @@ -1128,11 +1137,12 @@ namespace AzToolsFramework AZStd::vector entities; AZStd::vector instances; - bool success = RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); + PrefabOperationResult retrieveEntitiesAndInstancesOutcome = + RetrieveAndSortPrefabEntitiesAndInstances(inputEntityList, commonOwningInstance->get(), entities, instances); - if (!success) + if (!retrieveEntitiesAndInstancesOutcome.IsSuccess()) { - return AZ::Failure(AZStd::string("DeleteEntitiesAndAllDescendantsInInstance")); + return AZStd::move(retrieveEntitiesAndInstancesOutcome); } for (AZ::Entity* entity : entities) @@ -1405,13 +1415,16 @@ namespace AzToolsFramework return nullptr; } - bool PrefabPublicHandler::RetrieveAndSortPrefabEntitiesAndInstances( - const EntityList& inputEntities, Instance& commonRootEntityOwningInstance, - EntityList& outEntities, AZStd::vector& outInstances) const + PrefabOperationResult PrefabPublicHandler::RetrieveAndSortPrefabEntitiesAndInstances( + const EntityList& inputEntities, + Instance& commonRootEntityOwningInstance, + EntityList& outEntities, + AZStd::vector& outInstances) const { if (inputEntities.size() == 0) { - return false; + return AZ::Failure( + AZStd::string("An empty list of input entities is provided to retrieve the prefab entities and instances.")); } AZStd::queue entityQueue; @@ -1438,8 +1451,8 @@ namespace AzToolsFramework AZ_Assert( owningInstance.has_value(), "An error occurred while retrieving entities and prefab instances : " - "Owning instance of entity with id '%llu' couldn't be found", - entity->GetId()); + "Owning instance of entity with name '%s' and id '%llu' couldn't be found", + entity->GetName().c_str(), static_cast(entity->GetId())); // Check if this entity is owned by the same instance owning the root. if (&owningInstance->get() == &commonRootEntityOwningInstance) @@ -1480,7 +1493,10 @@ namespace AzToolsFramework else { // This can only happen if one entity does not share the common root! - return false; + return AZ::Failure(AZStd::string::format( + "Entity with name '%s' and id '%llu' has an owning instance that doesn't belong to the instance " + "hierarchy of the selected entities.", + entity->GetName().c_str(), static_cast(entity->GetId()))); } } } @@ -1501,7 +1517,12 @@ namespace AzToolsFramework outInstances.push_back(instancePtr); } - return (outEntities.size() + outInstances.size()) > 0; + if ((outEntities.size() + outInstances.size()) == 0) + { + return AZ::Failure( + AZStd::string("An empty list of entities and prefab instances were retrieved from the selected entities")); + } + return AZ::Success(); } EntityIdList PrefabPublicHandler::GenerateEntityIdListWithoutLevelInstance( diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index f0c88a7a79..0e24b0841d 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -64,8 +64,11 @@ namespace AzToolsFramework private: PrefabOperationResult DeleteFromInstance(const EntityIdList& entityIds, bool deleteDescendants); - bool RetrieveAndSortPrefabEntitiesAndInstances(const EntityList& inputEntities, Instance& commonRootEntityOwningInstance, - EntityList& outEntities, AZStd::vector& outInstances) const; + PrefabOperationResult RetrieveAndSortPrefabEntitiesAndInstances( + const EntityList& inputEntities, + Instance& commonRootEntityOwningInstance, + EntityList& outEntities, + AZStd::vector& outInstances) const; EntityIdList GenerateEntityIdListWithoutLevelInstance(const EntityIdList& entityIds) const; InstanceOptionalReference GetOwnerInstanceByEntityId(AZ::EntityId entityId) const;