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

Fix for short animation blend taking too long when played during a long animation blend. #37001

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

jitspoe
Copy link
Contributor

@jitspoe jitspoe commented Mar 12, 2020

If you play an animation with a long blend time and, while that blend is still happening, play one with a short (or zero) blend time, the long blend time will continue to be used (logic was sort of reversed). Also, if you played an animation with a 0 blend time while a blend was active, it still tried to blend. This PR fixes those issues.

@akien-mga akien-mga requested a review from reduz March 12, 2020 08:54
@akien-mga akien-mga added this to the 4.0 milestone Mar 12, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Mar 12, 2020
@Flavelius
Copy link
Contributor

This sounds like it fixes what i reported #43570 . Does it?

@akien-mga akien-mga requested a review from a team January 28, 2021 09:50
@jitspoe
Copy link
Contributor Author

jitspoe commented Jan 28, 2021

This sounds like it fixes what i reported #43570 . Does it?

Should fix the second part, when calling the animation player directly. I'm not sure if the animation tree uses the same code path.

@fire
Copy link
Member

fire commented Jan 28, 2021

@jitspoe This needs a rebase so it's testable @TokageItLab Can you take a look?

@TokageItLab
Copy link
Member

@fire I don't use StateMachine often, so I don't really understand the original bug. I need a sample project with clear examples of bugs and fixes.

I created a project file to try it out, but it doesn't seem to be like this.
blend_test.zip

@Flavelius
Copy link
Contributor

Flavelius commented Jan 29, 2021

@fire I don't use StateMachine often, so I don't really understand the original bug. I need a sample project with clear examples of bugs and fixes.

I created a project file to try it out, but it doesn't seem to be like this.
blend_test.zip

Your project unfortunately also shows a different bug; if the state transition is inside a BlendTree it skips to the animation end of the first state input when transitioning back instead of continuing from the current intermediate position.
I've attached an example where the issue is visible, this is using a StateMachine .
There, click the BendUp state then immediately BendSide again. The first transition to BendUp will continue blending until its full duration is completed (5s) before going back (1s) even though it's set to immediate

BugProject.zip

@TokageItLab
Copy link
Member

@Flavelius Thanks for the project file. As it turns out, the issue in #43570 does not seem to be related to this PR. The #43570 issue seems to be probably a bug only in the transition back to the start state.

@lyuma
Copy link
Contributor

lyuma commented Aug 7, 2021

I'm digging through open PRs, and I'm confused what this PR is fixing.

The linked issue #43570 is now closed.
Proposal godotengine/godot-proposals#2200 appears to be more broadly suggesting to allow interruptions in transitions/blends.

So my question is: what exactly does this change fix?
Does it fix a bug? If so, what is the reproduction project.
Does it implement 2200 or bring us closer to an implementation of that? What work still needs to be done to implement 2200, and is it feasible for 4.0?

@jitspoe
Copy link
Contributor Author

jitspoe commented Aug 8, 2021

This is a bug. For an extreme example:

Using an AnimationPlayer on a 3D skeletal mesh, play Animation A.
Then play Animation B with a transition time of 1 second (or something really long).
Then play Animation C with a blend time of 0 (or something really short). Animation C will blend using Animation B's blend time if Animation B is still active. What SHOULD happen is that Animation C should start IMMEDIATELY.

The linked issue had multiple things, and this only addressed a small part of one of them.

@fire
Copy link
Member

fire commented Oct 17, 2022

@jitspoe are you ok checking if this is still valid in Godot Engine master?

Also nudged @TokageItLab.

@jitspoe
Copy link
Contributor Author

jitspoe commented Oct 20, 2022

I'll try to find time to test it. Lots of stuff happening at the moment. 😅

@jitspoe
Copy link
Contributor Author

jitspoe commented Dec 20, 2022

@fire Ok, blending from a blend is definitely broken in Godot 4 (as of beta 9). Here's a simple project:
test_anim_blend_g4.zip

Here's the sample project code for reference:

func _ready():
	$test_anim/AnimationPlayer.play("up_front")
	var timer = Timer.new()
	add_child(timer)
	timer.wait_time = 1.0
	timer.one_shot = true
	timer.connect("timeout", start_anim2)
	timer.start()


func start_anim2():
	print("Starting anim 2")
	$test_anim/AnimationPlayer.play("up_side", 5.0) # 5 second blend time
	var timer = Timer.new()
	add_child(timer)
	timer.wait_time = 1.0
	timer.one_shot = true
	timer.connect("timeout", start_anim3)
	timer.start()


func start_anim3():
	print("Starting anim 3")
	$test_anim/AnimationPlayer.play("up_back", 0.1) # 100ms blend time (should be very fast)

Note that if you change the first timer to go directly to start_anim3, the end result is what's expected. The arm will start rotating forward, then blend to rotating back. If you do all 3 blends, it starts correctly, blending slowly into the sidways up anim, but when it gets to the 3rd anim, it pops and the end result is the arm in the up forward position instead of up back.

Edit: Seems this behavior did change slightly, and looking at the code it seems there were some attempts to fix, but they are still not correct for blending with a long blend that exceeds the anim time. This PR is still relevant.

@TokageItLab
Copy link
Member

TokageItLab commented Dec 20, 2022

I have been working on various animation bug fixes lately and I just realized that this is a problem with AnimationPlayer's Blending feature. Yes, AnimationPlayer's Blending is almost completely broken and has already come to the point where it is impossible to fix. See godotengine/godot-proposals#5952 and #70241 for a detail of the problem.

@jitspoe jitspoe force-pushed the master.anim_blend_fix branch 3 times, most recently from 3d17b43 to 14cf5e5 Compare December 20, 2022 04:17
@jitspoe
Copy link
Contributor Author

jitspoe commented Dec 20, 2022

I've done a rebase, so this should be compatible with the latest 4.x, now. Hopefully I didn't mess anything up. Spent like 2 hours dealing with mystery parts of code and commits appearing/disappearing.

scene/animation/animation_player.cpp Show resolved Hide resolved
scene/animation/animation_player.cpp Outdated Show resolved Hide resolved
scene/animation/animation_player.cpp Show resolved Hide resolved
… played immediately after an animation with a long blend time would play with a long blend time or cause popping/incorrect animation positions.
@jitspoe jitspoe force-pushed the master.anim_blend_fix branch from 9d9a57b to 7a7f5a2 Compare December 21, 2022 22:40
@akien-mga akien-mga merged commit d894fa8 into godotengine:master Dec 22, 2022
@akien-mga
Copy link
Member

Thanks!

@jitspoe
Copy link
Contributor Author

jitspoe commented Dec 22, 2022

Would be great to get this backported to 3.x (what the PR was originally for) as well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:3.x Considered for cherry-picking into a future 3.x release topic:core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants