Fix Node `duplicate` mapping of child properties.

[0xcafeb33f]

trafficstars

In Godot 4.3, the way the duplicate function copied properties was changed. This results in many error messages when any Node with internal nodes as children is duplicated, as there is not a one-to-one correspondence between the original and copied children indices. Instead, Node paths need to be used, as they are when duplicating signals. This commit fixes the log spam when duplicating Nodes with internal children by finding the correct corresponding child to copy properties to.

Fixes #92880 . Please cherrypick to 4.3 if possible.

P.S. this is my first pull request, please let me know if any of the metadata is incorrect.

[100gold]

In my case, I get the "Child node disappeared while duplicating" message when I'm using duplicate() for a scene with dynamically added objects. By default, the duplicate function sets the DUPLICATE_USE_INSTANTIATION flag, which is not compatible with dynamically modified scenes (#92829). I think this means the original problem with GraphEdit is not in the 'duplicate' function.

[0xcafeb33f]

In my case, I get the "Child node disappeared while duplicating" message when I'm using duplicate() for a scene with dynamically added objects. By default, the duplicate function sets the DUPLICATE_USE_INSTANTIATION flag, which is not compatible with dynamically modified scenes (#92829). I think this means the original problem with GraphEdit is not in the 'duplicate' function.

Yes, before this commit, the "Child node disappeared while duplicating" error appears whenever internal nodes / nodes added by scripts are duplicated (depending on the exact node tree layout, possibly also a crash). GraphEdit adds internal nodes when instantiated: https://github.com/godotengine/godot/blob/db76de5de8a415b29be4c7dd84b99bd0fe260822/scene/gui/graph_edit.cpp#L2775-L2793

This commit fixes the property mapping so that regardless of internal nodes, it will still copy the properties correctly. This fixes the duplicate() behavior on GraphEdit nodes as a byproduct.

The internal nodes being skipped mentioned in #92829 seems to be expected behavior, and is in another function:

https://github.com/godotengine/godot/blob/f1d43ed49a969d4c499b36c343b13ed47ccfa7a8/scene/main/node.cpp#L2719-L2721

[wagnerfs]

Just coming by to say this PR doesn't work either, breaks on dynamically created objects, so for it to work like it used to it should actually only be:

for (int i = 0; i < p_copy->get_child_count(); i++) {
    Node *copy_child = p_copy->get_child(i);
    ERR_FAIL_NULL_MSG(copy_child, "Child node disappeared while duplicating.");
    _duplicate_properties(p_root, p_original->get_child(i), copy_child, p_flags);
}

[0xcafeb33f]

Just coming by to say this PR doesn't work either, breaks on dynamically created objects, so for it to work like it used to it should actually only be:

for (int i = 0; i < p_copy->get_child_count(); i++) {
    Node *copy_child = p_copy->get_child(i);
    ERR_FAIL_NULL_MSG(copy_child, "Child node disappeared while duplicating.");
    _duplicate_properties(p_root, p_original->get_child(i), copy_child, p_flags);
}

This code won't work as-is because p_copy->get_child(i); does not correspond to p_original->get_child(i); when internal nodes exist.

PR #89442 fixes this by converting the function calls to get_child(i, false) which ignores internal nodes. After reviewing that PR, I am in favor of closing this one as it looks like #89442 has a more succinct approach to resolving this issue.

One thing to note: _duplicate_signals currently uses this same code pattern of get_path_to and get_node.

https://github.com/godotengine/godot/blob/2c136e6170a40f58f2dfb89d32eadfca7156ef37/scene/main/node.cpp#L2979-L2980

Can you check if, on the master branch, DUPLICATE_SIGNALS currently copies signals of dynamically created objects with internal nodes correctly? If not, I will submit a separate PR to make the _duplicate_signals function consistent with the fix in #89442 (thereby fixing the [potential] bug). I was not able to reproduce the problem with dynamically created objects myself.

[wagnerfs]

@0xcafeb33f here's the thing, neither of the PRs solve dynamically created objects.

PR https://github.com/godotengine/godot/pull/89442 doesn't change a thing when it comes to that, this one also somewhat doesn't work because it works on some dynamic added objects (like Meshinstance3D) but breaks for Timer.

At this point I believe the regression might be somewhere else.

[0xcafeb33f]

@wagnerfs Thanks for sharing the error messages. You said it breaks on Timer, and it looks like there's an error with Skeleton(2D/3D)? Would you be able to share a minimally reproducible project or scene, or point me to a bug report that describes this specific issue? I'd like to track down the cause.

[wagnerfs]

@0xcafeb33f it's a little late around here already, I'll most likely debug the _duplicate_properties method tomorrow to see where exactly the copy nodes are skipped, I'll drop a small project here as well.

[wagnerfs]

Okay so as I expected, _duplicate_properties will always fail because the nodes added by script are just being fully ignored by https://github.com/godotengine/godot/commit/f19c4191260041eed72daa8a2633187e71500d10 :

if (!descendant->get_owner()) {
	continue; // Internal nodes or nodes added by scripts.
}

So it'll always fail when p_copy is matched with p_original no matter what. I did try to manually set owner for my script added nodes but no luck. Oddly enough, fully removing the lines above fully fixes duplication, of course at the cost of duplicating internal nodes as well, so MAYBE https://github.com/godotengine/godot/pull/89442 should fix the internal node duplication issue allowing the PR that prevents nodes added by script to be removed.

Oh, the PR here is actually giving me crashes, probably because it's still trying to duplicate a property of a child that simply doesn't exist, I guess that's why I got a crash with the Timer on the error output, I got a timer somewhere added to the scene by script and then have the entire scene duplicated.

I personally don't understand why preventing the user to have a node they added through code themselves from being duplicated, and adding several steps to make it work like adding owner and all those shenanigans. But anyway, I'll probably test KoBeWi's PR along with removing the descendant->get_owner() line and see how it goes.

[0xcafeb33f]

@wagnerfs I tried to reproduce your issue but wasn't able to. Minimum reproduction project: duplicate-error.zip. If you are able to reproduce the issue, please open a bug report and tag me.

Per the above discussion supporting #89442 instead, I'm closing this PR.