Fix animation mixer breaking animations with redundant check

[TCROC]

The animation mixer is breaking animations as described here: #88428

Shoutout to @monxa and @personalmountains for helping with the fix! Simply removing the incorrect and redundant check fixes it.

Incorrect reasoning here: #88428 (comment)

Redundant reasoning here: #88428 (comment)

[TokageItLab]

If you look at the inside of get_node_and_resource(), you can see that get_node() is called in the second line.

Node *Node::get_node_and_resource(const NodePath &p_path, Ref<Resource> &r_res, Vector<StringName> &r_leftover_subpath, bool p_last_is_property) const {
	ERR_THREAD_GUARD_V(nullptr);
	Node *node = get_node(p_path);

This means that if the node is not found, an error will occur that cannot be controlled by check_path, so the check is necessary. Perhaps some additional method for checking that does not target path needs to be implemented.

[akien-mga]

I confirm that this fixes broken animations on https://github.com/gdquest-demos/godot-4-3d-third-person-controller and https://github.com/godotengine/tps-demo/

Warning: The Godot TPS demo has another rendering regression since 4.3-dev3, which makes crazily flashing lights, this is not epilepsy safe.

[akien-mga]

@TokageItLab's assessment seems correct, but the demos I mentioned do not seem to raise any get_node errors, yet this check seems to completely break their animations. I'm puzzled.

[akien-mga]

Ok right, see #88428 (comment) which @TCROC linked in the OP. The difference is the value of the p_last_is_property boolean. So this means that has_node_and_resource fails in this case, when what you seem to want is to just prevent get_node from failing.

So you should just check has_node, or add p_last_is_property to has_node_and_resource if it's needed to do all the checks.

[akien-mga]

diff --git a/scene/animation/animation_mixer.cpp b/scene/animation/animation_mixer.cpp
index 706d210064..eec70e737f 100644
--- a/scene/animation/animation_mixer.cpp
+++ b/scene/animation/animation_mixer.cpp
@@ -661,7 +661,7 @@ bool AnimationMixer::_update_caches() {
 				Ref<Resource> resource;
 				Vector<StringName> leftover_path;
 
-				if (!parent->has_node_and_resource(path)) {
+				if (!parent->has_node(path)) {
 					if (check_path) {
 						WARN_PRINT_ED(mixer_name + ": '" + String(E) + "', couldn't resolve track:  '" + String(path) + "'. This warning can be disabled in Project Settings.");
 					}

This is a more minimal fix that should still prevent errors when get_node(p_path) fails.

[TokageItLab]

@akien-mga I think it's a problem when resource is not found.

I think it would be straighforward to add a p_last_is_property argument to has_node_and_resource() and propagate it to the internal get_node_and_resource(), but I am sure you are still double checking for its existence.

Probably the most intelligent way is to add an optional argument to get_node_and_resource(const NodePath &p_path, Ref<Resource> &r_res, Vector<StringName> &r_leftover_subpath, bool p_last_is_property, bool p_throw_error = true) and get_node(const NodePath &p_path, bool p_throw_error = true) to hide errors.

[akien-mga]

I wouldn't change get_node (you can always just call has_node first), but get_node_and_resource can simply use has_node.

[TokageItLab]

Ah right, get_node_and_resource() does not give any additional errors. So #88554 (comment) seems correct way.

[akien-mga]

I would just add this change to this PR:

diff --git a/scene/main/node.cpp b/scene/main/node.cpp
index e814e4f0cd..f827f68def 100644
--- a/scene/main/node.cpp
+++ b/scene/main/node.cpp
@@ -3025,9 +3025,9 @@ Array Node::_get_node_and_resource(const NodePath &p_path) {
 
 Node *Node::get_node_and_resource(const NodePath &p_path, Ref<Resource> &r_res, Vector<StringName> &r_leftover_subpath, bool p_last_is_property) const {
 	ERR_THREAD_GUARD_V(nullptr);
-	Node *node = get_node(p_path);
 	r_res = Ref<Resource>();
 	r_leftover_subpath = Vector<StringName>();
+	Node *node = get_node_or_null(p_path);
 	if (!node) {
 		return nullptr;
 	}

And then I think it's good to go. It's probably reasonable for get_node_and_resource not to print an error and just return nullptr.

[TokageItLab]

However, since path checking should be enabled by default (to detect errors for invalid path), the following code would be necessary.

if (check_path && !parent->has_node(path)) {
	WARN_PRINT_ED(mixer_name + ": '" + String(E) + "', couldn't resolve track:  '" + String(path) + "'. This warning can be disabled in Project Settings.");
	continue;
}

[akien-mga]

@TokageItLab This is already handled by the next check:

				Node *child = parent->get_node_and_resource(path, resource, leftover_path);
				if (!child) {
					if (check_path) {
						WARN_PRINT_ED(mixer_name + ": '" + String(E) + "', couldn't resolve track:  '" + String(path) + "'. This warning can be disabled in Project Settings.");
					}
					continue;
				}

child will be nullptr if it wouldn't pass has_node.

[TokageItLab]

@akien-mga Ah, make sense. So just replacing get_node to get_node_or_null is sufficient.

[akien-mga]

Superseded by #88557 which adds the Node::get_node_and_resource change.

Thanks for the contribution!

[TCROC]

Superseded by #88557 which adds the Node::get_node_and_resource change.

Thanks for the contribution!

Absolutely! :) Glad to help! :)