From 4b9c2cceb420dd6c0b8d24f1a03d74c3d571c163 Mon Sep 17 00:00:00 2001 From: hultonha Date: Thu, 15 Jul 2021 13:25:00 +0100 Subject: [PATCH] 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