Fixed editor crash using cylinder shape component (#5037)

Fixed cylinder shape by considering shape config list to be empty.

The crash came from clearing shape config list when cylinder height is zero, but when the height is restored to zero it was expecting an element in the list.

The code to set the shape configuration was the same for many shape types, so it has been refactored to a helper function.

Fixes #4999

Gems/PhysX/Code/Source/EditorShapeColliderComponent.cpp

@@ -348,19 +348,7 @@ namespace PhysX
         LmbrCentral::BoxShapeComponentRequestsBus::EventResult(boxDimensions, GetEntityId(),
             &LmbrCentral::BoxShapeComponentRequests::GetBoxDimensions);
 
-        if (m_shapeType != ShapeType::Box)
-        {
-            m_shapeConfigs.clear();
-            m_shapeConfigs.emplace_back(AZStd::make_shared<Physics::BoxShapeConfiguration>(boxDimensions));
-
-            m_shapeType = ShapeType::Box;
-        }
-        else
-        {
-            Physics::BoxShapeConfiguration& configuration =
-                static_cast<Physics::BoxShapeConfiguration&>(*m_shapeConfigs.back());
-            configuration = Physics::BoxShapeConfiguration(boxDimensions);
-        }
+        SetShapeConfig(ShapeType::Box, Physics::BoxShapeConfiguration(boxDimensions));
 
         m_shapeConfigs.back()->m_scale = scale;
         m_geometryCache.m_boxDimensions = scale * boxDimensions;
@@ -374,19 +362,7 @@ namespace PhysX
         const Physics::CapsuleShapeConfiguration& capsuleShapeConfig =
             Utils::ConvertFromLmbrCentralCapsuleConfig(lmbrCentralCapsuleShapeConfig);
 
-        if (m_shapeType != ShapeType::Capsule)
-        {
-            m_shapeConfigs.clear();
-            m_shapeConfigs.emplace_back(AZStd::make_shared<Physics::CapsuleShapeConfiguration>(capsuleShapeConfig));
-
-            m_shapeType = ShapeType::Capsule;
-        }
-        else
-        {
-            Physics::CapsuleShapeConfiguration& configuration =
-                static_cast<Physics::CapsuleShapeConfiguration&>(*m_shapeConfigs.back());
-            configuration = capsuleShapeConfig;
-        }
+        SetShapeConfig(ShapeType::Capsule, capsuleShapeConfig);
 
         m_shapeConfigs.back()->m_scale = scale;
         const float scalarScale = scale.GetMaxElement();
@@ -400,19 +376,7 @@ namespace PhysX
         LmbrCentral::SphereShapeComponentRequestsBus::EventResult(radius, GetEntityId(),
             &LmbrCentral::SphereShapeComponentRequests::GetRadius);
 
-        if (m_shapeType != ShapeType::Sphere)
-        {
-            m_shapeConfigs.clear();
-            m_shapeConfigs.emplace_back(AZStd::make_shared<Physics::SphereShapeConfiguration>(radius));
-
-            m_shapeType = ShapeType::Sphere;
-        }
-        else
-        {
-            Physics::SphereShapeConfiguration& configuration =
-                static_cast<Physics::SphereShapeConfiguration&>(*m_shapeConfigs.back());
-            configuration = Physics::SphereShapeConfiguration(radius);
-        }
+        SetShapeConfig(ShapeType::Sphere, Physics::SphereShapeConfiguration(radius));
         
         m_shapeConfigs.back()->m_scale = scale;
         m_geometryCache.m_radius = scale.GetMaxElement() * radius;
@@ -455,19 +419,7 @@ namespace PhysX
 
         if (shapeConfig.has_value())
         {
-            if (m_shapeType != ShapeType::Cylinder)
-            {
-                m_shapeConfigs.clear();
-                m_shapeConfigs.push_back(AZStd::make_shared<Physics::CookedMeshShapeConfiguration>(shapeConfig.value()));
-
-                m_shapeType = ShapeType::Cylinder;
-            }
-            else
-            {
-                Physics::CookedMeshShapeConfiguration& configuration =
-                    static_cast<Physics::CookedMeshShapeConfiguration&>(*m_shapeConfigs.back());
-                configuration = Physics::CookedMeshShapeConfiguration(shapeConfig.value());
-            }
+            SetShapeConfig(ShapeType::Cylinder, shapeConfig.value());
 
             CreateStaticEditorCollider();
         }

Gems/PhysX/Code/Source/EditorShapeColliderComponent.h

@@ -8,6 +8,7 @@
 
 #pragma once
 
+#include <AzCore/std/smart_ptr/make_shared.h>
 #include <AzCore/Component/TransformBus.h>
 #include <AzCore/Component/NonUniformScaleBus.h>
 #include <AzFramework/Physics/Shape.h>
@@ -90,12 +91,15 @@ namespace PhysX
         void UpdateBoxConfig(const AZ::Vector3& scale);
         void UpdateCapsuleConfig(const AZ::Vector3& scale);
         void UpdateSphereConfig(const AZ::Vector3& scale);
+        void UpdateCylinderConfig(const AZ::Vector3& scale);
         void UpdatePolygonPrismDecomposition();
         void UpdatePolygonPrismDecomposition(const AZ::PolygonPrismPtr polygonPrismPtr);
 
-        void RefreshUiProperties();
+        // Helper function to set a specific shape configuration
+        template<typename ConfigType>
+        void SetShapeConfig(ShapeType shapeType, const ConfigType& shapeConfig);
 
-        void UpdateCylinderConfig(const AZ::Vector3& scale);
+        void RefreshUiProperties();
 
         AZ::u32 OnSubdivisionCountChange();
         AZ::Crc32 SubdivisionCountVisibility();
@@ -154,4 +158,28 @@ namespace PhysX
         AZ::NonUniformScaleChangedEvent::Handler m_nonUniformScaleChangedHandler; //!< Responds to changes in non-uniform scale.
         AZ::Vector3 m_currentNonUniformScale = AZ::Vector3::CreateOne(); //!< Caches the current non-uniform scale.
     };
