godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.41k stars 21.26k forks source link

Invalid case for `PlaceHolder` & `UserData` in GDExtension function pointers #95345

Open touilleMan opened 3 months ago

touilleMan commented 3 months ago

Tested versions

Godot 4.2 to master

System information

all platforms

Issue description

https://github.com/godotengine/godot/blob/88f3b5f9d52f740b24fabfb8bc01b8b7026ba279/core/extension/gdextension_interface.h#L2606-L2621

In gdextension_interface.h, functions placeholder_script_instance_create & placeholder_script_instance_update are incorrectly capitalized in their corresponding function pointer signatures (GDExtensionInterfacePlaceHolderScriptInstanceCreate & GDExtensionInterfacePlaceHolderScriptInstanceUpdate)

So placeholder in snake case becomes PlaceHolder in camelcase.

This is an issue when using automated tools that convert back the camelcase into snakecase (resulting in place_holder_script_instance_create, for instance in Godot-Python)

Fortunately Godot displays a warning when pythonscript_gdextension_get_proc_address is used on an unknown function (ERROR: Attempt to get non-existent interface function: place_holder_script_instance_create.). But this is nevertheless an anoying footgun...

The fix would simply be to rename PlaceHolder -> Placeholder. This would obviously breaks any code using those methods (though fixing is trivial), I'm not aware of the policy concerning GDExtension compat but I guess this kind of break is only allowed when bumping minor version (so 4.3, or even 4.4 given 4.3 is right around the corner ^^).

Steps to reproduce

n/a

Minimal reproduction project (MRP)

n/a

akien-mga commented 3 months ago

Tested versions

Godot 4.0 to master

Did you? ;) the docs say it's a new API in 4.2. Still means breaking compat with 4.2/4.3 if/when we change this, not sure how it can be handled for object types like this.

touilleMan commented 3 months ago

Did you? ;) the docs say it's a new API in 4.2.

image

Sorry for mistake, I did check the code on branch 4.0 but I have no idea how I ended up thinking the bug was present there (while as you say it's only a 4.2+ bug)

BTW, there is another method with similar issue (also added in 4.2): callable_custom_get_userdata https://github.com/godotengine/godot/blob/88f3b5f9d52f740b24fabfb8bc01b8b7026ba279/core/extension/gdextension_interface.h#L2676

dsnopek commented 3 months ago

Sorry I didn't see this last week! We could have perhaps changed the PlaceHolder ones because they were new in 4.3, but at this point we can't, at least with the way we are currently maintaining the GDExtension interface.

An idea was proposed and discussed at a GDExtension meeting (I don't think there's a formal proposal - I should write one!) of keeping the GDExtension interface in XML or JSON or some machine readable format and then generating the header file. If we did that, I think we'd have a lot more freedom in how we can change this.

What I'm envisioning is storing like name and legacy_name in the data, and then we could generate either a legacy or "latest" version of the gdextension_interface.h. We've discussed other changes in the past, like switching from void *s everywhere to opaque pointers, which would allow the compiler to do more type checking for us, but doing so would be a compat break - again we could have the void *s in the legacy version, and opaque pointers in the latest.