Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The tutorial of Using AnimationTree is outdated #6640

Closed
TokageItLab opened this issue Jan 20, 2023 · 7 comments · Fixed by #6764
Closed

The tutorial of Using AnimationTree is outdated #6640

TokageItLab opened this issue Jan 20, 2023 · 7 comments · Fixed by #6764
Assignees
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug needs work Needs additional work by the original author, someone else or in another repo.

Comments

@TokageItLab
Copy link
Member

TokageItLab commented Jan 20, 2023

Your Godot version:
4.0

Issue description:
In many AnimationNode sample codes, the property names are out of date or lacking explanation.

The StateMachine now got a major addition to the functionality so it needs to be described.

Also, most of the images are outdated.

URL to the documentation page:
https://docs.godotengine.org/en/latest/tutorials/animation/animation_tree.html

@TokageItLab TokageItLab added needs work Needs additional work by the original author, someone else or in another repo. area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Jan 20, 2023
@TokageItLab TokageItLab self-assigned this Jan 20, 2023
@hsandt
Copy link
Contributor

hsandt commented Feb 18, 2023

Indeed. I also have a hard time understanding how to fill the transition expression (not condition). Can I just use custom parameter values like "is_on_floor" after setting them via code for custom expressions like "is_on_floor and abs(speed.x) > 3"? Maybe I can even replace 3 with a constant taken from somewhere else, but how to access external values?

Explaining this would show how truly powerful the conditional expressions are. But for now, only the tooltip of the simple one-parameter condition is well explained, so I'm reduced to setting simple bool parameters and using one per transition.

@Calinou Calinou added the bug label Feb 18, 2023
@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 18, 2023

I have fixed the document about most of the AnimationBlendTree in #6764, but have not been able to update much of the StateMachine because we plan to change the StateMachine again in 4.1; Although the changes are not that big, there should be some changes in grouping (nested StateMachine).

@hsandt
Copy link
Contributor

hsandt commented Feb 19, 2023

OK, because I had a hard time understanding what to put in the Condition field, not mentioning the Expression field.

Combining various info from video tutorials and godotengine/godot#54327, I found out the following:

  • Condition can only take a single bool parameter like "is_moving". You cannot negate it with ! or not. You can define a redundant opposed parameter not_is_moving, although that's a waste.
  • Expressions work fine with any non-contextual expression like 1 + 2 == 3 (which is of course pointless)
  • Expressions are supposed to be evaluated from the context of some NodePath, which by default is the AnimationTree itself. Therefore, we should write them as if writing code inside a script deriving from AnimationTree and attached to it. One screenshot suggests expressions such as get("parameters/conditions/ISAttacking")...
  • ... but I tried it and it didn't work. Even expressions that should work on any node like get_path().begins_with("/") just fail.

So even without the doc, we definitely need a running example/demo of an Animation Tree with parameters so we can understand how to use it. Even one with only one level of state machine, or a simple blending, to start with.

After that, we can always improve the doc for 4.1 with the new StateMachine. If the gist of it remains the same, I think it's worth drafting it already. This way, we can start using them and send feedback toward improvement in 4.1.

Of course, in the changes in question are about completely overhauling the Expression parsing, then I understand, you could quickly document that they are unstable in 4.0 and we can avoid using them until they become stable.

@hsandt
Copy link
Contributor

hsandt commented Feb 20, 2023

After debugging AnimationNodeStateMachinePlayback::_check_advance_condition heavily, I found the cause of the issue and some workaround:

  • Object::set() -> AnimationTree::_set() only works if the key already exists in the property map. So set("parameters/conditions/arbitrary_name") will not work in general, because the key is unknown. Therefore, when we get() it later, we find NIL and the expression is never true
  • AnimationNodeStateMachine::get_parameter_list is responsible for filling the property map with advance condition names. So we should fill at least one condition with the parameter name we want.

Workaround:

To register the custom parameter name while avoiding messing up with a negative condition, we should place it on a positive expression so the condition doesn't affect the result of the advance check. Ex: Idle -> Run transition has condition "is_moving" and the expression is "get(parameters/[optional_sub_state/]conditions/is_moving)". Run -> Idle transition has NO condition and the expression is "not is_moving"

If the expression is more complex, we may want to move the condition with parameter name away, onto some dummy transition. But really, that's a hack to register the name... Having a list of parameters somewhere on the side of the Animation Tree window may be cleaner (or in the inspected properties of the selected node, since technically parameters are scoped by node; but they could be global as in Unity Animator Controller).

I also tried to call set_parameter on the root node, or any other node, because unlike set it will actually set a value even if the key doesn't exist yet. But for some reason I get a !state error, while I'm doing the same thing as set() does behind the hood...

Anyway, my conclusion at the moment is that it's better to use expression to use actual members of arbitrary objects from the animation tree's PoV. For instance, get_parent().velocity.x != 0 actually works fine if the Animation Tree is right under your CharacterBody2D. In contrast, accessing parameters in expressions is not really convenient. They seem to have been made for Conditions, really.

For the rest, I'll just keep logic on code side and call travel() when I need to, using Enabled mode instead of Auto mode.

@TandersT
Copy link

TandersT commented Mar 1, 2023

I'll just add some related information that is currently wrong. If this should be a separate issue, please to tell.

On the page for the AnimationNodeStateMachine (https://docs.godotengine.org/en/latest/classes/class_animationnodestatemachine.html)

The code bit at the start

var state_machine = $AnimationTree.get("parameters/playback")

seems to be wrong for GDScript and C#. It should be

var state_machine = $AnimationTree.get("parameters/@NAME_OF_NODE@/playback")

Or perhaps

var state_machine = $AnimationTree.get("tree_root/playback")

This documentation is also present on the stable branch, which makes me doubt if I'm using the animation tree wrong.

@hsandt
Copy link
Contributor

hsandt commented Mar 9, 2023

In https://docs.godotengine.org/en/latest/tutorials/animation/animation_tree.html#statemachine, I still see that "Advance Condition" and Priority/travel paragraphs say "(more on this later)". Does "later" mean we're going to upgrade the page further later, or does it mean "more below" here? Because I don't see more information on conditions more below (although I do see a section on travel)

If you don't want to reopen this issue, it's fine, but then we should add AnimationTree to the big list of tasks in #5121 so we don't forget about it (and possibly change the update status of the page to WIP).

@TokageItLab
Copy link
Member Author

StateMachine could be a separate page from AnimationTree. Nevertheless, since StateMachine has many problems and is still incomplete, we could not update the documentation in 4.0. It should be updated when it is improved in 4.1 or 4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation bug needs work Needs additional work by the original author, someone else or in another repo.
Projects
None yet
4 participants