From 0c0d307caed58bc1277f825766aad30118155157 Mon Sep 17 00:00:00 2001 From: hultonha Date: Tue, 13 Jul 2021 14:49:27 +0100 Subject: [PATCH 01/12] ensure empty geometry buffers are not submitted for render Signed-off-by: hultonha --- .../Code/Source/Shape/ShapeGeometryUtil.cpp | 20 ++++---- .../Code/Source/Shape/TubeShape.cpp | 9 +++- .../Code/Tests/ShapeGeometryUtilTest.cpp | 47 +++++++++++++++++-- 3 files changed, 61 insertions(+), 15 deletions(-) diff --git a/Gems/LmbrCentral/Code/Source/Shape/ShapeGeometryUtil.cpp b/Gems/LmbrCentral/Code/Source/Shape/ShapeGeometryUtil.cpp index 9a52fa49f0..b0a52d4ca1 100644 --- a/Gems/LmbrCentral/Code/Source/Shape/ShapeGeometryUtil.cpp +++ b/Gems/LmbrCentral/Code/Source/Shape/ShapeGeometryUtil.cpp @@ -31,22 +31,20 @@ namespace LmbrCentral return vertices + 1; } - void DrawShape( - AzFramework::DebugDisplayRequests& debugDisplay, - const ShapeDrawParams& shapeDrawParams, const ShapeMesh& shapeMesh) + void DrawShape(AzFramework::DebugDisplayRequests& debugDisplay, const ShapeDrawParams& shapeDrawParams, const ShapeMesh& shapeMesh) { if (shapeDrawParams.m_filled) { - debugDisplay.DrawTrianglesIndexed( - shapeMesh.m_vertexBuffer, - shapeMesh.m_indexBuffer, - shapeDrawParams.m_shapeColor - ); + if (!shapeMesh.m_vertexBuffer.empty() && !shapeMesh.m_indexBuffer.empty()) + { + debugDisplay.DrawTrianglesIndexed(shapeMesh.m_vertexBuffer, shapeMesh.m_indexBuffer, shapeDrawParams.m_shapeColor); + } } - debugDisplay.DrawLines( - shapeMesh.m_lineBuffer, - shapeDrawParams.m_wireColor); + if (!shapeMesh.m_lineBuffer.empty()) + { + debugDisplay.DrawLines(shapeMesh.m_lineBuffer, shapeDrawParams.m_wireColor); + } } /// Determine if a list of vertices constitute a simple polygon diff --git a/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp b/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp index 13762ad1ed..5c3e416008 100644 --- a/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp +++ b/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp @@ -379,6 +379,13 @@ namespace LmbrCentral const float radius, const AZ::u32 capSegments, const AZ::u32 sides, AZStd::vector& vertexBufferOut) { + if (const size_t segmentCount = spline->GetSegmentCount(); segmentCount == 0) + { + // clear the buffers so we no longer draw anything + vertexBufferOut.clear(); + return; + } + // notes on vert buffer size // total end segments // 2 verts for each segment @@ -401,7 +408,7 @@ namespace LmbrCentral const size_t numVerts = totalEndSegments + totalSegments + totalLoops; vertexBufferOut.resize(numVerts); - AZ::Vector3* vertices = vertexBufferOut.begin(); + AZ::Vector3* vertices = vertexBufferOut.data(); // start cap auto address = spline->GetAddressByFraction(0.0f); diff --git a/Gems/LmbrCentral/Code/Tests/ShapeGeometryUtilTest.cpp b/Gems/LmbrCentral/Code/Tests/ShapeGeometryUtilTest.cpp index b486645694..eb8b0f4c51 100644 --- a/Gems/LmbrCentral/Code/Tests/ShapeGeometryUtilTest.cpp +++ b/Gems/LmbrCentral/Code/Tests/ShapeGeometryUtilTest.cpp @@ -6,12 +6,14 @@ */ #include "LmbrCentral_precompiled.h" -#include #include -#include -#include #include +#include +#include +#include +#include +#include namespace UnitTest { @@ -91,4 +93,43 @@ namespace UnitTest EXPECT_TRUE(triangles.size() == 18); } + + // test double to record if DrawTrianglesIndexed or DrawLines are called + class DebugShapeDebugDisplayRequests : public AzFramework::DebugDisplayRequests + { + public: + void DrawTrianglesIndexed( + [[maybe_unused]] const AZStd::vector& vertices, + [[maybe_unused]] const AZStd::vector& indices, + [[maybe_unused]] const AZ::Color& color) override + { + m_drawTrianglesIndexedCalled = true; + } + + void DrawLines([[maybe_unused]] const AZStd::vector& lines, [[maybe_unused]] const AZ::Color& color) override + { + m_drawLinesCalled = true; + } + + bool m_drawTrianglesIndexedCalled = false; + bool m_drawLinesCalled = false; + }; + + // DrawShape internally calls DrawTrianglesIndexed and DrawLines - with no geometry + // we want to make sure the shape is not submitted to be drawn + TEST(ShapeGeometry, Shape_not_attempted_to_be_drawn_with_no_geometry) + { + using ::testing::Eq; + + // given + DebugShapeDebugDisplayRequests debugDisplayRequests; + + // when + LmbrCentral::DrawShape( + debugDisplayRequests, LmbrCentral::ShapeDrawParams{ AZ::Colors::White, AZ::Colors::White, true }, LmbrCentral::ShapeMesh{}); + + // then + EXPECT_THAT(debugDisplayRequests.m_drawTrianglesIndexedCalled, Eq(false)); + EXPECT_THAT(debugDisplayRequests.m_drawLinesCalled, Eq(false)); + } } From b0707d3295f73cbe6367585d57ac08872fe0d252 Mon Sep 17 00:00:00 2001 From: hultonha Date: Tue, 13 Jul 2021 17:51:06 +0100 Subject: [PATCH 02/12] ensure the final vertex in a spline cannot be deleted using a manipulator Signed-off-by: hultonha --- .../Manipulators/EditorVertexSelection.cpp | 6 +++ .../Tests/EditorVertexSelectionTests.cpp | 54 +++++++++++++++++-- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/Code/Framework/AzToolsFramework/AzToolsFramework/Manipulators/EditorVertexSelection.cpp b/Code/Framework/AzToolsFramework/AzToolsFramework/Manipulators/EditorVertexSelection.cpp index ce12d4839f..a0e689e61b 100644 --- a/Code/Framework/AzToolsFramework/AzToolsFramework/Manipulators/EditorVertexSelection.cpp +++ b/Code/Framework/AzToolsFramework/AzToolsFramework/Manipulators/EditorVertexSelection.cpp @@ -1145,6 +1145,12 @@ namespace AzToolsFramework { if (interaction.m_keyboardModifiers.Alt()) { + if (!CanDeleteSelection(entityComponentIdPair.GetEntityId(), /*selectedCount=*/1)) + { + ShowVertexDeletionWarning(); + return; + } + SafeRemoveVertex(entityComponentIdPair, vertexIndex); } else diff --git a/Code/Framework/AzToolsFramework/Tests/EditorVertexSelectionTests.cpp b/Code/Framework/AzToolsFramework/Tests/EditorVertexSelectionTests.cpp index e1656eb504..1376aa21dd 100644 --- a/Code/Framework/AzToolsFramework/Tests/EditorVertexSelectionTests.cpp +++ b/Code/Framework/AzToolsFramework/Tests/EditorVertexSelectionTests.cpp @@ -27,6 +27,8 @@ using namespace AzToolsFramework; namespace UnitTest { + const auto TestComponentId = AZ::ComponentId(1234); + // test implementation of variable/fixed vertex request buses // (to be used in place of spline/polygon prism etc) class TestVariableVerticesVertexContainer @@ -86,6 +88,9 @@ namespace UnitTest void TearDownEditorFixtureImpl() override { + AzToolsFramework::EditorEntityContextRequestBus::Broadcast( + &AzToolsFramework::EditorEntityContextRequestBus::Events::DestroyEditorEntity, m_entityId); + m_vertexContainer.Disconnect(); m_vertexSelection.Destroy(); } @@ -106,7 +111,7 @@ namespace UnitTest void EditorVertexSelectionFixture::RecreateVertexSelection() { m_vertexSelection.Create( - AZ::EntityComponentIdPair(m_entityId, AZ::InvalidComponentId), + AZ::EntityComponentIdPair(m_entityId, TestComponentId), g_mainManipulatorManagerId, AZStd::make_unique(), TranslationManipulators::Dimensions::Three, ConfigureTranslationManipulatorAppearance3d); } @@ -116,7 +121,7 @@ namespace UnitTest for (size_t vertIndex = 0; vertIndex < EditorVertexSelectionFixture::VertexCount; ++vertIndex) { InsertVertexAfter( - AZ::EntityComponentIdPair(m_entityId, AZ::InvalidComponentId), 0, AZ::Vector3::CreateZero()); + AZ::EntityComponentIdPair(m_entityId, TestComponentId), 0, AZ::Vector3::CreateZero()); } } void EditorVertexSelectionFixture::ClearVertices() @@ -124,7 +129,7 @@ namespace UnitTest for (size_t vertIndex = 0; vertIndex < EditorVertexSelectionFixture::VertexCount; ++vertIndex) { SafeRemoveVertex( - AZ::EntityComponentIdPair(m_entityId, AZ::InvalidComponentId), 0); + AZ::EntityComponentIdPair(m_entityId, TestComponentId), 0); } } @@ -197,7 +202,7 @@ namespace UnitTest { using ::testing::Eq; - const auto entityComponentIdPair = AZ::EntityComponentIdPair(m_entityId, AZ::InvalidComponentId); + const auto entityComponentIdPair = AZ::EntityComponentIdPair(m_entityId, TestComponentId); const float horizontalPositions[] = {-1.5f, -0.5f, 0.5f, 1.5f}; for (size_t vertIndex = 0; vertIndex < std::size(horizontalPositions); ++vertIndex) @@ -252,4 +257,45 @@ namespace UnitTest // deleting all vertices is disallowed - size should remain the same EXPECT_THAT(vertexCountAfter, Eq(EditorVertexSelectionFixture::VertexCount)); } + + TEST_F(EditorVertexSelectionManipulatorFixture, CannotDeleteLastVertexWithManipulator) + { + using ::testing::Eq; + + const auto entityComponentIdPair = AZ::EntityComponentIdPair(m_entityId, TestComponentId); + + // add a single vertex (in front of the camera) + InsertVertexAfter(entityComponentIdPair, 0, AZ::Vector3::CreateAxisY(5.0f)); + + // rebuild the vertex selection after adding the new verts + RecreateVertexSelection(); + + AzFramework::ScreenPoint vertexScreenPosition; + { + AZ::Vector3 localVertex; + bool found = false; + AZ::FixedVerticesRequestBus::EventResult( + found, m_entityId, &AZ::FixedVerticesRequestBus::Handler::GetVertex, 0, localVertex); + + if (found) + { + // note: entity position is at the origin so localVertex position is equivalent to world + vertexScreenPosition = AzFramework::WorldToScreen(localVertex, m_cameraState); + } + } + + // attempt to delete the vertex by clicking with Alt held + m_actionDispatcher->CameraState(m_cameraState) + ->MousePosition(vertexScreenPosition) + ->KeyboardModifierDown(AzToolsFramework::ViewportInteraction::KeyboardModifier::Alt) + ->MouseLButtonDown() + ->MouseLButtonUp(); + + size_t vertexCountAfter = 0; + AZ::VariableVerticesRequestBus::EventResult( + vertexCountAfter, m_entityId, &AZ::VariableVerticesRequestBus::Events::Size); + + // deleting the last vertex through a manipulator is disallowed - size should remain the same + EXPECT_THAT(vertexCountAfter, Eq(1)); + } } // namespace UnitTest From 402fd2ae4ef42a3dd5dd75fe7d12ac453f084780 Mon Sep 17 00:00:00 2001 From: hultonha Date: Tue, 13 Jul 2021 18:10:33 +0100 Subject: [PATCH 03/12] small updates following review feedback Signed-off-by: hultonha --- Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp b/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp index 5c3e416008..b9d7a628a0 100644 --- a/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp +++ b/Gems/LmbrCentral/Code/Source/Shape/TubeShape.cpp @@ -1,6 +1,6 @@ /* * Copyright (c) Contributors to the Open 3D Engine Project. For complete copyright and license terms please see the LICENSE at the root of this distribution. - * + * * SPDX-License-Identifier: Apache-2.0 OR MIT * */ @@ -131,7 +131,7 @@ namespace LmbrCentral m_variableRadius.SetElement(vertIndex, radius); ValidateVariableRadius(vertIndex); } - + ShapeComponentNotificationsBus::Event( m_entityId, &ShapeComponentNotificationsBus::Events::OnShapeChanged, ShapeComponentNotifications::ShapeChangeReasons::ShapeChanged); @@ -379,9 +379,10 @@ namespace LmbrCentral const float radius, const AZ::u32 capSegments, const AZ::u32 sides, AZStd::vector& vertexBufferOut) { - if (const size_t segmentCount = spline->GetSegmentCount(); segmentCount == 0) + const size_t segmentCount = spline->GetSegmentCount(); + if (segmentCount == 0) { - // clear the buffers so we no longer draw anything + // clear the buffer so we no longer draw anything vertexBufferOut.clear(); return; } @@ -400,7 +401,7 @@ namespace LmbrCentral // 2 verts for each segment // loops == sides // 2 loops per segment - const AZ::u32 segments = spline->GetSegmentCount() * spline->GetSegmentGranularity(); + const AZ::u32 segments = segmentCount * spline->GetSegmentGranularity(); const AZ::u32 totalEndSegments = capSegments * 2 * 2 * 2 * 2; const AZ::u32 totalSegments = segments * 2 * 2 * 2; const AZ::u32 totalLoops = 2 * sides * segments * 2; @@ -429,7 +430,7 @@ namespace LmbrCentral // body const float stepDelta = 1.0f / static_cast(spline->GetSegmentGranularity()); auto nextAddress = address; - const auto endIndex = address.m_segmentIndex + spline->GetSegmentCount(); + const auto endIndex = address.m_segmentIndex + segmentCount; while (address.m_segmentIndex < endIndex) { address.m_segmentFraction = 0.f; From 9939913c8df0b9e3b5644a622f7d696883b94d82 Mon Sep 17 00:00:00 2001 From: hultonha Date: Tue, 13 Jul 2021 18:23:19 +0100 Subject: [PATCH 04/12] update MSVC to use new lambda processing Signed-off-by: hultonha --- cmake/Platform/Common/MSVC/Configurations_msvc.cmake | 1 + 1 file changed, 1 insertion(+) diff --git a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake index cad0e818cb..a1ded72255 100644 --- a/cmake/Platform/Common/MSVC/Configurations_msvc.cmake +++ b/cmake/Platform/Common/MSVC/Configurations_msvc.cmake @@ -77,6 +77,7 @@ ly_append_configurations_options( /Zc:forScope # Force Conformance in for Loop Scope /diagnostics:caret # Compiler diagnostic options: includes the column where the issue was found and places a caret (^) under the location in the line of code where the issue was detected. /Zc:__cplusplus + /Zc:lambda # Use the new lambda processor (See https://developercommunity.visualstudio.com/t/A-lambda-that-binds-the-this-pointer-w/1467873 for more details) /favor:AMD64 # Create Code optimized for 64 bit /bigobj # Increase number of sections in obj files. Profiling has shown no meaningful impact in memory nore build times COMPILATION_DEBUG From d69897fd86ab16e54ba531a8bffe2e250b3e942b Mon Sep 17 00:00:00 2001 From: hultonha Date: Wed, 14 Jul 2021 10:33:39 +0100 Subject: [PATCH 05/12] add [[maybe_unused]] attribute to prevent innocuous warning Signed-off-by: hultonha --- Code/Framework/AzCore/AzCore/Debug/Trace.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Code/Framework/AzCore/AzCore/Debug/Trace.h b/Code/Framework/AzCore/AzCore/Debug/Trace.h index 594322f1f3..ecf951ae6c 100644 --- a/Code/Framework/AzCore/AzCore/Debug/Trace.h +++ b/Code/Framework/AzCore/AzCore/Debug/Trace.h @@ -127,7 +127,7 @@ namespace AZ #define AZ_TraceFmtCompileTimeCheck(expression, isVaArgs, baseMsg, msg, msgVargs) \ { \ using namespace AZ::TraceInternal; \ - const auto& rTraceFmtCompileTimeCheckExpressionHelper = (expression); /* This is needed for edge cases for expressions containing lambdas, that were unsupported before C++20 */ \ + [[maybe_unused]] const auto& rTraceFmtCompileTimeCheckExpressionHelper = (expression); /* This is needed for edge cases for expressions containing lambdas, that were unsupported before C++20 */ \ constexpr ExpressionValidResult isValidTraceFmtResult = ExpressionIsValid::value; \ /* Assert different message depending whether it's const char array or if we have extra arguments */ \ static_assert(!(isVaArgs) ? isValidTraceFmtResult != ExpressionValidResult::Invalid_ConstCharArray : true, baseMsg " " msg); \ From f8a80468ab27e34edc0d75d475da6928a645a8ec Mon Sep 17 00:00:00 2001 From: hultonha Date: Wed, 14 Jul 2021 13:13:28 +0100 Subject: [PATCH 06/12] remove structured binding from lambda to workaround msvc bug Signed-off-by: hultonha --- .../Components/MeshOptimizer/MeshOptimizerComponent.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshOptimizerComponent.cpp b/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshOptimizerComponent.cpp index e3eb617601..a5d5510f8a 100644 --- a/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshOptimizerComponent.cpp +++ b/Gems/SceneProcessing/Code/Source/Generation/Components/MeshOptimizer/MeshOptimizerComponent.cpp @@ -493,7 +493,8 @@ namespace AZ::SceneGenerationComponents // Copy node attributes AZStd::apply([](const auto&&... nodePairView) { ((AZStd::for_each(begin(nodePairView), end(nodePairView), [](const auto& nodePair) { - auto& [originalNode, optimizedNode] = nodePair; + auto& originalNode = nodePair.first; + auto& optimizedNode = nodePair.second; optimizedNode->CloneAttributesFrom(&originalNode.get()); })), ...); }, std::tuple { From 4b9c2cceb420dd6c0b8d24f1a03d74c3d571c163 Mon Sep 17 00:00:00 2001 From: hultonha Date: Thu, 15 Jul 2021 13:25:00 +0100 Subject: [PATCH 07/12] fix bug with wrong number of tube manipulators returned Signed-off-by: hultonha --- .../Shape/EditorTubeShapeComponentMode.cpp | 55 ++++---- .../Shape/EditorTubeShapeComponentMode.h | 22 ++-- .../Tests/EditorTubeShapeComponentTests.cpp | 118 ++++++++++++++++++ .../Code/lmbrcentral_editor_tests_files.cmake | 1 + 4 files changed, 163 insertions(+), 33 deletions(-) create mode 100644 Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp diff --git a/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.cpp b/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.cpp index 48a68a30a5..21d7d6bc33 100644 --- a/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.cpp +++ b/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.cpp @@ -268,28 +268,6 @@ namespace LmbrCentral ContainerChanged(); } - AZStd::vector EditorTubeShapeComponentMode::GenerateTubeManipulatorStates( - const AZ::Spline& spline) - { - const AZ::u64 startVertex = spline.GetAddressByFraction(0.0f).m_segmentIndex; - const AZ::u64 endVertex = startVertex + spline.GetSegmentCount() + (spline.IsClosed() ? 0 : 1); - - AZStd::vector splineAddresses; - for (AZ::u64 vertIndex = startVertex; vertIndex < endVertex; ++vertIndex) - { - if (vertIndex + 1 == endVertex) - { - splineAddresses.push_back({ AZ::SplineAddress(vertIndex - 1, 1.0f), vertIndex }); - } - else - { - splineAddresses.push_back({ AZ::SplineAddress(vertIndex), vertIndex }); - } - } - - return splineAddresses; - } - void EditorTubeShapeComponentMode::RefreshManipulatorsLocal(const AZ::EntityId entityId) { AZ::SplinePtr spline; @@ -318,4 +296,37 @@ namespace LmbrCentral m_radiusManipulators[manipulatorIndex]->SetBoundsDirty(); } } + + AZStd::vector GenerateTubeManipulatorStates(const AZ::Spline& spline) + { + if (spline.GetVertexCount() == 0) + { + return {}; + } + + const auto segmentCount = spline.GetSegmentCount(); + if (segmentCount == 0) + { + return { { AZ::SplineAddress(0), 0 } }; + } + + const AZ::u64 startVertex = spline.GetAddressByFraction(0.0f).m_segmentIndex; + const AZ::u64 endVertex = startVertex + segmentCount + (spline.IsClosed() ? 0 : 1); + + AZStd::vector splineAddresses; + for (AZ::u64 vertIndex = startVertex; vertIndex < endVertex; ++vertIndex) + { + if (vertIndex + 1 == endVertex) + { + AZ_Assert(vertIndex > 0, "vertexIndex is 0 and not safe to subtract from") + splineAddresses.push_back({ AZ::SplineAddress(vertIndex - 1, 1.0f), vertIndex }); + } + else + { + splineAddresses.push_back({ AZ::SplineAddress(vertIndex), vertIndex }); + } + } + + return splineAddresses; + } } // namespace LmbrCentral diff --git a/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.h b/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.h index 4d32c72f3a..9713ff2064 100644 --- a/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.h +++ b/Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.h @@ -31,6 +31,13 @@ namespace LmbrCentral public: AZ_CLASS_ALLOCATOR_DECL + /// Data required per TubeShape manipulator. + struct TubeManipulatorState + { + AZ::SplineAddress m_splineAddress; + AZ::u64 m_vertIndex; + }; + EditorTubeShapeComponentMode( const AZ::EntityComponentIdPair& entityComponentIdPair, AZ::Uuid componentType); ~EditorTubeShapeComponentMode(); @@ -64,18 +71,11 @@ namespace LmbrCentral void RefreshManipulatorsLocal(AZ::EntityId entityId); - /// Data required per TubeShape manipulator. - struct TubeManipulatorState - { - AZ::SplineAddress m_splineAddress; - AZ::u64 m_vertIndex; - }; - - /// For a given Tube + Spline combo, generate data required for each manipulator at - /// each vertex required for modifying the tube. - AZStd::vector GenerateTubeManipulatorStates(const AZ::Spline& spline); - AZ::Transform m_currentTransform; ///< The current localToWorld transform of the TubeShape. AZStd::vector> m_radiusManipulators; ///< Manipulators to control the radius (volume) of the tube at each vertex. }; + + /// For a given Tube + Spline combo, generate data required for each manipulator at + /// each vertex required for modifying the tube. + AZStd::vector GenerateTubeManipulatorStates(const AZ::Spline& spline); } // namespace LmbrCentral diff --git a/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp b/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp new file mode 100644 index 0000000000..b3fb374189 --- /dev/null +++ b/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp @@ -0,0 +1,118 @@ +/* + * Copyright (c) Contributors to the Open 3D Engine Project. For complete copyright and license terms please see the LICENSE at the root of this distribution. + * + * SPDX-License-Identifier: Apache-2.0 OR MIT + * + */ + +#pragma once + +#include +#include +#include + +namespace AZ +{ + void PrintTo(const AZ::SplineAddress& splineAddress, std::ostream* os) + { + *os << "SplineAddress { segmentIndex: " << splineAddress.m_segmentIndex << ", segmentFraction: " << splineAddress.m_segmentFraction + << " }"; + } +} // namespace AZ + +namespace UnitTest +{ + class EditorTubeShapeFixture + : public AllocatorsFixture + , public ::testing::WithParamInterface + { + }; + + // test both open and closed versions of the spline + INSTANTIATE_TEST_CASE_P(GenerateTubeManipulatorStates, EditorTubeShapeFixture, ::testing::Values(true, false)); + + TEST_P(EditorTubeShapeFixture, GenerateTubeManipulatorStates_returns_no_TubeManipulatorStates_when_spline_is_empty) + { + using ::testing::Eq; + + // given (an empty spline) + AZ::BezierSpline spline; + spline.SetClosed(GetParam()); + + // when (tube manipulator states are attempted to be created) + const auto tubeManipulatorStates = LmbrCentral::GenerateTubeManipulatorStates(spline); + + // then (none are returned) + EXPECT_THAT(tubeManipulatorStates.empty(), Eq(true)); + } + + TEST_P(EditorTubeShapeFixture, GenerateTubeManipulatorStates_returns_one_TubeManipulatorStates_when_spline_has_one_vertex) + { + using ::testing::Eq; + + // given (an empty spline) + AZ::BezierSpline spline; + spline.SetClosed(GetParam()); + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateZero()); + + // when (tube manipulator states are attempted to be created) + const auto tubeManipulatorStates = LmbrCentral::GenerateTubeManipulatorStates(spline); + + // then (one is returned) + EXPECT_THAT(tubeManipulatorStates.size(), Eq(1)); + EXPECT_THAT(tubeManipulatorStates[0].m_splineAddress, Eq(AZ::SplineAddress(0, 0.0f))); + EXPECT_THAT(tubeManipulatorStates[0].m_vertIndex, Eq(0)); + } + + TEST_P(EditorTubeShapeFixture, GenerateTubeManipulatorStates_returns_two_TubeManipulatorStates_when_spline_has_two_vertices) + { + using ::testing::Eq; + + // given (an empty spline) + AZ::BezierSpline spline; + spline.SetClosed(GetParam()); + + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateZero()); + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateAxisX(1.0f)); + + // when (tube manipulator states are attempted to be created) + const auto tubeManipulatorStates = LmbrCentral::GenerateTubeManipulatorStates(spline); + + // then (two are returned) + EXPECT_THAT(tubeManipulatorStates.size(), Eq(2)); + + EXPECT_THAT(tubeManipulatorStates[0].m_splineAddress, Eq(AZ::SplineAddress(0, 0.0f))); + EXPECT_THAT(tubeManipulatorStates[0].m_vertIndex, Eq(0)); + + EXPECT_THAT(tubeManipulatorStates[1].m_splineAddress, Eq(AZ::SplineAddress(0, 1.0f))); + EXPECT_THAT(tubeManipulatorStates[1].m_vertIndex, Eq(1)); + } + + TEST_P(EditorTubeShapeFixture, GenerateTubeManipulatorStates_returns_three_TubeManipulatorStates_when_spline_has_three_vertices) + { + using ::testing::Eq; + + // given (an empty spline) + AZ::BezierSpline spline; + spline.SetClosed(GetParam()); + + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateAxisX(-1.0f)); + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateZero()); + spline.m_vertexContainer.AddVertex(AZ::Vector3::CreateAxisX(1.0f)); + + // when (tube manipulator states are attempted to be created) + const auto tubeManipulatorStates = LmbrCentral::GenerateTubeManipulatorStates(spline); + + // then (three are returned) + EXPECT_THAT(tubeManipulatorStates.size(), Eq(3)); + + EXPECT_THAT(tubeManipulatorStates[0].m_splineAddress, Eq(AZ::SplineAddress(0, 0.0f))); + EXPECT_THAT(tubeManipulatorStates[0].m_vertIndex, Eq(0)); + + EXPECT_THAT(tubeManipulatorStates[1].m_splineAddress, Eq(AZ::SplineAddress(1, 0.0f))); + EXPECT_THAT(tubeManipulatorStates[1].m_vertIndex, Eq(1)); + + EXPECT_THAT(tubeManipulatorStates[2].m_splineAddress, Eq(AZ::SplineAddress(1, 1.0f))); + EXPECT_THAT(tubeManipulatorStates[2].m_vertIndex, Eq(2)); + } +} // namespace UnitTest diff --git a/Gems/LmbrCentral/Code/lmbrcentral_editor_tests_files.cmake b/Gems/LmbrCentral/Code/lmbrcentral_editor_tests_files.cmake index 1490ceb1b3..c23305289c 100644 --- a/Gems/LmbrCentral/Code/lmbrcentral_editor_tests_files.cmake +++ b/Gems/LmbrCentral/Code/lmbrcentral_editor_tests_files.cmake @@ -15,6 +15,7 @@ set(FILES Tests/EditorCompoundShapeComponentTests.cpp Tests/EditorCylinderShapeComponentTests.cpp Tests/EditorPolygonPrismShapeComponentTests.cpp + Tests/EditorTubeShapeComponentTests.cpp Tests/SpawnerComponentTest.cpp Tests/Builders/CopyDependencyBuilderTest.cpp Tests/Builders/SliceBuilderTests.cpp From 3b419c02e62c26f01c38c9cdb5d62d5aba753389 Mon Sep 17 00:00:00 2001 From: John Jones-Steele Date: Thu, 15 Jul 2021 13:42:02 +0100 Subject: [PATCH 08/12] CHange the way the Mute Audio button works depending on the Gem availability Signed-off-by: John Jones-Steele --- Code/Editor/ViewportTitleDlg.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/Code/Editor/ViewportTitleDlg.cpp b/Code/Editor/ViewportTitleDlg.cpp index 3090cf8e63..cb544ce5c0 100644 --- a/Code/Editor/ViewportTitleDlg.cpp +++ b/Code/Editor/ViewportTitleDlg.cpp @@ -868,7 +868,16 @@ void CViewportTitleDlg::OnBnClickedMuteAudio() void CViewportTitleDlg::UpdateMuteActionText() { - m_audioMuteAction->setText(gSettings.bMuteAudio ? tr("Un-mute Audio") : tr("Mute Audio")); + if (!Audio::AudioSystemRequestBus::HasHandlers()) + { + m_audioMuteAction->setEnabled(false); + m_audioMuteAction->setText(tr("Mute Audio: Enable Audio Gem")); + } + else + { + m_audioMuteAction->setEnabled(true); + m_audioMuteAction->setText(gSettings.bMuteAudio ? tr("Un-mute Audio") : tr("Mute Audio")); + } } void CViewportTitleDlg::OnHMDInitialized() From 23d08d411ff0626796501445e39557e5468ed824 Mon Sep 17 00:00:00 2001 From: hultonha Date: Thu, 15 Jul 2021 14:23:18 +0100 Subject: [PATCH 09/12] remove pragma once in cpp file Signed-off-by: hultonha --- Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp b/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp index b3fb374189..54b0b2f7b4 100644 --- a/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp +++ b/Gems/LmbrCentral/Code/Tests/EditorTubeShapeComponentTests.cpp @@ -5,8 +5,6 @@ * */ -#pragma once - #include #include #include From d16b810075d46bb4b8272d4101d82a272807a57d Mon Sep 17 00:00:00 2001 From: Benjamin Jillich <43751992+amzn-jillich@users.noreply.github.com> Date: Fri, 16 Jul 2021 04:47:47 -0700 Subject: [PATCH 10/12] Transform node animations ignored (#2160) * Fixing user asset issue having a transform node as root rather than a bone making the whole character not animate. * Enabled the ability to export animations for transform nodes. Signed-off-by: Benjamin Jillich --- .../RCExt/Motion/MotionDataBuilder.cpp | 50 +++++++++++++------ .../Pipeline/RCExt/Motion/MotionDataBuilder.h | 11 ++++ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.cpp b/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.cpp index d0963a33ee..00eff6c047 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.cpp +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.cpp @@ -11,7 +11,6 @@ #include #include #include -#include #include #include @@ -166,6 +165,34 @@ namespace EMotionFX return finalMotionData; } + AZ::SceneAPI::DataTypes::MatrixType MotionDataBuilder::GetLocalSpaceBindPose(const SceneContainers::SceneGraph& sceneGraph, + const SceneContainers::SceneGraph::NodeIndex rootBoneNodeIndex, + const SceneContainers::SceneGraph::NodeIndex nodeIndex, + const SceneDataTypes::ITransform* transform, + const SceneDataTypes::IBoneData* bone) const + { + if (nodeIndex != rootBoneNodeIndex) + { + const SceneContainers::SceneGraph::NodeIndex parentNodeIndex = sceneGraph.GetNodeParent(nodeIndex); + const SceneDataTypes::IGraphObject* parentNode = sceneGraph.GetNodeContent(parentNodeIndex).get(); + if (const SceneDataTypes::IBoneData* parentBone = azrtti_cast(parentNode)) + { + return parentBone->GetWorldTransform().GetInverseFull() * bone->GetWorldTransform(); + } + } + + if (bone) + { + return bone->GetWorldTransform(); + } + else if (transform) + { + return transform->GetMatrix(); + } + + return AZ::SceneAPI::DataTypes::MatrixType::CreateIdentity(); + } + AZ::SceneAPI::Events::ProcessingResult MotionDataBuilder::BuildMotionData(MotionDataBuilderContext& context) { if (context.m_phase != AZ::RC::Phase::Filling) @@ -220,8 +247,10 @@ namespace EMotionFX continue; } - AZStd::shared_ptr nodeBone = azrtti_cast(it->second); - if (!nodeBone) + // Check if we are dealing with a transform node or a bone and only recurse down the node hierarchy in this case. + const SceneDataTypes::IBoneData* nodeBone = azrtti_cast(it->second.get()); + const SceneDataTypes::ITransform* nodeTransform = azrtti_cast(it->second.get()); + if (!nodeBone && !nodeTransform) { it.IgnoreNodeDescendants(); continue; @@ -289,24 +318,13 @@ namespace EMotionFX // Get the bind pose transform in local space. using SceneAPIMatrixType = AZ::SceneAPI::DataTypes::MatrixType; - SceneAPIMatrixType bindSpaceLocalTransform; - const SceneContainers::SceneGraph::NodeIndex parentIndex = graph.GetNodeParent(boneNodeIndex); - if (boneNodeIndex != rootBoneNodeIndex) - { - auto parentNode = graph.GetNodeContent(parentIndex); - AZStd::shared_ptr parentNodeBone = azrtti_cast(parentNode); - bindSpaceLocalTransform = parentNodeBone->GetWorldTransform().GetInverseFull() * nodeBone->GetWorldTransform(); - } - else - { - bindSpaceLocalTransform = nodeBone->GetWorldTransform(); - } + const SceneAPIMatrixType bindSpaceLocalTransform = GetLocalSpaceBindPose(graph, rootBoneNodeIndex, boneNodeIndex, nodeTransform, nodeBone); // Get the time step and make sure it didn't change compared to other joint animations. const double timeStep = animation->GetTimeStepBetweenFrames(); lowestTimeStep = AZ::GetMin(timeStep, lowestTimeStep); - SceneAPIMatrixType sampleFrameTransformInverse; + AZ::SceneAPI::DataTypes::MatrixType sampleFrameTransformInverse; if (additiveRule) { size_t sampleFrameIndex = additiveRule->GetSampleFrameIndex(); diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.h b/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.h index 5cf865324b..ca7402f83b 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.h +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/RCExt/Motion/MotionDataBuilder.h @@ -9,6 +9,9 @@ #include #include +#include +#include +#include namespace EMotionFX { @@ -28,6 +31,14 @@ namespace EMotionFX static void Reflect(AZ::ReflectContext* context); AZ::SceneAPI::Events::ProcessingResult BuildMotionData(MotionDataBuilderContext& context); + + private: + //! Get the bind pose transform in local space. + AZ::SceneAPI::DataTypes::MatrixType GetLocalSpaceBindPose(const AZ::SceneAPI::Containers::SceneGraph& sceneGraph, + const AZ::SceneAPI::Containers::SceneGraph::NodeIndex rootBoneNodeIndex, + const AZ::SceneAPI::Containers::SceneGraph::NodeIndex nodeIndex, + const AZ::SceneAPI::DataTypes::ITransform* transform, + const AZ::SceneAPI::DataTypes::IBoneData* bone) const; }; } // namespace Pipeline } // namespace EMotionFX From facd1e18b5bea237d5128767a93dbc54c4049a0c Mon Sep 17 00:00:00 2001 From: jjjoness <82226755+jjjoness@users.noreply.github.com> Date: Fri, 16 Jul 2021 16:17:11 +0100 Subject: [PATCH 11/12] One hamburger menu to rule them all (#2232) * Icon changes Signed-off-by: John Jones-Steele * Icon changes to svg files Signed-off-by: John Jones-Steele --- AutomatedTesting/Gem/Code/enabled_gems.cmake | 7 ++++++- .../UI/Outliner/EntityOutliner.qss | 2 +- .../AzQtComponents/Components/img/UI20/Cards/menu_ico.svg | 2 +- .../AzQtComponents/AzQtComponents/Images/Menu/menu.svg | 6 +++--- 4 files changed, 11 insertions(+), 6 deletions(-) diff --git a/AutomatedTesting/Gem/Code/enabled_gems.cmake b/AutomatedTesting/Gem/Code/enabled_gems.cmake index dd68e379dd..9ef9e4f874 100644 --- a/AutomatedTesting/Gem/Code/enabled_gems.cmake +++ b/AutomatedTesting/Gem/Code/enabled_gems.cmake @@ -20,7 +20,7 @@ set(ENABLED_GEMS QtForPython PythonAssetBuilder Metastream - AudioSystem + Camera EMotionFX PhysX @@ -51,4 +51,9 @@ set(ENABLED_GEMS AWSCore AWSClientAuth AWSMetrics + + + + + AudioSystem ) diff --git a/Code/Editor/Plugins/ComponentEntityEditorPlugin/UI/Outliner/EntityOutliner.qss b/Code/Editor/Plugins/ComponentEntityEditorPlugin/UI/Outliner/EntityOutliner.qss index 624c89aad7..9e71cbe194 100644 --- a/Code/Editor/Plugins/ComponentEntityEditorPlugin/UI/Outliner/EntityOutliner.qss +++ b/Code/Editor/Plugins/ComponentEntityEditorPlugin/UI/Outliner/EntityOutliner.qss @@ -8,7 +8,7 @@ OutlinerWidget #m_display_options { - qproperty-icon: url(:/stylesheet/img/UI20/menu-centered.svg); + qproperty-icon: url(:/Menu/menu.svg); qproperty-iconSize: 16px 16px; qproperty-flat: true; } diff --git a/Code/Framework/AzQtComponents/AzQtComponents/Components/img/UI20/Cards/menu_ico.svg b/Code/Framework/AzQtComponents/AzQtComponents/Components/img/UI20/Cards/menu_ico.svg index d70293f3a5..236f0f6d7c 100644 --- a/Code/Framework/AzQtComponents/AzQtComponents/Components/img/UI20/Cards/menu_ico.svg +++ b/Code/Framework/AzQtComponents/AzQtComponents/Components/img/UI20/Cards/menu_ico.svg @@ -6,7 +6,7 @@ - + diff --git a/Code/Framework/AzQtComponents/AzQtComponents/Images/Menu/menu.svg b/Code/Framework/AzQtComponents/AzQtComponents/Images/Menu/menu.svg index e97da32e09..117feb534f 100644 --- a/Code/Framework/AzQtComponents/AzQtComponents/Images/Menu/menu.svg +++ b/Code/Framework/AzQtComponents/AzQtComponents/Images/Menu/menu.svg @@ -3,9 +3,9 @@ Buttons / Dropdown button with Icon / no arrow - - - + + + \ No newline at end of file From c1ebbbc176a2e383d226c61ca4c28732f70518ef Mon Sep 17 00:00:00 2001 From: Benjamin Jillich <43751992+amzn-jillich@users.noreply.github.com> Date: Fri, 16 Jul 2021 08:53:04 -0700 Subject: [PATCH 12/12] EMotion FX: Root bone not initialized without opening Scene Settings #2088 (#2231) User provided model was exporting correctly with an .assetinfo while it was not when just placing the .fbx file in the project folder. Turned out that the best matching root bone was set by opening the Scene Settings for the first time, so after saving it again it worked correctly. We're now chosing the best matching root bone when initializing the actor group, which fixes the issue. Signed-off-by: Benjamin Jillich --- .../Behaviors/ActorGroupBehavior.cpp | 1 + .../SceneAPIExt/Groups/ActorGroup.cpp | 20 +++++++++++++++++++ .../Pipeline/SceneAPIExt/Groups/ActorGroup.h | 2 +- .../Pipeline/SceneAPIExt/Groups/IActorGroup.h | 6 ++++++ 4 files changed, 28 insertions(+), 1 deletion(-) diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Behaviors/ActorGroupBehavior.cpp b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Behaviors/ActorGroupBehavior.cpp index c3202398b4..1e9c14d2e0 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Behaviors/ActorGroupBehavior.cpp +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Behaviors/ActorGroupBehavior.cpp @@ -143,6 +143,7 @@ namespace EMotionFX Group::ActorGroup* group = azrtti_cast(&target); group->SetName(AZ::SceneAPI::DataTypes::Utilities::CreateUniqueName(scene.GetName(), scene.GetManifest())); + group->SetBestMatchingRootBone(scene.GetGraph()); // LOD Rule need to be built first in the actor, so we know which mesh and bone belongs to LOD. // After this call, LOD rule will be populated with all the LOD bones diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.cpp b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.cpp index e4065f58e0..b623086bc1 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.cpp +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.cpp @@ -9,10 +9,14 @@ #include #include #include +#include +#include +#include #include #include #include #include +#include #include #include #include @@ -76,6 +80,22 @@ namespace EMotionFX m_selectedRootBone = selectedRootBone; } + void ActorGroup::SetBestMatchingRootBone(const AZ::SceneAPI::Containers::SceneGraph& sceneGraph) + { + auto nameContentView = AZ::SceneAPI::Containers::Views::MakePairView(sceneGraph.GetNameStorage(), sceneGraph.GetContentStorage()); + auto graphDownwardsView = AZ::SceneAPI::Containers::Views::MakeSceneGraphDownwardsView( + sceneGraph, sceneGraph.GetRoot(), nameContentView.begin(), true); + + for (auto it = graphDownwardsView.begin(); it != graphDownwardsView.end(); ++it) + { + if (it->second && it->second->RTTI_IsTypeOf(AZ::SceneData::GraphData::RootBoneData::TYPEINFO_Uuid())) + { + SetSelectedRootBone(it->first.GetPath()); + return; + } + } + } + void ActorGroup::Reflect(AZ::ReflectContext* context) { AZ::SerializeContext* serializeContext = azrtti_cast(context); diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.h b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.h index fddb0d6afc..0725b3e88b 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.h +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/ActorGroup.h @@ -45,8 +45,8 @@ namespace EMotionFX // IActorGroup overrides const AZStd::string& GetSelectedRootBone() const override; - void SetSelectedRootBone(const AZStd::string& selectedRootBone) override; + void SetBestMatchingRootBone(const AZ::SceneAPI::Containers::SceneGraph& sceneGraph) override; static void Reflect(AZ::ReflectContext* context); static bool IActorGroupVersionConverter(AZ::SerializeContext& context, AZ::SerializeContext::DataElementNode& classElement); diff --git a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/IActorGroup.h b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/IActorGroup.h index 398ec2c559..b57a9dfb13 100644 --- a/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/IActorGroup.h +++ b/Gems/EMotionFX/Code/EMotionFX/Pipeline/SceneAPIExt/Groups/IActorGroup.h @@ -10,6 +10,11 @@ #include #include +namespace AZ::SceneAPI::Containers +{ + class SceneGraph; +} + namespace EMotionFX { namespace Pipeline @@ -26,6 +31,7 @@ namespace EMotionFX virtual const AZStd::string& GetSelectedRootBone() const = 0; virtual void SetSelectedRootBone(const AZStd::string& selectedRootBone) = 0; + virtual void SetBestMatchingRootBone(const AZ::SceneAPI::Containers::SceneGraph& sceneGraph) = 0; }; } }