Editor Crash when duplicating a GDExtension node if it have an internal child node event if it is added in constructor.

[Daylily-Zeleen]

Tested versions

4.3dev[7529c0bec597d70bc61975a82063bb5112ac8879]

System information

Windows 10, Vulkan, GTX1060

Issue description

Editor Crah when duplicating a GDExtension node(A) if it have an internal child node(B) event if it is added in constructor. Because the flag data.in_constructor already be cleared when A adding B in A's constructor.

This can be fixed by #91019 or #91018.

Steps to reproduce

1. Create a GDExtension node which have an internal node, for example:

class MyNode : public Node {
	GDCLASS(MyNode, Node)

	Node *child;

protected:
	static void _bind_methods() {}

public:
	MyNode() {
		child = memnew(Node);
		add_child(child);
	}
};

2. Add a MyNode node in editor node tree. than duplicate this node.
3. The editor will crash.

---

Please download the attached file for testing, the test project is in test_project folder. If you are not using windows, please compile GDExtension plugin by youself.

Minimal reproduction project (MRP)

bug report.zip

[AThousandShips]

What happens if you construct it with NOTIFICATION_POSTINITIALIZE instead?

See:

• https://github.com/godotengine/godot-cpp/issues/1312

[Daylily-Zeleen]

I have encountered the same signal connecting issue. To solve this issue, I connect signals when receiving NOTIFICATION_POSTINITIALIZE.

---

But in this case, add child in constructor and add child when receiving NOTIFICATION_POSTINITIALIZE in _notification will bring the same result. Currently, the child node is internal or not is depend on the flag data.in_constructor of its parent. The reason of this issue is that this flag is cleared too early, even before construction, let alone when the NOTIFICATION_POSTINITIALIZE is received in GDExtension node.

[AThousandShips]

The flag isn't cleared until NOTIFICATION_POSTINITIALIZE is sent, so it should be called on the node first, unless it's not sent to the child first

But this is a limitation that exists for GDScript as well (and C# too possibly, haven't tested), so it's a limitation to expand on, and should be handled directly IMO, a limitation to work around, and existing code works around it if they do create nodes in the constructor, I'd say the solution is to handle that correctly instead of depending on a missing feature

Existing code handling this properly will likely be broken by fixes to that side, as mentioned in the PR

[dsnopek]

I don't think we can support calling add_child() in a constructor in godot-cpp, because depending on how the class was created, it won't be fully initialized at that point.

I do think NOTIFICATION_POSTINITIALIZE is the way to go.

Currently, the child node is internal or not is depend on the flag data.in_constructor of its parent.

I think we need another way to mark a child node as internal. I had thought that the 3rd argument of add_child() would let you mark a child as internal, but looking at the code, it seems that doesn't have an effect on duplicate(). :-/ Maybe it should?

[Daylily-Zeleen]

Let's clearify the concept first, the "internal node" in the issue is means a node is added in parent's construcotr, and its flags data.parent_owned will be set by the flag data.in_constructor of its parent. "A node owned by its parent" should be a more appropriate statement. A node owned by its parent will be skiped when duplicating its parent, because an equivalent node will be constructed by the copied parent node‘s constructor.

I don't think we can support calling add_child() in a constructor in godot-cpp, because depending on how the class was created, it won't be fully initialized at that point.

This question need more discussion. For my situation, I already use a lot of add_child() in a constructor in godot-cpp. Because there have not explicit prohibition, and this is intuitive like in godot. Until I encounter this issue, add a node in constructor is not owned by parents like in godot srouce code. However, this is not due to add_child() in constructor, _owner field already be set and can be called successfully.

If a node is not fully initialized in constructor is the reason that we should not support add_child() in a constructor, it means that use native apis in constructor is not supported. This is obviously very counter intuitive.

So I think we should try to advance the timing of setting instance and setting instance binding in godot-cpp, this is another topic.

I think we need another way to mark a child node as internal. I had thought that the 3rd argument of add_child() would let you mark a child as internal, but looking at the code, it seems that doesn't have an effect on duplicate(). :-/ Maybe it should?

I agree that a new way to mark a node owned by parent should be provided, I think not all internal node can be decided in constructor. I think the 3rd argument of add_child() should have similar usage to mark a node to be sikped when duplicating its parent, too. But now, let this argument can mark a node owned by parent will break compatibility (we can wait for Godot 5.0 😂).