Bugfix: CompoundShape::GetWorldSpaceBounds should also return a singl…

…e point when there are no sub shapes (#1470) new MutableCompoundShape() should also return a single point as bounding box
jrouwe · Jan 22, 2025 · 21d4bac · 21d4bac
1 parent 748edb7
commit 21d4bac
Show file tree

Hide file tree

Showing 4 changed files with 38 additions and 11 deletions.
diff --git a/Jolt/Physics/Collision/BroadPhase/QuadTree.cpp b/Jolt/Physics/Collision/BroadPhase/QuadTree.cpp
@@ -57,6 +57,15 @@ void QuadTree::Node::GetChildBounds(int inChildIndex, AABox &outBounds) const
 
 void QuadTree::Node::SetChildBounds(int inChildIndex, const AABox &inBounds)
 {
+	// Bounding boxes provided to the quad tree should never be larger than cLargeFloat because this may trigger overflow exceptions
+	// e.g. when squaring the value while testing sphere overlaps
+	JPH_ASSERT(inBounds.mMin.GetX() >= -cLargeFloat && inBounds.mMin.GetX() <= cLargeFloat
+			   && inBounds.mMin.GetY() >= -cLargeFloat && inBounds.mMin.GetY() <= cLargeFloat
+			   && inBounds.mMin.GetZ() >= -cLargeFloat && inBounds.mMin.GetZ() <= cLargeFloat
+			   && inBounds.mMax.GetX() >= -cLargeFloat && inBounds.mMax.GetX() <= cLargeFloat
+			   && inBounds.mMax.GetY() >= -cLargeFloat && inBounds.mMax.GetY() <= cLargeFloat
+			   && inBounds.mMax.GetZ() >= -cLargeFloat && inBounds.mMax.GetZ() <= cLargeFloat);
+
 	// Set max first (this keeps the bounding box invalid for reading threads)
 	mBoundsMaxZ[inChildIndex] = inBounds.mMax.GetZ();
 	mBoundsMaxY[inChildIndex] = inBounds.mMax.GetY();

diff --git a/Jolt/Physics/Collision/Shape/CompoundShape.cpp b/Jolt/Physics/Collision/Shape/CompoundShape.cpp
@@ -92,7 +92,12 @@ MassProperties CompoundShape::GetMassProperties() const
 
 AABox CompoundShape::GetWorldSpaceBounds(Mat44Arg inCenterOfMassTransform, Vec3Arg inScale) const
 {
-	if (mSubShapes.size() <= 10)
+	if (mSubShapes.empty())
+	{
+		// If there are no sub-shapes, we must return an empty box to avoid overflows in the broadphase
+		return AABox(inCenterOfMassTransform.GetTranslation(), inCenterOfMassTransform.GetTranslation());
+	}
+	else if (mSubShapes.size() <= 10)
 	{
 		AABox bounds;
 		for (const SubShape &shape : mSubShapes)

diff --git a/Jolt/Physics/Collision/Shape/CompoundShape.h b/Jolt/Physics/Collision/Shape/CompoundShape.h
@@ -338,7 +338,7 @@ class JPH_EXPORT CompoundShape : public Shape
 	}
 
 	Vec3							mCenterOfMass { Vec3::sZero() };						///< Center of mass of the compound
-	AABox							mLocalBounds;
+	AABox							mLocalBounds { Vec3::sZero(), Vec3::sZero() };
 	SubShapes						mSubShapes;
 	float							mInnerRadius = FLT_MAX;									///< Smallest radius of GetInnerRadius() of child shapes
 

diff --git a/UnitTests/Physics/ShapeTests.cpp b/UnitTests/Physics/ShapeTests.cpp
@@ -752,22 +752,35 @@ TEST_SUITE("ShapeTests")
 	TEST_CASE("TestEmptyMutableCompound")
 	{
 		// Create empty shape
-		RefConst<Shape> mutable_compound = new MutableCompoundShape();
+		Ref<MutableCompoundShape> mutable_compound = new MutableCompoundShape();
 
 		// A non-identity rotation
 		Quat rotation = Quat::sRotation(Vec3::sReplicate(1.0f / sqrt(3.0f)), 0.1f * JPH_PI);
 
-		// Check that local bounding box is invalid
+		// Check that local bounding box is a single point
 		AABox bounds1 = mutable_compound->GetLocalBounds();
-		CHECK(!bounds1.IsValid());
+		CHECK(bounds1 == AABox(Vec3::sZero(), Vec3::sZero()));
 
-		// Check that get world space bounds returns an invalid bounding box
-		AABox bounds2 = mutable_compound->GetWorldSpaceBounds(Mat44::sRotationTranslation(rotation, Vec3(100, 200, 300)), Vec3(1, 2, 3));
-		CHECK(!bounds2.IsValid());
+		// Check that get world space bounds returns a single point
+		Vec3 vec3_pos(100, 200, 300);
+		AABox bounds2 = mutable_compound->GetWorldSpaceBounds(Mat44::sRotationTranslation(rotation, vec3_pos), Vec3(1, 2, 3));
+		CHECK(bounds2 == AABox(vec3_pos, vec3_pos));
 
-		// Check that get world space bounds returns an invalid bounding box for double precision parameters
-		AABox bounds3 = mutable_compound->GetWorldSpaceBounds(DMat44::sRotationTranslation(rotation, DVec3(100, 200, 300)), Vec3(1, 2, 3));
-		CHECK(!bounds3.IsValid());
+		// Check that get world space bounds returns a single point for double precision parameters
+		AABox bounds3 = mutable_compound->GetWorldSpaceBounds(DMat44::sRotationTranslation(rotation, DVec3(vec3_pos)), Vec3(1, 2, 3));
+		CHECK(bounds3 == AABox(vec3_pos, vec3_pos));
+
+		// Add a shape
+		mutable_compound->AddShape(Vec3::sZero(), Quat::sIdentity(), new BoxShape(Vec3::sReplicate(1.0f)));
+		AABox bounds4 = mutable_compound->GetLocalBounds();
+		CHECK(bounds4 == AABox(Vec3::sReplicate(-1.0f), Vec3::sReplicate(1.0f)));
+
+		// Remove it again
+		mutable_compound->RemoveShape(0);
+
+		// Check that the bounding box has zero size again
+		AABox bounds5 = mutable_compound->GetLocalBounds();
+		CHECK(bounds5 == AABox(Vec3::sZero(), Vec3::sZero()));
 	}
 
 	TEST_CASE("TestSaveMeshShape")