GDExtension objects made without memnew miss callbacks and crash

[paulmiller]

Godot version

4.0

System information

Arch Linux

Issue description

Say I have a GDExtension class Foo:

class Foo : public godot::Node {
  GDCLASS(Foo, godot::Node);

protected:
  static void _bind_methods();

public:
  void _enter_tree() override;
  void hello();
};

void Foo::_bind_methods() {
  ClassDB::bind_method(D_METHOD("hello"), &Foo::hello);
}

void Foo::_enter_tree() {
  UtilityFunctions::print("Foo::_enter_tree");
}

void Foo::hello() {
  UtilityFunctions::print("Foo::hello");
}

If I instantiate Foo in C++ using new, the resulting object seems to work at first; I can add it to the scene tree, and add a Sprite2D under it which appears normally:

var f : Foo = … # Foo created in C++ with new
add_child(f)

var s = Sprite2D.new()
s.position = Vector2(100,100)
s.texture = …
f.add_child(s)

But it misbehaves in surprising ways: Foo::_enter_tree() doesn't get called for add_child(f), and if I try to call f.hello(), the game crashes.

Foo objects instantiated with memnew don't have this problem.

---

If I'm reading the code correctly, every extension object (Foo in this example) gets assigned a corresponding Godot engine object (Node in this example). Each contains an opaque pointer to the other:

• Foo inherits from Wrapped. The Wrapped constructor sets Wrapped::_owner to point to the Node.
• Node inherits from Object. ClassDB::set_object_extension_instance sets Object::_extension_instance to point to the Foo.

It seems like it shouldn't matter how I instantiate Foo - new Foo, memnew(Foo), and std::make_unique<Foo>() should all call the Wrapped constructor.

Although, the existence of the _extension_instance pointer implies some limitations on how I can use Foo. For example, a std::vector<Foo> presumably wouldn't work, since vector reallocation would
invalidate the _extension_instance of each corresponding Node.

It's not clear to me exactly how I'm allowed to use GDExtension objects, and so I can't tell whether this is a Godot bug or a documentation bug. Maybe new Foo isn't supposed to work in the first place?

Specifically, which of the following is allowed?

• instantiating a Foo with new (as above)
• freeing a Foo with delete (throws exception if the Foo was created with memnew!)
• putting Foo objects in a reallocating container like std::vector<Foo>
• handling Foo with a managed pointer like std::unique_ptr<Foo>
• including Foo as a field of another GDExtension class, like:

class Bar : public godot::Node {
  GDCLASS(Bar, godot::Node);
  Foo f;
};

• including Foo as a field of a regular class, like:

class Baz {
  Foo f;
};

If any of these uses are forbidden, they should be called out in the documentation.

Steps to reproduce

The attached project has the above code, and some other test functions.

• unzip make-node-test.zip
• check out godot-cpp into make-node-test/extension/godot-cpp/
• cd make-node-test/extension/out/
• cmake .. and build, e.g. CC=gcc CXX=g++ cmake -G Ninja .. && ninja
• cp libfoo.so ../../project/
• <Godot executable> --path ../../project/

Minimal reproduction project

make-node-test.zip

[paulmiller]

I investigated a bit more.

memnew calls godot::Wrapped::_postinitialize > gdextension_object_set_instance > ClassDB::set_object_extension_instance which sets Object::_extension_instance.

None of the other ways of instantiating Foo call _postinitialize, but they do call the Wrapped constructor, which instantiates Node anyway:

Wrapped::Wrapped(const StringName p_godot_class) {
  _owner = godot::internal::gde_interface->classdb_construct_object(reinterpret_cast<GDExtensionConstStringNamePtr>(p_godot_class._native_ptr()));
}

So you end up with both Foo and Node objects, and the Foo's _owner points to the Node, but the Node's _extension_instance is null. That's why I can't call Foo::hello() from gdscript, and why Foo:_enter_tree() doesn't get called; Node::_propagate_enter_tree() calls GDVIRTUAL_CALL(_enter_tree) which finds _extension_instance null and does nothing.

