Make accessing AnimationNodeOneShot properties more intuitive

AidasLit commented 7 months ago

Describe the project you are working on

A third person player controller.

Describe the problem or limitation you are having in your project

When working with OneShot nodes in AnimationTree accessing it's properties through code is inconsistant. For request state, it could be accessed in code like this self["parameters/attack/request"] Meanwhile if I wanted to access it's fadeout_time property, I'd have to do it like this tree_root.get_node("attack").fadeout_time

This is unintuitive for several reasons:

When working in the editor, clicking on the node opens it's properties in the inspector, therefore there is a very direct link between the node and the properties. Request state is part of the node, so are the rest of the properties, so accessing them should be the same. Yes this is obvious, because the inspector "inspects" the properties of the selected node, but as I will later show, it is perhaps not as obvious as it might seem.
Both request state and fadeout_time are properties, therefore they should be treated the same in code. As seen in these images, both can be accessed in the inspector, yet through different paths. So what makes them so different?
The official documentation page for AnimationNodeOneShot not entirely clear. https://docs.godotengine.org/en/stable/classes/class_animationnodeoneshot.html#class-animationnodeoneshot Nowhere in the page is it directly mentioned "what" the request state is. We know it's a property, yet it is not listed alongside the other properties. There lies my problem: it is very unclear what the request state is. It is simultaniously a property, and not.

In essence, if I were to control my OneShot node through code, I have to use two very different approaches to accessing it's properties. I am not the first one to notice this problem, #1293 describes exactly the kind of solution I'm proposing here. However, given that it's a 3 year old issue that has received no attention, and it doesn't mention the confusing documentation, I decided to write this issue.

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

It makes the AnimationTree easier to use.

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

Allow the same syntax as self["parameters/attack/request"] be used to access ALL properties of a OneShot node. As described in #1293, accssing a property like fadeout_time would preferably be done like so: self["parameters/attack/fadeout_time"]

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

This is a conveniance and user-friendliness problem. It can be worked around, and might even be simply the result of my own lack of knowledge about the engine. However, it is a pain to work with and I would rather other developers not go though the same confusion as I did.

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

Because it's an issue with user experience, not functionality.

AlexOtsuka commented 7 months ago

Hey there,

The thing about parameters such as active and request is that they actually belong to AnimationTree, not to AnimationNodeOneShot. Internally, they're stored in a HashMap belonging to the AnimationTree class in scene/animation/animation_tree.cpp. The only description I could find for those parameters is in the AnimationNode doc page:

Parameters are custom local memory used for your animation nodes, given a resource can be reused in multiple trees.

In contrast, fadeout_time is a property of AnimationNodeOneShot, which given that it is a Resource, will effect changes in every single AnimationTree it is a part of.

I think the main issue lies in a lack of clarity as to how those are two different kinds of values belonging to different entities. The documentation for AnimationTree doesn't mention anything about those parameters. The AnimationTree tutorial and the AnimationNodeOneShot doc page do showcase this approach to getting and setting the parameters, but I think we could do a better job explaining where those values actually live.

Looking through the source code, I can see AnimationNode (which is inherited by AnimationNodeOneShot) also has set_parameter() and get_parameter(). From reading those, I assume those are all equivalent, but please correct me if I'm wrong:

animation_tree.set("parameters/animation_node_oneshot/request", AnimationNodeOneShot.ONE_SHOT_REQUEST_FIRE)
animation_tree["parameters/animation_node_oneshot/request"] = AnimationNodeOneShot.ONE_SHOT_REQUEST_FIRE)
animation_node_oneshot.set_parameter("request", AnimationNodeOneShot.ONE_SHOT_REQUEST_FIRE)

I wonder if accessing those parameters could be made more intuitive somehow? Another thing is that attempting to set() or get() using an invalid path or trying to modify a read-only value doesn't ever seem to return a warning or an error, although those surely are easy avenues for potential mistakes and confusion.

I'd love to hear everyone's thoughts, as my knowledge is definitely lacking here and I'd like to understand those classes in more detail. I'd love to contribute in order to make those features clearer and more user-friendly. Please feel free to correct me on anything I might've misunderstood.

Calinou commented 2 months ago

Another thing is that attempting to set() or get() using an invalid path or trying to modify a read-only value doesn't ever seem to return a warning or an error, although those surely are easy avenues for potential mistakes and confusion.

I recall seeing a proposal or issue about this, but I can't find it right now.

Note that the engine itself regularly uses those methods with the expectation that no error message is printed, so set_or_null() and get_or_null() would have to be added and the engine would need to have most of its own uses replaced with that.

godotengine / godot-proposals