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

The master branch is incompatible with 4.1 #1195

Closed
DmitriySalnikov opened this issue Jul 29, 2023 · 11 comments
Closed

The master branch is incompatible with 4.1 #1195

DmitriySalnikov opened this issue Jul 29, 2023 · 11 comments

Comments

@DmitriySalnikov
Copy link
Contributor

Godot version

4.1.1.stable

godot-cpp version

master c5d8447

System information

Windows 10

Issue description

I tried to update godot-cpp to the latest version, but I got a bunch of errors on startup:

ERROR: Attempt to get non-existent interface function: classdb_register_extension_class_property_indexed
   at: (core/extension/gdextension.cpp:448)
ERROR: Unable to load GDExtension interface function classdb_register_extension_class_property_indexed()
   at: (src\godot.cpp:364)
ERROR: GDExtension initialization function 'debug_draw_3d_library_init' returned an error.
   at: open_library (core/extension/gdextension.cpp:476)
ERROR: Failed loading resource: res://addons/debug_draw_3d/debug_draw_3d.gdextension. Make sure resources have been imported by opening the project in the editor at least once.
   at: (core/io/resource_loader.cpp:273)
ERROR: Error loading extension: res://addons/debug_draw_3d/debug_draw_3d.gdextension
   at: (core/extension/gdextension_manager.cpp:143)
Godot Engine v4.1.1.stable.official.bd6af8e0e - https://godotengine.org

It seemed to me that adding a compatibility system should have added the ability to run on different versions of Godot, and not just on the latest one.
Then why do we need this line?

compatibility_minimum = "4.1"

Steps to reproduce

  • Use the godot-cpp library from the master branch in Godot 4.1

Minimal reproduction project

N/A

@Faless
Copy link
Contributor

Faless commented Jul 29, 2023

As far as I understand, if you want the minimum compatibility to 4.1 you need to use the extension API from 4.1. That will be guaranteed to work with 4.2, 4.3, etc.

If you use the extension API from 4.2, you won't be compatible with 4.1.

The compatibility system guarantees backwards compatibility of Godot with extensions from previous versions, not the compatibility of extensions built for the latest Godot version with older Godot versions.

@DmitriySalnikov
Copy link
Contributor Author

If you use the extension API from 4.2, you won't be compatible with 4.1.

And then what is the point in compatibility_minimum = "4.1". Why not take this version from godot-cpp itself?

Android, for example, also allows you to specify the minimum and target version, and there is no such problem there. Is it too difficult to maintain?

I just wanted to use the latest version because it's faster 😐

@Faless
Copy link
Contributor

Faless commented Jul 29, 2023

And then what is the point in compatibility_minimum = "4.1".

That tells Godot to skip loading the extension completely (and warn the user about it).

Why not take this version from godot-cpp itself?

That file is is not generated by the build system right now, but if we add that to the build system, then it could be read from the extension API version indeed.

and there is no such problem there.

Beside the gigabytes-sized SDK 😅

Is it too difficult to maintain?

I don't know, it might be possible in the future by maintaining a compatibility library like Android does.

@DmitriySalnikov
Copy link
Contributor Author

That tells Godot to skip loading the extension completely (and warn the user about it).

Thanks. Apparently I will have to continue to support only the latest version.

Beside the gigabytes-sized SDK 😅

But I don't need all versions from 19 to 33 if I specify them when compiling. I only have the SDK for version 33, but it still supports all the selected versions. (without NDK, it's only 440 MB)
I took a quick look at the sources and it seems like there are just more if's being added to each new version.

@paddy-exe
Copy link
Contributor

Thanks. Apparently I will have to continue to support only the latest version.

This should help then: #1193

@DmitriySalnikov
Copy link
Contributor Author

This should help then: #1193

