Open SaracenOne opened 3 years ago
@sharedrory I've noticed you leave a lot of :-1: reactions on proposals. Could you justify why in a comment each time you do so? Otherwise, it's impossible to understand why the proposal is getting opposition.
@Calinou Sorry. From now on I will leave comments
My view on this proposal is it solves two big issues with the current AnimationTree:
So I saw some discussion that blend parameters were not fully defined... Here are my ideas on how to deal with blends and conflicts. I'd also like to reap the benefits of this proposal without having to type in parameters every time you create an AnimationNode:
For blend parameters, I would suggest that users be allowed to type a custom name for its parameter, rather than "add_amount" or similar names. If this name matches one of the globally defined parameters, then it would use that parameter's value.
I would propose that rather than in the AnimationNode, any AnimationNode (BlendSpace or AnimationTree) can declare parameters as defined in this proposal, and those will show up as top-level parameters in the inspector.
If a parameter is defined in multiple places with the same type but different defaults, the highest level AnimationTree that defines it can take precedence; or the first one encountered if they are multiply defined on equal levels. I do not think this should be an error if they have matching type: just pick a sane consistent resolution order.
If any condition, blend or parameter declaration has a mismatched type with any declaration, it can cause an error (since the user probably wants to know that there is a mistake), or create another parameter with a full path to disambiguate it.
Finally, if a parameter is not declared anywhere, it can be automatically created with default of 0 or false.
Just thought I would chime in on this - Both Unreal, Unity, and O3DE have a Blackboard style animation system and they are pretty good. I would strongly recommend the devs who are going to do the coding on this look into O3DE's Emotion FX Gem which is opensource under the Apache 2.0 license. This is a battle tested tool that has been used in production of wide variety of games.
Bit of a question also - Would it be possible to add support for custom parameters or would that be out of scope for this?
I love this proposal as it empowers animators in a project without them having to suffer through learning to code.
On that note, I would like to add that the API for messing with parametres is quite painful even for us programmers. And by that I mean that this:
$animation_tree["parameters/conditions/idle"] = ...
To change a condition is... not ideal. Not to mention poorly documented. I think having a specific method to change parameters would be best. Accessing an arbitrary string field is almost always a redflag to me. Basing off of the proposal's visuals and a certain commercial engine's approach, I'm thinking of something like this:
# should these methods be directly implemented to the AnimationTree node?
# or should they be implemented to the AnimationNodeStateMachine class?
# I'm purely guessing here in this sample code
state_machine.set_integer( "float_param_name", number_here )
state_machine.set_bool( "bool_param_name", bool_here )
state_machine.set_trigger( "trigger_param_name" )
I'm adding this suggestion to this issue instead of making another because I feel like this is a relevant aspect of the proposed feature's design and it would probably be part of the effort to implement it.
I'd really like to see this implemented, I was about to come here and suggest the same thing.
I'm currently working on a project where our animator doesn't know how to code too well, and making them modify the player controller code every time they want to tweak the AnimationTree seems unnecessarily complicated. Not to mention, having one variable go to multiple places would be very useful.
I generally support this direction and indeed the current implementation needlessly exposes animation tree structure through parameter naming which would be better to avoid. I couple of notes though on the way to integrate parameters into animation tree follows.
This proposal talks about adding new nodes with parameters and exposing those parameters in e.g. AnimationBlendTree nodes so that they can be wired the same way as shader nodes do that. While this is a sound, commonly used and familiar approach, it has drawbacks. The same approach does not work for state machines. In state machines those parameters need to control state transitions, so you would need to expose parameter ports on node links rather than nodes which just makes it even more messier. Parameters in transitions now are controlled either with "advance_condition" parameter or "advance_expression". IMO, it would be more uniform across all implementations to allow entering expressions anywhere a parameter value is expected. It's kind of the same concept as "drivers" in Blender. E.g. selecting an input field and either typing "=" or pressing some shortcut will convert plain value into an expression. The expression should be able to reference any values defined in the top-level animation player blackboard (and maybe UI to add more values right from expression editor).
I like the idea behind "advance_expression" implementation, but the fact that it requires advance_expression_base_node which would source parameters either requires creating just another script to hold all those parameters OR exposes implementation details of whatever script is controlling the animation, introducing tight coupling and implicit dependencies. Getting rid of advanced_expression_base_node concept and instead having it contained right in the AnimationTree would be a more elegant approach.
On the implementation side, I think that there will need to be new data types like ExpressionFloat, ExpressionInt, ExpressionVector2, etc (or Expression<float>, Expression<Vector3>, etc... or AnimationExpression<type> if you want to keep it scoped to animations only), which inspectors would allow entering plain values unless an "expression mode" is activated, in which case it converts to a code editor. All animation parameters will need to be defined in terms of those types.
Is there any work being done on this? I see godotengine/godot#61680 still open, but I noticed @TokageItLab mentioned in another PR that its implementation has lots of issues. This is one of the biggest issues I have with working with AnimationTree. The worst is having to set multiple variables in multiple locations to the same value because their paths differ.
Describe the project you are working on
3D VR sandbox with a heavy emphasis on user content driven by animation.
Describe the problem or limitation you are having in your project
The existing parameter system, while at first glance seeming intuitive, begins to show itself to be overly cumbersome once a certain level of complexity in the AnimationTree is reached. As the system currently exists, as you add new nodes and transitions to your tree, the AnimationTree node automatically recursively walks them and then automatically updates a list of parameters which you can then update via the inspector. With nodes like the AnimationNodeTimeScale, they always create implicit parameters, regardless of whether their values are intended to be constant or variable. With transitions, a corresponding parameter is created if they are given a named advance condition, and the condition is always evaluated as to whether this parameter is set as true; there is no option for choices concerning conditional evaluation.
The problem with this approach is that if your tree gets particularly complex, you can end up with multiple parameters becoming responsible for what could otherwise be the same input. Take this as a scenario: imagine having a blend tree for a game character with multiple state machines running in parallel which in turn have their own blend trees. These blend trees may be responsible for animating specific parts of the character but they all care about knowing the character's movement vector. In order to make this setup work, a script will have to be responsible for knowing paths to nodes requiring this input and setting the same value on several different parameters, keeping track of the names and paths to each parameter, and updating the script any time a new node requires this information.
It can get even worse when dealing with complex state machines with a lot of transitions with advance conditions, since none of these can share the same parameters, it can result in a lot of useless boilerplate code in order to keep everything in sync. It many cases, it almost makes sense to forgo implicit conditional travel of a state machine entirely and instead make use of the 'travel' function instead, but this still requires always keeping a mental map of you animation tree and removes the ability to treat the animation tree as a self-contained system controlled via external input.
Describe the feature / enhancement and how it helps to overcome the problem or limitation
My proposal is to change the way more parameters are derived (with AnimationTree node walking its own tree to extract a list of valid parameters) and instead allow said parameters to be defined explicitly by the user via the AnimationTree's inspector or other similar menu. By doing it this way, it provides an easy way to reuse parameters across the tree and makes driving the tree less dependent on scripting. This also doesn't require the internal design of the animation tree to be a consideration when driving it, making it more suitable for iterative design and even live-editing.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
From what I have assessed this should not require too much change on a technical level, it would mostly involve removing the implicit parameter creation function and adding new functions for adding and removing parameters in the AnimationNode. Changes would also have to be applied to the nodes themselves to allow parameters to be referenced via name or use a default hardcoded value.
Another, perhaps more robust design to consider for nodes intended for blend trees is one where most of the parameters on the existing nodes are removed and instead, AnimationNodes are permitted to accept other data types aside from just blending animations (floats, vectors, integers, similar to the VisualShader editor), they are instead provided as generic inputs on the nodes with something like a separate dedicated 'AnimationNodeParameter' node which can then feed its output into the other nodes' input. This would open the possibility of some basic mathematical mutation to be done on existing parameters, allowing them to be further reused. I feel this might have broader implications, but want to at least open it for consideration.
As for state machine transitions nodes specifically, these should likely be given the ability to reference multiple parameters and also perhaps customise their evaluation function (valid if a bool is true/false; greater-than, less-than for ints/enums, ect.).
I feel for this proposal, the majority of effort would likely go into designing the relevant UIs required for creating and referencing parameters. There isn't really anything which fits this exact use case in Godot yet that I can find, however, the VisualScripting system does provide a similar interface for assigning member variables, and I feel something similar might make sense here too.
If this enhancement will not be used often, can it be worked around with a few lines of script?
This proposal alters the workflow and many of the inner workings of the AnimationTree system, so I do not believe script could easily address this problem.
Is there a reason why this should be core and not an add-on in the asset library?
This proposal alters the workflow and many of the inner workings of the AnimationTree system, so I do not believe script could easily address this problem.