Rework AnimationNode process for retrieving the semantic time info

[TokageItLab]

• Closes https://github.com/godotengine/godot-proposals/issues/8083

Transfer detailed time information bidirectional between AnimationNodes

For the sake of explanation, assume that "Output is the most upstream" and "AnimationNodeAnimation is the most downstream", and imagine that information such as delta and seeked is flowing from Output to reach AnimationNodeAnimation.

This PR ensures that process() returns the animation length and position, not the remaining time. It means that the informations are passed from downstream to upstream to calculate the exact remaining animation time.

Also, upstream to downstream, the time information was a mixture of delta and current position like seek ? current_pos : delta, but now the exact delta and current position are passed separately. By allowing the exchange of semantic time information in both directions solves some of the time-related problems.

Also, this PR adds a break_loop_at_end option to some AnimationNodes to solve transition with looping animation issues.

• Fixes https://github.com/godotengine/godot/issues/67953
• Fixes https://github.com/godotengine/godot/issues/79208
• Fixes https://github.com/godotengine/godot/issues/79505
• Fixes https://github.com/godotengine/godot/issues/81715
• Fixes https://github.com/godotengine/godot/issues/82581
• Fixes https://github.com/godotengine/godot/issues/86984
• Fixes https://github.com/godotengine/godot/issues/87009
• Supersedes https://github.com/godotengine/godot/pull/82089
• Supersedes https://github.com/godotengine/godot/pull/84586

Add custom timeline

This allows some time-related modification (length, timescale, offset, loop) on NodeAnimation with referring to the Animation without modifying the Animation resource.

For example, if you want to blend a walking motion with a running motion, simply set the same length and align period of the different length animations in NodeAnimation, then simply place a TimeScale upstream and the synchronization / adjust speed will work.

• Supersedes https://github.com/godotengine/godot/pull/62424
• Supersedes https://github.com/godotengine/godot/pull/79400

Other changes

The inconsistency between NodeBlendSpace and NodeBlend as to which time information was given priority in blending has been unified to give priority to the highest blend amount. As noted in https://github.com/godotengine/godot-proposals/issues/8083, it would be ideal to allow users to make choices about obtaining preferred time, but this was abandoned because it would require more work. I have reduced the number of cases where the amount of time remaining is compared, which should reduce the number of cases where this is a problem.

The remaining time can be calculated from the length and current time, but calculating the length and current time from the remaining time is irreversible, so there is breaking compatibility as the changing return type of the GDVIRTUAL _process() as double remain to Dictionary node_time_info. As noted in the documentation, the time information must be stored and returned in a dictionary type.

[jcostello]

Supersedes https://github.com/godotengine/godot/pull/62424

Now animations would sync properly?

[TokageItLab]

Supersedes https://github.com/godotengine/godot/pull/62424

Now animations would sync properly?

Although not exactly the same, this PR adds the ability to align the length of those animations, so it should work as an alternative.

The problem with #62424 is that if the animation time is modified upstream by sync, the correctness cannot be guaranteed if there are branches or nests in the downstream. However, this PR should be safe because such time modifications are limited to the most downstream as NodeAnimation.

[SaracenOne]

I think on principle, this is fine and pretty close to the solutions we discussed to solving the underlying semantic time calculations. While quite a few options have been added, I think hiding them behind the contextually relevant use_custom_timeline toggle goes a fair ways to avoiding my concerns about frontloading complexity on the user experimenting with this system.

I know we discussed the possibility of alternative modes for automatically deriving semantic time in situations where it might be ambigious, but honestly, I think just letting the user set an explicit loop time in those situations will be okay.

There are a few issues I want to point out though. I'm not sure if the custom timeline ping-pong is working correctly; it seems to get locked at the end when setting in. I'll keep poking around at it though before writing a formal review.

[fire]

Animation team meeting notes:

1. Lyuma: Needs more testing.
2. Fire: where is Tokage test scene?
3. Saracen will do a formal review.
4. Saracen: update and create a general animation test suite.
5. Tokage: https://github.com/godotengine/godot/pull/75759 As for StateMachine Tokage says:There is a demo project out there dedicated to testing the behavior of nested StateMachine travel and start
6. Lyuma: Does semantic time pr affect nested test machines? Do it impact all cases? What is semantic time fixing?
7. Saracen: If your animation is looping you don't know where the loop point is. You don't know the time of the animation.
8. Lyuma: Nested state machines are also ambiguous and is added to this pr.
9. Tokage: https://github.com/godotengine/godot/files/13993286/state_machine_improvement_demo.zip

