From b0707d3295f73cbe6367585d57ac08872fe0d252 Mon Sep 17 00:00:00 2001 From: hultonha Date: Tue, 13 Jul 2021 17:51:06 +0100 Subject: [PATCH 1/3] 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 4b9c2cceb420dd6c0b8d24f1a03d74c3d571c163 Mon Sep 17 00:00:00 2001 From: hultonha Date: Thu, 15 Jul 2021 13:25:00 +0100 Subject: [PATCH 2/3] 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 23d08d411ff0626796501445e39557e5468ed824 Mon Sep 17 00:00:00 2001 From: hultonha Date: Thu, 15 Jul 2021 14:23:18 +0100 Subject: [PATCH 3/3] 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