+
+    template<typename ConfigType>
+    void EditorShapeColliderComponent::SetShapeConfig(ShapeType shapeType, const ConfigType& shapeConfig)
+    {
+        if (m_shapeType != shapeType)
+        {
+            m_shapeConfigs.clear();
+            m_shapeType = shapeType;
+        }
+
+        if (m_shapeConfigs.empty())
+        {
+            m_shapeConfigs.emplace_back(AZStd::make_shared<ConfigType>(shapeConfig));
+        }
+        else
+        {
+            AZ_Assert(m_shapeConfigs.back()->GetShapeType() == shapeConfig.GetShapeType(),
+                "Expected Physics shape configuration with shape type %d but found one with shape type %d.",
+                static_cast<int>(shapeConfig.GetShapeType()), static_cast<int>(m_shapeConfigs.back()->GetShapeType()));
+            ConfigType& configuration =
+                static_cast<ConfigType&>(*m_shapeConfigs.back());
+            configuration = shapeConfig;
+        }
+    }
 } // namespace PhysX

Gems/PhysX/Code/Tests/ShapeColliderComponentTests.cpp

@@ -320,7 +320,7 @@ namespace PhysXEditorTests
     
     TEST_F(PhysXEditorFixture, EditorShapeColliderComponent_ShapeColliderWithCylinderWithNullHeight_HandledGracefully)
     {
-        ValidateInvalidEditorShapeColliderComponentParams(0.f, 1.f);
+        ValidateInvalidEditorShapeColliderComponentParams(1.f, 0.f);
     }
     
     TEST_F(PhysXEditorFixture, EditorShapeColliderComponent_ShapeColliderWithCylinderWithNullRadiusAndNullHeight_HandledGracefully)
@@ -338,6 +338,44 @@ namespace PhysXEditorTests
         ValidateInvalidEditorShapeColliderComponentParams(0.f, -1.f);
     }
 
+    TEST_F(PhysXEditorFixture, EditorShapeColliderComponent_ShapeColliderWithCylinderSwitchingFromNullHeightToValidHeight_HandledGracefully)
+    {
+        // create an editor entity with a shape collider component and a cylinder shape component
+        EntityPtr editorEntity = CreateInactiveEditorEntity("ShapeColliderComponentEditorEntity");
+        editorEntity->CreateComponent<PhysX::EditorShapeColliderComponent>();
+        editorEntity->CreateComponent(LmbrCentral::EditorCylinderShapeComponentTypeId);
+        editorEntity->Activate();
+
+        const float validRadius = 1.0f;
+        const float nullHeight = 0.0f;
+        const float validHeight = 1.0f;
+
+        LmbrCentral::CylinderShapeComponentRequestsBus::Event(editorEntity->GetId(),
+            &LmbrCentral::CylinderShapeComponentRequests::SetRadius, validRadius);
+
+        {
+            UnitTest::ErrorHandler dimensionWarningHandler("Negative or zero cylinder dimensions are invalid");
+            UnitTest::ErrorHandler colliderWarningHandler("No Collider or Shape information found when creating Rigid body");
+
+            LmbrCentral::CylinderShapeComponentRequestsBus::Event(editorEntity->GetId(),
+                &LmbrCentral::CylinderShapeComponentRequests::SetHeight, nullHeight);
+
+            EXPECT_EQ(dimensionWarningHandler.GetExpectedWarningCount(), 1);
+            EXPECT_EQ(colliderWarningHandler.GetExpectedWarningCount(), 1);
+        }
+
+        {
+            UnitTest::ErrorHandler dimensionWarningHandler("Negative or zero cylinder dimensions are invalid");
+            UnitTest::ErrorHandler colliderWarningHandler("No Collider or Shape information found when creating Rigid body");
+
+            LmbrCentral::CylinderShapeComponentRequestsBus::Event(editorEntity->GetId(),
+                &LmbrCentral::CylinderShapeComponentRequests::SetHeight, validHeight);
+
+            EXPECT_EQ(dimensionWarningHandler.GetExpectedWarningCount(), 0);
+            EXPECT_EQ(colliderWarningHandler.GetExpectedWarningCount(), 0);
+        }
+    }
+
     TEST_F(PhysXEditorFixture, EditorShapeColliderComponent_ShapeColliderWithBoxAndRigidBody_CorrectRuntimeComponents)
     {
         // create an editor entity with a shape collider component and a box shape component