[fire]

Animation team meeting notes:

1. Lyuma: Flattened state machines. Want to use nested state machines for using.
2. Saracen: Start point and end point. Group button wasn't merged. Proposal to add: "Collapse a bunch of state machines to a group."
3. Saracen: Nervous and want everyone to test more. Not only Saracen. More more rigious testing for big tests.
4. Tokage: Testing projects: https://github.com/godotengine/godot/files/13993496/state_machine_improvement_demo_2.zip
5. Lyuma: Need to be able to redistribute the test cases.

[fire]

Animation tests from Tokage:

I think we can test this by preparing several nested StateMachines with the same connection and different Types, and checking the value after each transition.

The same goes for NodeTransition and Oneshot.

[adamscott]

Comments from the production team:

• @TokageItLab Could you confirm what you did or what you didn't have the time to do from the list of the animation team.
• Animation team (@fire, @lyuma and @SaracenOne) + @TokageItLab, is it possible to have an explanation on how the tests work? To have instructions on how to run the test and a clear "before"/"after" state.
• In general, to know if it's mergeable for 4.3, we need a reassurance from the team that you agree on the design and that it's safe to merge for users upgrading from 4.2.

[TokageItLab]

Due to the absence of unit tests for animation state machine, it has not been possible for everyone in the animation team to cover checking there is no problem or not for all possible transition cases.

I have made as many cases as possible with old projects attached to some issues, projects I have created, etc. to make sure the transitions are fine.

I have confirmed that all the listed issues have been resolved, so I believe we can merge this if someone else can confirm it, although it was mainly @SaracenOne who focused on this issue, so it would be best to have his approve.

[SaracenOne]

I apologize for not properly reviewing this sooner. Regarding the bugs I pointed out, if we want to get this merged and you're not sure if you can fix it in time, I could possibly recommend simply leaving those options out of this PR for now and including them in a seperate PR since I don't nessecarily know if they're essential to the broader need of having proper semantic time retrieval. There may be some tricky nuance to getting them working correctly.

I'm also a little bit unsure of the context the break_loop_at_end flag is presented at. While its quite easy for users to understand what it does, I'm still nervous about cluttering the property field. It might make more sense to place this property in the 'switch' property field anyway, but if you feel different and want to push back on that, don't let me dissuade you if you think it makes sense.

[TokageItLab]

Thanks, I did confirm the bug. But once I confirmed that it was working, it seems to have regressed in me when I was last tidying up the code... I will look into it.

[SaracenOne]

Thanks! There was also another concern raised in a discussion I had with @lyuma, mainly that while its nessecary to change the type in order to have the semantic time information, this PR does inevitably break API compatibility, and I'm not sure how we're supposed to handle situations like this in a point release. Lyuma was also concerned that even if this can be resolved, using dictionary may have some awkward performance implications. Ideally we would want to use a struct, but since we don't have those yet exposed in the scripting layer, a pre-allocated RefCounted object might be sufficent to remove any allocation overhead.

[TokageItLab]

@SaracenOne Alright, I believe the bugs pointed out have been fixed.

Whether break_loop_at_end should be a bool/flag or a enum/switch

Outside of StateMachine, same options exist for NodeTransition and NodeOneShot, which are not enum. I think it makes sense for it to be a bool flag to unify the look and functionality with those, so I would like go with that.

Whether it is a dictionary or a resource to be exported

Unless an unextended(base) AnimationNode is added, this should not be a problem. However, if we do that, then AnimationMixer::PlaybackInfo should be RefCounted as well.

Furthermore, there are also pieces https://github.com/godotengine/godot/pull/69641 and https://github.com/godotengine/godot/pull/69802 missing, so it is currently quite difficult to extend the AnimationTree system using only GDScript.

Probably we need a rework to extend the AnimationTree system with GDScript at some point in the future, where we will have to rethink all of that.

But for now, to focus is on the 4.3 release, this is probably fine as is.

[reduz]

