Skip to content
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

[GDNative] Rethink the backward compatibily for Godot classes & ptrcall #35464

Closed
touilleMan opened this issue Jan 23, 2020 · 1 comment · Fixed by godotengine/godot-docs#5744

Comments

@touilleMan
Copy link
Member

Currently GDNative rely on a linked list of structures of function pointers to provide it API in a backward compatible way.
It's a very cool idea given a plugin build with GDNative API v1.1 will still work with Godot 3.1 (which provide GDNative API v1.2).

However GDNative also give access the the Godot internal classes (e.g. OS, Node2D etc.) whose changes are made without backward compatibility it mind !

For instance considering cc626ac that has been added for Godot 3.2.
This patch basically turns void SceneTree::quit() into void SceneTree::quit(int p_exit_code).
The unfortunate consequence of the change in the signature is that doing a ptrcall will cause a segfault on Godot>=3.2 if the plugin has been built with GDNative header from Godot<3.2 :'(

Of course, Variant based called are also touched given the call which used to be valid will now failed due to invalid parameters provided.

Solution 1 - Freeze the Godot classes signatures

The idea would be to consider the Godot classes signatures are part of the GDNative API.
Only adding new signature would be allowed between minor GDNative versions (and signature modifications would only occurs on GDNative major version like what will likely happen for the Godot 4.0 ^^)
Pros:

  • real backward compatibility ;-)

Cons:

  • probably need to develop a tool to ensure the signatures hasn't been modified (given it's something really easy to forget for developers not involved in GDNative)
  • add burden to the development, especially new features that need to iterate over multiple releases

I guess the feasibility of this solution depends a lot of how mature the Godot class signatures are (i.e. if signature modification are the exception or the norm)

Solution 2 - Provide tools to compare signature at runtime

The idea would be to give a unique id to each signature, and change it every time the signature is modified. This could be done for instance by hashing the string of the signature (or a concatenation of the return type and parameters types).

Those per-signature id would be then exposed in the GDNative headers and also available at runtime through the GDNative API. This way a module could make checks as part of it init routine to make sure the Godot binary it is running against provides compatible signatures to the methods it needs.

Pros:

  • no impact on day-to-day development of the Godot classes

Cons:

  • need to develop a tool that generate the unique id
  • doesn't provide compatibility, but only detects incompatibility...
@pchasco
Copy link

pchasco commented May 13, 2020

I think the only workable solution to this problem would be for GDNative libraries to be run against the intended version of Godot. I don't know of any workable solution to allow GDNative libraries written against prior versions of Godot to be future proof. One could get more granular and perform checks at a class/interface level as suggested under solution #2 to prevent being run against an incorrect version, but that would be a significant amount of work and maintenance for the same result as if the GDNative library just checked for the correct Godot version at initialization.

A real solution to the problem would be to implement some COM-like interface/contract system where we only expose interfaces to GDNative classes, and future versions of Godot need to maintain that regardless of any underlying changes to the concrete implementation, the consumer of any interface method can assume that they will function equivalently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants