While working on Godot-Python, I've encountered my share of quirks in the GDNative API, I guess it's time to fix them for Godot 4.0 ;-)
Issues in api.json
:
StringName
are exposed as String
.BulletPhysicsServer
not marked as singleton but inherits PhysicsServer
Object.free
is virtual (should use godot_object_destroy
instead of calling it directly), but not info about it in api.json
instanciable
renamed into instantiable
?Script.has_property
, Script.has_method
and Script.has_script_signal
/
in there name to indicate they need an additional parameter (provided by the "index" field, which is set to -1
when the value is not needed...). Would be better to provide a additional_arguments
parameter that take a list, or have a boolean has_index_argument
field (in the style of the has_default_value
boolean field)Object.set/get
doesn't return a boolean to indicate if the operation couldn't succeed if the property doesn't existsoperator String()
which often lack clarity (and is subject of innocent modifications breaking the api.json
format !). It would be better to define those informations throught a custom string (this is already done in the doc xml files). Exemple of strange values:[Object:null]
and Null
[RID]
((1, 0), (0, 1), (0, 0))
(should be Vector3((1, 0), (0, 1), (0, 0))
)[PoolColorArray]
but PoolVector2Array displayed as []
1, 0, 0, 0, 1, 0, 0, 0, 1 - 0, 0, 0
for Transform here the -
is especially misleading :/PrimitiveMesh.material
has type SpatialMaterial,ShaderMaterial
. This is fine from a documentation point of view, but given the documentation already overwrite fields in it xml files, I guess it would be better to replace that by a simplerMaterial
valueObject.get_class_name
(which returns a StringName
, useful for fast retrieval of a wrapper class when getting a generic object from variant). This would bring a bit of confusion with Object.get_class
though, I guess we could rename that into String Object.get_class_name()
and StringName Object.get_class_name_as_string_name()
Issues in gdnative api:
godot_quat_new_with_axis_angle
inconsistent with godot_basis_new_with_axis_and_angle
godot_basis_get_column
(but oddly godot_basis_get_row
is present)godot_array_operator_equal
& godot_pool_x_array_operator_equal
Transform2D
origin/axis getter/settergodot_basis_axis
(unlike godot_vector3_axis
which is present)Transform2D(Transform)
Transform(Basis)
Transform(Transform2D)
godot_string
everywhere where most of the time godot_string_name
would be better suited (e.g. godot_bool godot_pluginscript_instance_desc_set_prop(godot_pluginscript_instance_data* p_data, godot_string* p_name, godot_variant* p_value)
) (#35812)~ScriptLanguage::add_named_global_constant
(exposed by pluginscript) api is weird: global constant can only be integer, but the function takes a variant as parameterScriptLanguage::add_named_global_constant
and ScriptLanguage::remove_named_global_constant
. On top of that add_named_global
and add_global_constant
have misleading names (from what I understand, the difference between them is the later can have any type of value where the former is only an integer...)get_rpc_mode
and get_rset_mode
fields in godot_pluginscript_instance_desc
are never used and should be removed (#35811)~godot_dictionary_operator_equal
doesn't do key/value comparison but only test if the underlying hashmap is the same (#35816)Issues in ptrcall:
OS.get_static_memory_usage
returns a uint64_t which is not compatible with godot_int (see #34254)That's a big wishlist I guess ! I guess I've already found what I'll be working on at next week's GodotCon sprints ^^
another related PR #35795 (Fix return type for GDNative's godot_color_to_XXX64
)
gdnative_init_options
should include a Godot version field.
The GDNative JSON API should include the Godot version as well.
ALL generated struct
names should include their version. Right now only "next
-filed extensions" have the version in the name.
Push more code about POD core types (such as Vector2/3
, Color
, Basis
) into bindings. It seems like most bindings do not use many other functions other than the "construct" and "deconstruct" functions.
For example the Rust bindings receive those values, then convert them to types from the euclid
crate and then do all operations on that. So all "accessor" functions are unused. Similar for the C++ bindings.
Those types would be defined as actual structs with proper members, for example
struct godot_vector3 {
float x;
float y;
float z;
};
Functions should never return those complex values directly but always by pointer. Languages like Haskell have troubles with functions returning unboxed data.
Push more code about POD core types (such as Vector2/3, Color, Basis) into bindings. It seems like most bindings do not use many other functions other than the "construct" and "deconstruct" functions.
For example the Rust bindings receive those values, then convert them to types from the euclid crate and then do all operations on that. So all "accessor" functions are unused. Similar for the C++ bindings.Those types would be defined as actual structs with proper members, for example
struct godot_vector3 { float x; float y; float z; };
Functions should never return those complex values directly but always by pointer. Languages like Haskell have troubles with functions returning unboxed data.
Wouldn't this create API disparity between the native bindings and GDScript (core godot)? Going this route means will just create duplicate code across the bindings. Having a consistent API in terms of the interface and behaviour is pretty advantageous, specially for users not wanting to use GDScript. That's why my binding (https://github.com/raniejade/godot-kotlin) does not re-implement any of the core types, it just delegates everything to Godot. Users can always refer to Godot's official documentation regardless of what binding they are using.
Pulling this in from the godot_headers repo. https://github.com/GodotNativeTools/godot_headers/issues/65
Yes, but generally there are idiomatic ways to handle linear algebra
types in most languages. So it might be better to use common libraries
that will work well with other libraries for example, which is what Rust
does.
When new functions are added to those types the GDNative API has to
change. It takes some care to not break binary compatibility and also
makes it harder for bindings to just correctly add new functionality.
Also by going through the C API you have some unavoidable overhead, as
no LTO or cross-language inlining works with runtime provided function
pointers. For things that should be as easy as a simple add
or mov
you now need to convert arguments and return values from a C API.
Of course, the behavior being the same as with Godot is useful, but this
is not a technical problem but an ecosystem/idiomatic one. We could keep
those functions, but I think in environments that already have
more-or-less common ways to do those operations going through a C API
with custom types is not very nice.
introduce the concept of constants to NativeScript
so that scripts can create them
make all object references be ObjectID
s and not raw pointers. This includes all calls to ptrcall
or varcall
I totally agree with the technical side of things, however, the ecosystem aspect is something that should not be dismissed as it could hurt the adoption of Godot. Learning resources available about Godot is quite sparse (relative to Unity and Unreal), transferability of those resources to other bindings is useful for people who prefers not to use GDScript (for various reasons).
api.json
. Various languages have different tools to show documentation (rustdoc, kdoc, ...), it will be nice to have the actual documentation of a class/method/property locally. godot_array
consistently without leaking memory (initializing pointers even when they're overwritten).In NativeScript:
Rich registration: Correct me if I'm wrong, but godot_nativescript_register_method
doesn't seem to allow for declaration of argument types, return type and names. Which leaves Godot and users completely in the dark when it comes to use registered methods. They are "trusted to use methods properly", with no reliable typing, auto-completion or static checks, runtime checks also being left to implement by every binding. When using GDScript at least, this is not the expected experience. Obscure issues like https://github.com/godotengine/godot-cpp/issues/430 could be related to this.
C#: using rich registration as mentioned above, could allow better C# integration (if possible?), because right now I heard using a GDNative library from C# is really daunting. It's frustrating to see GDNative and C# are two built-in modules, yet they are two fragmented parts of the scripting system. This is one of the big reasons I'm still not all-in for providing C++ NativeScript plugins, preferring C++ modules instead for their universal reach (despite the hurdles of engine compilation). Such fragmentation is understandable for GDScript, but probably not for the engine's very language.
Being able to register static functions would be nice.
Add concept of editor/release. Currently GDNative libraries only come in one version that is supposed to be release. However there is no way to provide 2 versions for editor and release... that means any checks and editor-specific code we may want to write in a library will have to ship in the final game, and it's not clean. Right now all we can do is either tell users to recompile the library before exporting, or telling them to swap prebuilt libraries somehow.
@karroffel
make all object references be ObjectIDs and not raw pointers. This includes all calls to ptrcall or varcall
Why? Is it for safety? Wouldn't this incur a performance overhead? I have a library which uses set_pixel
and get_pixel
a lot on images. If I chose to use C++, I don't expect to have such indirection baked in every call.
Push more code about POD core types (such as Vector2/3, Color, Basis) into bindings.
I've seen that. In C++ I find there is no point to use method pointers to do linear algebra, other than having the exact same behavior, but the overhead becomes quite stupid. If the Godot math library can be copied around more easily, it's easier. For C++ only though, other languages may have to rewrite their own, or use other math libraries as long as they allow the same conventions as Godot
Why? Is it for safety? Wouldn't this incur a performance overhead?
Yes and yes. In the Rust bindings we want to model the API is correctly as possible while making it as safe as possible too. One problem with that are non-reference counted types. If you know you hold a reference to an object (and the rest of the world doesn't just run around doing .unreference()
) then you know it's going to be okay to call methods on an object that's reference counted.
For Rust that means that it's considered safe to call methods on reference counted objects. With non-reference counted objects anybody could .free()
at any point from a different thread and then the object might still be valid while the Rust bindings are doing the call, but when it enters Godot (or anywhere in between really) it might suddenly be freed. At that point this call will ideally cause a crash or possibly even worse things when an ABA problem occurs.
One way to get rid of this is to not use pointers directly but use IDs. That way the engine could make sure that the object is safe to call even when it passed the language-binding border and in an atomic way. Even if Godot just says "This call was invalid" it's still better than the current situation where things can just go horribly wrong.
make all object references be ObjectIDs and not raw pointers. This includes all calls to ptrcall or varcall
@karroffel Have you created an issue about this ? I feel like this proposition is far from trivial so better have a dedicated consultation about it ;-)
I guess this change would impact the whole engine (and not only gdnative) given it can benefit from anywhere (especially GDscript !), so I'm curious about the performance impact.
On top of that I wonder if leaving the old raw pointer stuff for GDNative as an alternative would be worth it (it should be fairly simple given all the code is already written, and it would be up to the GDNative module creator to bear the responsibility of the segfaults that could occur ^^), however having one way to do thing and keeping people avoid from their overconfidence of dodging segfault is a good thing ! So I guess it all really depend on the impact on performances.
One final though: this check-on-access-before-use behavior seems to be the exact opposite of the refcounting check-on-unref-if-delete-is-needed behavior.
So wouldn't it be just simpler (and faster given unreference occurs less often than regular access) to make all object in Godot refcounted ?
@touilleMan
Have you created an issue about this ? I feel like this proposition is far from trivial so better have a dedicated consultation about it ;-)
It is indeed a non trivial discussion, but I have not created an issue about it yet, but maybe that's a good idea. This idea of using Object IDs for the current ptrcall
s was something proposed by @reduz on IRC a while ago.
So wouldn't it be just simpler [..] to make all object in Godot refcounted?
That would be my preferred solution too, but @reduz expressed that this is not something he wants to do. Maybe a dedicated issue for the safety of GDNative calls might be better suited for this discussion?
((1, 0), (0, 1), (0, 0))
(should beVector3((1, 0), (0, 1), (0, 0))
)
No, each element of a Vector3 is a floating-point number, not two of them, and you are missing type information for the inner items. I think you meant it should be Transform2D(Vector2(1, 0), Vector2(0, 1), Vector2(0, 0))
As mentionned in https://github.com/godotengine/godot-cpp/issues/438, we need a proper +=
String operator, which is one of the rare methods that don't create a new string. We'd need two versions: one taking a String
, and one taking a wchar_t
. Or, eventually, a wchar_t*
and a size for nice interop.
Need a function to instanciate a script with arguments and return a raw godot_object*
, without having the result converted to Variant
(as it is the case if we use the API's NativeScript.new
function). This is because the current way initializes refcount, which makes it hard to support consistent Refset_script
instead, which however doesn't let us support constructor arguments. See details here https://github.com/godotengine/godot/pull/33532#issuecomment-636268406
Ability to obtain a script resource from its class name, or better, its type tag pointer. Because as stated in the previous point, creating custom class instances from a NativeScript requires quite dirty hacks, since we don't know where the .gdns
file is, and shouldnt need to.
in godot_gdnative_ext_arvr_api_struct
: godot_arvr_blit
and godot_arvr_get_texid
should take const godot_rid*
arguments, instead of godot_rid*
. They don't modify them, only read them.
Profiler API taking line number as a separate argument so C++ can use FUNCTION
without having to concatenate it with line number at runtime (ruining it)
get_script()
returns Reference*
instead of something like Script*
or Ref<Script>
. Is this intended? And it seems like the returned Reference*
is either a proper Script*
or nullptr, so it really should return Script*
in my opinion.
@hnOsmium0001 from my experience of this oddity, it seems to be the case because if it was wrapped in Ref<>
it woud cause a cyclic reference. The declaration of Object
would depend on Reference
, but that can't happen because Reference
depends on Object
. It's the case in Godot's source code as well. C++ bindings also failed to compile at some point because of this, which forced us to make sure Object::get_script()
returns Reference*
instead. It's not really related to GDNative itself, though.
At least, in the GDNative API, maybe it could return Script
.
There is also godotengine/godot-cpp#431, which should be a really simple fix of changing
https://github.com/godotengine/godot-cpp/blob/master/include/core/Godot.hpp#L469
to
void register_signal(String name, Dictionary args)
Sorry I didn't see this issue is only for GDNative.
@hnOsmium0001 if issues are only related to the C++ bindings, please keep them in godot_cpp
instead, not here. This issue is about the GDNative API itself.
ptrcall
that allows omission of optional argumentsThis should help improve the compatibility between compiled binaries and future minor/patch versions of the engine, since new optional arguments can get added in minor/patch versions. Currently, it's UB to ptrcall
a method that has been changed in this way, since the engine assumes that the full argument list has been provided and would dereference invalid memory.
I'm not sure why they weren't passed to the library in the first place, but this is a good chance to change that.
Allow NativeScript constructors to take arguments
It's possible to create a new instance from a NativeScript by specifying arguments, but the problem is, the current method does not allow us to get a direct pointer and causes an issue with refcounting. I mentionned it in second point here https://github.com/godotengine/godot/issues/35467#issuecomment-674431018 . Next point also expands on other related issues.
I don't know about the other way around though (i.e registering a class with constructor arguments)
It's possible to create a new instance from a NativeScript by specifying arguments ... I don't know about the other way around though (i.e registering a class with constructor arguments)
While you can pass arguments to NativeScript::_new
, it can't actually do anything with them, since the current signature of godot_nativescript_instance_create_func::create_func
doesn't take any arguments beyond a pointer to the owner object and a pointer to bindings-specific data. I was suggesting that the signature of create_func
be changed so the argument list can actually be passed to the constructors.
This is because the current way initializes refcount, which makes it hard to support consistent Ref in the C++ bindings, so we had to use set_script instead,
Interesting, because we have recently switched from set_script
to NativeScript::new
in Rust (since the latter works when called from tool scripts), but no leaks or use-after-frees were noticed. Perhaps this could be an indication of some issue in godot-cpp regarding Variant
handling in general instead?
Interesting, because we have recently switched from set_script to NativeScript::new in Rust (since the latter works when called from tool scripts), but no leaks or use-after-frees were noticed. Perhaps this could be an indication of some issue in godot-cpp regarding Variant handling in general instead?
That's strange because from all the research we did, the problem really was the fact Godot initializes the refcount on its side because it returns us a Variant. See https://github.com/godotengine/godot-cpp/blob/c9a740be34438ce999402b7b76304a38daaaa811/include/core/Godot.hpp#L39 (there is Variant in the code here but that's because new
is not even a GDNative function, it's the Godot script API returning us that, not a choice from us)
Maybe Rust exposes that differently than we do. Could try to investigate again but that's what I remember. The idea was then to have a GDNative-specific function that gives us the pointer directly so we can just use a verbatim copy of the behavior found in Godot, without the need to somehow differenciate between various origins of that pointer.
A version of ptrcall that allows omission of optional arguments
This should help improve the compatibility between compiled binaries and future minor/patch versions of the engine, since new optional arguments can get added in minor/patch versions.
@toasteater that's an interesting idea
if we have godot_foo(int a)
in Godot 4.0, then godot_foo(int a, int b)
in Godot 4.1
godot_foo
gets called with fewer args than expected and knows how to deal with it)godot_foo
would be called with more args than expected ! (I'm not saying it's impossible, just than it should be taken into account given it's going to be really common when a new Godot version is released to use plugin build with older version given the upstream maintainer hasn't released new builds yet)I personally think the key point here is to provide a consistent and clear rule on GDNative compatibility (GDNative and Godot version must be the same ? or we allow compatibility between patch versions ? or minor ?).
My current guess is the simplest thing to do is to guarantee compatibility only between patch versions (so Godot 4.1.x is compatible with GDNative 4.1.y no matter x and y).
This allow simple version check for plugins during initialization (we could even add a mandatory version field in the .gdnlib
so that Godot does the compatibility check before loading the plugin).
Of course this means no changes in the api (including everything that's exposed by ClassDB) between patch versions. We could enforce this in CI by comparing the generated api.json
with a committed version.
however what about using GDNative header 4.1 with Godot 4.0 ? godot_foo would be called with more args than expected !
@touilleMan While this is a plausible scenario, I believe it's far more common to expect backwards compatibility than forward compatibility. This also appears to be the sentiment behind this related proposal regarding editor plugins: https://github.com/godotengine/godot-proposals/issues/1613. Also, calling godot_foo
with more args would, at worst, leads to memory leaks and less-than-ideal (but defined and expected from the older engine version) behavior, which I believe is a much less serious problem compared to what would happen when 4.0 headers are used with 4.1.
...given it's going to be really common when a new Godot version is released to use plugin build with older version given the upstream maintainer hasn't released new builds yet
I'm not sure if I understood what you mean here correctly. Shouldn't this lead to the "4.0 headers with 4.1, less args" situation, if I'm not mistaken?
We could enforce this in CI by comparing the generated api.json with a committed version.
This would be really great for versioning purposes, even outside GDNative.
That's strange because from all the research we did, the problem really was the fact Godot initializes the refcount on its side when boxing it into Variant, really. Its not something the bindings do AFAIK
@Zylann Perhaps you can make create_custom_class_instance
return either T*
or Ref<T>
depending on whether the base class is reference counted with templates? Then you can deal with reference count initialization properly in the function.
@Zylann Perhaps you can make create_custom_class_instance return either T* or Ref
depending on whether the base class is reference counted with templates?
That's one of the possible outcomes but it's the worst... I don't even know if I can do that, because then I need to duplicate the entire logic just to return something different, and would make code depending on it even more complicated (especially more with arguments!) because of the variation of return type. Would require the user to have different header declarations if inheriting Reference or not, would break the Ref<T>.instance()
logic as well... new
even requires the return type to be a pointer AFAIK so it's inconsistent. And all this because of a behavior that we don't need (the Variant step), and solves the problem if it was just straight up not there.
Sorry. Didn't mean to start an argument here. To be clear, I'm not against adding the new API: it would make our lives much better as well. I was merely suggesting the possibility of an issue in godot-cpp, but since I'm not as familiar with the inner workings and/or goals of the godot-cpp project, I could of course be very wrong. In any case, I meant no offense and feel sorry for any that I might have caused. If there is a good reason for the user-facing API to remain as-is, then I agree that it is hard to work around, and I'm not the one to judge whether that goal is a good one to pursue.
Sorry if I sounded harsh, it wasnt the intention^^ your solution makes sense but as you said it would require significant changes in the way we expose the API to users, which we wanted to be unified and faithful to how it is when you write an engine module (which allows to easily mirror logic as well, since C++ is the language of the engine).
...given it's going to be really common when a new Godot version is released to use plugin build with older version given the upstream maintainer hasn't released new builds yet
I'm not sure if I understood what you mean here correctly. Shouldn't this lead to the "4.0 headers with 4.1, less args" situation, if I'm not mistaken?
@toasteater you're right, I messed things up ^^
So as you said the "regular" use case would be
1) people are using Godot x with a plugin using GDNative x
2) Godot x+1 is released, people start using it day one
3) plugin is updated by it maintainer to use GDNative x+1
So between 2) and 3) we get users using Godot x+1 with GDNative x
However after 3) there is people still relying on Godot x (because they don't want to change version in the middle of a game dev, or because there game is a fork of Godot ?) but will update (or just start using) the plugin.
I have no idea how common this thing is, however I'm pretty sure we need to address this issue in a clean way otherwise people will end up with hidden&nasty bug:
Consider the (hypothetical) String.split
function working like "foo bar".split() == ["foo", "bar"]
, in next release we provide another parameter that allow to choose what is the splitting character (eg. "foo-bar".split("-") == ["foo", "bar"]
)
Now if we use "foo-bar".split("-")
on an older version of Godot and the "-"
param gets simply ignored (so we end up with "foo-bar".split("-") == ["foo-bar"]
!), we have recipe for disaster ^^
I think it would be far better to introduce a mechanism to prevent the plugin from being loaded in the first place.
...I think it would be far better to introduce a mechanism to prevent the plugin from being loaded in the first place.
@touilleMan Then I think semver specs as was suggested in https://github.com/godotengine/godot-proposals/issues/1613 is a great option for that: it covers the "regular" use case well, and would also allow libraries to require a minimum minor/patch version of the engine that supports the extra features required. Then, it's possible to prevent the libraries from being loaded by older versions and warn the user, preventing the "ignored argument" situation from happening.
For example, suppose that the splitting character argument is added to the hypothetical String.split
in Godot A.3.0
. A plugin that makes use of the argument may set its godot_version
spec to ^A.3.1
, which would mean that:
Godot A.2.0
or Godot A.3.0
.Godot B.0.0
.A.3.1
, or any compatible versions, e.g. A.3.2
or A.4.0
.In this case, no engine version that may not have the additional parameter may load the plugin in question.
It should also be possible to emit an error message when extra arguments are found (and ignored), so even in the case a plugin mis-specified its engine version requirement, the issue can at least be signaled to the end user (game developer).
Here is everything that was proposed and discussed during the meeting on 22.10.2020.
Cleaned things up a bit and formatted it properly.
Feel free to ping me on discord if you feel I missed something or want me to edit this list.
(My spam filter sometimes deletes GitHub notifications)
Most helpful comment
Here is everything that was proposed and discussed during the meeting on 22.10.2020.
Cleaned things up a bit and formatted it properly.
Changes to GDNative itself
iOS without static linkinghttps://github.com/godotengine/godot/pull/40050Improvements to GDNative C++
Moving stuff to GDNative
Changes/Additions to the Editor
Feel free to ping me on discord if you feel I missed something or want me to edit this list.
(My spam filter sometimes deletes GitHub notifications)