godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.75k stars 578 forks source link

Macro memnew_arr() crashes the game. #1551

Open vinni-richburgh opened 3 months ago

vinni-richburgh commented 3 months ago

Godot version

4.3

godot-cpp version

godot-4.3-stable

System information

Apple M1 Homebrew Godot and dependencies.

Issue description

Since the update to 4.3 the memnew_arr() macro/the code it expands to crashes the scene. Initially saw this happen in my projects using CMake, but it also happens with scons built projects. (See MRP)

Steps to reproduce

Download the mrp. Run "git init". Run "git submodule add https://github.com/godotengine/godot-cpp.git". Run "scons". Open the project and observe a crash when opening the test_scene.tscn scene, where the Demo node is already present.

Minimal reproduction project

memnew_arr.zip

dsnopek commented 3 months ago

Thanks!

I tested your project, and it crashes for me too. This is the key code:

godot::memnew_arr(godot::Node, 4);

I don't think memnew_arr() is meant to be used with classes descending from Object. The template that implements it doesn't do anything special for them, it just calls new, whereas they would need to be created with memnew().

Grepping through Godot's source code, I don't see any instance of memnew_arr() used for Object types (and the memnew_arr() in godot-cpp is the same as the one in Godot).

If we can confirm that this isn't intended to work, I think we could add a static_assert() to give developers a friendly message about it, rather than crashing.

vinni-richburgh commented 3 months ago

Thank you very much for your kind reply!

Yeah, now that you mentioned it, the new operator and memnew_arr() not being documented for such use cases should have raised a few concerns for me!

If indeed applicable, telling the devs about their misuse of the macro/function sounds like a good idea to me, since there already are GDExtension tutorials in the wild promoting this usage.

Best Regards Vinni