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

Some GDExtension methods are unavailable in Release builds #86206

Closed
Bromeon opened this issue Dec 15, 2023 · 6 comments · Fixed by #86209
Closed

Some GDExtension methods are unavailable in Release builds #86206

Bromeon opened this issue Dec 15, 2023 · 6 comments · Fixed by #86209

Comments

@Bromeon
Copy link
Contributor

Bromeon commented Dec 15, 2023

Tested versions

4.2.0-stable
4.2.1-stable

System information

Windows 10

Issue description

Godot release builds (aka export templates) do not provide the ResourceImporterOggVorbis methods load_from_buffer() and load_from_file(). There might be more (now or in the future).

In godot-rust, we noticed it because we check for method availability at startup time. Downstream issue: godot-rust/gdext#489.
However, this affects all bindings, including godot-cpp. If someone tries to call the methods, it will fail for exported games. What makes this delicate is that it works in Debug mode but breaks in Release, so it can go undetected.

I think it's OK that not all methods are available, but this information should be exposed via extension_api.json, so that bindings can take appropriate steps, e.g. to remove the methods or warn upon their usage.

Steps to reproduce

In a GDExtension binding, try to call either of the following methods:

  • ResourceImporterOggVorbis::load_from_buffer
  • ResourceImporterOggVorbis::load_from_file

Minimal reproduction project (MRP)

N/A

@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 15, 2023

Indeed, this class seems to be only registered in editor builds (tools enabled) for the purpose of documenting import options. Likely all similar classes are affected, see #49524.

Interestingly enough, the class is present in template builds anyway, because it's needed by AudioStreamOggVorbis to load OGG files at runtime. But it doesn't need to be exposed for that itself. So the question is, what should happen with classes which are exposed but are not supposed to be used in the API.

@AThousandShips
Copy link
Member

The importer itself isn't exposed on non-tool builds, what happens if you use the method on AudioStreamOggVorbis instead?

@YuriSizov
Copy link
Contributor

YuriSizov commented Dec 15, 2023

IIUC, the issue is that the class is marked as core and not as editor. This is what needs to be fixed:

			"name": "ResourceImporterOggVorbis",
			"is_refcounted": true,
			"is_instantiable": true,
			"inherits": "ResourceImporter",
			"api_type": "core",

Same for ResourceImporterMP3, which is also registered in its own module. The rest are registered with editor classes and are fine.

@MeganSpencer
Copy link

MeganSpencer commented Dec 18, 2023

It looks like 4.3 is a long way off. I hit this bug on the very literal actual first simple Godot rust hello world project. I suspect that most people trying gdext with rust is going to hit this problem between now and May. So it may be worth considering merging this into an earlier minor release if one is planned. The impact of the bug on Godot-rust is that when you export your project to a mobile device, the rust library will not initialize at all, making mobile exports fail in an obscure and difficult to understand way. (Basically rust nodes are initialized to be placeholder objects, and calls to the placeholder objects fail only on the mobile device with no clear reason why its happening)

@Bromeon
Copy link
Contributor Author

Bromeon commented Dec 18, 2023

@MeganSpencer I implemented the same change downstream in godot-rust, to bridge the time until this hits a stable release. If you update your version to latest master, you should not have problems in export builds anymore.

godot-rust/gdext#537

@YuriSizov
Copy link
Contributor

So it may be worth considering merging this into an earlier minor release if one is planned.

It is already considered, hence the cherrypick label on the PR.

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.

4 participants