godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.66k stars 503 forks source link

Custom GDNative class as class variable triggers crash #322

Open SnailBones opened 5 years ago

SnailBones commented 5 years ago

I have one GDNative object listed as a property of another. When returning a pointer to the property or casting said pointer to a reference, the engine crashes.

However, if I initialize a new instance and copy the values over, it works as intended. Unfortunately, my workaround creates a drain on performance. Perhaps Godot only has access to the heap?

C++ code:

Grid *Model::getState()
// Ref<Grid> Model::getState()
{
    // This fails:
    return next_state;

    // This also fails:
    Ref<Grid> ref = Ref<Grid>(next_state);
    return ref;

    // This works:
    Grid *v = Grid::_new();
    v->init(width, height); // Coping object
    v->imitate(next_state); 
    return v;
}

Backtrace:

[0] RaiseException
[1] _CxxThrowException (d:\agent\_work\2\s\src\vctools\crt\vcruntime\src\eh\throw.cpp:133)
[2] __RTDynamicCast (d:\agent\_work\2\s\src\vctools\crt\vcruntime\src\eh\rtti.cpp:291)
[3] Object::cast_to<Reference> (C:\Users\Aidan\Downloads\godot-master\godot-master\core\object.h:610)
[4] godot_variant_new_object (C:\Users\Aidan\Downloads\godot-master\godot-master\modules\gdnative\gdnative\variant.cpp:175)
[5] godot::Variant::Variant (C:\Users\Aidan\Documents\Fun Stuff\Godot\gdnative_cpp_example\godot-cpp\src\core\Variant.cpp:122)
[6] godot::_WrappedMethod<godot::Model,godot::Grid *>::apply<> (C:\Users\Aidan\Documents\Fun Stuff\Godot\Cells d2\godot-cpp\include\core\Godot.hpp:183)
[7] godot::__wrapped_method<godot::Model,godot::Grid *> (C:\Users\Aidan\Documents\Fun Stuff\Godot\Cells d2\godot-cpp\include\core\Godot.hpp:210)
[8] NativeScriptInstance::call (C:\Users\Aidan\Downloads\godot-master\godot-master\modules\gdnative\nativescript\nativescript.cpp:719)
[9] Object::call (C:\Users\Aidan\Downloads\godot-master\godot-master\core\object.cpp:912)
[10] Variant::call_ptr (C:\Users\Aidan\Downloads\godot-master\godot-master\core\variant_call.cpp:1074)
[11] GDScriptFunction::call (C:\Users\Aidan\Downloads\godot-master\godot-master\modules\gdscript\gdscript_function.cpp:1079)
[12] GDScriptInstance::call_multilevel (C:\Users\Aidan\Downloads\godot-master\godot-master\modules\gdscript\gdscript.cpp:1180)
[13] Node::_notification (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\main\node.cpp:60)
[14] Node::_notificationv (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\main\node.h:46)
[15] CanvasItem::_notificationv (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\2d\canvas_item.h:166)
[16] Control::_notificationv (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\gui\control.h:51)
[17] Object::notification (C:\Users\Aidan\Downloads\godot-master\godot-master\core\object.cpp:945)
[18] SceneTree::_notify_group_pause (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\main\scene_tree.cpp:959)
[19] SceneTree::idle (C:\Users\Aidan\Downloads\godot-master\godot-master\scene\main\scene_tree.cpp:515)
[20] Main::iteration (C:\Users\Aidan\Downloads\godot-master\godot-master\main\main.cpp:1920)
[21] OS_Windows::run (C:\Users\Aidan\Downloads\godot-master\godot-master\platform\windows\os_windows.cpp:3010)
[22] widechar_main (C:\Users\Aidan\Downloads\godot-master\godot-master\platform\windows\godot_windows.cpp:162)
[23] _main (C:\Users\Aidan\Downloads\godot-master\godot-master\platform\windows\godot_windows.cpp:184)
[24] main (C:\Users\Aidan\Downloads\godot-master\godot-master\platform\windows\godot_windows.cpp:196)
[25] __scrt_common_main_seh (d:\agent\_work\2\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[26] BaseThreadInitThunk
-- END OF BACKTRACE --
BastiaanOlij commented 5 years ago

The problem with structuring it like this is that Godot looses the ability to reference count the object and destroys it before its ready.

You need to define next_state as a Ref as well and then call instance on it to create it:

Ref<Grid> next_state;

...

// instantiate our object properly initialising our refcount.
next_state.instance()

...

Ref<Grid> Model::get_state() {
    return next_state;
}

When you did Ref<Grid> ref = Ref<Grid>(next_state); you are creating a new reference counter object with reference count set to 1. Once that goes out of scope next_state will be destructed even though you are still using it, hence the crash.

Zylann commented 4 years ago

Is this issue fixed? (tbh there are others like this which don't seem to be issues with Godot-Cpp but more like questions/troubleshootings which are still hanging around)