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.

[akien-mga]

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.

This may be a silly idea, but could we somehow generate a hash from the type names in the struct, so it changes if fields are reordered, removed or added?

[mihe]

This may be a silly idea, but could we somehow generate a hash from the type names in the struct, so it changes if fields are reordered, removed or added?

I assume that means that you're feeding the types and names into some place for it to be hashed, at which point you might as well just replace GDREGISTER_NATIVE_STRUCT with that facility and thereby always have an accurate binding instead of the manually written to string that we have now.

[dsnopek]

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.

This is exactly how we're planning on handling changes to structs in gdextension_interface.h. We haven't fully done one yet, but there's currently 3 PRs that want to change GDExtensionClassCreationInfo. So far only PR #78634 has started adding the compatibility bits.

Doing this in the engine itself (ie not isolated to gdextension_interface.h) feels messier, but maybe it's the right way to do it?

This may be a silly idea, but could we somehow generate a hash from the type names in the struct, so it changes if fields are reordered, removed or added?

I think we could maybe wrap the whole original struct declaration in a macro that might be able to do this? Or, maybe include special comments that would allow a SCons tool to scan the source file and get the struct declaration? Both options are a little hacky, though.

[akien-mga]

We should revisit this ideally before beta 1, so we don't have to break compatibility further during beta.

[dsnopek]

Eek, did we not actually fix the compatibility break here? At minimum, we should move the face_index property to the end of the structure. This isn't a "complete solution" to the problem, but that would at least be compatible.

[mihe]

At minimum, we should move the face_index property to the end of the structure.

Yeah, I'm cooking up a PR for it as we speak.

[mihe]

PR for moving face_index created as #82403.