-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal for changes in GDNative for Godot 4.0 #35467
Comments
another related PR #35795 (Fix return type for GDNative's |
|
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. godotengine/godot-headers#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.
|
|
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). |
|
#39588 is another case of return value initialization inconsistency. In this case there is no way for API consumers to handle methods that return |
In NativeScript:
Why? Is it for safety? Wouldn't this incur a performance overhead? I have a library which uses
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 |
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 For Rust that means that it's considered safe to call methods on reference counted objects. With non-reference counted objects anybody could 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. |
@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. |
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
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? |
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 |
|
|
@hnOsmium0001 from my experience of this oddity, it seems to be the case because if it was wrapped in At least, in the GDNative API, maybe it could return |
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 |
A version of
|
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 #35467 (comment) . Next point also expands on other related issues. |
While you can pass arguments to
Interesting, because we have recently switched from |
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 |
@toasteater that's an interesting idea if we have
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). |
@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: godotengine/godot-proposals#1613. Also, calling
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?
This would be really great for versioning purposes, even outside GDNative.
@Zylann Perhaps you can make |
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 |
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). |
@toasteater you're right, I messed things up ^^
So between 2) and 3) we get users using Godot x+1 with GDNative x Consider the (hypothetical) |
@touilleMan Then I think semver specs as was suggested in godotengine/godot-proposals#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
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. Changes to GDNative itself
Improvements 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. |
I was just thinking of a use case that might be hairy to handle right: Currently, I have a module that allows to subclass a "Generator" class to define a custom scripted one. One of the functions is called very often from inside threads. It passes a custom "VoxelBuffer" instance which the user fills with voxel data (it's a custom data structure so it doesn't use Godot's primitives). Currently this is easy to do if you use GDScript, C#, or C++ or Rust through GDNative. This is a very CPU-intensive task so performance is key. But now imagine this module is ported as a C++ GDNative library. I wonder how UE4 deals with this. So far I suppose if you use C++ only, it could have all game code + plugins in source forms, and they get compiled into one single library, which then gets loaded by the engine. But for C# or Rust I have no idea. |
Personally, I see this encapsulation as something usually positive, since it decouples the consumer code from the library implementation . With Godot acting as a mediator, it becomes a lot easier for library consumers to not care about what is used to write the underlying code. Nevertheless, I agree that there are cases where this can affect performance as Zylann has said. It should be possible even today, though, to just use a C struct as the userdata, and publish the headers. You'll have to bypass the C++ bindings to do this, but I see that more as a limitation in the bindings rather than the GDNative API itself. The engine can, in the end, do no better than that, as it can also only get access to function pointers at runtime: by gaining the convenience of not having to recompile the engine, you have to trade in the ability to let the engine know about the API at compile time. As a tangent, I wonder if the ability to operate on script instances directly can be expanded beyond just the userdata pointer. Right now, the code for creating a new instance of a NativeScript from a library is quite complicated since we have to go through a |
Given we won't have GDNative since it's replaced by extensions, some of these aren't really relevant anymore. We need to recheck this list to see if something is still missing. |
I suggest opening new issues and dedicated proposals for problems and features that are still relevant under the new GDExtension system. |
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 asString
.BulletPhysicsServer
not marked as singleton but inheritsPhysicsServer
Object.free
is virtual (should usegodot_object_destroy
instead of calling it directly), but not info about it inapi.json
instanciable
renamed intoinstantiable
?Script.has_property
,Script.has_method
andScript.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 aadditional_arguments
parameter that take a list, or have a booleanhas_index_argument
field (in the style of thehas_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 theapi.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]
andNull
[RID]
((1, 0), (0, 1), (0, 0))
(should beVector3((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 typeSpatialMaterial,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 aStringName
, useful for fast retrieval of a wrapper class when getting a generic object from variant). This would bring a bit of confusion withObject.get_class
though, I guess we could rename that intoString Object.get_class_name()
andStringName Object.get_class_name_as_string_name()
Issues in gdnative api:
godot_quat_new_with_axis_angle
inconsistent withgodot_basis_new_with_axis_and_angle
godot_basis_get_column
(but oddlygodot_basis_get_row
is present)godot_array_operator_equal
&godot_pool_x_array_operator_equal
Transform2D
origin/axis getter/settergodot_basis_axis
(unlikegodot_vector3_axis
which is present)Transform2D(Transform)
Transform(Basis)
Transform(Transform2D)
pluginscript api usegodot_string
everywhere where most of the timegodot_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)
) (Use StringName in pluginscript's set/get_prop and add_global_constant #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
andScriptLanguage::remove_named_global_constant
. On top of thatadd_named_global
andadd_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
andget_rset_mode
fields ingodot_pluginscript_instance_desc
are never used and should be removed (Remove useless pluginscript godot_pluginscript_script_desc.get_rpc/rset_mode fields #35811)godot_dictionary_operator_equal
doesn't do key/value comparison but only test if the underlying hashmap is the same (Modify Array/Dictionary::operator== to do real key/value comparison #35816)Remove deprecated GDNative wrapper code (Is GDNATIVE_API_INIT & GDnative wrapper lib still used ? #27054)Issues in ptrcall:
OS.get_static_memory_usage
returns a uint64_t which is not compatible with godot_int (see [GDnative] ptrcall on OS.get_static_memory_usage cause segfault due to godot_int != uint64_t #34254, solved by GDNative: Make godot_int an int64_t #36163)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 ^^
The text was updated successfully, but these errors were encountered: