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: Improper blending of bone location when blending more than two animations #46038

Closed
needleful opened this issue Feb 15, 2021 · 27 comments · Fixed by #57675
Closed

Comments

@needleful
Copy link
Contributor

needleful commented Feb 15, 2021

Godot version:
Godot 3.2.4 rc1

OS/device including version:
Windows 10 version 1909

Issue description:

When blending more than two animations using two separate Animation Nodes (such as a Blend2 being consumed by another Blend2), the location of bones isn't properly interpolated if more than 2 animations are being blended together.
This is most noticeable when transitioning from a Blend Amount less than 1 to exactly 1, which causes a noticeable "snapping", because values arbitrarily close to 1 have a noticeably different location. It seems rotations are interpolated smoothly.
I've also tested this with a BlendSpace2D within a BlendSpace1D, and using transitions to have a similar effect. Both have the same problem, where there's a jarring transition when reaching an animation.
However, here's a strange exception: if the blended node is put into the in slot of a Blend2 and the third animation the blend slot, it works fine.

video of the problem

Steps to reproduce:

  1. Create a model with three skeletal animations. For two of them, translate one or more bones (make sure to not just rotate them).
  2. Import the model and animations and create an AnimationTree using the model's AnimationPlayer. Set its root as an AnimationNodeBlendTree
  3. Create a Blend2 of the translated animations. I also tested with a BlendSpace1D and BlendSpace2D. Set the blend_amount between 0 and 1 (exclusive)
  4. Create another Blend2. Set the in slot to use the third, non-translated animation, and set blend to the translated animation blend.
    • (Important info: if you swap the in and blend inputs, the problem goes away in this example. This could be a useful workaround, unless you're blending between two blending nodes, which is my real use case, in which case the issue is present regardless of the input order).
  5. Slowly move the blend_amount of this new node from 0 to 1
  6. Watch as the animation snaps into place when going from 0.99 to 1

Minimal reproduction project:

project.zip

Things to do in this project:

  • Open the AnimationTree and slowly move the blend amount of problem_blend from 0 to 1. The box will snap when going from 0.99 to 1 and back.
  • Set the blend amount of down to 0 or 1 and repeat the previous step. The problem disappears
  • Swap the inputs of in and blend for problem_blend and repeat step 1. The problem disappears
@needleful
Copy link
Contributor Author

More experimentation:
Someone on Reddit suggested using a StateMachine to transition between the two nodes with a crossfade. I tried it with my minimum reproduction, and going from the Blend1D to the Animation had a perfectly smooth transition, but going from the Animation to the Blend1D had the exact same problem, where it would jump a sizeable distance at the start.
This is unfortunately not a usable workaround for my use case, where I need to smoothly transition between two BlendSpace2Ds.

@needleful
Copy link
Contributor Author

I've discovered the bug is related to the order in which blend_node is called while blending between the nodes.
I figured this out by modifying AnimationNodeStateMachinePlayback::process

// By default, the skip happens when fading from the blend node to another animation
// If I move the following line after the if statement, the jump occurs when fading into it
float rem = p_state_machine->blend_node(current, p_state_machine->states[current].node, p_time, p_seek, fade_blend, AnimationNode::FILTER_IGNORE, false);

if (fading_from != StringName()) {

	p_state_machine->blend_node(fading_from, p_state_machine->states[fading_from].node, p_time, p_seek, 1.0 - fade_blend, AnimationNode::FILTER_IGNORE, false);
}

I will investigate further tomorrow

@Calinou
Copy link
Member

Calinou commented Feb 15, 2021

Duplicate of #23414 and/or #37661?

@needleful
Copy link
Contributor Author

@Calinou

I don't believe so. This issue occurs the same even if the animations are the same length, so I don't think it's related to syncing, and I'm not doing anything with Add2 or Add3.

@TokageItLab
Copy link
Member

Probably #34134

@needleful
Copy link
Contributor Author

@TokageItLab That seems exactly it! I'll pull that branch and see if it works for me.

@TokageItLab
Copy link
Member

TokageItLab commented Oct 17, 2021

@needleful Can you open issue about #34134 (comment) separately from #34134? I think that it seems to be a StateMachine issue, separate from the blending issue.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

I'm not sure if this is the exact same issue as what I've been having, but it seems likely related to an issue I noticed where there's some inconsistency in the way that the BlendSpace2D grid interpolates animations. I've created a test app in Godot 4 Alpha, but the same issue has been present in previous 3.x releases.

Godot_4_Blendspace2D_Test.zip

BlendSpace2DIssue.mp4

All three boxes are attempts to interpolate between three single-keyframe animations using a 2D Blend Space. Each animation is meant to Notice that at the corners they all correctly position and scale the boxes, however they don't line up at points along the edges or within the triangle.

The red box is when the position and scale are animated using the special 3D Position and 3D Scale tracks. The green box is when the position and scale are animated using property tracks. The blue box was programmed manually in a GDScript tool file that calculates barycentric coordinates and then blends the three animations in that ratio. The blue box is what I think most people would expect.

You can see the problem numerically if you place the blend position in the grid to (0, .333), which is the center of mass of the blend space's triangle. At this point, each animation should have equal influence over the resultant transformation, which will be true for the blue box, but not for the green or red box.

I have a fix for it here. If it seems good enough I'll make a PR out of it but I likely haven't thoroughly tested it.

@TokageItLab
Copy link
Member

@Shnazzy That issue is not related this issue but same with #54407, and the fix is already exist at #54205.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

@TokageItLab

Thanks for the quick reply. I just finished cloning the branch you linked, and after building it and rerunning that same project, I'm unfortunately still getting blending issues:

BlendSpace2DIssue2.mp4

Can you confirm that the same result happens on your environment?

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

@Shnazzy We already know that there are two major problems. The first thing is that the current Godot 4.0 blending 3D transform is perfectly broken. The second is that there is inconsistency in the results depending on the blend order. See this comment.

#54205 fixes the first issue (deference between red and green) but doesn't fix second issue (deference between red and blue, jump the values when cross triangle border problem). For second issue, as I mentioned in my comment, we probably have to change the blend stack and change the way of blending with triangle in order to have consistent blend results for all cases. And I think we need to separate those issues first.

BTW, in 4.0, modifying Transform3D by property tracks is deprecated for performance reason.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

@TokageItLab Understood. For what it's worth though, the commit I linked fixed the triangle blending issue on my end (at least in the simple case I implemented).

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

@Shnazzy I tested it more. Your commit has the potential to get things right, but there are a few things to consider on how to implement it.

  • Whether to do the same fix for rotation track, root motion and other tracks for consistency
  • Whether or not to implement blend() as a Math calculation

I'll discuss these with @reduz, @fire, @SaracenOne and @lyuma once. We can help you if you want to create a PR, or you can let us do it for you.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

@TokageItLab I'm not interested in credit or attention. I'd say besides the value types that I gave attention to in the test project, I really don't know much about the other track and property types. Rotation, for instance, I'd have to test it some more to determine if the same problems happen. It probably also depends on quaternions vs. eulers.

I'll take a look at the other track types, but you're free to do what you want with the changes to apply them to other components.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

@Shnazzy To be honest, I'm not sure why your fix is working correctly. I will look into this further, but we need a complete mathematical explanation of what is going on.

It looks like you are fixing a problem related to the initial values of the branches increased by the blend, but when I do the same fix for Quaternion, rotation tries to go through the initial values of Quaternion instead of the shortest path, so the Quaternions (e.g. Y: 179deg & Y: -179deg in Euler) are not interpolated correctly.

Edited:
Perhaps there is a same fix #34134 that was going to be done in the past. However, in it we found a problem in Quaternion as @reduz is concerned. I think we need to find a way to fix this problem.

2022-02-05.15.29.54.mov

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

Okay I did some more experimenting. Try this:

image
image

rot_raw needs to be added to the t structure's definition. It's just an accumulator that can be accumulated without normalizing until the very end. t->rot becomes the normalized version of rot_raw while avoiding any mid-step interpolation math.

Edit:
Oh and remove this part:

image

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

@Shnazzy Excellent! Looks like it is complementing correctly. Perhaps the root motion needs a separate initialization and rot_accum is no longer needed, so I think the final fix will look like this.
TokageItLab@7ea1f35

@TokageItLab
Copy link
Member

@Shnazzy However, I suspect that if it normalize by adding each element of the quaternion as it is, it may have problems with large movements. There is similar experiment #40592 (comment).
Perhaps it needs to be rewrite to adopt slerp.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

A few things:

  1. Perhaps rot_blend_accum should be the name instead of rot_raw
  2. I think since rotation tracks aren't always there, the initial value of t->rot should actually still be 0,0,0,1. That way it still has a normalized value even if it doesn't hit that part of the rotation blending. If rotation blending does happen then it will be overwritten all the same because the raw accumulator is initialized at 0
  3. I'm not sure how the loop exactly goes but should all 3 aspects (pos/rot/scl) of the transform be initialized by each other type of track? Like, in the position3d section it initializes rot and scale.

@TokageItLab
Copy link
Member

Using slerpni() instead of slerp() may solve the interpolation problem in Quaternion. Therefore, it seems that neither accumulator nor rot_raw is necessary.

I will do some more testing on the initialization. There was a problem with infinitely large scales when not all values were initialized in each tracks.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

To elaborate more on why the math of blend works and why lerp doesn't:

Points A, B, C have weights wA, wB, wC in the current blending step.
wA = .02
wB = .49
wC = .49

Call the current accumulated point P.

Lerp Math (Old way):

lerp(P1, P2, b) = P1 * (1 - b) + P2 * b

First iteration (A) is made the initial value of P.
Next iteration (B):
P = lerp(P, B, wB) = (A) * .51 + B * .49

(As a side note, if it were just two nodes, A and B, then this would be accurate since wB being .49 would imply wA is .51)

Next iteration (C):
P = lerp(P, C, wC) = (A * .51 + B * .49) * .51 + C * .49 = A * .2601 + B * .2499 + C * .49

Thus, C's blending is correct, but A and B look way off.

Blend Math (new way):

blend(P1, P2, b) = P1 + P2 * b
Init P = 0
First Iteration (A):
P = blend(P, A, wA) = (0) + A * .02
B:
P = blend(P, B, wB) = (A * .02) + B * .49
C:
P = blend(P, C, wC) = (A * .02 + B * .49) + C * .49

This should work with scale and position since they are linear spaces. As for slerp and quaternions, I think since quaternions aren't linear vector spaces (can't multiply a quat by a scalar value, can't add two quats together) it can't work the same way. It's sort of like how coordinates on a sphere won't follow the same geometric rules as those on a flat surface.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

By setting the initial value of each track to a fixed value, for example if animation A has a PositionTrack and animation B does not have a PositionTrack, the object to which A's PositionTrack is assigned will force the following value in animation B Vector3(0,0,0), it's odd. In 3.x and #54205, the initial value is set to the original pose, so nothing happens. Assuming a new fix is needed in blending, but we need to fix that problem.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

I don't think there's a "right" answer to "what should the blend triangle do to A if A doesn't have a track like B and C do?" One answer would be to make A to default to the identity transform. Another option is to find some way to have B and C act as if the weight distribution was only between them. As another option, there could be a "Default Tracks" animation like the reset track, or perhaps use the reset track if it's available...

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

If we choose way of the using default ID Transform, we get the problem of odd poses in character models. I believe that at least that should be the implementation that uses the latter RESET track.

@TokageItLab
Copy link
Member

TokageItLab commented Feb 5, 2022

I tested that to implement init_loc, init_rot, init_scale to TRSTracks with Skeleton and bone_rest. For blendshape/property/bezier track, we need to do same thing with using RESET track. Off course if TRSTracks have RESET track, it should use RESET track even if it has bone_rest.
TokageItLab@f3f55a9
This allows us to set the initial value of Quaternion to an optional value, so the interpolation using slerp should work to some extent. More cases need to be tested.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

Well here's something I found:
https://user-images.githubusercontent.com/22860318/152657819-27dbe357-de6b-4e88-bacb-5e927be58bb4.mp4

Godot_4_Blendspace2D_Test.zip

Also the resource I found here might be of use if it's not already been looked at.

@TokageItLab
Copy link
Member

@Shnazzy We may need to think a bit more about the Quaternion interpolation, but for now I have sent a PR #57675. Please test it if you can.

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