Merge pull request #2131 from aws-lumberyard-dev/hultonha_LYN-2348_tube_render_crash

Ensure empty geometry buffers are not submitted for render

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

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,6 +379,14 @@ namespace LmbrCentral
         const float radius, const AZ::u32 capSegments, const AZ::u32 sides,
         AZStd::vector<AZ::Vector3>& vertexBufferOut)
     {
+        const size_t segmentCount = spline->GetSegmentCount();
+        if (segmentCount == 0)
+        {
+            // clear the buffer so we no longer draw anything
+            vertexBufferOut.clear();
+            return;
+        }
+
         // notes on vert buffer size
         // total end segments
         // 2 verts for each segment
@@ -393,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;
@@ -401,7 +409,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);
@@ -422,7 +430,7 @@ namespace LmbrCentral
         // body
         const float stepDelta = 1.0f / static_cast<float>(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;

Gems/LmbrCentral/Code/Tests/ShapeGeometryUtilTest.cpp

@@ -6,12 +6,14 @@
  */
 
 #include "LmbrCentral_precompiled.h"
-#include <AzTest/AzTest.h>
 
 #include <AzCore/Component/ComponentApplication.h>
-#include <Shape/ShapeGeometryUtil.h>
-#include <AzCore/UnitTest/TestTypes.h>
 #include <AzCore/Math/Vector3.h>
+#include <AzCore/UnitTest/TestTypes.h>
+#include <AzFramework/Entity/EntityDebugDisplayBus.h>
+#include <AzTest/AzTest.h>
+#include <LmbrCentral/Shape/ShapeComponentBus.h>
+#include <Shape/ShapeGeometryUtil.h>
 
 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<AZ::Vector3>& vertices,
+            [[maybe_unused]] const AZStd::vector<AZ::u32>& indices,
+            [[maybe_unused]] const AZ::Color& color) override
+        {
+            m_drawTrianglesIndexedCalled = true;
+        }
+
+        void DrawLines([[maybe_unused]] const AZStd::vector<AZ::Vector3>& 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));
+    }
 }