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

Bugfix: Fix Anim Tree blending inconsistency #34134

Closed
wants to merge 1 commit into from

Conversation

marstaik
Copy link
Contributor

@marstaik marstaik commented Dec 6, 2019

Reduz had done some work on this to make it a "nicer" blend, but the logic is non-linear and non-deterministic.

In the old blending method, there is a rot_blend_accum that gets increased for each blend, and then the blend percentage is divided over the maximum.

This leads to non-linear scaling that is non-deterministic, since the order of the blends makes the blend weights different.

IE:
Blend 1 : 0.4 percent
Blend 2: 0.5 percent
Blend 3: 0.1 percent
-> (0.4/0.4 * rotation) * (0.5 / 0.9 * rotation) * ( 0.1 / 1.0 * rotation)

If you change the order:
Blend 3: 0.1
Blend 1: 0.4
Blend 2: 0.5
-> (0.1/0.1 * rotation) * (0.4 / 0.5 * rotation) * (0.5 / 1.0 * rotation)
...Which is inconsistent for the reasons above

This change also causes the triangles in blend spaces to jump dramatically between two zones, as you would go from blending triangle A into triangle B (where Blend 2 & 3 are the common edge between the triangles):
Blend 1: 0.1
Blend 2: 0.5
Blend 3: 0.4
->
Blend 2: 0.5
Blend 3: 0.4
Blend 4: 0.1

Strangely enough, the root motion code above it actually calculates all of the transforms correctly.

@marstaik marstaik changed the title Fix anim tree Bugfix: Fix Anim Tree blending inconsistency Dec 6, 2019
@akien-mga akien-mga requested a review from reduz December 6, 2019 06:40
@akien-mga akien-mga added this to the 3.2 milestone Dec 6, 2019
@reduz
Copy link
Member

reduz commented Dec 13, 2019

I had to change it because there was also a problem with the previous approach (blending from an empty quaternion) that I can't remember now.. let me grab lunch and I can give a check to this

@akien-mga akien-mga modified the milestones: 3.2, 4.0 Jan 23, 2020
@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jan 23, 2020
@marstaik
Copy link
Contributor Author

Updated to master

@marstaik
Copy link
Contributor Author

Updated to master 4.0

@aaronfranke
Copy link
Member

@marstaik Is this still desired? If so, it needs to be rebased on the latest master branch.

@marstaik
Copy link
Contributor Author

marstaik commented Jan 19, 2021

Yes, it needs to go in still. Ill see if I can rebase it today.

Edit: Done

@fire
Copy link
Member

fire commented Jan 20, 2021

CC @TokageItLab

@TokageItLab
Copy link
Member

I agree this change. I also think that the inconsistency in the old blending method is not good. Perhaps if there is a problem, it is incompatibility, especially with the old NodeAdd.

Ref: #37661 or #42302

However, I am still in favor of this change.

@needleful
Copy link
Contributor

needleful commented Feb 18, 2021

I've tried cherry-picking this commit for 3.2.4 to fix my own issue (#46038). This pull request fixes my issue, but it introduces a smaller bug. For some reason this causes animations to have the wrong transforms for a single frame after calling AnimationNodeStateMachinePlayback::start. It looks like the location, rotation, and scale are default.

(Edit) I fixed the bug. I don't know if it would work in the general case, but I'll just share it here. I modified AnimationStateMachinePlayback::process to return early when starting a new animation. Some of the logic later on is messing up the first frame, but it's not relevant to starting a new animation anyway as far as I can tell

// animation_node_state_machine.cpp, line 349
if (do_start) {

	if (p_state_machine->start_node != StringName() && p_seek && p_time == 0) {
		current = p_state_machine->start_node;
	}

	len_current = p_state_machine->blend_node(current, p_state_machine->states[current].node, 0, true, 1.0, AnimationNode::FILTER_IGNORE, false);
	pos_current = 0;
	loops_current = 0;
	// added early return (not sure if the checks for fading are even needed)
	if(fading_time == 0.0 || fading_from == StringName()) {
		return len_current;
	}
}

@aaronfranke aaronfranke removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Feb 18, 2021
@belzecue
Copy link

belzecue commented Mar 27, 2021

Does this issue explain this, where I'm blending two identical rotation degrees (0,0,0) at 50% and it's returning a glitchy rotation?

See: https://media4.giphy.com/media/vG6EWD3HUkbqU3Umf0/giphy.gif
giphy

@Calinou
Copy link
Member

Calinou commented Oct 17, 2021

Superseded by #53903. Thanks for the contribution nonetheless!

@TokageItLab
Copy link
Member

This is a too late, but I found a hint to fix this correctly in Godot3, so I am leaving this as a reminder.

In Godot4, some of the blend calculation methods have been changed, but as described in #80813, the correct blending can be calculated by computing the total weights first.

After all, the main problem with Godot3 blending is that it calculates each blend without knowing the total weights, which leads to incorrect final weight for each.

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

Successfully merging this pull request may close these issues.

10 participants