I have some feedback on this, will try to review ASAP.

[rsubtil]

We came across the #86984 bug in our project, where we essentially need animation 1D blending with discrete blending due to pixel-art style, and the BLEND_MODE_DISCRETE_CARRY mode is not respecting previous playback positions when switching animations.

I've made a reproduction project to exemplify this, and have tried to apply this PR in hopes of fixing it, but for this repro case it seems to break the position property: repro.zip

Vanilla, blend mode to continuous

https://github.com/godotengine/godot/assets/6501975/45338190-41c2-4703-9050-69063cc191ee

Vanilla, blend mode to discrete / discrete carry

https://github.com/godotengine/godot/assets/6501975/8eff78a3-2a43-4e9c-b575-b3db6f557e70

This PR, blend mode to continuous

https://github.com/godotengine/godot/assets/6501975/0d83f9c7-c91e-44d9-bedd-c364ca5aca21

This PR, blend mode to discrete / discrete carry

https://github.com/godotengine/godot/assets/6501975/d256f378-0767-4c4b-96bd-73ed69428b16

[TokageItLab]

It seems like the regression is happening with something that probably has nothing to do with this. I will investigate.

[TokageItLab]

Since pi.time is no longer used as delta (so needs to replace with pi.delta), it seems that https://github.com/godotengine/godot/pull/86644 was causing potential conflicts when rebasing. Now it should have been fixed and confirm solving https://github.com/godotengine/godot/pull/87171#issuecomment-1952391649 issue.

[rsubtil]

Since pi.time is no longer used as delta (so needs to replace with pi.delta), it seems that #86644 was causing potential conflicts when rebasing. Now it should have been fixed and confirm solving #87171 (comment) issue.

Can confirm it works perfectly, thank you so much! :heart:

[TokageItLab]

As mentioned in https://github.com/godotengine/godot/pull/87171#discussion_r1486044677, for GDVIRTUAL _process() of AnimationNode, using a Dictionary is not good from a performance point of view, and there are currently many missing APIs to extend AnimationNode. So in Animation/Pipeline Team Meeting, we agree with that we just make it deprecated without any change for now.

Perhaps a new GDVIRTUAL method will be added after https://github.com/godotengine/godot-proposals/issues/7329 is implemented.

[akien-mga]

Thanks!

[David-Ochoa]

Add custom timeline

This allows some time-related modification (length, timescale, offset, loop) on NodeAnimation with referring to the Animation without modifying the Animation resource.

