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

GDExtension "RandomNumberGenerator rng;" crashes editor in 4.3 but not in 4.2 #95745

Closed
NatGazer opened this issue Aug 18, 2024 · 5 comments
Closed

Comments

@NatGazer
Copy link

Tested versions

Tested in Godot 4.3 official and Godot 4.2.2 official

System information

Windows 10 - Vulkan (Forward+) - RTX 3060 laptop - AMD Ryzen 5 5600H - 24GB RAM

Issue description

That's it, I'm migrating my project and I had to change RandomNumberGenerator rng; to RandomNumberGenerator* rng; and now it doesn't crash anymore. I don't know if the bug here is from 4.2 or 4.3 because I don't know what's the expected behaviour😅. Here's the console output when the crash occurs:

Godot Engine v4.3.stable.official.77dcf97d8 - https://godotengine.org
Vulkan 1.3.278 - Forward+ - Using Device #0: NVIDIA - NVIDIA GeForce RTX 3060 Laptop GPU

ERROR: BUG: Godot Object created without binding callbacks. Did you forget to use memnew()?
   at: (src\classes\wrapped.cpp:87)

================================================================
CrashHandlerException: Program crashed with signal 4
Engine version: Godot Engine v4.3.stable.official (77dcf97d82cbfe4e4615475fa52ca03da645dbd8)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] error(-1): no debug info in PE/COFF executable
[2] error(-1): no debug info in PE/COFF executable
[3] error(-1): no debug info in PE/COFF executable
[4] error(-1): no debug info in PE/COFF executable
[5] error(-1): no debug info in PE/COFF executable
[6] error(-1): no debug info in PE/COFF executable
[7] error(-1): no debug info in PE/COFF executable
[8] error(-1): no debug info in PE/COFF executable
[9] error(-1): no debug info in PE/COFF executable
[10] error(-1): no debug info in PE/COFF executable
[11] error(-1): no debug info in PE/COFF executable
[12] error(-1): no debug info in PE/COFF executable
[13] error(-1): no debug info in PE/COFF executable
[14] error(-1): no debug info in PE/COFF executable
[15] error(-1): no debug info in PE/COFF executable
[16] error(-1): no debug info in PE/COFF executable
[17] error(-1): no debug info in PE/COFF executable
[18] error(-1): no debug info in PE/COFF executable
[19] error(-1): no debug info in PE/COFF executable
[20] error(-1): no debug info in PE/COFF executable
[21] error(-1): no debug info in PE/COFF executable
[22] error(-1): no debug info in PE/COFF executable
[23] error(-1): no debug info in PE/COFF executable
-- END OF BACKTRACE --
================================================================

Another line of code that causes exactly the same error/crash is this one:

RID cachedSphereRID = SphereShape3D().get_rid();

I fixed it by using this expression instead:

SphereShape3D* sphere = memnew(SphereShape3D);
RID cachedSphereRID = sphere->get_rid();

I believe they crash for the same reason.

Steps to reproduce

Scenario 1:
Define a variable from RandomNumberGenerator class in your .h file, compile GDExtension and open editor

Scenario 2:
Try to get the RID of a SphereShape3D() using the following line of code in your header:
RID cachedSphereRID = SphereShape3D().get_rid();

Minimal reproduction project (MRP)

N/A

@NatGazer
Copy link
Author

I found out that the line of code below makes the game crash as well in 4.3 but not 4.2:
MultiMesh* tempMultimesh = new MultiMesh();

To solve it I replaced it by:

MultiMesh* tempMultimesh = memnew(MultiMesh());

Looks like I was creating variables in the wrong way, but only now with this update Godot complained...

@raulsntos
Copy link
Member

This is intentional, see godotengine/godot-cpp#1446 (comment):

Well, creating bound Godot classes in godot-cpp without memnew() really shouldn't ever have worked; doing so would mean not using the GDExtension API as intended. So, if it worked in the past, it was entirely accidental. I suspect there are cases where that would lead to unexpected behavior that you just didn't manage to encounter.

And if you read the the crash message you got, it tells you to use memnew() (this note was added in godotengine/godot-cpp#1510):

ERROR: BUG: Godot Object created without binding callbacks. Did you forget to use memnew()?

Also, specifically for RandomNumberGenerator:

If a Godot module wanted to use that class, it should also use Ref<>, and if it isn't, I suspect that could also lead to unexpected behavior in some cases. Those uses in the tests should probably be fixed!

So it should be created like this:

Ref<RandomNumberGenerator> rng;
rng.instantiate();

@NatGazer
Copy link
Author

Ok, thank you @raulsntos. Just to finalize, do you think using RandomNumberGenerator* rng; can create unexpected behaviour? And one more thing, I think this change should be in Upgrading from Godot 4.2 to Godot 4.3 since this can be considered a breaking compatibility change! Thanks for the clarification

@raulsntos
Copy link
Member

RandomNumberGenerator is a RefCounted object, but if you don't use the Ref<> template then it won't actually be reference counted.

I think this change should be in Upgrading from Godot 4.2 to Godot 4.3 since this can be considered a breaking compatibility change

Considering how this should never have worked, I have mixed feelings about documenting it. But if there's a lot of users that were relying on the bug then maybe we could at least mention it. I'll leave the decision to the @godotengine/gdextension team.

@dsnopek
Copy link
Contributor

dsnopek commented Aug 22, 2024

I'm also not crazy about documenting this on the "Upgrading..." page.

However, I do think it would be really good to mention on a GDExtension page related to godot-cpp that Ref<T> needs to be used for all RefCounted objects, and memnew() for all other objects descending from Object. Now that we have the error message, that'll probably do a lot to help developers learn about this, but it'd be nice to have it called out somewhere in the docs as well.

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

No branches or pull requests

5 participants