This situation could be avoided by setting both _owner and _extension_instance in the Wrapped constructor, rather than having a separate _postinitialize step. I don't understand why it was designed this way, so I hesitate to try to write a fix.

[akien-mga]

Moved to godot-cpp repository as it appears to be specific to the C++ Wrapped and not the GDExtension interface.

[paulmiller]

I tested composition (Bar and Baz examples, above), and the member objects do indeed end up with null _extension_instance pointers. Even so, composed objects mostly work. For example, you can do something like:

class Test : public Node3D {
  GDCLASS(Test, Node3D);

protected:
  static void _bind_methods();

public:
  Area3D area;

  void on_mouse_entered_area();
  void _ready() override;
};

void Test::_bind_methods() {
  ClassDB::bind_method(D_METHOD("on_mouse_entered_area"),
      &Test::on_mouse_entered_area);
}

void Test::on_mouse_entered_area() {
  UtilityFunctions::print("hello");
}

void Test::_ready() {
  area.connect("mouse_entered", Callable(this, "on_mouse_entered_area"));
  area.add_child( /* some collision shape */ );
  add_child(&area);
}

area->_owner->_extension_instance will be null here, but it still works, so long as Test is instantiated properly. The mouse_entered signal is routed to the Test, not the Area3D, and Test->_owner->_extension_instance is valid, so it doesn't crash.

I still can't tell whether this usage is meant to be supported or not.

[follower]

FWIW, in case it's useful:

• I recall the previous documentation for GDNative specifically mentions not using new nor delete operators (among other restrictions):

  godot-cpp/README.md

  Lines 322 to 372 in 7c09b54

  	### Using Godot classes in C++
  	Godot expects you to manage its classes the same way the engine does. These rules apply to all Godot classes, including your NativeScripts, but not to any normal C++ classes used in your library.
  	- Instantiate Objects using `_new()`, not C++'s `new` operator.
  	```cpp
  	Sprite *sprite = Sprite::_new();
  	```
  	- Destroy Nodes using `queue_free()`, not C++'s `delete` operator.
  	```cpp
  	some_old_node->queue_free();
  	```
  	- Wrap References in `Ref` instead of passing around raw pointers. They are reference-counted and don't need to be freed manually.
  	```cpp
  	Ref<Texture> texture = resource_loader->load("res://icon.png");
  	```
  	- Pass core types that do *not* inherit Object by value. The containers (Array, Dictionary, PoolArray, String) manage their own memory and do not need to be explicitly initialized or freed.
  	```cpp
  	Array ints;
  	ints.append(123);
  	return ints;
  	```
  	- Initialize your NativeScript classes in their `_init()` method, not their constructor. The constructor can't access the base class's methods.
  	- Cast objects using `Object::cast_to`, not unsafe C-style casts or `static_cast`.
  	```cpp
  	MeshInstance *m = Object::cast_to<MeshInstance>(get_node("ChildNode"));
  	// `m` will be null if it's not a MeshInstance
  	if (m) { ... }
  	```
  	- **Never** use Godot types in static or global variables. The Godot API isn't loaded until after their constructors are called.
  	```cpp
  	String s; // crashes
  	class SomeClass {
  	static Dictionary d; // crashes
  	static Node *node_a = NULL; // fine, it's just a pointer
  	static Node *node_b = Node::_new(); // crashes
  	};
  	```

  I have no idea whether/which of these constraints have changed with GDExtension.

• I did notice one possibly related commit which included a commit message with more context: 38ee8bf

  If you haven't already, it may be worth checking out the commit messages for any related code (e.g. via the "blame" view) to see if they help clarify anything in terms of correct/intended usage.

[Faless]

@paulmiller GDExtension objects must indeed be instantiated using memnew which ends up calling the Wrapped::_postinitialize method as you identified.

This should be properly documented.

[dsnopek]

There is a docs issue for mentioning that memnew() must be used, so I'm going to close this one. Thanks!