Add `sync` to `NodeTransition` and `BlendSpace1D/2D` and refactor `sync` in `AnimationTree` #62623

TokageItLab · 2022-07-02T03:49:56Z

Added AnimationSyncNode with the sync option as a base class and made any AnimationNode that may branch in the AnimationTree inherit from it.

Thus, AnimationTransitionNode is now has sync.

However, if the animation is played from the beginning after switching state, sync will be broken. To prevent this, the option from_start is added in AnimationTransitionNode.

Also, BlendSpace1D/2D now has sync. This allows animations in multiple directions to be blended without breaking sync.

In order to blend animations of different lengths neatly, the TimeScale must be changed depending on the actual animation blend value.

If the AnimationTree is not too complex, the actual blend value can be pre-computed from user input, etc., and this can be solved by adding a few lines of code in GDScript like below:

# Blend Walk and Run
_animtree["parameters/TimeScaleRun/scale"] = lerp(ANIMATION_LENGTH_RUN / ANIMATION_LENGTH_WALK, 1, _dash)
_animtree["parameters/TimeScaleWalk/scale"] = lerp(1, ANIMATION_LENGTH_WALK / ANIMATION_LENGTH_RUN, _dash)
_animtree["parameters/BlendDash/blend_amount"] = _dash

This process could be automated in some way, but it is quite complex. It requires recognizing chains between NodeAnimations and recognizing the actual blend values from the blends that exist in between.

In the past, #34179 has tried to solve this, but it still had the strong limitation that auto-adjustment would only occur between NodeAnimations that were directly chained in BlendSpace1D/2D.

More recently, there is an approach of #62424, but so far this completely corrupts the blending process and makes it inconsistent.

In my opinion, if #62424 is to be implemented correctly, an iterating process that only calculates the blend values for adjusting time scale needs to be added before the iterating process that calculates the actual track values without changing blend process. However, even if this were done, there would still be inconsistencies due to TimeScale and Seek.

In summary, now all we need to do for AnimationTree is to add sync correctly in some nodes which lacks it.

After all, when we create a game, we need a process to calculate the blend values depending on inputs and other factors. If we want to synchronize animations of different lengths, we should calculate the TimeScale properly in there. It can be solved with a few lines of code in GDScript and it is not difficult.

With this PR, the synchronization methods mentioned by @David-Ochoa in #23414 should now work perfectly correctly. No longer needed the hack for sync.

Fixed #23414.

#34179 and #62424 may be possible to keep it as a proposal, but perhaps it should be transferred to a godot-proposals for discussion so that we can reach a consensus. At least this PR reduce what is needed to implement them.

capture.avi-muxed_0.mp4

There is a sample project for TPS-Lock-on movement.

character_controller_3d_sample.zip

When rotating the hips in that TPS-Lock-on movement, the minimum number of animations required is high and blending can be complex, but the fixed AnimationTree uses sync to solve this problem with a tree of a certain size.

TokageItLab

Left some explanatory comments for the review.

TokageItLab · 2022-07-02T17:06:29Z

scene/animation/animation_blend_tree.cpp

+		if (p_seek) {
+			blend_input(prev, p_time, true, p_seek_root, blend, FILTER_IGNORE, true);


Seek should be applied instead of stopping the animation, as internal Seek caused by OneShot or Transition can break sync. Even if it is not sync, it seems to me that there is no reason to stop it.

TokageItLab · 2022-07-02T18:57:39Z

scene/animation/animation_tree.cpp

 	// If tracks for blending don't exist for one of the animations, Rest or RESET animation is blended as init animation instead.
-	// Then, blend weight is 0 means that the init animation blend weight is 1.
+	// Then blend weight is 0 means that the init animation blend weight is 1.
+	// In that case, processing only the animation with the lacking track will not process the lacking track, and will not properly apply the Reset value.
+	// This means that all tracks which the animations in the branch that may be blended have must be processed.
 	// Therefore, the blending process must be executed even if the blend weight is 0.
-	if (!p_seek && p_optimize && !any_valid) {
+	if (!p_seek && !p_sync && !any_valid) {


After blending algorithm was changed by #57675, p_optimize actually functioned only as !sync. p_optimize is replaced by p_sync and logical negation (false -> true) of the function arguments at each part.

TokageItLab · 2022-07-02T18:59:19Z

scene/animation/animation_tree.h

+	double _blend_node(const StringName &p_subpath, const Vector<StringName> &p_connections, AnimationNode *p_new_parent, Ref<AnimationNode> p_node, double p_time, bool p_seek, bool p_seek_root, real_t p_blend, FilterAction p_filter = FILTER_IGNORE, bool p_sync = true, real_t *r_max = nullptr);

 protected:
 	void blend_animation(const StringName &p_animation, double p_time, double p_delta, bool p_seeked, bool p_seek_root, real_t p_blend, int p_pingponged = 0);
-	double blend_node(const StringName &p_sub_path, Ref<AnimationNode> p_node, double p_time, bool p_seek, bool p_seek_root, real_t p_blend, FilterAction p_filter = FILTER_IGNORE, bool p_optimize = true);
-	double blend_input(int p_input, double p_time, bool p_seek, bool p_seek_root, real_t p_blend, FilterAction p_filter = FILTER_IGNORE, bool p_optimize = true);
+	double blend_node(const StringName &p_sub_path, Ref<AnimationNode> p_node, double p_time, bool p_seek, bool p_seek_root, real_t p_blend, FilterAction p_filter = FILTER_IGNORE, bool p_sync = true);
+	double blend_input(int p_input, double p_time, bool p_seek, bool p_seek_root, real_t p_blend, FilterAction p_filter = FILTER_IGNORE, bool p_sync = true);


Considering that p_optimeze = !p_sync, it seems that the default value for p_sync should be false, but in fact in most cases p_sync = true.

Also, there are only 2 parts that actually used that default value, AnimationNodeBlendTree::process and AnimationNodeOutput::process, which now explicitly pass p_sync = true instead of the default value.¹

Footnotes

If p_sync = false in these parts, sync will break in the nested cases such as BlendTree within BlendTree and BlendTree within BlendSpace etc. ↩

and-rad · 2022-07-04T09:04:06Z

What I don't understand is why we just don't get rid of p_sync completely. In other words: When would I ever want to set Sync to false?

TokageItLab · 2022-07-04T09:33:53Z

There are two reasons for leaving the sync flag in. The biggest reason is to be compatible with the behavior of Godot 3.x, which freezes animation when the blend value is 0.

The other reason is that currently 4.0 blending processing is required even when sync = false, but we expect performance improvements by not processing those blends if the track blending problem with RESET animations is resolved by better way than current one.

If there is enough consensus, it would be fine to remove it, but it would be tedious to re-implement it later, and since there is not enough consensus at this point, I think that we should make it compatible with old behavior.

and-rad · 2022-07-04T09:39:44Z

That makes sense, thanks. Although I don't think compatibility with Godot 3 should be that high on the list of priorities in that regard.

TokageItLab · 2022-07-04T09:47:22Z

Well sure, 4.0 allows for some level of breaking compatibility.

The only real use case I can explain is to use the freeze to prevent speed hacks that use only the acceleration portion of the nonlinear root motion.

For example, if only the first few frames of the running motion are slightly faster, switching repeatedly between jumping and running animations may be faster than continuing to run. By resuming the animation from the frozen portion, the speed can be averaged out. However, this can also be handled by properly setting TimeScale to 0.

and-rad · 2022-07-04T09:53:03Z

scene/animation/animation_blend_space_1d.h

@@ -63,6 +63,8 @@ class AnimationNodeBlendSpace1D : public AnimationRootNode {
 	StringName blend_position = "blend_position";

 protected:
+	bool sync = false;


Would it make sense to enable sync by default for BlendSpace1D/2D? I imagine it would be needed for the most common use cases anyway.

Suggested change

bool sync = false;

bool sync = true;

It would make sense for it to default to true at least for BlendSpace2D, but I don't know about BlendSpace1D. Or maybe it should default to true for all syncs for consistency.
@reduz How do you think?

For BlendSpace1D, I'm thinking about use cases like blending between walking and running without any strafing movement. Where velocity is the only input that you need.

That means it is the same thing as NodeBlend2 and NodeBlend3, so if we set the sync default for BlendSpace1D to true, then the other Nodes sync default should also set true.

scene/animation/animation_blend_space_2d.h

fire · 2022-07-11T16:21:21Z

What issues are blocking this?

TokageItLab · 2022-07-12T01:01:28Z

I think there is no issue which block this.

doc/classes/AnimationSyncNode.xml

akien-mga · 2022-07-16T14:53:40Z

Thanks!

TokageItLab added enhancement topic:animation labels Jul 2, 2022

TokageItLab added this to the 4.0 milestone Jul 2, 2022

TokageItLab requested review from a team as code owners July 2, 2022 03:49

TokageItLab changed the title ~~Refactor sync in AnimationTree~~ Refactor sync in AnimationTree Jul 2, 2022

TokageItLab changed the title ~~Refactor sync in AnimationTree~~ Add sync to NodeTransition and BlendSpace1D/2D and refactor sync in AnimationTree Jul 2, 2022

TokageItLab force-pushed the refactor-sync-animtree branch 2 times, most recently from ac222ab to 3077a9a Compare July 2, 2022 18:43

TokageItLab commented Jul 2, 2022

View reviewed changes

and-rad reviewed Jul 4, 2022

View reviewed changes

scene/animation/animation_blend_space_2d.h Show resolved Hide resolved

TokageItLab requested a review from reduz July 12, 2022 01:02

reduz approved these changes Jul 16, 2022

View reviewed changes

akien-mga reviewed Jul 16, 2022

View reviewed changes

doc/classes/AnimationSyncNode.xml Outdated Show resolved Hide resolved

TokageItLab force-pushed the refactor-sync-animtree branch from 3077a9a to 7e1d3d1 Compare July 16, 2022 10:55

refactor sync in AnimationTree

9be288e

TokageItLab force-pushed the refactor-sync-animtree branch from 7e1d3d1 to 9be288e Compare July 16, 2022 12:45

TokageItLab requested a review from akien-mga July 16, 2022 14:49

akien-mga merged commit c39223e into godotengine:master Jul 16, 2022

TokageItLab mentioned this pull request Aug 24, 2022

Better way of handling one shot action animations godotengine/godot-proposals#5155

Open

TokageItLab mentioned this pull request Sep 1, 2022

Sync animation in BlendSpace1D/2D #23414

Closed

TokageItLab deleted the refactor-sync-animtree branch September 16, 2022 21:05

TokageItLab mentioned this pull request Nov 13, 2022

Optimize animation blend tree process #68593

Merged

TokageItLab mentioned this pull request Jan 13, 2023

Add next/reset function to AnimationStateMachine #71264

Merged

TokageItLab mentioned this pull request Apr 24, 2023

[3.x] Additive Animation Fix (and Sub2 node feature) #76310

Open

TokageItLab mentioned this pull request Aug 11, 2023

Add support for animation sync markers godotengine/godot-proposals#7476

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `sync` to `NodeTransition` and `BlendSpace1D/2D` and refactor `sync` in `AnimationTree` #62623

Add `sync` to `NodeTransition` and `BlendSpace1D/2D` and refactor `sync` in `AnimationTree` #62623

TokageItLab commented Jul 2, 2022 •

edited

Loading

TokageItLab left a comment

TokageItLab Jul 2, 2022

TokageItLab Jul 2, 2022

TokageItLab Jul 2, 2022

and-rad commented Jul 4, 2022

TokageItLab commented Jul 4, 2022

and-rad commented Jul 4, 2022

TokageItLab commented Jul 4, 2022 •

edited

Loading

and-rad Jul 4, 2022

TokageItLab Jul 4, 2022

and-rad Jul 4, 2022

TokageItLab Jul 4, 2022 •

edited

Loading

fire commented Jul 11, 2022

TokageItLab commented Jul 12, 2022 •

edited

Loading

akien-mga commented Jul 16, 2022

		if (p_seek) {
		blend_input(prev, p_time, true, p_seek_root, blend, FILTER_IGNORE, true);

Add sync to NodeTransition and BlendSpace1D/2D and refactor sync in AnimationTree #62623

Add sync to NodeTransition and BlendSpace1D/2D and refactor sync in AnimationTree #62623

Conversation

TokageItLab commented Jul 2, 2022 • edited Loading

TokageItLab left a comment

Choose a reason for hiding this comment

TokageItLab Jul 2, 2022

Choose a reason for hiding this comment

TokageItLab Jul 2, 2022

Choose a reason for hiding this comment

TokageItLab Jul 2, 2022

Choose a reason for hiding this comment

Footnotes

and-rad commented Jul 4, 2022

TokageItLab commented Jul 4, 2022

and-rad commented Jul 4, 2022

TokageItLab commented Jul 4, 2022 • edited Loading

and-rad Jul 4, 2022

Choose a reason for hiding this comment

TokageItLab Jul 4, 2022

Choose a reason for hiding this comment

and-rad Jul 4, 2022

Choose a reason for hiding this comment

TokageItLab Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

fire commented Jul 11, 2022

TokageItLab commented Jul 12, 2022 • edited Loading

akien-mga commented Jul 16, 2022

Add `sync` to `NodeTransition` and `BlendSpace1D/2D` and refactor `sync` in `AnimationTree` #62623

Add `sync` to `NodeTransition` and `BlendSpace1D/2D` and refactor `sync` in `AnimationTree` #62623

TokageItLab commented Jul 2, 2022 •

edited

Loading

TokageItLab commented Jul 4, 2022 •

edited

Loading

TokageItLab Jul 4, 2022 •

edited

Loading

TokageItLab commented Jul 12, 2022 •

edited

Loading