GDExtension: pointer parameters of type `real` are declared as `float*` in JSON

[Bromeon]

Tested versions

4.2.2

System information

N/A

Issue description

The virtual function PhysicsDirectSpaceState2D::_cast_motion() uses pointer parameters, and it's unclear if the type is declared correctly in the API spec.

The extension_api.json entry for that function looks as follows:

{
    "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": "float"
        },
        {
            "name": "collision_mask",
            "type": "int",
            "meta": "uint32"
        },
        {
            "name": "collide_with_bodies",
            "type": "bool"
        },
        {
            "name": "collide_with_areas",
            "type": "bool"
        },
        {
            "name": "closest_safe",
            "type": "float*"
        },
        {
            "name": "closest_unsafe",
            "type": "float*"
        }
    ]
},

So both pointers are declared as float*.

float usually means 64-bit floating point (C++ double), unless it's accompanied by meta = float key-value, see e.g. margin argument.

However, the method on C++ side is declared as real, which is 32-bit floating point in single precision builds: https://github.com/godotengine/godot/blob/1567a498cbe0d2aeb73a9d2a67d3ac6b22b340a1/servers/physics_2d/godot_space_2d.h#L54

And I also found this: https://github.com/godotengine/godot/blob/1567a498cbe0d2aeb73a9d2a67d3ac6b22b340a1/servers/extensions/physics_server_2d_extension.h#L133

CC @Ughuuu, who noticed this and had a crash in this code.

---

I would expect that if a type is declared as T* (here float*) in the JSON, that it behaves like a pointer to T. Otherwise, a meta could be used, just like value types.

Is this assumption wrong?

Steps to reproduce

Override _cast_motion virtual method (e.g. in Rust bindings), try to dereference the f64 pointers -> UB.

Minimal reproduction project (MRP)

N/A

[dsnopek]

I did a little bit of testing, and it looks like it'll use a "type" of "double*" if it's 64-bit. So, currently, we have "float*" to indicate 32-bit, and "double*" to indicate 64-bit.

I agree that that is inconsistent with how we're using "float" elsewhere to mean either 32-bit or 64-bit, depending on what is in the "meta" (defaulting to 64-bit).

But it is consistent with other pointer types! For example, integer pointers use types like "int32_t*", rather than something like {"type": "int*", "meta": "int32"}.

So, we could either:

1. Decide that pointer types are special, and "float*" is 32-bit, while "float" is 64-bit by default (with "meta" to maybe make it 32-bit). This is a little weird, but if we actually documented extension_api.json it wouldn't be that big of a deal. :-)

2. Change the way that pointer types are done so that we instead do something like {"type": "float*", "meta": "float"}. However, if we do this, we should probably do it for all the pointer types (including the integer and char types), which I think would amount to a pretty large source compatibility breakage. (I wonder if we should version extension_api.json to account for inevitable changes?)

I guess my personal inclination is to go with option 1, but I could be convinced of option 2 if we can come up with a way to minimize the compat issues.

[Bromeon]

Ah OK, so it's not actually wrong, just a bit unintuitive 😉

The problem is, even if we went with 2), we would still need to understand 1) because of older versions (godot-rust supports all Godot versions). So we'd need to have two code paths.

On the other hand, people who are writing newer bindings may stumble upon this, too... Maybe documentation would really solve this? What would be a good place for this to be documented (I assume godot-docs repo, but where)?

[dsnopek]

Maybe documentation would really solve this? What would be a good place for this to be documented (I assume godot-docs repo, but where)?

There has been lots of discussion about how to re-arrange the structure of the GDExtension docs, however, for now it looks like we're putting everything under tutorials/scripting/gdextension/, so I think putting it there for now makes sense.

Even the PRs documenting the structure of .gdextension files and George's monumental C example are adding their pages in that same place. We can move all those pages later if we come up with a better way to organize it.

Anyway, it would be really amazing to have some documentation for the extension_api.json file!

[dsnopek]

Discussed at the GDExtension meeting and we decided to stick with the current representation in the extension_api.json, because the "meta" for normal floats is more of "advice" for the binding generator since we always encode the data as double, however, for pointers the difference between float* and double* change the underlying type of the data, so it does make sense that these two things are represented differently.

So, I guess the next step is documenting these types in the extension_api.json.