godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.11k stars 21.18k forks source link

AnimationTree fails to call method at last frame of animation depending on animation length #64644

Closed eh-jogos closed 1 year ago

eh-jogos commented 2 years ago

Godot version

4.0.alpha14

System information

Manjaro Linux, Nvidia proprietary drivers, Vulkan Clustered

Issue description

I have the same animation, with a method track calling a simple method that only prints a message at it's last frame.

When this animation has a length of 4 frames at 24 FPS the method is called. When the length is 5 frames, the method is called When the length is 6 frames, the method is almost never called. I was only able to see the print if this is the first animation played by the animation tree, after that it is not called anymore. When the length is 0.25 seconds, with any value for step (at least I tried both 0.05 and 0.01) the method is never called.

For any of the animations above, if the AnimationPlayer is used directly, everything works fine.

Steps to reproduce

Use the MRP below or:

Create any animation with 0.25s of length and call a method at the last frame of the animation. Alternatively, create an animation with 6 frames at 24 FPS (which according to the AnimationPlayer editor, also results in a 0.25s animation) and call a method at the last frame of the animation.

Then call any of the above animations through an animation tree.

Minimal reproduction project

broken_method_call_on_animation_tree.zip

The project is already setup with the animations and the animation tree and some code (there are some extra animations but ignore them, they were from another MRP)

Run the project:

eh-jogos commented 2 years ago

@TokageItLab here is the issue and MRP you asked on https://github.com/godotengine/godot/issues/61766

TokageItLab commented 1 year ago

@eh-jogos I would like to clarify the issue. Does this only occur when using StateMachine? Does this problem still occur when using AnimationTree without using StateMachine?

eh-jogos commented 1 year ago

@TokageItLab I've only tested it with StateMachine and with BlendSpace1D inside a StateMachine to be honest. After I saw your comment I tried to add a new AnimationTree to the MRP without using StateMachine, like this: image

And it seems to work if they only play once, but if I turn active off and on again so that the animation plays again, here is what happens:

I have to stop running the game to change the animation in the blendtree, as if I change the animation in the blendtree with the game running it doesn't have any effect. I don't know if this is a proper way to test this as I haven't used AnimationTree without StateMachine before.

TokageItLab commented 1 year ago

Use multiple NodeOneshot or use NodeTransition. At least StateMachine has a large number of bugs and its playing process is different from BlendTree, so to investigate where the problem is occurring, we must investigate them separately.

eh-jogos commented 1 year ago

I just learned how to use NodeTransition and updated the MRP on this repository: https://github.com/eh-jogos/godot4_method_track_bugs

If I run the anim_tree_blend_tree.tscn scene and change the current animation using the remote inspector, I get the same result as always, the 6 frames and the 0.25s animation almost never prints their method, the others work fine.

If I try to do that through code, using the arrow keys to change current state on _anim_tree["parameters/Transition/current"] (the script in the same scene) it doesn't work:

This seems like yet another bug to me, unless I'm doing something wrong in the code.

But all of the ways of playing these animations through an AnimationTree that I've tried so far have resulted in the same output, animations with 0.25 seconds duration with a method keyframe at the last frame do not call their method reliably, if at all. The only way these methods have been called reliably in all these animation durations has been when using the AnimationPlayer directly.

Maybe there are other durations that are also "cursed"? I tried 0.5s but it worked fine.

eh-jogos commented 1 year ago

@TokageItLab Just updated the MRP repository with a new scene using multiple NodeOneShot instead of NodeTransition and the result is the same as the others. In some rare ocasions the 6 frame animation calls it's method, but it seems to be the exception while most of the time its method never gets called.

If there are any other test cases that need to be added do let me know.

TokageItLab commented 1 year ago

Thank you! I'll look into it later.

TokageItLab commented 1 year ago

@eh-jogos I sent a PR, please check #68992. BTW, NodeTransition props should be int(enum) value, not String (maybe you can send usability issue for about it).

eh-jogos commented 1 year ago

@TokageItLab thanks a lot! The explanation in the PR helped understand what was happening! Hope it gets merged to one of the beta updates soon!

Also I've updated the MRP repo so that the NodeTransition example now uses int values and it worked! I'll try to remember to open an usability issue tonight after work!

TokageItLab commented 1 year ago

68992 addresses this issue, but it is not perfect as the #61853 issue remains.

I will continue to work on refactoring for key processing, probably the right way to go would be to rework the key getting algorithm to do the look-ahead and then implement the special case on frame 0. The current implementation is like implementing the special case on the last frame.

TokageItLab commented 1 year ago

@eh-jogos Finally, I sent #69336. I think it's much more stable, but I would appreciate if you test it.

eh-jogos commented 1 year ago

@TokageItLab sure! Just to confirm as I've never built godot from source outside of stable commits, but to test it I need to clone this repo https://github.com/TokageItLab/godot/tree/get-anim-keys-more-exactly and build godot from source, right?

TokageItLab commented 1 year ago

@eh-jogos Yes! A better workflow is to Fork the original godot repository and do

git remote add Tokage https://github.com/TokageItLab/godot.git

to check out my remote repository. See also https://docs.godotengine.org/en/latest/development/compiling/index.html.

eh-jogos commented 1 year ago

@TokageItLab So sorry for taking so long, had a busy week and was only able to test this today. I wasn't sure if this is in beta 7 or not so I compiled Godot from your fork. I also tested the MRP with beta 7 afterwards (thinking about it now, probably should have started with that haha).

In both of them the MRP worked perfeclty, no more bugs! The method was called at the last frame of all animations, in all the AnimationTrees, the one using AnimationNodeStateMachine, the one using AnimationNodeOneshot, and the one using AnimationNodeTransition!

Just one question though, both with "StateMachine" and "Transition", if I try to use travel or change the "current" property in case of Transition, to the same state I'm already in, it doesn't play that state again. Is this expected?

For example, in the actual project I'm working on character have a "hurt" state and it's possible for them to be hurt again while they are in that state, if I want to restart the hurt state on every hit, I have to check if I'm already on the hurt state and if so use _playback.start(target_path) instead of _playback.travel(target_path). Just wanted to check this is the expected behavior.

TokageItLab commented 1 year ago

@eh-jogos Thank you for testing!

Just one question though, both with "StateMachine" and "Transition", if I try to use travel or change the "current" property in case of Transition, to the same state I'm already in, it doesn't play that state again. Is this expected?

Do you mean that you want behavior like restart? Yeah, certainly that is not expected behavior at the moment, but I think it would be useful since there is actual use cases for it. However, since it is not a bug, I would recommend writing it in proposal, not issue.