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

AnimationTree ignores first frame of first child state when traveling from root to SubStateMachine #71562

Closed
Tracked by #73534
eh-jogos opened this issue Jan 17, 2023 · 11 comments · Fixed by #75759
Closed
Tracked by #73534

Comments

@eh-jogos
Copy link

Godot version

4.0 beta12

System information

Manjaro Linux using i3, Foward+

Issue description

Here is a short video:

ignores_03.mp4

In the first part the "outside" animation counts from 00 to 02, and when you travel to "group" it stays a while on 00 and starts counting from 04 to 06, but once you edit the "group" AnimationNodeStateMachine you see that the first animation in there should actually have been 03, but it was skipped.

Further testing while creating this MRP has made me realize what is happening is that it is playing the correct animation, but it is skipping it's first frame, which in fact, ends up using only the duration of the animation in this case:
image

I have a similar case in the project I'm working on, it's a beat'em up and we have a "ground_recovey" AnimationNodeStateMachine, where the first "state" inside it is just a lying down animation which sets everything in the first frame, and once it finishes it automatically travels to a "getting up" animation, but now it's stuck in the last frame of the previous "falling" animation and then transitions to getting up.

Steps to reproduce

Have an animation with tracks that only use the first frame, and set it as the first state in AnimationNodeStateMachine inside another AnimationNodeStateMachine. The tracks that only use the first frame will be ignored.

Or see MRP below.

Minimal reproduction project

ignored_first_frame.zip

The MRP above is slight different from the video, as I've added some more things, but the "counting" sprites are the same.

The animation "inside_1_broken" is the one that should be working but it's first frame is being ignored, including the method track.
The animation "inside_1_work_around" is the work around I did to confirm the first frame was indeed being ignored, by moving the method call to the second frame and duplicating the other keyframes on frame 2.

The AnimationTree connects to a group that uses the "broken" animation and to another one that uses the "work_around" animation.

If you run the project, you can press "ui_right" to transition to the "broken" group, "ui_down" to transition back to the "outside" state and "ui_left" to transition to the "work_around" group.

@akien-mga akien-mga added this to the 4.0 milestone Jan 17, 2023
@Remi123
Copy link

Remi123 commented Jan 17, 2023

Didn't test it on your MRP, but for my needs I call playback.travel or start for both animationstatemachineplayback. Since entering the substate , the first animation is Start ( which is the TPOSE, don't ask me why it works like that) so you have to travel from this state to your desired animation.

So, for my needs and correctly blending stuff, I do this :
root_playback.travel('SubState') substate_playback.start('AnimationNode')

@eh-jogos
Copy link
Author

@Remi123

Didn't test it on your MRP, but for my needs I call playback.travel or start for both animationstatemachineplayback. Since entering the substate , the first animation is Start ( which is the TPOSE, don't ask me why it works like that) so you have to travel from this state to your desired animation.

So, for my needs and correctly blending stuff, I do this : root_playback.travel('SubState') substate_playback.start('AnimationNode')

I think it's much simpler to just have the Start transition to the first node set to "Auto", I've never had to keep track of more than the "main" playback, and if I had to I would have already abandoned AnimationTree a long time ago.
image
image

And my problem is not that the inside animation is not being played, it is, it's just ignoring the first frame of the animation.

@TokageItLab
Copy link
Member

@eh-jogos Is it not fixed in #71264 or #71418? I know about the strange blending issue on the first frame, but this is the first time I've seen the method track being ignored. I'll do some more testing later.

@eh-jogos
Copy link
Author

@TokageItLab

@eh-jogos Is it not fixed in #71264 or #71418?

Just tested the MRP with the artifacts from #71418 that I downloaded yesterday to test the other MRP, and the results are exactly the same as with beta13 and what I described in the issue.

It looks like the first frame from the animation is being ignored as this doesn't work:
image

but this "works":
image

But really, only when you're traveling to another StateMachine node, and only on the transition from Start to the first node of that "sub" state machine, other transitions seem to be fine for me.

In my main project I'm not using any methods on the first frame, what breaks for me there is the AnimatedSprite:animation track being ignored, but on the MRP is easy to see that the method is being ignored because it doesn't print anything, only prints in the "work_around" case.

@TokageItLab
Copy link
Member

