Expose AnimationNode::get_animation_tree() to GDScript.

[Daylily-Zeleen]

Describe the project you are working on

Creating a complicated AnimationTree.

Describe the problem or limitation you are having in your project

For AnimationNodeStateMachine, we can set advance expression for transitions. to avoid setting parameters.

For AnimationNodeBlendTree, we can only set parameters to control the logic with a long parameter's path.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

By exposing AnimationNode::get_animation_tree(), we can extend AnimationNode to control it's parameters by overriding AnimationNode::_process(), instead of set parameters with a long path, and seperate contol logic to different AnimationNodes, avoiding code bloat of a single script.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Just bind AnimationNode::get_animation_tree() in _bind_methods().

==============================================================

Another solution:
The principle is get the main object which is related to specific animation tree.
Instead of getting animation tree, we can get the main object directly.

What I want to say is rename AnimationTree's property advance_expression_base_node to logic_base_node or base_node, then implement AnimationNode::get_base_node() to get base_node.

If this enhancement will not be used often, can it be worked around with a few lines of script?

Obviously it is impossible and unreasonable.

Is there a reason why this should be core and not an add-on in the asset library?

Same as above.

[fire]

@TokageItLab thoughts on exposing get animation tree?

[TokageItLab]

Having a reference from a Resource to a Node is basically not allowed.

A similar issue has been raised several times before. See also godotengine/godot#23629 (comment).

An AnimationTree is a Node, but an AnimationNodeBlendTree or an AnimationNodeStateMacine is a Resource and can be referenced by multiple AnimationTrees.

These resources have a reference to the Node for the blend calculation only when the process is called from the Node, but this reference to the Node may be rewritten many times during the process. Therefore, it makes no sense to expose them and it could the user to break references during the process.

Exposing internal variable in the process for gdscript is never happen without a fundamental change in the current AnimationTree design.

If you really want to use it, you can create a class that extends AnimationNode as a custom module, but in any case, current AnimationTree design cannot guarantee the safety and consistency of references from Resource to Node.

[TokageItLab]

instead of set parameters with a long path,