or the scons min_version=4.1 argument could be added to be responsible for generating compatibility_version.h. If we do not specify this argument, then the version from api.json will be used. Leave only Major, Minor, Patch and version code (VERSION_CODE (MAJOR<<16, MINOR<<8, PATCH)) (or add all the other versions just for information). Just like in Android, we can add a file with constants with possible engine versions (android.os.Build.VERSION_CODES.JELLY_BEAN, GODOT_VERSION_CODE_4_1). And based on this data, it will be possible to compile sources with the minimum supported version.
It would be worth adding extern "c" int godot_get_min_supported_version() to the same file (compatibility_version.h). If this function is not found, then consider that it is 4.1. compatibility_minimum = 4.1 is no longer required and godot_cpp can be updated normally.
Further, it is only required to leave the old and new methods in the godot itself and use them when compiling for different versions.

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 30, 2023

Why not take this version from godot-cpp itself?

That file is is not generated by the build system right now, but if we add that to the build system, then it could be read from the extension API version indeed.

Yeah, with PR #1193 we could have GDExtensionBinding::init() ensure that the Godot version godot-cpp was compiled for isn't higher than the version of Godot that's loading the GDExtension. I think we should absolutely do this!

It would be worth adding extern "c" int godot_get_min_supported_version() to the same file (compatibility_version.h). If this function is not found, then consider that it is 4.1. compatibility_minimum = 4.1 is no longer required and godot_cpp can be updated normally.

On a technical level, that won't totally work because the function couldn't be named godot_get_min_support_version() in every GDExtension, because if you loaded two GDExtensions we'd get a symbol conflict. So, at minimum, you'd need to put something in the .gdextension file telling Godot the name of the function to load. And if we're going to do that, why not just put the compatibility minimum right in the .gdextension file itself?

In any case, I think it's good that we have compatibility_minimum outside the ABI. It was added so that we could distinguish GDExtensions built for Godot 4.0 from those built for 4.1, because they are completely binary incompatible. If we ever do something like that again (I hope we don't, but we'll see), then we'll need still need some way to signal to Godot to not even try to load binary incompatible extensions (or perhaps load them differently if that's a possibility).

@DmitriySalnikov
Copy link
Contributor Author

DmitriySalnikov commented Jul 30, 2023

in every GDExtension, because if you loaded two GDExtensions we'd get a symbol conflict.

Can't different assemblies have the same functions? It seemed to me that they should load for each assembly separately and do not overlap in any way.

In any case, I think it's good that we have compatibility_minimum outside the ABI. It was added so that we could distinguish GDExtensions built for Godot 4.0 from those built for 4.1, because they are completely binary incompatible.

I don 't mind an extra line in .gdextension, let it be compatibility_minimum or the name of the version obtaining function.
In previous comments, I said that godot-cpp is being updated, it is getting faster, and I can only use these updates with the latest version of godot simply because in the previous version of godot there is no function with the index [7d8cb7c].
It would be cool if I could specify scons min_version=4.1 when compiling, which would tell the compiler to use the old version of the function without an index, instead of the new one with an index. In this case, my library could be run in both Godot 4.1 and Godot 4.2. I would not be able to specify property indexes, but there would be wider support.

At the moment, in order to use the latest version of godot-cpp, I have to add a patch for compatibility.


Also, when specifying min_version=4.1, a notice in the console may be displayed that "4.1 does not have support for specifying property indexes", and in the future also add information about other restrictions. This is without taking into account the obvious lack of methods and classes in the previous versions of godot itself.

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 30, 2023

Oh, in that case, I think you just want the 4.1 branch. The plan is to cherry-pick over any changes that don't require Godot changes (which will include the performance improvement PR), but we haven't done the first round of cherry-picking yet.

@DmitriySalnikov
Copy link
Contributor Author

Sounds good.
However, I would prefer a version with conditional compilation, so it would be faster to get updates (just update master without cherry-picks).

It turns out that this issue can be closed?

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 31, 2023

Sure, let's close it

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

No branches or pull requests

4 participants