body_shape_entered yields extraneous shape indices on rigidbodies with only one shape (RigidBody with Bullet)

[crodnu]

Godot version:
v3.2.1 mono 64 bits

OS/device including version:
Windows 10, also happens in linux machines

Issue description:
When a rigidbody with only one shape in it (either added by a collision shape or one added programatically with the shape_owner syntax) the body_shape_entered signal yields extraneous shape indices (this can happen both on the body_shape and the local_shape parameters). The signal seems to work correctly more than one shape are present. This error is present both in GDScript and in c# (examples for the latter not included).

Steps to reproduce:

1. Add 2 rigidbody nodes with one shape in them.
2. Connect the signal to a function that prints the parameters body_shape and local_shape.
3. Collide.

Minimal reproduction project:
Tests
All the tests print the body_shape and local_shape of the signal in question, catched by the upper sphere.
Test 1 and 4 showcase the error in question (1 adds the shapes programatically while 4 uses CollisionShapes from the editor).
Test 2 shows the signal working correctly by adding one more shape to the same shape owner.
Test 3 shows the signal working by adding a different shape owner with a different shape to the rigidbody.

[akien-mga]

Confirmed in 3.2.2 RC 3.

This seems to happen with Bullet but not GodotPhysics, so I guess Bullet is using some uninitialized memory.

[pouleyKetchoupp]

Here's what I found about this issue.

The members mIndex0 & mIndex1 in Bullet manifolds that we use for getting shape indices are not initialized by default (unless DEBUG_PART_INDEX was defined which is not the case by default):

godot/thirdparty/bullet/BulletCollision/CollisionDispatch/btManifoldResult.h

Lines 60 to 71 in b92477d

	btManifoldResult()
	:
	#ifdef DEBUG_PART_INDEX
	m_partId0(-1),
	m_partId1(-1),
	m_index0(-1),
	m_index1(-1)
	#endif //DEBUG_PART_INDEX
	m_closestPointDistanceThreshold(0)
	{
	}

In bullet, this is only set when btManifoldResult::setShapeIdentifiersA and btManifoldResult::setShapeIdentifiersB methods are called.

These indices are set to child shape indices for compound shapes (which is what we want):

godot/thirdparty/bullet/BulletCollision/CollisionDispatch/btCompoundCompoundCollisionAlgorithm.cpp

Lines 198 to 199 in b92477d

	m_resultOut->setShapeIdentifiersA(-1, childIndex0);
	m_resultOut->setShapeIdentifiersB(-1, childIndex1);

And they are also set to triangle indices for concave shapes (which is not what we want):

godot/thirdparty/bullet/BulletCollision/CollisionDispatch/btConvexConcaveCollisionAlgorithm.cpp

Lines 122 to 133 in b92477d

	if (m_resultOut->getBody0Internal() == m_triBodyWrap->getCollisionObject())
	{
	tmpWrap = m_resultOut->getBody0Wrap();
	m_resultOut->setBody0Wrap(&triObWrap);
	m_resultOut->setShapeIdentifiersA(partId, triangleIndex);
	}
	else
	{
	tmpWrap = m_resultOut->getBody1Wrap();
	m_resultOut->setBody1Wrap(&triObWrap);
	m_resultOut->setShapeIdentifiersB(partId, triangleIndex);
	}

That means a static body with multiple concave shapes also reports wrong values if what we expect is the shape index, as it would be the triangle index instead (tested on 4.0 branch, that's what happens with Bullet, while GodotPhysics reports the shape index as expected).

So there's an easy quick fix we can do on Godot side to solve the current issue in simple cases, by making sure the shape index is set to 0 in case of single shapes in SpaceBullet::check_body_collision:

godot/modules/bullet/space_bullet.cpp

Lines 835 to 846 in b92477d

	if (bodyA->can_add_collision()) {
	B_TO_G(pt.getPositionWorldOnB(), collisionWorldPosition);
	/// pt.m_localPointB Doesn't report the exact point in local space
	B_TO_G(pt.getPositionWorldOnB() - contactManifold->getBody1()->getWorldTransform().getOrigin(), collisionLocalPosition);
	bodyA->add_collision_object(bodyB, collisionWorldPosition, collisionLocalPosition, normalOnB, appliedImpulse, pt.m_index1, pt.m_index0);
	}
	if (bodyB->can_add_collision()) {
	B_TO_G(pt.getPositionWorldOnA(), collisionWorldPosition);
	/// pt.m_localPointA Doesn't report the exact point in local space
	B_TO_G(pt.getPositionWorldOnA() - contactManifold->getBody0()->getWorldTransform().getOrigin(), collisionLocalPosition);
	bodyB->add_collision_object(bodyA, collisionWorldPosition, collisionLocalPosition, normalOnB * -1, appliedImpulse * -1, pt.m_index0, pt.m_index1);
	}

By using something like that:

int shapeA = (bodyA->get_shape_count() > 1) ? pt.m_index0 : 0;
int shapeB = (bodyB->get_shape_count() > 1) ? pt.m_index1 : 0;

But that wouldn't solve the case with a body with multiple concave shapes, as this would still report triangle indices instead of shape indices. The information about shape indices is currently lost in this case AFAIK.

@AndreaCatania: Do you know of any way to work around this issue with concave shape manifolds reporting triangle indices instead of shape indices? Or do you think it's something that would require a fix in Bullet?

[Shfty]

I'm seeing what looks like the same issue using PhysicsDirectSpaceState::intersect_ray under Bullet. Tracing a single ray down onto a StaticBody with a single CollisionShape comprised of a cube ConcavePolygonShape causes 9 to come back as the shape index, whereas GodotPhysics returns the expected 0.

This is a significant blocker for 3D physics work - I'm currently trying to implement a custom intersect_ray-based movement system due to various issues with move_and_slide, and ConcavePolygonShape seems to be the only way to get consistent intersection results from the Bullet backend. Unfortunately, the returned normals for ConcavePolygonShape don't account for CollisionShape scale and need to be manually fixed up in script, but that can't happen without being able to look up the hit shape and query its transform.

As an aside, i's interesting that the incorrect values are being driven by triangle index for ConcavePolygonShape intersections - that would be useful data to have as its own key in the result dictionary alongside correct shape indices.

[madmiraal]

So there's an easy quick fix we can do on Godot side to solve the current issue in simple cases, by making sure the shape index is set to 0 in case of single shapes in SpaceBullet::check_body_collision
...
But that wouldn't solve the case with a body with multiple concave shapes, as this would still report triangle indices instead of shape indices. The information about shape indices is currently lost in this case AFAIK.

I think the "easy, quick fix" makes sense. We could also set it to zero if the partId is not -1 i.e. there is a ConcavePolygonShape. The scenario where there are multiple shapes and one or more is a ConcavePolygonShape is an unlikely one. We can document the failure to detect the shape id if a ConcavePolygonShape is used.

Do you know of any way to work around this issue with concave shape manifolds reporting triangle indices instead of shape indices? Or do you think it's something that would require a fix in Bullet?

It looks like the problem is the other way around. The index should be the triangle index, but compound shapes have hijacked it to report the child index. The change to support the triangle index was done here, and later the change to use it to store the child index was done here. So there's no easy fix.