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

Advance expression for AnimationTree doesn't seem to work #62530

Closed
joaopedrosgs opened this issue Jun 29, 2022 · 17 comments · Fixed by #63238, #66301 or #67020
Closed

Advance expression for AnimationTree doesn't seem to work #62530

joaopedrosgs opened this issue Jun 29, 2022 · 17 comments · Fixed by #63238, #66301 or #67020

Comments

@joaopedrosgs
Copy link
Contributor

Godot version

v4.0.alpha.calinou [caa9ec8]

System information

Windows 10, Vulkan

Issue description

I'm using a simple state machine within the AnimationTree
image

Along with Advance Expressions like (this one is for idle_1 to walking):
image

However, it simply doesn't trigger the transition

Steps to reproduce

  1. Create a CharacterBody3D
  2. Drop a model with animations (a gltf with a character for example) inside the CharacterBody3D
  3. Add an AnimationTree node, assign it to the AnimationPlayer from your model, enable it and assign the Advance expression base node to be the CharacterBody3D
  4. Create a new Tree Root in your AnimationTree. I used a simple State Machine.
  5. Setup your tree using Advance Expressions. I just created an idle and a walking state, with a transition idle->walking using the expression "velocity.length()>1.0"
  6. Check the results in-game

Minimal reproduction project

TestExpressions.zip

@Calinou
Copy link
Member

Calinou commented Jun 29, 2022

For reference, advance expressions were added in #61196.

cc @fire

@fire
Copy link
Member

fire commented Jun 29, 2022

I'll take a look. Is length a const method?

@joaopedrosgs
Copy link
Contributor Author

I'll take a look. Is length a const method?

It's the native method from Vector3. I think that it is a const method, but I also tried methods that are not const and it didn't work.
I just posted with length() because it's the same example used by Reduz in the original PR.

@permelin
Copy link
Contributor

permelin commented Jul 1, 2022

You can set the expression to simply true and it still won't advance if Expression Base Node has been assigned on the transition. But if I clear the Expression Base Node, it does trigger, as long as Advance Expression Base Node is set on the AnimationTree itself.

@permelin
Copy link
Contributor

permelin commented Jul 1, 2022

I also just noticed that which node I assign to Advance Expression Base Node doesn't matter. The expression will always be executed in the context of the AnimationTree node.

@joaopedrosgs
Copy link
Contributor Author

any news about this?

@joaopedrosgs
Copy link
Contributor Author

I also just noticed that which node I assign to Advance Expression Base Node doesn't matter. The expression will always be executed in the context of the AnimationTree node.

Hey, I've created a new pull request #63238 that solves this issue

@permelin
Copy link
Contributor

permelin commented Jul 20, 2022

Hey, I've created a new pull request #63238 that solves this issue

Nice. I have a theory about the second issue: why not even true works when Expression Base Node is set on the transition itself.

When you set the node property in the inspector on the transition, the editor makes a path that is relative to the scene root.
When you set the corresponding property on the AnimationTree you get a path that is relative to the tree node.

The path is then used relative to the AnimationTree node with tree_base->get_node_or_null(advance_expression_base_node_path) which is correct in one case but wrong in the other. The expression is then silently ignored if the path can't be resolved.

I assume this happens because AnimationNodeStateMachineTransition is not a node but a resource, and therefore not part of the tree, so the path can't be made relative to it.

@joaopedrosgs
Copy link
Contributor Author

Hey, I've created a new pull request #63238 that solves this issue

Nice. I have a theory about the second issue: why not even true works when Expression Base Node is set on the transition itself.

When you set the node property in the inspector on the transition, the editor makes a path that is relative to the scene root. When you set the corresponding property on the AnimationTree you get a path that is relative to the tree node.

The path is then used relative to the AnimationTree node with tree_base->get_node_or_null(advance_expression_base_node_path) which is correct in one case but wrong in the other. The expression is then silently ignored if the path can't be resolved.

I assume this happens because AnimationNodeStateMachineTransition is not a node but a resource, and therefore not part of the tree, so the path can't be made relative to it.

Not a theory, I checked this and it is what happens. I'm not sure how to solve that

@permelin
Copy link
Contributor

I'm not sure how to solve that

Maybe the solution is to take a step back. Should this base node property exist on the individual transitions in the first place? It creates dependencies deep inside a nested resource on the structure of the node tree in which the resource will be used. Doesn't seem like something that should be encouraged.

@alfredbaudisch
Copy link
Contributor

It seems that the issue is still present in alpha17, no matter the expression you put, even if you write true directly, the transition does not happen.

I don't think attaching a reproduction project is needed because it's simply the same issue as OP - or simply write true in the expression, there's no transition.

@permelin
Copy link
Contributor

@alfredbaudisch Are you setting the Expression Base Node field directly on the transition? There were two parts to this issue. One was fixed and one was not.

IIRC, if you set the expression base node only on the animation tree and not on the transition then it should work now.

That field should be removed from the transition node unless someone can figure out a solution.

@joaopedrosgs
Copy link
Contributor Author

Yep, precisely what @permelin said. If you leave the Base Node blank, it will work (and use the animation tree node as the source for the variables).

@alfredbaudisch
Copy link
Contributor

alfredbaudisch commented Sep 15, 2022

Are you setting the Expression Base Node field directly on the transition?

That was it, thanks. Confirmed the issue as you said: setting in the animation tree directly made it work, on the other hand the transition base node is ignored / has no effect.

Repository owner moved this from To Assess to Done in 4.x Priority Issues Sep 26, 2022
@GuilhermeGSousa
Copy link
Contributor

GuilhermeGSousa commented Sep 28, 2022

@akien-mga I was testing and my previous fix does not work if there are autoloaded nodes in the tree, this issue should probably be reopened.
I should have a fix for that soon.

@akien-mga akien-mga reopened this Sep 28, 2022
@fire
Copy link
Member

fire commented Sep 28, 2022

Thanks for testing!

@GuilhermeGSousa
Copy link
Contributor

I'm sorry for not testing it in depth to begin with!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment