Extension bindings for cast_motion are incorrect in double-precision builds

[mihe]

Tested versions

Reproducible in: 4.3.dev [b15105a]

System information

N/A

Issue description

PhysicsDirectBodyState3DExtension::_cast_motion, which is overridden by a GDExtension to implement PhysicsDirectBodyState3D::cast_motion, has the resulting safe and unsafe fractions bound using GDExtensionPtr<real_t>, as seen here:

godot/servers/extensions/physics_server_3d_extension.h

Line 135 in b15105a

GDVIRTUAL10R(bool, _cast_motion, RID, const Transform3D &, const Vector3 &, real_t, uint32_t, bool, bool, GDExtensionPtr<real_t>, GDExtensionPtr<real_t>, GDExtensionPtr<PhysicsServer3DExtensionShapeRestInfo>)

godot/servers/extensions/physics_server_2d_extension.h

Line 133 in b15105a

GDVIRTUAL9R(bool, _cast_motion, RID, const Transform2D &, const Vector2 &, real_t, uint32_t, bool, bool, GDExtensionPtr<real_t>, GDExtensionPtr<real_t>)

This unfortunately leads to --dump-extension-api emitting a function signature of float* into extension_api.json, presumably because real_t isn't an actual nominal type, which means that even double-precision builds of a GDExtension end up implementing the function as float* when in fact a real_t* is being passed in (and moved across as void*) from the Godot side of things.

This then means that any result written to these fractions in double-precision builds end up garbled on the other end, leading to shape-casts being broken.

In terms of fixing this, I see the choices as being either:

1. Change the GDExtensionPtr<real_t> to GDExtensionPtr<float>. The extra precision seems overkill for something like this anyway. This however means it will break for anyone who might be currently be casting this to real_t*, like myself, although I haven't actually pushed any release with this yet.
2. Change the GDExtensionPtr<real_t> to GDExtensionPtr<double>, which will then break every precision=single extension build instead, as well as being a breaking change in terms of compilation (for godot-cpp).
3. Somehow allow for extension_api.json to emit real_t* for the function signature, which would be a breaking change in terms of compilation for precision=double builds, but would preserve the workaround mentioned in option 1.

EDIT: I suppose a 4th option could be to expose an entirely new method, like _cast_motion2, which uses GDExtensionPtr<double> instead, but it seems unfortunate to leave the old broken method as-is.

Steps to reproduce

N/A

Minimal reproduction project (MRP)

N/A

[akien-mga]

CC @godotengine/gdextension

[dsnopek]

My personal preference would be for GDExtensionPtr<real_t> to cause extension_api.json to have float * on single precision builds, and double * on double precision builds. The extension_api.json is already different between double and single precision builds (including for non-pointer real_t arguments), so it would make sense for this case too.

[dsnopek]

Hm, I just built Godot with double precision and ran with --dump-extension-api and they do already appear to be double *'s in the extension_json.api:

JSON data

				{
					"name": "_cast_motion",
					"is_const": false,
					"is_static": false,
					"is_vararg": false,
					"is_virtual": true,
					"return_value": {
						"type": "bool"
					},
					"arguments": [
						{
							"name": "shape_rid",
							"type": "RID"
						},
						{
							"name": "transform",
							"type": "Transform2D"
						},
						{
							"name": "motion",
							"type": "Vector2"
						},
						{
							"name": "margin",
							"type": "float",
							"meta": "double"
						},
						{
							"name": "collision_mask",
							"type": "int",
							"meta": "uint32"
						},
						{
							"name": "collide_with_bodies",
							"type": "bool"
						},
						{
							"name": "collide_with_areas",
							"type": "bool"
						},
						{
							"name": "closest_safe",
							"type": "double*"
						},
						{
							"name": "closest_unsafe",
							"type": "double*"
						}
					]
				}

Are you maybe trying to use the extension_api.json from a single precision Godot to build an extension with double precision?

[mihe]

Are you maybe trying to use the extension_api.json from a single precision Godot to build an extension with double precision?

Yes, that seems to be the crux here. For some reason I was under the impression that the default/bundled extension_api.json was compatible with both single- and double-precision builds, seeing as how the code emitted from godot-cpp's binding generator seems to translate every non-pointer real_t to a double regardless of precision, but I see now that they're in fact specified as float in extension_api.json as well, and that instead every single float is made into a double.

But alright, I suppose I can just generate a custom precision=double version of extension_api.json for the minimum Godot version I'm targeting.

Is there any particular reason why the same extension_api.json couldn't be used (in godot-cpp specifically), besides this obvious incompatibility of GDExtensionPtr<real_t>? Is it to cater to the possibility of having completely different bindings based on REAL_T_IS_DOUBLE or something?

Anyway, I'll go ahead and close this.

[dsnopek]

Is there any particular reason why the same extension_api.json couldn't be used (in godot-cpp specifically), besides this obvious incompatibility of GDExtensionPtr<real_t>?

Well, they are binary incompatible, the function signatures are actually different. The main consequence of this is that a number function hashes are different, but it also has an impact on virtual functions as shown here. We've discussed this a couple of times, the most recent one was here.

But at a higher-level, if you're re-compiling Godot with different options than the official builds, you can end up with differences that affect compatibility (for example, some additional modules may be enabled or disabled), and so any time you're doing that, you should generate your own extension_api.json

[mihe]

Well, they are binary incompatible, the function signatures are actually different. The main consequence of this is that a number function hashes are different

I'm very surprised that I didn't run into any issues with this at all. Everything ran just fine with no warnings or errors, and I was able to utilize the extra precision as expected. I guess it's just about hitting the right API call.

[dsnopek]

It'd probably be a good idea add something to godot-cpp that will detect if the extension_api.json was generated with a single or double precision build (if we can even detect that - we may need some changes to the JSON) and give an error if you try to build your extension with a mismatching setting. I'll add looking into that to my TODO :-)