Just received a notification about this. I can tell you this will not solve all the issues, I made something like this manually changing the animation length for walk and run, but couldn't do it for the idle animation (it's bigger compared to the other two) , and after a few cycles the blending broke again. I fixed it by changing all the animations based on the current position in the blendspace, I made a video (sorry it's in spanish) to explain the solution: https://youtu.be/GPcai3hZFGc?si=tBoqSqValg7mUXlO

[TokageItLab]

@David-Ochoa Unfortunately, beta1 has broken sync sorry, so I recommend trying beta2 or later. And in my opinion, the workflow for adjusting the period and length, which was particularly tired, should be reduced as follows:

If there is walking in 4 directions and running in 4 directions

• The animation cycle had to be modified in AnimationPlayer or Blender, but can now be modified in AnimationNode
• By unifying the length in the AnimationNode, eight timescale nodes is no longer needed and now only one node is needed to adjust the whole speed of the movement with foot step

[David-Ochoa]

@TokageItLab I'm trying to understand how the custom timeline works in my setup: I have 4 animations Idle - 10s Walk - 1.066667s Run - 0.7333333s Sprint - 0.4s I choose WALK as my base animation, then in the BlendSpace1D I set all the animations to use a Custom Timeline with the timeline length set to the WALK animation's length (1.066667s). If I do that setup the IDLE animation plays too fast, so I change the timeline length to a factor of the WALK animation (WALK * 6 = 9.6s) and then the animation plays correctly. My SPRINT animation is playin too slow, I change the timeline length to 0.5333335s (WALK / 2), but it blends incorrectly. I made a video with the steps I mentioned: Custom Timeline Am I doing something wrong or how is supposed to use this feature?

[David-Ochoa]

And this are the same animations but changing tha length based on the range of the Move Position slider: Change animations length

[TokageItLab]

@David-Ochoa So, do you think what we need is CustomTimeScale, as we should make StretchTimeScale becomee a preset and let the user enter a numerical value optionally?

My question is about the cycle of the steps. For example, if a walk animation has one left and one right footstep, and a run animation has two left and two right footsteps in one animation, even if the lengths are the same, this means that the run animation will loop for 2 cycles during 1 cycle of the walk animation.

Considering the solution to this case, the recommended solution is to extend the other animations to the animation with the largest number of cycles.

Then, align all animations to 1 second. Since the run animation has 2 cycles here, we want to adjust the timescale of the walk animation so that the number of cycles of the walk animation is aligned, but the lack of a custom timescale prevents us from doing so, is that your problem?

[David-Ochoa]

@TokageItLab No, the solution is to choose the correct frame to blend based on the current animation weight and the animation's length, I'm scaling the animations because I don't have acces to the internals of the AnimationTree and this way I feed the correct frames to the actual system (scaling the animations is a hack). I don't know how to explain how this works now and how to change it in the internal to work this way, Right now I'm not at my Godot machine, maybe later I'll make more tests.

[TokageItLab]

@David-Ochoa CustomTimeline is mainly intended for synchronization assuming that all cycles are matched. If you are looking for something more than CyclicSync https://github.com/godotengine/godot/pull/34179, it is probably something beyond the role of CustomTimeline and requires a different proposal.

FYI, I expect the following to be the general setup for synchronization of footsteps by CyclicBlending. If you want to gradually change animation with increasing moving speed, you need to blend synchronized walking, running, and sprinting in the same cycle, then adjust the speed of the cycle according to the master timescale.

If there is root motion, that would also need to be taken into account. In the past, I have experimented and written a paper https://tokage.info/lab/?id=3 (Japanese), so I basically believe that we can normalize based on the maximum moving value, but it is a bit complicated because of the calculation and the possibility of needing to use a straight line with inflection points for the blend value.

[David-Ochoa]

@David-Ochoa CustomTimeline is mainly intended for synchronization assuming that all cycles are matched. If you are looking for something more than CyclicSync #34179, it is probably something beyond the role of CustomTimeline and requires a different proposal.

Yes, it's more complicated than what this solves.

FYI, I expect the following to be the general setup for synchronization of footsteps by CyclicBlending. If you want to gradually change animation with increasing moving speed, you need to blend synchronized walking, running, and sprinting in the same cycle, then adjust the speed of the cycle according to the master timescale.

Yes, I'm achiving this with the setup I have. I just wanted this to be incorporated in the AnimationTree because it's too complicated with a BlendSpace2D

If there is root motion, that would also need to be taken into account. In the past, I have experimented and written a paper https://tokage.info/lab/?id=3 (Japanese), so I basically believe that we can normalize based on the maximum moving value, but it is a bit complicated because of the calculation and the possibility of needing to use a straight line with inflection points for the blend value.

I'm using root motion, but as I mentioned it's in a BlendSpace1D, what you mention in the paper would apply for a BlendSpace2D, and at the moment I won't touch it.

I'll change my setup to scale the Custom Timeline instead of adding a Timescale and let you know how it went.

[TokageItLab]

1. Blend the walk, run, and sprint animations with the same cycle (e.g. 1cycle), length (e.g. 1s), and timing of the same footstep (e.g. 0.0s: stamping a left foot, 0.5s: stamping a right foot) by CustomTimeline.
2. Add one NodeTimeScale on the result of those blends.
3. Link the blend amount and the Scale of the NodeTimeScale on the gdscript. Use remap(), Curve or Gradient as needed. Or it might work to have the animation track have a Timescale value.

If they are always synchronized, there should not occur any discrepancy between each of their cycles in 4.3.beta2 or later.

[David-Ochoa]

@TokageItLab I made an example using the method I implemented with the time scale now using the Custom Timeline, if you move the slider of Move Position (not the blend position itself) the Timeline Length is updated, and the animations blend correctly. My point is: a script should't be necesary, all the calculations could be done in the BlendSpace, it has all the information to get the correct frame to blend. TestCustomTimeline.zip

[David-Ochoa]

I belive it needs a little more work, I'm not 100% sure the time I'm setting is the correct one