Fix blend_node crash with invalid AnimationNode reference

[jsjtxietian]

Fixes #86070

It's a regression because 5acf6b4 changed node_state.connections.clear() to p_node->node_state.connections.clear() , previously the crash can be prevented by null check in _blend_node .

[TokageItLab]

It looks like the null check in the first line of _blend_node() could be removed, as it would be a duplicate check. If so, does it cause a problem that there is no check on the call within blend_input()?

[jsjtxietian]

If so, does it cause a problem that there is no check on the call within blend_input()

Reading the code, it looks like the provided p_node must be a valid one.

[akien-mga]

It looks like the null check in the first line of _blend_node() could be removed, as it would be a duplicate check. If so, does it cause a problem that there is no check on the call within blend_input()?

_blend_node() is also called in other places. Those would also need a check if we're removing it from _blend_node itself:

scene/animation/animation_blend_tree.cpp
1410:   return _blend_node(output, "output", this, pi, FILTER_IGNORE, true, p_test_only, nullptr);

scene/animation/animation_tree.h
87:     double _blend_node(Ref<AnimationNode> p_node, const StringName &p_subpath, AnimationNode *p_new_parent, AnimationMixer::PlaybackInfo p_playback_info, FilterAction p_filter = FILTER_IGNORE, bool p_sync = true, bool p_test_only = false, real_t *r_activity = nullptr);

scene/animation/animation_tree.cpp
146:    double ret = _blend_node(node, node_name, nullptr, p_playback_info, p_filter, p_sync, p_test_only, &activity);
157:    return _blend_node(p_node, p_subpath, this, p_playback_info, p_filter, p_sync, p_test_only, nullptr);

[jsjtxietian]

Those would also need a check if we're removing it from _blend_node itself:

I'm not 100% sure about the logic here. But like I said earlier, from the code, the other places calling _blend_node seems all get the AnimationNode reference from the nodes in defined in AnimationNodeBlendTree, which seems to assumes that it's always have a valid node.

But there's no harm in adding more checkes I guess, or just keep the check inside _blend_node

[TokageItLab]

I prefer to add more two checks to

animation_tree.cpp line 167:

	Ref<AnimationNode> node = blend_tree->get_node(node_name);
	ERR_FAIL_COND_V (node.is_null(), NodeTimeInfo());

	real_t activity = 0.0;
	Vector<AnimationTree::Activity> *activity_ptr = process_state->tree->input_activity_map.getptr(node_state.base_path);
	NodeTimeInfo nti = _blend_node(node, node_name, nullptr, p_playback_info, p_filter, p_sync, p_test_only, &activity);

	if (activity_ptr && p_input < activity_ptr->size()) {
		activity_ptr->write[p_input].last_pass = process_state->last_pass;
		activity_ptr->write[p_input].activity = activity;
	}
	return nti;

animation_blend_tree.cpp line 1635:

AnimationNode::NodeTimeInfo AnimationNodeBlendTree::_process(const AnimationMixer::PlaybackInfo p_playback_info, bool p_test_only) {
	Ref<AnimationNodeOutput> output = nodes[SceneStringNames::get_singleton()->output].node;
	node_state.connections = nodes[SceneStringNames::get_singleton()->output].connections;
	ERR_FAIL_COND_V (output.is_null(), NodeTimeInfo());

	AnimationMixer::PlaybackInfo pi = p_playback_info;
	pi.weight = 1.0;

	return _blend_node(output, "output", this, pi, FILTER_IGNORE, true, p_test_only, nullptr);
}

[akien-mga]

Thanks!