fix bug with wrong number of tube manipulators returned

Signed-off-by: hultonha <hultonha@amazon.co.uk>

Gems/LmbrCentral/Code/Source/Shape/EditorTubeShapeComponentMode.cpp

@@ -268,28 +268,6 @@ namespace LmbrCentral
         ContainerChanged();
     }
 
-    AZStd::vector<EditorTubeShapeComponentMode::TubeManipulatorState> 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<TubeManipulatorState> 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<EditorTubeShapeComponentMode::TubeManipulatorState> 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<EditorTubeShapeComponentMode::TubeManipulatorState> 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

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<TubeManipulatorState> GenerateTubeManipulatorStates(const AZ::Spline& spline);
-
         AZ::Transform m_currentTransform; ///< The current localToWorld transform of the TubeShape.
         AZStd::vector<AZStd::shared_ptr<AzToolsFramework::LinearManipulator>> 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<EditorTubeShapeComponentMode::TubeManipulatorState> GenerateTubeManipulatorStates(const AZ::Spline& spline);
 } // namespace LmbrCentral

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 <AzCore/UnitTest/TestTypes.h>
+#include <AzTest/AzTest.h>
+#include <Source/Shape/EditorTubeShapeComponentMode.h>
+
+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<bool>
+    {
+    };
+
+    // 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

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