Fix Make Unique (Recursive) failing with AnimationNodes

[Wierdox]

Fixes #87766

Issue(regression from 93d180b) is caused by Make Unique (Recursive) eventually calling AnimationNodeBlendTree::add_node() and AnimationNodeStateMachine::add_node(), which will throw an error if the new node(AnimationNode) shares the name of any in-use node. On top of that, even if it doesn't err, it will reset the position(BlendTree, StateMachine) and connections(BlendTree) in the AnimationTree bottom panel. My naïve solution was to add if checks, to know when it's trying to replace a node with a duplicate.

There is probably a better solution by intercepting earlier, i.e. as seen in the below code snippet. However, I do not know enough, so I would only be fumbling if I tried it myself.

void EditorResourcePicker::_duplicate_selected_resources() {
	[...]

	if (meta.size() == 1) { // Root.
		edited_resource = unique_resource;
		emit_signal(SNAME("resource_changed"), edited_resource);
		_update_resource();
	} else {
                ////////////////////////////////////////////////////////////
                /* Add check here to intercept eventual add_node() call with
                   an alternate assignment that works for AnimationNodes  */
                ////////////////////////////////////////////////////////////
		Array parent_meta = item->get_parent()->get_metadata(0);
		Ref<Resource> parent = parent_meta[0];
		parent->set(meta[1], unique_resource); //This leads to add_node()
	}

}

Note: The way it worked before the regression, it would do a deep duplication (res->duplicate(true)) of the root node and replace it(doesn't cause errors for some reason). Maybe that info helps in finding a better solution.

[KoBeWi]

I'm wondering how #77855 caused the regression and why the fix is in the affected resource... Did it expose some hidden bug?

[KoBeWi]

The standard duplicate() method that resource picker was using before is also doing set(), but maybe it's order of operations, idk. I don't fully get the explanation, but it's clear to me that it's up to animation team to asses it. You said it's a bandaid, but if the added code is not detrimental then it might be fine. Although this also means that some other resource can be affected too.

[Wierdox]

OK, so after some time to think, I realize I jumped the gun when it came to figuring the problem. My previous post had some baseless assumptions and misunderstandings that I didn't realize until later. Fairly confidant this one is more accurate.

A big thing I found out when stepping through is that deep duplicate calls AnimationNodeBlendTree::_set()/AnimationNodeStateMachine::_set() the correct amount of times. It sends a duplicated node(AnimationNode) to _set(), then sends the position to _set(), and connections after all the nodes/positions are sent. However, using parent->set(meta[1], unique_resource); as seen below, it will send the child nodes to _set() again, after where deep duplicate would have finished. This obviously causes the error in add_node(), and if it doesn't err it would zero out the values for the nodes.

void EditorResourcePicker::_duplicate_selected_resources() {
	for (TreeItem *item = duplicate_resources_tree->get_root(); item; item = item->get_next_in_tree()) {
		[...]

		if (meta.size() == 1) { // Root.
			edited_resource = unique_resource;
			emit_signal(SNAME("resource_changed"), edited_resource);
			_update_resource();
		} else {
			Array parent_meta = item->get_parent()->get_metadata(0);
			Ref<Resource> parent = parent_meta[0];
			parent->set(meta[1], unique_resource);
		}
	}
}

Due to a better understanding, I can simplify my bandaid solution to just an if check of !has_node(node_name), before adding the node in _set() like so:

bool AnimationNodeBlendTree::_set(const StringName &p_name, const Variant &p_value) {
	[...]

	if (what == "node") {
		Ref<AnimationNode> anode = p_value;
		if (anode.is_valid() /*->*/&& !has_node(node_name)/*<-*/) {
			add_node(node_name, p_value);
		}
		return true;
	} [...]

Or that's what I would say, but it solves nothing, it isn't fully duplicating. It works in that it will no longer update position/name changes simultaneously between copied AnimationTrees, but the animation in a Animation node, and filters on a Add/Sub/Blend/OneShot node are STILL being updated across both, despite being made unique. This is not the case when using deep duplicate or my other solution.

The above description actually matches the outcome of using no fix... which makes sense. It's effectively the same as with the error, since it disregards the second set of nodes being sent to _set(). Therefore, the second set of nodes must include data for those sub resources on the aforementioned AnimationTree nodes. Or that's what I gather. The exact cause may be more elusive yet.

From brief testing with print in Object::set(), it seems that no matter the child's resource type for Make Unique (Recursive), it will be set twice. ie I used it on a mesh with a StandardMaterial3D child resource, and it printed the StandardMaterial3D twice(though not identical values). Meanwhile, switching back to a deep duplicate, it only printed once. I think that's unintended, though I don't have a great understanding of how that all works.

Images from test.

*I'm only printing StandardMaterial3D that pass through Object::set()

Summary:

Deep duplicate on root.	Duplicating one child at a time.
Calls set() once per child resource.	Calls set() twice per child resource. For AnimationNodes, the second time(after all the other _sets() have executed) it will throw the error.

Pre Regression	Master	Ignoring error in add_node()	(PR)Fix in add_node()
Nominal, uses deep duplication on root.	Partial duplication and throws an error, duplicates one child at a time.	Zeros out position/connections, but is properly duplicated.	Fixed, but messy and goes against the safeguards placed there.

That's my best estimation, for now. I'll probably prod it some more later, when I have time.

[KoBeWi]

I found out why the resource is set twice.

This is the old behavior:
Duplicate Resource (true) -> Subresource is duplicated -> Set Subresource

New Behavior:
Duplicate Resource (false) -> Set Subresource (not duplicated)
Go over all subresources -> If user enabled duplicating -> Set Subresource (duplicated)

The steps of setting and duplicating sub-resources while Resource's duplicate() is invoked are now separate, which means that the parent resource will always assign the old subresource and then they are optionally duplicated and set again.

And animation blend tree does not like if you set the same property again, even if to another value. This is problem with _set() logic I think. It should check if node already exists and if it does, it should replace it instead of trying to add a new one. While this fix makes sense, someone has to asses that it doesn't break animation tree editor.