[PhysicsDirectSpaceState] Impossible to retrieve shape if owner_id has more than one shape

[elim2g]

Godot version

3.4.2.stable [Bullet Physics]

System information

Win10

Issue description

I've been doing some custom collision stuff, and that has involved using intersect_shape from PhysicsDirectSpaceState.

Now according to the documentation for this we have the following:

Checks the intersections of a shape, given through a PhysicsShapeQueryParameters object, against the space. The intersected shapes are returned in an array containing dictionaries with the following fields:

collider: The colliding object.

collider_id: The colliding object's ID.

rid: The intersecting object's RID.

shape: The shape index of the colliding shape.

To retrieve the actual Shape, I have this code running:

var collide_shapes: Array = space_state.intersect_shape(query_params);

for i in collide_shapes.size():
    var col = collide_shapes[i];
    var s_idx: int = col.shape;
    var shape_owner_id = col.collider.shape_find_owner(s_idx);
    var colshape = col.collider.shape_owner_get_shape(shape_owner_id, s_idx);

In this case, s_idx is 500. When I get the shape_owner_id, it's also 500. This is expected since I'm using regular CollisionShapes, and they appear to create one owner_id for one Shape.

However it all breaks down in the final line - shape_owner_get_shape actually expects the index of the shape in its own internal array of shapes, and not the actual shape index that Godot generates for a Shape.

Simple solution, right? Change the last line to:
var colshape = col.collider.shape_owner_get_shape(shape_owner_id, 0);
and it all works fine. Awesome!

So what's the issue?
Well according to the documentation on CollisionObject, we can read the following:

The CollisionObject can have any number of shape owners. Shape owners are not nodes and do not appear in the editor, but are accessible through code using the shape_owner_* methods.

Now this is a problem! If we have more than one Shape per owner_id, then how do we know what index to pass into shape_owner_get_shape()?

For proof that this function is not behaving as documented, I've taken this directly from Godot's C++ source:

Ref<Shape3D> CollisionObject3D::shape_owner_get_shape(uint32_t p_owner, int p_shape) const {
	ERR_FAIL_COND_V(!shapes.has(p_owner), Ref<Shape3D>());
	ERR_FAIL_INDEX_V(p_shape, shapes[p_owner].shapes.size(), Ref<Shape3D>());

	return shapes[p_owner].shapes[p_shape].shape;
}

That shapes member is a simple Vector, and not a mapping of any kind.

This means there's either an oversight or a bug somewhere. If intersect_shape were to return a local array offset index, then we would never be able to get the owner_id. But in its current state, we'll never be able to find our Shape if the owner has more than one, assuming that the shape was created with a shape index that's higher than the length of the internally stored array of owned shapes.

Steps to reproduce

I've attached a minimal example project to demonstrate the following steps:

• Create a bunch of CollisionShapes on a StaticBody

• When the game starts, remove the first owner_id (since one owner_id will be created for each shape)

• Grab the now-unreferenced shape and add it to a different owner_id [There will now be one owner_id with two shapes]

• Perform a intersect_shape against the shapes in the scene

• Try and find a way to obtain the 2nd shape from the owner_id containing multiple shapes using the result of the intersect_shape, the shape index it provides and the CollisionObject.shape_owner_* functions.

You'll quickly notice it's impossible using the methods given to us.

Of course, there's probably a hackaround by comparing instance_ids, but this seems like a bug or an oversight and I believe it requires some attention.

===
MINIMAL REPRO PROJECT

Please see multiple_shapes.gs to see what I'm doing regarding setting up collision shapes and their owners.
Then take a look in reproduce.gd to see where the problems lie with intersect_shapes.

Minimal reproduction project

shape_id_problem.zip

[elim2g]

At the risk of not understanding how the shapes are indexed here, this seems a bit odd:

int CollisionObject3D::shape_owner_get_shape_index(uint32_t p_owner, int p_shape) const {
	ERR_FAIL_COND_V(!shapes.has(p_owner), -1);
	ERR_FAIL_INDEX_V(p_shape, shapes[p_owner].shapes.size(), -1);

	return shapes[p_owner].shapes[p_shape].index;
}

So this function looks like what one would need to get the appropriate "child index" in the p_owner's vector of shapes, however if we look at how they are added:

void CollisionObject3D::shape_owner_add_shape(uint32_t p_owner, const Ref<Shape3D> &p_shape) {
	ERR_FAIL_COND(!shapes.has(p_owner));
	ERR_FAIL_COND(p_shape.is_null());

	ShapeData &sd = shapes[p_owner];
	ShapeData::ShapeBase s;
	s.index = total_subshapes;
	s.shape = p_shape;

        // etc
        
	sd.shapes.push_back(s);

	total_subshapes++;

	_update_shape_data(p_owner);
}

we can see that s.index will always be its position in the array, effectively making shape_owner_get_shape_index completely redundant. This is solidified further by looking at shape_owner_remove_shape, where it appears to cycle through the rest of the shapes that weren't removed, and decrementing their index so that it stays equivalent to their position within the array.

My intuition tells me that shape_owner_get_shape_index should look something like the following instead:

int CollisionObject3D::shape_owner_get_shape_index(uint32_t p_owner, int p_shape) const {
	ERR_FAIL_COND_V(!shapes.has(p_owner), -1);

	for (const ShapeBase &sb : shapes[p_owner].shapes) {
		if (sb.shape.shape == p_shape) {
			return sb.index;
		}
	}

	return -1; // or some other error, probably an assert
}

Now I'm not sure if the RID is what we would be comparing here, but hopefully you see what I'm trying to get across.

[rburing]

This is a documentation issue. The return values of PhysicsDirectSpaceState3D.intersect_shape can be used with e.g. PhysicsServer3D.body_get_shape.

The CollisionObject3D node keeps a separate organization of shapes per shape owner (the internal variable/class names are just a bit awkward and could be improved), which you can just ignore in this case. If you really want you can use CollisionObject3D.shape_find_owner to find the owner, and then I guess a method could be added to find the ID (= local index) of the shape in the shape owner (for now, you could iterate over all of them to find it).

[Pizzaandy]

Here's the method I use to obtain the shape from intersect_shape results:

# Usage:
# var results = get_world_2d().direct_space_state.intersect_shape(params)
# for result in results:
#	var shape_info = get_shape_info(result)

func get_shape_info(result: Dictionary) -> Dictionary:
	var collider = result["collider"] as CollisionObject2D
	var shape_index = result["shape"] as int
	
	var owner_id = collider.shape_find_owner(shape_index)
	var shape_count = collider.shape_owner_get_shape_count(owner_id)

	var shape: Shape2D
	var shape_transform: Transform2D = collider.global_transform * collider.shape_owner_get_transform(owner_id)

	for i in range(shape_count):
		var index = collider.shape_owner_get_shape_index(owner_id, i)
		if index == shape_index:
			shape = collider.shape_owner_get_shape(owner_id, i)
			break

	return {"shape": shape, "transform": shape_transform}

This is kind of a mess... I don't know how we can improve this without breaking compatibility. The terms "shape id" and "shape index" are treated as interchangeable in the current API, which is pretty misleading (shape_owner_get_shape_index returns a value that can be passed as a shape_id argument). I worry that adding another method to find the local index of a shape would muddy the waters even further.

Improving documentation is probably the best we can do for now.