`PhysicsDirectSpaceState3D::RayResult` is not correctly bound

[mihe]

Godot version

4.2.dev [f7bc653]

System information

Windows 11 (10.0.22621)

Issue description

#71233 introduced a new field to RayResult called face_index, as seen here:

godot/servers/physics_server_3d.h

Lines 151 to 159 in f7bc653

	struct RayResult {
	Vector3 position;
	Vector3 normal;
	int face_index = -1;
	RID rid;
	ObjectID collider_id;
	Object *collider = nullptr;
	int shape = 0;
	};

... but unfortunately the binding for RayResult (aliased as PhysicsServer3DExtensionRayResult) wasn't updated, as seen here:

godot/servers/register_server_types.cpp

Line 160 in f7bc653

GDREGISTER_NATIVE_STRUCT(PhysicsServer3DExtensionRayResult, "Vector3 position;Vector3 normal;RID rid;ObjectID collider_id;Object *collider;int shape");

... which leads to extension_api.json being emitted with a different struct layout for PhysicsServer3DExtensionRayResult compared to Godot's RayResult.

Since PhysicsDirectSpaceState3DExtension::_intersect_ray directly manipulates a pointer to a RayResult, you end up corrupting it when writing to anything past the normal field from a GDExtension, due to mismatching binary layouts.

Steps to reproduce

• Download the MRP
• Run the project's main scene with Godot 4.1.1
• Note how the output mentions a non-null collider
• Run the project's main scene with Godot 4.2 (after a56e960)
• Note how the output mentions a null collider, as well as other errors (due to the instance ID being corrupted)

Minimal reproduction project

RayResult_MRP.zip

[mihe]

Just to clarify, the MRP here does technically use an extension that targets 9704596 (4.1-stable), but this is not an issue of being incompatible with 4.1 (despite my previous poorly chosen title). Even if you update the extension to use the latest extension_api.json you will still end up with mismatching struct layouts and run into the exact same problem of writing to the wrong fields.

[PrecisionRender]

Would simply updating the binding be an acceptable short-term fix?

[mihe]

It might be a good idea to do it just to get things to a somewhat correct state, although that will presumably introduce a CI failure (from --validate-extension-api) until the bigger issue of allowing for struct layout changes is resolved somehow.

To my knowledge the only extension affected by this is Godot Jolt, and even if the binding were to be updated it'd still be incompatible since Godot Jolt expects the RayResult from 4.1. To remain compatible with that you'd need to also move face_index to the end of RayResult, which would prevent any previously existing fields from having their offset shifted. However, I have no plans to support 4.2 until stable is released some time later this year anyway, so there's no immediate fire that needs to be put out right now as far as I'm concerned.

[dsnopek]

Hm. Is PhysicsServer3DExtensionRayResult only ever passed from Godot to GDExtension? Or, does GDExtension also pass PhysicsServer3DExtensionRayResults to Godot?

If the former, then we can do as @mihe suggests and move the face_index field to the end of the struct. It will be ignored by older GDExtensions that don't even know that field is there. However, if the latter happens, then we're going to need a much more complicated fix!

Do we also have an issue about how to handle the process side of this in the future? We need to come up with a way for these sort of breaking changes to not slip through so easily.

[dsnopek]

For the problem of catching these sort of issues in the future, I wonder if we could put the Godot struct's size into the extension_api.json, like:

GDREGISTER_NATIVE_STRUCT(PhysicsServer3DExtensionRayResult, "Vector3 position;Vector3 normal;RID rid;ObjectID collider_id;Object *collider;int shape", sizeof(PhysicsDirectSpaceState3D::RayResult)); 

This wouldn't catch re-ordering of the struct's fields (which would also break compatibility), but it would have caught the issue here when a new field is added.

[mihe]

Is PhysicsServer3DExtensionRayResult only ever passed from Godot to GDExtension?

That specific struct is only ever instantiated from Godot and written to through a pointer from GDExtension, so the workaround would suffice in this particular case.

However, this type of workaround wouldn't work for some imaginary future breakage of things like PhysicsServer3DExtensionMotionCollision (aka PhysicsServer3D::MotionCollision), since that struct is written to as part of an array from PhysicsServer3DExtension::body_test_motion, and as such is dependent on the size and layout of the struct being the same on both ends. So I feel like there's still some bigger discussion to be had with regards to allowing for struct layout changes in the future. Although, I'm honestly not sure what you could even do about that particular struct, in terms of preserving compatibility, other than to create a whole new version of body_test_motion that uses a different struct.

I wonder if we could put the Godot struct's size into the extension_api.json

That sounds like it would be an improvement at least, in terms of catching any added/removed fields.

Do we also have an issue about how to handle the process side of this in the future?

Not that I'm aware of.