From 3fd2f1305f2f8c6de2982570763efd37783264f2 Mon Sep 17 00:00:00 2001 From: rhongAMZ <69218254+rhongAMZ@users.noreply.github.com> Date: Wed, 9 Jun 2021 09:15:09 -0700 Subject: [PATCH 1/2] Selecting and deleting the level prefab root entity crashes editor (#1179) Early remove the level instance from the entity id list in the delete function and duplicate function. --- .../Prefab/PrefabPublicHandler.cpp | 40 +++++++++++++++---- .../Prefab/PrefabPublicHandler.h | 1 + 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp index fcdbc5ce07..690d408007 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.cpp @@ -901,7 +901,13 @@ namespace AzToolsFramework return AZ::Failure(AZStd::string("No entities to duplicate.")); } - if (!EntitiesBelongToSameInstance(entityIds)) + const EntityIdList entityIdsNoLevelInstance = GenerateEntityIdListWithoutLevelInstance(entityIds); + if (entityIdsNoLevelInstance.empty()) + { + return AZ::Failure(AZStd::string("No entities to duplicate because only instance selected is the level instance.")); + } + + if (!EntitiesBelongToSameInstance(entityIdsNoLevelInstance)) { return AZ::Failure(AZStd::string("Cannot duplicate multiple entities belonging to different instances with one operation." "Change your selection to contain entities in the same instance.")); @@ -909,7 +915,7 @@ namespace AzToolsFramework // We've already verified the entities are all owned by the same instance, // so we can just retrieve our instance from the first entity in the list. - AZ::EntityId firstEntityIdToDuplicate = entityIds[0]; + AZ::EntityId firstEntityIdToDuplicate = entityIdsNoLevelInstance[0]; InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDuplicate); if (!commonOwningInstance.has_value()) { @@ -929,7 +935,7 @@ namespace AzToolsFramework // This will cull out any entities that have ancestors in the list, since we will end up duplicating // the full nested hierarchy with what is returned from RetrieveAndSortPrefabEntitiesAndInstances - AzToolsFramework::EntityIdSet duplicationSet = AzToolsFramework::GetCulledEntityHierarchy(entityIds); + AzToolsFramework::EntityIdSet duplicationSet = AzToolsFramework::GetCulledEntityHierarchy(entityIdsNoLevelInstance); AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); @@ -1004,17 +1010,19 @@ namespace AzToolsFramework PrefabOperationResult PrefabPublicHandler::DeleteFromInstance(const EntityIdList& entityIds, bool deleteDescendants) { - if (entityIds.empty()) + const EntityIdList entityIdsNoLevelInstance = GenerateEntityIdListWithoutLevelInstance(entityIds); + + if (entityIdsNoLevelInstance.empty()) { return AZ::Success(); } - if (!EntitiesBelongToSameInstance(entityIds)) + if (!EntitiesBelongToSameInstance(entityIdsNoLevelInstance)) { return AZ::Failure(AZStd::string("Cannot delete multiple entities belonging to different instances with one operation.")); } - AZ::EntityId firstEntityIdToDelete = entityIds[0]; + AZ::EntityId firstEntityIdToDelete = entityIdsNoLevelInstance[0]; InstanceOptionalReference commonOwningInstance = GetOwnerInstanceByEntityId(firstEntityIdToDelete); // If the first entity id is a container entity id, then we need to mark its parent as the common owning instance because you @@ -1025,7 +1033,7 @@ namespace AzToolsFramework } // Retrieve entityList from entityIds - EntityList inputEntityList = EntityIdListToEntityList(entityIds); + EntityList inputEntityList = EntityIdListToEntityList(entityIdsNoLevelInstance); AZ_PROFILE_FUNCTION(AZ::Debug::ProfileCategory::AzToolsFramework); @@ -1081,7 +1089,7 @@ namespace AzToolsFramework } else { - for (AZ::EntityId entityId : entityIds) + for (AZ::EntityId entityId : entityIdsNoLevelInstance) { InstanceOptionalReference owningInstance = m_instanceEntityMapperInterface->FindOwningInstance(entityId); // If this is the container entity, it actually represents the instance so get its owner @@ -1437,6 +1445,22 @@ namespace AzToolsFramework return (outEntities.size() + outInstances.size()) > 0; } + EntityIdList PrefabPublicHandler::GenerateEntityIdListWithoutLevelInstance( + const EntityIdList& entityIds) const + { + EntityIdList outEntityIds; + outEntityIds.reserve(entityIds.size()); // Actual size could be smaller. + + for (const AZ::EntityId& entityId : entityIds) + { + if (!IsLevelInstanceContainerEntity(entityId)) + { + outEntityIds.emplace_back(entityId); + } + } + return outEntityIds; + } + bool PrefabPublicHandler::EntitiesBelongToSameInstance(const EntityIdList& entityIds) const { if (entityIds.size() <= 1) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h index f3d778d8de..65e1391722 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabPublicHandler.h @@ -70,6 +70,7 @@ namespace AzToolsFramework PrefabOperationResult DeleteFromInstance(const EntityIdList& entityIds, bool deleteDescendants); bool RetrieveAndSortPrefabEntitiesAndInstances(const EntityList& inputEntities, Instance& commonRootEntityOwningInstance, EntityList& outEntities, AZStd::vector& outInstances) const; + EntityIdList GenerateEntityIdListWithoutLevelInstance(const EntityIdList& entityIds) const; InstanceOptionalReference GetOwnerInstanceByEntityId(AZ::EntityId entityId) const; bool EntitiesBelongToSameInstance(const EntityIdList& entityIds) const; From 4241eb9c2cec0d525bf0b3ff5ea2c616268e65cf Mon Sep 17 00:00:00 2001 From: amzn-sean <75276488+amzn-sean@users.noreply.github.com> Date: Wed, 9 Jun 2021 17:42:16 +0100 Subject: [PATCH 2/2] fix crash in ragdoll (#1210) --- .../Source/PhysXCharacters/API/Ragdoll.cpp | 22 ++++++++++++++++--- .../Code/Source/PhysXCharacters/API/Ragdoll.h | 2 ++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.cpp b/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.cpp index 02ebe7281c..4725212a9d 100644 --- a/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.cpp +++ b/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -40,6 +41,8 @@ namespace PhysX } // namespace Internal // PhysX::Ragdoll + /*static*/ AZStd::mutex Ragdoll::m_sceneEventMutex; + void Ragdoll::Reflect(AZ::ReflectContext* context) { AZ::SerializeContext* serializeContext = azrtti_cast(context); @@ -114,7 +117,10 @@ namespace PhysX Ragdoll::~Ragdoll() { - m_sceneStartSimHandler.Disconnect(); + { + AZStd::scoped_lock lock(m_sceneEventMutex); + m_sceneStartSimHandler.Disconnect(); + } m_nodes.clear(); //the nodes destructor will remove the simulated body from the scene. } @@ -212,7 +218,13 @@ namespace PhysX } } - sceneInterface->RegisterSceneSimulationStartHandler(m_sceneOwner, m_sceneStartSimHandler); + // the handler is also connected in EnableSimulationQueued(), + // which will call this function, so if called from that path dont connect here. + if (!m_sceneStartSimHandler.IsConnected()) + { + AZStd::scoped_lock lock(m_sceneEventMutex); + sceneInterface->RegisterSceneSimulationStartHandler(m_sceneOwner, m_sceneStartSimHandler); + } sceneInterface->EnableSimulationOfBody(m_sceneOwner, m_bodyHandle); } @@ -225,6 +237,7 @@ namespace PhysX if (auto* sceneInterface = AZ::Interface::Get()) { + AZStd::scoped_lock lock(m_sceneEventMutex); sceneInterface->RegisterSceneSimulationStartHandler(m_sceneOwner, m_sceneStartSimHandler); } @@ -244,7 +257,10 @@ namespace PhysX return; } - m_sceneStartSimHandler.Disconnect(); + { + AZStd::scoped_lock lock(m_sceneEventMutex); + m_sceneStartSimHandler.Disconnect(); + } physx::PxScene* pxScene = Internal::GetPxScene(m_sceneOwner); const size_t numNodes = m_nodes.size(); diff --git a/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.h b/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.h index 7182a3a44c..006ce5878b 100644 --- a/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.h +++ b/Gems/PhysX/Code/Source/PhysXCharacters/API/Ragdoll.h @@ -12,6 +12,7 @@ #pragma once +#include #include #include #include @@ -84,5 +85,6 @@ namespace PhysX bool m_queuedDisableSimulation = false; AzPhysics::SceneEvents::OnSceneSimulationStartHandler m_sceneStartSimHandler; + static AZStd::mutex m_sceneEventMutex; }; } // namespace PhysX