Ah, the problem with SubStateMachine, I got it.

@TokageItLab TokageItLab changed the title AnimationTree ignores first frame of first child state when traveling from root to another AnimationNodeStateMachine AnimationTree ignores first frame of first child state when traveling from root to SubStateMachine Jan 19, 2023
@TokageItLab TokageItLab modified the milestones: 4.0, 4.1 Jan 27, 2023
@eh-jogos
Copy link
Author

eh-jogos commented Jan 27, 2023

@TokageItLab This being moved to 4.1 means any first node inside "sub" state machines in 4.0 life time will be broken, effectively making it not a feature, or weird work around being necessary for it to work, at least for 2D animations. Don't know if this error is the same in 3D animations.

Again, same case as the transitions from #62576 it's a "feature" that I can create in the editor, but that doesn't work when the project is running.

Maybe make sure to add this as a "known regression" when 4.0 is released? Because I don't have this problem in 3.x

@TokageItLab
Copy link
Member

TokageItLab commented Jan 27, 2023

@eh-jogos To begin with, the current SubStateMachine should not be really called a GroupedStateMachine, rather a nested another StateMachine. So we need to fundamentally change the design of the SubStateMachine. See godotengine/godot-proposals#6130.

Therefore, fixing SubStateMachine-related issues requires that proposal as a prerequisite. The milestone 4.0 is intended to be 4.0 RC1. There is too short a time frame to implement this in 4.0 RC1, so it will be at least RC1 or later. Since the priority is set high, it could be implemented in RC2 or RC3, but I set the milestone to 4.1 because it is unknown how much compatibility breakdown will occur.

@qaptoR
Copy link

qaptoR commented Feb 4, 2023

So, for my needs and correctly blending stuff, I do this :
root_playback.travel('SubState') substate_playback.start('AnimationNode')

@Remi123 could you go into a little more detail about your workaround?
I'm wondering how it can be seamlessly integrated with a pre-existing state-machine graph.

I seem to have found my own workaround to get correct blending on the transition out of the sub-statemachine (see figures below)
But it's a bit tedious (although still preferable) and still doesn't address the transition IN

image
fig. 1: transition from this ADS state to either a left or right turning state

Fig. 1 is the ideal solution for handling this transition. The state machine nodes and transitions seems to imply that it is the intended way to resolve this.
However, blending does not occur appropriately because as Remi123 has already pointed out, the start node is processed first, so whatever xfade transition there should be from ADS to turning, is actually ADS to TPose.

image
fig. 2: transition from ADS current-sm to ADS sub-sm

image
fig. 3: transition from ADS sub-sm to LEFT or RIGHT then back to ADS sub-sm to ADS parent-sm

This approach seems to work for getting around the END node TPose, because the blending happens before crossing that threshold between LEFT and ADS. Between state-machine levels there is just an auto-advance.

However, the documented bug in this issue still rears it's ugly head and attempting to thwart the START threshold still results in a quick frame of TPose before the blend.

Overall, the current solution is to NOT try and use sub-statemachines to group together complex branches that require blending. Even if it leads to clutter

@Remi123
Copy link

Remi123 commented Feb 7, 2023

So, for my needs and correctly blending stuff, I do this :
root_playback.travel('SubState') substate_playback.start('AnimationNode')

@Remi123 could you go into a little more detail about your workaround? I'm wondering how it can be seamlessly integrated with a pre-existing state-machine graph.

@qaptoR My "trick" isn't working at all time, and there is some update since I wrote that. but basically if the substate is already in a node, and the parent state travel to the substate, it would blend correctly. However exiting the substate isn't blending correctly, but that's maybe me.

I would wait until substates are more defined in godot 4 as there is more work in this.

For now I'm just adding functionality and not blending to the animation.

@TokageItLab
Copy link
Member

@eh-jogos @Remi123 I ping you as you may still be interested in this topic. I've sent a PR #75759 for the SubStateMachine rework so it would be helpful to test it and get your feedback.

@eh-jogos
Copy link
Author

eh-jogos commented Apr 9, 2023

I saw this on twitter yesterday but couldn't test it during weekend, will definitely try to test it monday or tuesday and give some feedback!

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

Successfully merging a pull request may close this issue.

5 participants