From 21d4bac7086d4e023b4e7463a14c269f4df28cb8 Mon Sep 17 00:00:00 2001 From: Jorrit Rouwe Date: Wed, 22 Jan 2025 14:00:46 +0100 Subject: [PATCH] Bugfix: CompoundShape::GetWorldSpaceBounds should also return a single point when there are no sub shapes (#1470) new MutableCompoundShape() should also return a single point as bounding box --- .../Physics/Collision/BroadPhase/QuadTree.cpp | 9 ++++++ .../Physics/Collision/Shape/CompoundShape.cpp | 7 ++++- Jolt/Physics/Collision/Shape/CompoundShape.h | 2 +- UnitTests/Physics/ShapeTests.cpp | 31 +++++++++++++------ 4 files changed, 38 insertions(+), 11 deletions(-) diff --git a/Jolt/Physics/Collision/BroadPhase/QuadTree.cpp b/Jolt/Physics/Collision/BroadPhase/QuadTree.cpp index 4908248fd..0814a2464 100644 --- 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 index f5fe04ecf..5f2730961 100644 --- 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 index 0bbd7c4d7..65fc23434 100644 --- 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 index 2cffd0e05..f854364e2 100644 --- a/UnitTests/Physics/ShapeTests.cpp +++ b/UnitTests/Physics/ShapeTests.cpp @@ -752,22 +752,35 @@ TEST_SUITE("ShapeTests") TEST_CASE("TestEmptyMutableCompound") { // Create empty shape - RefConst mutable_compound = new MutableCompoundShape(); + Ref 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")