BTW, for this use case, you could propose something like a blackboard system (like godotengine/godot#61680 but that PR has many problems so you need to rewrite it from zero) or an alias feature.

[Daylily-Zeleen]

Another solution: The principle is get the main object which is related to specific animation tree. Instead of getting animation tree, we can get the main object directly.

What I want to say is rename AnimationTree's property advance_expression_base_node to logic_base_node or base_node, then implement AnimationNode::get_base_node() to get base_node.

Maybe implement this solution is better, or just add new property base_node to AnimationTree.

Then add and expose this methods to AnimationNode.

Node * AnimationNode::get_base_node() {
	ERR_FAILD_COND_V_MSG(!state, nullptr, "You can only call \"get_base_node\" during `_process()`");
	// ERR_FAIL_COND_V(!state->tree, nullptr); // Never happen.
	return state->tree->get_base_node();
}

Instead of get AnimationTree directly, the base_node is set to AnimationTree by users, it is the responsibility of the users themselves.

[TokageItLab]

I don't recommend setting the path string to the AnimationNode.

Instead, I remember that in a previous discussion with @SaracenOne, we talked about the ability to have a Map with a type in the property of the AnimationTree and make the AnimationNode get the value of the AnimationTree's Map.

[Daylily-Zeleen]

You may misunderstand my purpose.

The base_node(Node* or NodePath) is in AnimationTree.
Get_base_node in AnimationNode is get from state->tree, and only can be accessed during AnimationNode::_process().

The usage in GDScript may like this.

# my_blend2.gd
extend AnimationNodeBlend2

func _process(time: float, seek: bool, is_external_seeking: bool, test_only: bool) -> float:
	# Set paratemers before internal process by doing some calculation or check by "get_base_node()".
	var my_player := get_base_node() as MyPlayer
	if is_instance_valid(my_player):
		set_parameter(&"blend_amount", my_player.speed)

	return super(time, seek, is_external_seeking, test_only)

If this is implemented, user can use cast base_node to a specific type which is defined by user, all codes are static, I think it is better than use string base code.

[TokageItLab]

Extending AnimationNode to interrupt the process is clearly not for general users and it is never versatile.

[AThousandShips]

(Note you can just write super(time, seek, is_external_seeking, test_only) it will call the relevant function, to simplify things)

[Daylily-Zeleen]

Extending AnimationNode to interrupt the process is clearly not for general users and it is never versatile.

Of course not for general users, this only for user who need create a complicated AnimationTree.

Usually we use single script attach to AnimationTree node, if logic is complicated, may separate to differenet scripts and hold the reference of AnimationTree node.

Currently, we can only control animation logic outside the AnimationTree(except AnimationStateMachine) and must use parameter path to set parameters.

If the graph of AnimationTree be edited, we need changed many parameter paths in scripts which contol the logic of AnimationTree node.
The most important is that they are Strings, they can't be did valid check in script editor, it is very easy to generate mistakes.

By extends AnimationNode, the control logic is seperated to different AnimationNode and do some thing they concern naturally, and reduce mistakes by using set_parameter() insted of using parameter path string to anim_tree.set() or anim_tree[].

In other word, this proposal is aim at Improving maintainability of animation system.

[TokageItLab]

No, rather exposing internal variables means less maintainability. This is because when the processing of internal variables is changed, it provides a breakdown in compatibility for users who utilize them.

I recognize that you have a tendency to carelessly expose internal variables, please focus on safe and versatile API proposals that are consistent with actual use cases.

[Daylily-Zeleen]

Not internal variable.

The base_node is set to AnimationTree by users themselves.
Users need to be responsible for base_node, not any internal variables.

[TokageItLab]

You used expression_base as an example. But it needs to be passed to the AnimationNode by the AnimationTree and should not be set arbitrarily by the user to the AnimationNode. By "internal" I mean that "the user should not access".

[Daylily-Zeleen]

A new and independent property named "base_node", only used for this purpose, not participate in any internal logic.
Instead of using advance_expression_base_node directly.

Please look close into my comments.

[TokageItLab]

I already understood. The advance_expression_base_node is a property of the AnimationTree, not the AnimationNode. In order to use it from the AnimationNode you would need to get the AnimationTree from the AnimationNode or expose the internal variables of the AnimationNode, which is not allowed due to the safety issues mentioned above.

[Daylily-Zeleen]

In order to use it from the AnimationNode you would need to get the AnimationTree from the AnimationNode, which is not allowed due to the safety issues mentioned above.

Do you mean that set_parameter()/get_parameter() in GDScript is not allowed?

set_parameter() and get_parameter() are access variables in AnimationTree, too.

[TokageItLab]

The set/get parameter is done for the AnimationTree, not the AnimationNode.

The AnimationNode then internally fetches and processes those values from the AnimationTree (then, the values are never permanently stored in the AnimationNode). The user does not have access to the AnimationNode's internal variables.

In other words, you can implement an interface to the AnimationTree, but it is not allowed to access variables inside the AnimationNode that may be updated during the process.

[Daylily-Zeleen]

Like I said before, AnimationTree only hold the reference of base_node and not paticipate in any internal logic.
The status of this variable just like blend_amount. If it has something changed, it must be changed by user self.
User should responsible for this.

If it is not allow AnimationNode to access objects through AniamtionTree, except createing a fullly new type, what is the meaning of extending AnimationNode?

[TokageItLab]

We can allow base_nodes to be implemented in AnimationTree. However, we do not allow AnimationNode to expose the fetched base_node.

The extended AnimationNode is intended to be used to create AnimationNodes for example, mathematical processes such as Add4 and Sub3, or seeking processes such as ChangeFPS, using the blend_node() and blend_animation() functions.

[TokageItLab]

If the AnimationNode can correctly fetch information from the AnimationTree and process it, then there is no need for the user to fetch the AnimationTree again in the AnimationNode.

# my_blend2.gd
extend AnimationNodeBlend2

func _process(time: float, seek: bool, is_external_seeking: bool, test_only: bool) -> float:
	# Set paratemers before internal process by doing some calculation or check by "get_base_node()".
	var my_player := get_base_node() as MyPlayer
	if is_instance_valid(my_player):
		set_parameter(&"blend_amount", my_player.speed)

	return super(time, seek, is_external_seeking, test_only)

Instead, I remember that in a previous discussion with @SaracenOne, we talked about the ability to have a Map with a type in the property of the AnimationTree and make the AnimationNode get the value of the AnimationTree's Map.

For example, in this use case, we would have a VariableMap in the AnimationTree and implement a variable with a key called PlayerSpeed.

Next, prepare an interface like Use Variable for the blend amount in AnimatioNodeBlend2 and specify a key PlayerSpeed¹.

Then, if there are multiple players, simply write code like tree.[VariableMap/PlayerSpeed] = player.speed at the beginning of the tree._process(). Now there is no need to extend the AnimationNode and no need to use long paths and no need to expose internal variable.

Footnotes

1. It is possible that the key does not exist, this is almost equivalent to the possibility that your base_path points to something that does not exist. But it can just return the warning and the initial value, so it is much safer. ↩

[Daylily-Zeleen]

This still doesn't solve the code bloat of control logic (the part of "interface" in your comment).

I still believe that my solution is safe for internal process, more flexible for users, less over design for core animation system.

In my opinion, it is more elegent that implementing specific control logic in specific AnimationNode, but this is vialot current AnimationNode design principle.

Anyway, for this proposal, I will not implement and request to be merged.

Waiting for others to express their opinions.

[TokageItLab]

To implement a user-friendly interface, the code is always a bit cumbersome in the core/editor so that is acceptable.

I can assure that exposing the code for internal processes in AnimationNode is outside the current design of AnimationTree, and should not be done for the sake of system stability or future compatibility issues.

It may be flexible for you, but it exposes an unsafe variable to the user and imposes an esoteric code implementation on the user. Instead, it is important to implement the elements that are minimally necessary for the actual use case that makes it easier for the common users.

[TokageItLab]

If you really need to get a pointer to an AnimationTree in AnimationNode::_process(), I think it is possible to expose the tree indirectly by GDVIRTUAL by passing the p_tree as an argument to AnimationNode::_process(AnimationTree *p_tree) and replacing the state->tree currently used in _process() with it, instead of exposing state->tree directly. Just like _process(double p_delta).

Also, in that case, it would be necessary to remove state->tree from AnimationNode and refactor AnimationNode::set_parameter() and AnimationNode::get_parameter() to have p_tree as an argument.

This should probably work, but some testing needs to be done to see if it can handle multiple AnimationTrees, NestedAnimationStateMachine, and multi-threading.

[Daylily-Zeleen]

If you really need to get a pointer to an AnimationTree in AnimationNode::_process(), I think it is possible to expose the tree indirectly by GDVIRTUAL by passing the p_tree as an argument to AnimationNode::_process(AnimationTree *p_tree) and replacing the state->tree currently used in _process() with it, instead of exposing state->tree directly. Just like _process(double p_delta).

Also, in that case, it would be necessary to remove state->tree from AnimationNode and refactor AnimationNode::set_parameter() and AnimationNode::get_parameter() to have p_tree as an argument.

Although this clearify that "this AnimationNode being processed with this AnimationTree", it sounds dangurous, too.
User may store the p_tree and use it outside the “_process()”, or free the tree directly.

Given the previous discussion, I prefer to hide the "processing tree", only pass the "base_node" ( the new property referenced by AnimationTree but not paticipate in any internal logic of AnimationTree), any manipulation of this variable is the responsibility of the user.

[TokageItLab]

Although GDVIRTUAL unfortunately ignores const declarations, so users may delete p_tree, but at least it does not directly reference AnimationNode's internal variables. This means that refactoring is needed to ensure tree consistency within _process() first, since the current design may assign trees outside of _process().

I also can't see the point of you wanting to go to the hassle of having a new property called base_node. Once you know which tree to fetch correctly, you can just create a class extending AnimationTree in GDScript and implement whatever variables you want in it.

[Daylily-Zeleen]

I don't know how do you define "the border of danger".
In order to avoid breaking the internal process, you don't allow user to access internal variable of AnimationTree, but you allow to access the processing tree, which means that user may do some dangerous operations to break the entire tree logic.

If the passed p_tree has a const keywords, it will block the calling of set_parameter().

By passing a object which is responsible by user self, user can't access any internal variable and the processing tree, and realize the internal safety.

If this is violate animation system design principle, we don't need to argue about this proposal, just discard it.

[TokageItLab]

I am simply saying that those internal variables should not be accessible from outside of AnimationNode. You should learn the principles of encapsulation, and do pondering why it is undesirable to send the PR "Expose XXX" carelessly, compared to implementing a secure API.

The availability of AnimationNode.get_animation_tree() anywhere outside of AnimationNode is unsafe, since it is not known what updates the animation_tree.

So we first refactor the internal animation_tree so that guarantee it is only updated by AnimationNode::_process(). This may allow us to explain in the documentation that it is updated by AnimationNode::_process(), but AnimationNode.get_animation_tree() is still unsafe because users do not know where AnimationNode::_process() occurs clearly.

If animation_tree can be obtained only as an argument to AnimationNode::_process(), we can make the flow clearly recognizable. Even when animation_tree is brought outside of AnimationNode, it must be assigned to the animation_tree obtained in the AnimationNode::_process().

The core will never update the animation_tree in any other part of the AnimationNode::_process(), so it is safe for consistency and now we can finally say that it is the "user's responsibility" at this point.

[Daylily-Zeleen]

What I want to say is not matter how to encapsulate, how to implemt this proposal elegent, how to make it internal safety, is a maintainability question.

You should clearify the fact that, If we not change the current AnimationTree logic and change the ability of AnimaitionNode, not matter how to provide a way to access the parocssing AnimationTree, AnimaitionNode only hold the referenced of specify AnimationTree during AnimationNode:process().
It means that pass the tree by AnimationNode.get_animation_tree() and pass by AnimationNode:process() is makes no difference to users. For users, these two ways just have a description different in document. The tree which be provided to user has the same meaning, the same life cycle.
In other word, any danger can be caused by calling AnimationNode.get_animation_tree() by user, can be brought by passing tree by AnimationNode:process(), too.

If you think provide tree is dangurous, I put forward that provide a independence object which is referenced by the tree.
If you think that any danger caused by providing tree can be tolerated, and allow to provide tree directly, I prefer to pass the tree, too. It means that more flexible.

---

Before we solve follow problems, talking about how to implement internal safety is nonsense.

1. Can we allow user to access a object which relate to processing tree in AnimationNode?
2. If 1 is yes, which object can be provided and not break safety.

I have no right to decide on these, my conclusion is that if you allow provide the tree to users, of course, I agree your implementation.

[TokageItLab]

It is possible to serve the way to retrieve the tree to the user in the way I have described above. There is already a similar implementation called post_process_key_value().

However, since that function currently raises concerns about its ability to support multi-threading, perhaps passing an AnimationTree to _process() could raise the same concerns, and would need to be tested as mentioned before to know if it is actually possible.

It means that pass the tree by AnimationNode.get_animation_tree() and pass by AnimationNode:process() is makes no difference to users. For users, these two ways just have a description different in document. The tree which be provided to user has the same meaning, the same life cycle.

A user who is not aware of safety may not be able to distinguish it from exposing an internal variable, but "allowing an external object to access an AnimationNode internal variable directly outside the AnimationNode:_process()" is a problem, so an implementation such as get_internal_variable() for AnimationNode is not allowed, it is same for other object variables which should generally be private or local variables such as it is updated/discarded several times in one frame.