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

Sync animation in BlendSpace1D/2D #23414

Closed
Kinwailo opened this issue Oct 31, 2018 · 36 comments · Fixed by #62623
Closed

Sync animation in BlendSpace1D/2D #23414

Kinwailo opened this issue Oct 31, 2018 · 36 comments · Fixed by #62623

Comments

@Kinwailo
Copy link

Godot version:
3.1 alpha1 (https://downloads.tuxfamily.org/godotengine/3.1/alpha1/)

OS/device including version:
Win7

Issue description:
Blending animation in BlendSpace1D/2D seem not synced and weird. I don't know how Godot blend the animation, I think if using two animation with similar action (footstep) with different speed, the blend position taken from two source animation should be the percent of time, so the different of length in two source animation does not affect the sync of final animation.

Sorry for my english.

Steps to reproduce:
Prepare two animation (run and run faster)
Create AnimationTree and add BlendSpace1D
Add run in 0 and run faster in 1
Click the cursor in 0.5

2018-10-31 11-17-59

@Kinwailo
Copy link
Author

Kinwailo commented Nov 2, 2018

I try to do a simulation in blender, I prepared a walk and a run animation:
walk
run

I add two animation in nla editor and blend in 50%:
1
The result is similar to godot.
weird

If I scale two animation in same length, the result is prefect.
2
correct

So, if the blendspace do correctly, I can have different walk speed with different blend factor.

@ghost
Copy link

ghost commented Mar 9, 2019

I'm struggling as well to synch two animation (walk-cycle and run-cycle)

@David-Ochoa
Copy link

I've seen in other engines they scale the longest animation to match the shortest, but sometimes it's not enough and other adjustments need to be applied. In my opinion a good solution would be to add a scale property to the AnimationNodeAnimation node.
Right now I'm scaling the animation before importing and it works fine, but having the option to change it in the editor would be better.

@David-Ochoa
Copy link

Well, after writing the last comment I started to find a way to scale the animation, and the solution is using a BlendTree inside the BlendSpace1D/2D node, I recorded a video where the process is explained:
Fix blending

@clayjohn
Copy link
Member

@Kinwailo did David-Ochoa's solution fix your problem?

@AdamRatai
Copy link

AdamRatai commented Jan 8, 2020

I just bumped on this problem in latest 3.2 beta 5. It is something really wrong, I think, because I have few animations not only with exact amount of frames (100 per each animation) but I even synced them so legs are more less in the same position in same frame (I mean left leg is for example in front in 15 frame in every animation). But when I use Blendspace 1D - they make exactly the same weird inbetweens as I see at provided gifs.
I will check David-Ochoa solution, but even if it will work for me, I found it a workaround. It should be fixed, or maybe we are doing something wrong and it needs better explanation in manual. In Blender NLA they blend perfectly.

@David-Ochoa
Copy link

I have the issue again, but this time is for three animations. I have Idle, Walk and Run. It starts correctly but if the point goes from Idle to walk/run sometimes the animation loops awkwardly. It happens using Godot's scale node or scaling before importing: Test

@SirPigeonz
Copy link
Contributor

Yeah, I had this problem too. Animations have to be same length for the blend to work which sucks a lot and is unintuitive why it is broken in the first place :(

@Shadowblitz16
Copy link

I think I just had this problem too.
I was blending a walking and running animation where the running animation was shorter and faster

@d3is
Copy link

d3is commented Nov 13, 2020

Been dealing with this bug for a while now and thought it was just something wrong with my animations. So happy to find this as a confirmed bug now.

I ended up editing the animations in Godot and scaled them to be the same duration for now, but this is far from ideal.

@TokageItLab
Copy link
Member

This problem is forcing me to use TransitionNode and SeekNode without using BlendSpace2DNode. Also, there is a lack of implementation for most node; we will have to improve the feature of the animation nodes a lot before we get to Godot 4.0.

@HungryProton
Copy link
Contributor

So, just in case #34134 and #34179 never get merged, did anyone found a workaround for this? Using a timescale node as suggested by David-Ochoa still desyncs the animations after a while.

@TokageItLab
Copy link
Member

TokageItLab commented Jan 10, 2021

@HungryProton This is a complete hack, but you can always force synchronization by directly setting the value to NodeSeek. However, if you use root motion, you will need a PR #41728. While, since NodeSeek doesn't support looping root motion, so you need to let the engine do the looping the animation when the frame is over the animation length with stopping your seeking temporarily.

@HungryProton
Copy link
Contributor

@TokageItLab Thanks for the reply!
That was more convoluted than I thought, but I finally got something working. Using two NodeSeek and a bit of GDScript seems to do the trick for now.

image
(In this screenshot, the Blend2 blend amount (move_blend variable) is calculated based on my character's speed).

@timkrief
Copy link

timkrief commented Apr 1, 2021

A "fix" for this is having all the animations at the same length and then putting them all in BlendTrees to add a TimeScale node. That way you can blend them together perfectly in a BlendSpace and change the overall speed by tweaking all the time scale parameters all at once.

@AdamRatai
Copy link

AdamRatai commented Apr 1, 2021

Maybe something changed, but thats exactly what wasnot working in my case.

@timkrief
Copy link

timkrief commented Apr 1, 2021

Maybe something changed, but thats exactly what was NOT WORKING in my case.

You're right, it seemed to work at first but for some reason it stills desync ...

@TokageItLab
Copy link
Member

TokageItLab commented Apr 1, 2021

The reason for the loss of synchronization is that current NodeBlendSpace2D does not have sync option, so the animations that are not used for blending are temporarily stopped among the several animations that exist on NodeBlendSpace2D.

Even if you use a Node that is not a BlendSpace, you must always set Sync to True in NodeBlend and NodeAdd for animations that you want to synchronize, but there are some Nodes that do not have Sync, such as NodeTransition. The current solution is to implement your own timer in GDScript and run all animations with that timer and NodeSeek.

Also, NodeSeek is not affected by TimeScale, so you need to pre-calculate the amount of TimeScale (means normalize animation length) for each animation in GDScript before applying it to NodeSeek.

@AdamRatai
Copy link

Thanks! In next attempt I will try it. But i hope it will be fixed some day ;).

@krm01
Copy link

krm01 commented Apr 12, 2021

I was banging my head against this for a while before finding the thread here, Godot version 3.2.3. Tried all of the suggestions and nothing worked.

Fed up, I decided to just accept it for now, and started up my game. To my horror... it looked fine. At least in my case, this seems to be a bug only with the editor viewport, and isn't manifesting in-game. Maybe my animations are simpler, I don't know. But I can live with the viewport bug. I won't get these hours back but maybe someone else in my situation will 🙃

@TokageItLab
Copy link
Member

TokageItLab commented Apr 13, 2021

Asynchronization in blending is not always a problem. For example, sideways walking, a simple blend of strafing and back-and-forth walking like in the tps-demo, will not cause problems, but cases where you have to turn your hips will cause problems because of the synchronization required. So, synchronization is necessary.

@and-rad
Copy link
Contributor

and-rad commented Jun 21, 2022

I'm looking at ways to implement this and I have a bunch of thoughts and questions.

The p_optimize argument for the AnimationNode::_blend_[animation|input|node] functions doesn't really optimize anything? As far as I can see, it only sets the p_time property to 0 when a blended node has a total weight of 0. But in terms of optimization, that only affects a single thing in all of the animation tree: Method tracks are ignored in AnimationTree::_process_graph. All the other tracks are fully evaluated. Is that correct?

When p_time is 0, animations are basically stopped from playing. They're still evaluated, but always at the same position. This is what causes the desynchronization of animations in blend spaces when one of them is weighted at 0.

The Sync parameter that some animation nodes provide disables the optimization step. But it's not passed down the tree because not all animation nodes have that property. Sync is effectively ignored when a TimeScale node is connected to one of the inputs of a Blend2 node, for example. Wouldn't it make sense to make sync a field of AnimationNode and expose it as a property for those nodes where it makes sense? That way, a node can call set_use_sync on its children nodes and the state would be passed all the way down.

With these changes alone, animation synchronization would become relatively easy by using TimeScale nodes inside of BlendTrees. The weighting has to be done in GDScript, but this setup works. I've done that just the other day. It's not very generic, but it gets the job done.

To sync animations in blend spaces, we need a way to apply the weighting right there in the blend space ::process function. That means we need to be able to change the playback speed of the affected animations. I find it difficult to figure out the best way to do that. Maybe there should be a way to set the playback speed as a property of an animation node?

@TokageItLab
Copy link
Member

TokageItLab commented Jun 21, 2022

@and-rad p_optimize may be a bad name. It is actually there to pass sync to the chained parent node.

To consider about optimization: in the Skeleton case, it is obvious that we need to blend stopped and playing animations, and in cases like the Audio and Method tracks, we need to blend them because bugs can occur at the end of the timeline even if it is stopped. Method track has a bug and I sent a PR #61885 to fix it last time.

Your understanding is correct, the p_optimize parameter only works to cause asynchrony if it is not necessary to blend completely.

I agree that this feature is necessary. Without it, it would be difficult to create a TPS game. As I mentioned the solution above, the combination of Sync and TimeScale can actually solve this problem as an alternative to #34179. Also, I think that a Sync parameter should be added to NodeTransition too.

@and-rad
Copy link
Contributor

and-rad commented Jun 21, 2022

So I think a good first step would be to move sync and its getter and setter to AnimationNode. The nodes that need to expose it as property can do that (like AnimationNodeAdd2 and AnimationNodeBlend2) and the other ones don't (like AnimationNodeOutput probably). Internally, the value can still be passed along freely.

Could it make sense to maybe remove the TimeScale node completely and instead add a time_scale property to the AnimationNode class? Including getters and setters, so I could query and change the node's playback speed directly.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 21, 2022

Do you mean implementing Sync and TimeScale on all AnimationNodes instead of AnimationNodeAnimation? Sounds good to me, but I am a little concerned about the effects of this. Well, I think that NodeTimeScale could be removed in that case.

It might be better to discuss this in the contributor chat with animation team. At the very least, I think we should be concerned about the exceptions, such as NodeSeek and NodeStateMachine.

@and-rad
Copy link
Contributor

and-rad commented Jun 21, 2022

Do you mean implementing Sync and TimeScale on all AnimationNodes instead of AnimationNodeAnimation?

Yes, exactly. At least for Sync. I'm not sure yet about TimeScale.

@TokageItLab
Copy link
Member

TokageItLab commented Jun 21, 2022

I believe that every node which have multiple inputs should have Sync. However, I feel that for TimeScale, it may be okay to leave it as an individual node.
Implementing TimeScale to all Nodes would be like having NodeTimeScale chained to every node child. But we need to look into what the fact that NodeTimeScale sets sync to false is affecting.

@and-rad
Copy link
Contributor

and-rad commented Jun 21, 2022

Let me write up a proper proposal. That would also make it easier to discuss in chat.

@TokageItLab
Copy link
Member

Sync may not be necessary for Nodes that do not have multiple inputs, such as NodeAnimation and NodeTimeScale. Therefore, it may be right way to implement a BaseClass for Nodes with multiple inputs.

@and-rad
Copy link
Contributor

and-rad commented Jun 21, 2022

That's true, but on the other hand, blend_input and blend_animation are both defined on the AnimationNode base class, even though they're not used by half of the derived classes. Sync is one bool and two one-liner functions, so I think it might be okay to add them there.

Also, Sync might also have to be passed to other nodes down the tree. So even though NodeOutput (for example) doesn't have a use for Sync, it might have to pass it to the Blend2 node that is connected to its input pin.

@TheCulprit
Copy link

Just had this issue in 3.5 and quickly tried the exact same model/animations in 4.0 and had the exact same issue with or without sync option enabled. It didn't seem to make a difference at all. The only solution, in 3.5 and 4.0, was editing the animations to the be the exact same length inside the AnimationPlayer.

@TokageItLab
Copy link
Member

TokageItLab commented Sep 1, 2022

See #62623. The workaround of synchronizing animations of different lengths by normalizing the animation length ratio by TimeScale works well.

The current blending system makes it difficult to get the lengths of all animations at the same time, and it is difficult to guarantee enough to cover all cases for normalization, so it is difficult to implement the automation of that normalization into the core.

However, it should be possible to script the automatic configuration of common case-specific animation combinations such as TPS and FPS as an add-on.

BTW in 3.5, BlendSpace does not have sync, so that workaround cannot be used. Even if the lengths are the same, the synchronization will be broken by something (by transition, oneshot, blend with 0 amount in Blendspace).

@Maveyyl
Copy link

Maveyyl commented Jun 16, 2024

On 4.1.beta1.mono [edit it's 4.3 sorry typo] I don't feel like sync is doing what is should be doing. I have 5 animations of same length: neutral and all 4 direction walk animations. If I repeatedly move the blend position from 1:0 to 1:1 I get different animations every time, with some having very low range of motions and some looking like the animations are backward, which is symptomatic of animations not being actually synchronized.

@TokageItLab
Copy link
Member

@Maveyyl I remember that sync was temporarily broken in 4.3.dev, but I don't remember any problems in 4.1. In any case, there was a major rework on time progression in 4.3, so I recommend trying 4.3 or later.

@Maveyyl
Copy link

Maveyyl commented Jun 16, 2024

@Maveyyl I remember that sync was temporarily broken in 4.3.dev, but I don't remember any problems in 4.1. In any case, there was a major rework on time progression in 4.3, so I recommend trying 4.3 or later.

Oops I meant 4.3.beta1 it's a typo sorry. So you confirm it's normal and I should wait?

@TokageItLab
Copy link
Member

@Maveyyl It has been fixed by #92739, so that fix will be included in beta2.

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