Add a backwards-compatibility system for GDExtension #76446
Conversation
| @@ -984,7 +984,12 @@ static GDExtensionScriptInstancePtr gdextension_script_instance_create(const GDE | |||
| static GDExtensionMethodBindPtr gdextension_classdb_get_method_bind(GDExtensionConstStringNamePtr p_classname, GDExtensionConstStringNamePtr p_methodname, GDExtensionInt p_hash) { | |||
| const StringName classname = *reinterpret_cast<const StringName *>(p_classname); | |||
| const StringName methodname = *reinterpret_cast<const StringName *>(p_methodname); | |||
| MethodBind *mb = ClassDB::get_method(classname, methodname); | |||
| bool exists; | |||
There was a problem hiding this comment.
There's a recent trend to initialize every variable, assuming the compiler will just be clever enough to optimize out superfluous initializations in optimized builds.
There was a problem hiding this comment.
Indeed, this should definitely be initialized to false. The method it's passed to actually only ever sets it to true when the method exists - if the method doesn't exist, it's uninitialized and will lead to bugs.
| } | ||
|
|
||
| template <class N, class M, typename... VarArgs> | ||
| static MethodBind *bind_compatibility_method(N p_method_name, M p_method, VarArgs... p_args) { |
There was a problem hiding this comment.
Maybe that train has sailed, but maybe there's a good way of sharing code at least between regular/compat versions of the functions, given that the only difference is a true/false. A way which is not a macro, I mean!
Something like this (not tried):
template <class N, class M, typename... VarArgs>
static MethodBind *_base_bind_method(bool p_is_compat, N p_method_name, M p_method, VarArgs... p_args) {
Variant args[sizeof...(p_args) + 1] = { p_args..., Variant() }; // +1 makes sure zero sized arrays are also supported.
const Variant *argptrs[sizeof...(p_args) + 1];
for (uint32_t i = 0; i < sizeof...(p_args); i++) {
argptrs[i] = &args[i];
}
MethodBind *bind = create_method_bind(p_method);
if constexpr (std::is_same<typename member_function_traits<M>::return_type, Object *>::value) {
bind->set_return_type_is_raw_object_ptr(true);
}
return bind_methodfi(METHOD_FLAGS_DEFAULT, bind, p_is_compat, p_method_name, sizeof...(p_args) == 0 ? nullptr : (const Variant **)argptrs, sizeof...(p_args));
}
template <class N, class M, typename... VarArgs>
static MethodBind *bind_method(N p_method_name, M p_method, VarArgs... p_args) {
return _base_bind_method(false, p_method_name, p_method, p_args...);
}
template <class N, class M, typename... VarArgs>
static MethodBind *bind_compatibility_method(N p_method_name, M p_method, VarArgs... p_args) {
return _base_bind_method(true, p_method_name, p_method, p_args...);
}
|
I think we should also warn extension devs when they are using a method that has been deprecated. I could imagine something similar to this: This would result in the following warnings when the extension is used with a debug build of godot: This would (hopefully)) make it easier for plugin devs to update to new extension versions. |
reduz
force-pushed
the
add-gdextension-api-compatibility
branch
from
6dbede1
to
fd143a6
Compare
reduz
changed the title
|
@Ansraer the problem with this warning is that it will be shown for users of the extension as well. If an extension isn't updated but it's working because of compatibility then the user will be stuck with this warning showing every time. Unless this under |
|
Yeah, I was talking about dev builds only. Not sure what other people do, but most of my development happens with a dev build. I only use non-dev builds when I need to check performance. My thought process was that players would always use a non-dev build, while extension developers (who are more likely to use a dev build) can get additional information that makes it easier to keep their code up to date with API changes. |
There are valid reasons that a plugin developer may want to keep using the older compatibility functions. At the last GDExtension meeting we discussed making our "compatibility promise" be: GDExtensions built for older versions of Godot will work in newer versions of Godot, but not the other way around. So, a GDExtension built against the Godot 4.0 APIs should work (unless we mess something up ;-)) in Godot 4.2, but a GDExtension built against Godot 4.2 won't work in Godot 4.0 (because if you call a Godot 4.2 API that didn't exist back in Godot 4.0, what would it do?). So, a plugin developer that doesn't actually need the latest Godot features/APIs, and wants a single build of their plugin to work on as many Godot versions as possible, will build their plugin against the earliest version of Godot that it'll work with, utilizing the compatibility methods. I agree that it'd be good to have a way to communicate to developers that they using deprecated functions. But we'd need to make sure that it isn't a warning that's printed every time they are calling the function, or that it's easy to turn off (even in debug builds). Because if they are using a compatibility function on purpose, perhaps very often (like every frame), it's not useful to get loads of console SPAM telling them about it. :-) |
reduz
force-pushed
the
add-gdextension-api-compatibility
branch
from
fd143a6
to
9227ba9
Compare
|
This also leaks the compatibility binds: https://github.com/godotengine/godot/actions/runs/4838476687/jobs/8622970764#step:11:400 Needs to delete them in class_db.cpp:1668 and class_db.cpp:1711 |
reduz
force-pushed
the
add-gdextension-api-compatibility
branch
from
9227ba9
to
ab52aa7
Compare
|
This looks great! :-) I tested using the compatibility version of I didn't notice any additional issues in skimming the code |
reduz
force-pushed
the
add-gdextension-api-compatibility
branch
from
ab52aa7
to
f878699
Compare
|
Discussed at the GDExtension meeting, and this looks good to us! |
| @@ -984,7 +984,12 @@ static GDExtensionScriptInstancePtr gdextension_script_instance_create(const GDE | |||
| static GDExtensionMethodBindPtr gdextension_classdb_get_method_bind(GDExtensionConstStringNamePtr p_classname, GDExtensionConstStringNamePtr p_methodname, GDExtensionInt p_hash) { | |||
| const StringName classname = *reinterpret_cast<const StringName *>(p_classname); | |||
| const StringName methodname = *reinterpret_cast<const StringName *>(p_methodname); | |||
| MethodBind *mb = ClassDB::get_method(classname, methodname); | |||
| bool exists; | |||
There was a problem hiding this comment.
Indeed, this should definitely be initialized to false. The method it's passed to actually only ever sets it to true when the method exists - if the method doesn't exist, it's uninitialized and will lead to bugs.
This adds a way to ensure that methods that were modified in the Godot API will continue working in older builds of GDExtension even if the new signature is different.
```C++
// New version (changed)
ClassDB::bind_method(D_METHOD("add_sphere","radius","position"),&MyShapes::add_sphere);
// Compatibility version (still available to extensions).
ClassDB::bind_compatibility_method(D_METHOD("add_sphere","radius"),&MyShapes::_compat_add_sphere);
```
**Q**: If I add an extra argument and provide a default value (hence can still be called the same), do I still have to provide the compatibility version?
**A**: Yes, you must still provide a compatibility method. Most language bindings use the raw method pointer to do the call and process the default parameters in the binding language, hence if the actual method signature changes it will no longer work.
**Q**: If I removed a method, can I still bind a compatibility version even though the main method no longer exists?
**A**: Yes, for methods that were removed or renamed, compatibility versions can still be provided.
**Q**: Would it be possible to automate checking that methods were removed by mistake?
**A**: Yes, as part of a future PR, the idea is to add a a command line option to Godot that can be run like : `$ godot --test-api-compatibility older_api_dump.json`, which will also be integrated to the CI runs.
reduz
force-pushed
the
add-gdextension-api-compatibility
branch
from
f878699
to
d8078d3
Compare
|
Thanks! |
This adds a way to ensure that methods that were modified in the Godot API will continue working in older builds of GDExtension even if the new signature is different.
Usage:
Compatibility testing:
It is also now possible to ask an engine build whether it breaks compatibility with a previous version by calling like this:
$ godot4.1 --validate-extension-api godot4.0-api.jsonThe following is output to the console right now:
If compatibility was broken, Godot will return with an exit code other than zero.
Which means it will need to be fixed by providing compatibility versions as described above.
The idea would be to eventually integrate this to the CI, by storing previous versions of the Godot stable APIs that will need to be tested.
FAQ:
Q: If I add an extra argument and provide a default value (hence can still be called the old way), do I still have to provide the compatibility version?
A: Yes, you must still provide a compatibility method. Most language bindings use the raw method pointer to do the call and process the default parameters in the binding language, hence if the actual method signature changes it will no longer work.
Q: If I removed a method, can I still bind a compatibility version even though the main method no longer exists?
A: Yes, for methods that were removed or renamed, compatibility versions can still be provided.