From a766e3af5c08a63e2f6edcddae26376ef8b57dd4 Mon Sep 17 00:00:00 2001 From: chcurran Date: Tue, 8 Jun 2021 13:54:25 -0700 Subject: [PATCH 1/6] Fixes for internal if-branch node parser bug (LYN-4347) and exposing properties for AZStd::tuple (LYN-3910) --- .../AzCore/RTTI/AzStdOnDemandPrettyName.inl | 51 +- .../AzCore/RTTI/AzStdOnDemandReflection.inl | 24 +- .../Grammar/AbstractCodeModel.cpp | 32 +- ...ionIfBranchWithConnectedInput.scriptcanvas | 2499 +++++++++++++++++ .../Tests/ScriptCanvas_RuntimeInterpreted.cpp | 5 + 5 files changed, 2588 insertions(+), 23 deletions(-) create mode 100644 Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseFunctionIfBranchWithConnectedInput.scriptcanvas diff --git a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandPrettyName.inl b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandPrettyName.inl index 26e269573d..4f4a21301c 100644 --- a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandPrettyName.inl +++ b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandPrettyName.inl @@ -161,7 +161,56 @@ namespace AZ return "A pair is an fixed size collection of two elements."; } }; - + + template + void GetTypeNamesFold(AZStd::vector& result, AZ::BehaviorContext& context) + { + result.push_back(OnDemandPrettyName::Get(context)); + }; + + template + void GetTypeNames(AZStd::vector& result, AZ::BehaviorContext& context) + { + (GetTypeNamesFold(result, context), ...); + }; + + template + void GetTypeNamesFold(AZStd::string& result, AZ::BehaviorContext& context) + { + if (!result.empty()) + { + result += ", "; + } + + result += OnDemandPrettyName::Get(context); + }; + + template + void GetTypeNames(AZStd::string& result, AZ::BehaviorContext& context) + { + (GetTypeNamesFold(result, context), ...); + }; + + template + struct OnDemandPrettyName> + { + static AZStd::string Get(AZ::BehaviorContext& context) + { + AZStd::string typeNames; + GetTypeNames(typeNames, context); + return AZStd::string::format("Tuple<%s>", typeNames.c_str()); + } + }; + + template + struct OnDemandToolTip> + { + static AZStd::string Get(AZ::BehaviorContext&) + { + return "A tuple is an fixed size collection of any number of any type of element."; + } + }; + template struct OnDemandPrettyName< AZStd::unordered_map > { diff --git a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl index 537d3e5c83..2132dff3c4 100644 --- a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl +++ b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl @@ -813,20 +813,27 @@ namespace AZ { using ContainerType = AZStd::tuple; - template - static void ReflectUnpackMethodFold(BehaviorContext::ClassBuilder& builder) + template + static void ReflectUnpackMethodFold(BehaviorContext::ClassBuilder& builder, const AZStd::vector& typeNames, [[maybe_unused]] size_t inputIndex) { const AZStd::string methodName = AZStd::string::format("Get%zu", Index); - builder->Method(methodName.data(), [](ContainerType& value) { return AZStd::get(value); }) - ->Attribute(AZ::Script::Attributes::ExcludeFrom, AZ::Script::Attributes::ExcludeFlags::All) + builder->Method(methodName.data(), [](ContainerType& thisPointer) { return AZStd::get(thisPointer); }) + ->Attribute(AZ::Script::Attributes::ExcludeFrom, AZ::Script::Attributes::ExcludeFlags::List) ->Attribute(AZ::ScriptCanvasAttributes::TupleGetFunctionIndex, Index) ; + + builder->Property + ( AZStd::string::format("element_%zu_%s", Index, typeNames[Index].c_str()).c_str() + , [](ContainerType& thisPointer) { return AZStd::get(thisPointer); } + , [](ContainerType& thisPointer, const t_Arg& element) { AZStd::get(thisPointer) = element; }); } - template + template static void ReflectUnpackMethods(BehaviorContext::ClassBuilder& builder, AZStd::index_sequence) { - (ReflectUnpackMethodFold(builder), ...); + AZStd::vector typeNames; + ScriptCanvasOnDemandReflection::GetTypeNames(typeNames, *builder.m_context); + (ReflectUnpackMethodFold(builder, typeNames, Indices), ...); } static void Reflect(ReflectContext* context) @@ -851,9 +858,10 @@ namespace AZ ->Attribute(AZ::ScriptCanvasAttributes::TupleConstructorFunction, constructorHolder) ; - ReflectUnpackMethods(builder, AZStd::make_index_sequence{}); + ReflectUnpackMethods(builder, AZStd::make_index_sequence{}); + builder->Method("GetSize", []() { return AZStd::tuple_size::value; }) - ->Attribute(AZ::Script::Attributes::ExcludeFrom, AZ::Script::Attributes::ExcludeFlags::All) + ->Attribute(AZ::Script::Attributes::ExcludeFrom, AZ::Script::Attributes::ExcludeFlags::List) ; } } diff --git a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp index 2a485c9501..bc07d16a7e 100644 --- a/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp +++ b/Gems/ScriptCanvas/Code/Include/ScriptCanvas/Grammar/AbstractCodeModel.cpp @@ -3356,22 +3356,26 @@ namespace ScriptCanvas if (executionIf->GetId().m_node->IsIfBranchPrefacedWithBooleanExpression()) { - auto removeChildOutcome = RemoveChild(executionIf->ModParent(), executionIf); - if (!removeChildOutcome.IsSuccess()) - { - AddError(executionIf->GetNodeId(), executionIf, ScriptCanvas::ParseErrors::FailedToRemoveChild); - } + ExecutionTreePtr booleanExpression; - if (!IsErrorFree()) { - return; - } + auto removeChildOutcome = RemoveChild(executionIf->ModParent(), executionIf); + if (!removeChildOutcome.IsSuccess()) + { + AddError(executionIf->GetNodeId(), executionIf, ScriptCanvas::ParseErrors::FailedToRemoveChild); + } + + if (!IsErrorFree()) + { + return; + } - const auto indexAndChild = removeChildOutcome.TakeValue(); + const auto indexAndChild = removeChildOutcome.TakeValue(); - ExecutionTreePtr booleanExpression = CreateChild(executionIf->ModParent(), executionIf->GetId().m_node, executionIf->GetId().m_slot); - executionIf->ModParent()->InsertChild(indexAndChild.first, { indexAndChild.second.m_slot, indexAndChild.second.m_output, booleanExpression }); - executionIf->SetParent(booleanExpression); + booleanExpression = CreateChild(executionIf->ModParent(), executionIf->GetId().m_node, executionIf->GetId().m_slot); + executionIf->ModParent()->InsertChild(indexAndChild.first, { indexAndChild.second.m_slot, indexAndChild.second.m_output, booleanExpression }); + executionIf->SetParent(booleanExpression); + } // make a condition here auto symbol = CheckLogicalExpressionSymbol(booleanExpression); @@ -3402,7 +3406,7 @@ namespace ScriptCanvas return; } - const auto indexAndChild2 = removeChildOutcome.TakeValue(); + const auto indexAndChild2 = removeChildOutcome2.TakeValue(); // parse if statement internal function ExecutionTreePtr internalFunction = CreateChild(booleanExpression->ModParent(), booleanExpression->GetId().m_node, booleanExpression->GetId().m_slot); @@ -4782,7 +4786,7 @@ namespace ScriptCanvas { PropertyExtractionPtr extraction = AZStd::make_shared(); extraction->m_slot = slot; - extraction->m_name = propertyField.first; + extraction->m_name = AZ::ReplaceCppArtifacts(propertyField.first); execution->AddPropertyExtractionSource(slot, extraction); } else diff --git a/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseFunctionIfBranchWithConnectedInput.scriptcanvas b/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseFunctionIfBranchWithConnectedInput.scriptcanvas new file mode 100644 index 0000000000..38df5159c2 --- /dev/null +++ b/Gems/ScriptCanvasTesting/Assets/ScriptCanvas/UnitTests/LY_SC_UnitTest_ParseFunctionIfBranchWithConnectedInput.scriptcanvas @@ -0,0 +1,2499 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp b/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp index 1633b0fc51..5bda0f3941 100644 --- a/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp +++ b/Gems/ScriptCanvasTesting/Code/Tests/ScriptCanvas_RuntimeInterpreted.cpp @@ -90,6 +90,11 @@ public: } }; +TEST_F(ScriptCanvasTestFixture, ParseFunctionIfBranchWithConnectedInput) +{ + RunUnitTestGraph("LY_SC_UnitTest_ParseFunctionIfBranchWithConnectedInput"); +} + TEST_F(ScriptCanvasTestFixture, UseRawBehaviorProperties) { RunUnitTestGraph("LY_SC_UnitTest_UseRawBehaviorProperties"); From 8cb6ef0721bb295b74bbf5c72c3bd1bc1d83ff86 Mon Sep 17 00:00:00 2001 From: chcurran Date: Tue, 8 Jun 2021 16:39:36 -0700 Subject: [PATCH 2/6] Fix tmeplate arg names --- .../AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl index 2132dff3c4..9bda5cdcce 100644 --- a/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl +++ b/Code/Framework/AzCore/AzCore/RTTI/AzStdOnDemandReflection.inl @@ -813,8 +813,8 @@ namespace AZ { using ContainerType = AZStd::tuple; - template - static void ReflectUnpackMethodFold(BehaviorContext::ClassBuilder& builder, const AZStd::vector& typeNames, [[maybe_unused]] size_t inputIndex) + template + static void ReflectUnpackMethodFold(BehaviorContext::ClassBuilder& builder, const AZStd::vector& typeNames) { const AZStd::string methodName = AZStd::string::format("Get%zu", Index); builder->Method(methodName.data(), [](ContainerType& thisPointer) { return AZStd::get(thisPointer); }) @@ -825,15 +825,15 @@ namespace AZ builder->Property ( AZStd::string::format("element_%zu_%s", Index, typeNames[Index].c_str()).c_str() , [](ContainerType& thisPointer) { return AZStd::get(thisPointer); } - , [](ContainerType& thisPointer, const t_Arg& element) { AZStd::get(thisPointer) = element; }); + , [](ContainerType& thisPointer, const Targ& element) { AZStd::get(thisPointer) = element; }); } - template + template static void ReflectUnpackMethods(BehaviorContext::ClassBuilder& builder, AZStd::index_sequence) { AZStd::vector typeNames; ScriptCanvasOnDemandReflection::GetTypeNames(typeNames, *builder.m_context); - (ReflectUnpackMethodFold(builder, typeNames, Indices), ...); + (ReflectUnpackMethodFold(builder, typeNames), ...); } static void Reflect(ReflectContext* context) From 9dec723e33e1bd2ab498b5516868ab076697dcb4 Mon Sep 17 00:00:00 2001 From: srikappa Date: Tue, 8 Jun 2021 17:28:23 -0700 Subject: [PATCH 3/6] Remove file size limits when loading prefabs and prefab-based-levels --- Code/Framework/AzCore/AzCore/Utils/Utils.cpp | 31 +++++++++++++++++++ Code/Framework/AzCore/AzCore/Utils/Utils.h | 7 ++++- .../PrefabEditorEntityOwnershipService.cpp | 8 +---- .../AzToolsFramework/Prefab/PrefabLoader.cpp | 2 +- .../Prefab/PrefabLoaderInterface.h | 2 -- 5 files changed, 39 insertions(+), 11 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp index 7025b3977e..5521ff6131 100644 --- a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp +++ b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp @@ -169,6 +169,37 @@ namespace AZ::Utils template AZ::Outcome, AZStd::string> ReadFile(AZStd::string_view filePath, size_t maxFileSize); template AZ::Outcome, AZStd::string> ReadFile(AZStd::string_view filePath, size_t maxFileSize); + template + AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath) + { + IO::FileIOStream file; + if (!file.Open(filePath.data(), IO::OpenMode::ModeRead)) + { + return AZ::Failure(AZStd::string::format("Failed to open '%.*s'.", AZ_STRING_ARG(filePath))); + } + + AZ::IO::SizeType length = file.GetLength(); + + if (length == 0) + { + return AZ::Failure(AZStd::string::format("Failed to load '%.*s'. File is empty.", AZ_STRING_ARG(filePath))); + } + + Container fileContent; + fileContent.resize(length); + AZ::IO::SizeType bytesRead = file.Read(length, fileContent.data()); + file.Close(); + + // Resize again just in case bytesRead is less than length for some reason + fileContent.resize(bytesRead); + + return AZ::Success(AZStd::move(fileContent)); + } + + template AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath); + template AZ::Outcome, AZStd::string> ReadFileWithNoSizeLimit(AZStd::string_view filePath); + template AZ::Outcome, AZStd::string> ReadFileWithNoSizeLimit(AZStd::string_view filePath); + AZ::IO::FixedMaxPathString GetO3deManifestDirectory() { AZ::IO::FixedMaxPath path = GetHomeDirectory(); diff --git a/Code/Framework/AzCore/AzCore/Utils/Utils.h b/Code/Framework/AzCore/AzCore/Utils/Utils.h index 4e64ce5a39..6aaa1b868f 100644 --- a/Code/Framework/AzCore/AzCore/Utils/Utils.h +++ b/Code/Framework/AzCore/AzCore/Utils/Utils.h @@ -113,8 +113,13 @@ namespace AZ //! Save a string to a file. Otherwise returns a failure with error message. AZ::Outcome WriteFile(AZStd::string_view content, AZStd::string_view filePath); - //! Read a file into a string. Returns a failure with error message if the content could not be loaded. + //! Read a file into a string. Returns a failure with error message if the content could not be loaded or if + //! the file size is larger than the max file size provided. template AZ::Outcome ReadFile(AZStd::string_view filePath, size_t maxFileSize = DefaultMaxFileSize); + + //! Read a file into a string. Returns a failure with error message if the content could not be loaded. + template + AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath); } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp index 7fd11ff9bf..fa7d5f6e9b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Entity/PrefabEditorEntityOwnershipService.cpp @@ -185,13 +185,7 @@ namespace AzToolsFramework bool PrefabEditorEntityOwnershipService::LoadFromStream(AZ::IO::GenericStream& stream, AZStd::string_view filename) { Reset(); - // Make loading from stream to behave the same in terms of filesize as regular loading of prefabs - // This may need to be revisited in the future for supporting higher sizes along with prefab loading - if (stream.GetLength() > Prefab::MaxPrefabFileSize) - { - AZ_Error("Prefab", false, "'%.*s' prefab content is bigger than the max supported size (%f MB)", AZ_STRING_ARG(filename), Prefab::MaxPrefabFileSize / (1024.f * 1024.f)); - return false; - } + const size_t bufSize = stream.GetLength(); AZStd::unique_ptr buf(new char[bufSize]); AZ::IO::SizeType bytes = stream.Read(bufSize, buf.get()); diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp index d7de634c11..25a16f18a8 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp @@ -74,7 +74,7 @@ namespace AzToolsFramework return InvalidTemplateId; } - auto readResult = AZ::Utils::ReadFile(GetFullPath(filePath).Native(), MaxPrefabFileSize); + auto readResult = AZ::Utils::ReadFileWithNoSizeLimit(GetFullPath(filePath).Native()); if (!readResult.IsSuccess()) { AZ_Error( diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoaderInterface.h b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoaderInterface.h index 0e551cee6b..a9a1ee0607 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoaderInterface.h +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoaderInterface.h @@ -21,8 +21,6 @@ namespace AzToolsFramework { namespace Prefab { - constexpr size_t MaxPrefabFileSize = 1024 * 1024; - /*! * PrefabLoaderInterface * Interface for saving/loading Prefab files. From b23c95cab3872bb619768274ec29949f71de27c4 Mon Sep 17 00:00:00 2001 From: srikappa Date: Tue, 8 Jun 2021 17:58:59 -0700 Subject: [PATCH 4/6] Removed the new added flavor of ReadFile and reused existing one --- Code/Framework/AzCore/AzCore/Utils/Utils.cpp | 31 ------------------- Code/Framework/AzCore/AzCore/Utils/Utils.h | 4 --- .../AzToolsFramework/Prefab/PrefabLoader.cpp | 2 +- 3 files changed, 1 insertion(+), 36 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp index 5521ff6131..7025b3977e 100644 --- a/Code/Framework/AzCore/AzCore/Utils/Utils.cpp +++ b/Code/Framework/AzCore/AzCore/Utils/Utils.cpp @@ -169,37 +169,6 @@ namespace AZ::Utils template AZ::Outcome, AZStd::string> ReadFile(AZStd::string_view filePath, size_t maxFileSize); template AZ::Outcome, AZStd::string> ReadFile(AZStd::string_view filePath, size_t maxFileSize); - template - AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath) - { - IO::FileIOStream file; - if (!file.Open(filePath.data(), IO::OpenMode::ModeRead)) - { - return AZ::Failure(AZStd::string::format("Failed to open '%.*s'.", AZ_STRING_ARG(filePath))); - } - - AZ::IO::SizeType length = file.GetLength(); - - if (length == 0) - { - return AZ::Failure(AZStd::string::format("Failed to load '%.*s'. File is empty.", AZ_STRING_ARG(filePath))); - } - - Container fileContent; - fileContent.resize(length); - AZ::IO::SizeType bytesRead = file.Read(length, fileContent.data()); - file.Close(); - - // Resize again just in case bytesRead is less than length for some reason - fileContent.resize(bytesRead); - - return AZ::Success(AZStd::move(fileContent)); - } - - template AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath); - template AZ::Outcome, AZStd::string> ReadFileWithNoSizeLimit(AZStd::string_view filePath); - template AZ::Outcome, AZStd::string> ReadFileWithNoSizeLimit(AZStd::string_view filePath); - AZ::IO::FixedMaxPathString GetO3deManifestDirectory() { AZ::IO::FixedMaxPath path = GetHomeDirectory(); diff --git a/Code/Framework/AzCore/AzCore/Utils/Utils.h b/Code/Framework/AzCore/AzCore/Utils/Utils.h index 6aaa1b868f..d082a3ebe0 100644 --- a/Code/Framework/AzCore/AzCore/Utils/Utils.h +++ b/Code/Framework/AzCore/AzCore/Utils/Utils.h @@ -117,9 +117,5 @@ namespace AZ //! the file size is larger than the max file size provided. template AZ::Outcome ReadFile(AZStd::string_view filePath, size_t maxFileSize = DefaultMaxFileSize); - - //! Read a file into a string. Returns a failure with error message if the content could not be loaded. - template - AZ::Outcome ReadFileWithNoSizeLimit(AZStd::string_view filePath); } } diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp index 25a16f18a8..44dff93cb5 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Prefab/PrefabLoader.cpp @@ -74,7 +74,7 @@ namespace AzToolsFramework return InvalidTemplateId; } - auto readResult = AZ::Utils::ReadFileWithNoSizeLimit(GetFullPath(filePath).Native()); + auto readResult = AZ::Utils::ReadFile(GetFullPath(filePath).Native(), AZStd::numeric_limits::max()); if (!readResult.IsSuccess()) { AZ_Error( 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 5/6] 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 6/6] 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