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 blend animation to solve TRS track bug & blend order inconsistency #57675

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

TokageItLab
Copy link
Member

@TokageItLab TokageItLab commented Feb 5, 2022

Fix blending errors in TRS tracks and inconsistencies due to blending order (also fix the problem of triangle blending in Blend2D). This is a result of several discussions with @Shnazzy in #46038.

With this change, if animation A has track X and animation B does not have track X, the internal value of track X in animation B in their blend will be the initial value that the type has. To avoid this, I have implemented the initial value to refer to the RESET track, if it exists. And for Bone3D, if there is no RESET track, it is implicitly based on bone_rest. RESET track is no longer used, just it use rest track as default value.

When using this initial value, blending with the initial value is required even when the blend value is 0, so I changed place the check from the blend process whether the blend value is 0 or not. Without it, the value isn't updated when the blend amount to be rapidly set to 0 or 1. This causes a temporary inconsistency in the blending results and after an unintentional abrupt change in the value occurs when the blend amount is changed.

This PR supersede #34134. Also, it solves the problem to a greater extent than #54205, but the changes are larger, so if you need to make minimal fix for TRS track, use #54205.

Fixed #46038. Fixed #54407. Fixed #55838.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

Looks good so far in the test projects I was using, but I'll keep testing other things to see if more complex stuff breaks anything.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 5, 2022

The only concern I have is Quaternion blending, and I have avoided the problem of slerp() with initial Quaternion by referring to the Reset track or bone_rest. In character models, it is almost rare to animate a joint more than 180 degrees, so this implementation is generally fine.

The problem may occur when the object is rotated more than 180 degrees, but I am not sure if blending should be used in such a case.

If we really need to solve the problem, we will need to interpolate using an accumulator, but we will need to experiment to ensure that the interpolation is consistent with slerp() in the normal case. Related: #40592 (comment).

And perhaps it should be implemented as a selectable option. This is because we want the bones of the character model to have a behavior that prevents them from rotating more than 180 degrees, but for other objects we want them to rotate in the shortest path without preventing it.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 5, 2022

It also probably would make sense to animate the rotation according to what rotation mode the object is using. Like a propeller on a helicopter model might want to animate from -180 to +180, and in such cases, it would be easier to describe that with Eulers than Quaternions, just by my guess at least.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 6, 2022

There are animations that rotate more than 180 degrees, but I can't think of many use cases for blending them. I think propeller rotation should be controlled by scripts rather than animations, and for cases like turrets that always face the target, I think IK or any script should be used instead of BlendSpace2D.

One example that comes to mind is the animation of a multiple-faces-robot that rotates its face 180 degrees to express its emotions. In any case, I think the current broken blends need to be fixed as soon as possible, as I think that fix/enhancement for interpolating rotation can be done later.

@Shnazzy
Copy link
Contributor

Shnazzy commented Feb 6, 2022

I agree. I have no issues with the current proposed changes as I'm not a rotation calculation expert by any means.

@fire
Copy link
Member

fire commented Feb 6, 2022

What results did slerpni() give?

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 6, 2022

By using RESET Track or bone_rest as initial values instead of Quaternion(0,0,0;1), slerpni() produces unstable results. Even when the initial value is Quaternion(0,0,0;1), it might have generated unstable results somewhere, but maybe it just didn't show up in my test case in the past.

Perhaps in the current slerp() produces the consistent result is that only blends that cross 180 degrees (e.g. between Y:179 and Y:-179 in Euler) from the initial value, go back through the initial value, but I want more test cases.

If my guess is right, it means that for example the model's legs are looked rotated 180 degrees from Quaternion(0,0,0;1) in hips space, but if bone_rest is used as the initial value, then 180 degrees will be determined from there. So it is ideal in terms of preventing the legs from transitioning from the face to the back.

@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 6, 2022

After more experimentation, I concluded that the results from slerp() are difficult to predict because they are sparse depending on the direction of the axis. For now, I adopt the accumulator that @Shnazzy suggested in Rotation3D Track. This will ensure that the rotation is done consistently and in the shortest distance.

Since it is difficult to implement an accumulator for Quaternions in Property Track, the slerp() will continue to be used in there, but since animating Quaternions with property tracks is deprecated in 4.0, this should not be a problem.

If the interpolation problem suggested in #40592 (comment) appears, it will need to be addressed in the future, but I don't think there are any major problems at the moment.

@TokageItLab TokageItLab force-pushed the fix-blending branch 3 times, most recently from f62dc10 to ed7a327 Compare February 6, 2022 07:43
@TokageItLab
Copy link
Member Author

TokageItLab commented Feb 6, 2022

Sorry, I take back my previous statement, the reason why slerp()'s limit was unpredictable was because an RESET track was unintentionally inserted (caused by #56387). And the my theory seems to be correct.

Perhaps in the current slerp() produces the consistent result is that only blends that cross 180 degrees (e.g. between Y:179 and Y:-179 in Euler) from the initial value, go back through the initial value, but I want more test cases.

Also, the accumulator probably does the similar thing as slerpni(), which could create unintended rotations.

I finalized using slerp() as a tentative solution.

@TokageItLab TokageItLab force-pushed the fix-blending branch 2 times, most recently from 8b63eb3 to 262be1e Compare February 6, 2022 20:08
@reduz
Copy link
Member

reduz commented Mar 17, 2022

Fantastic job! Apologies for the delay in the reviews as this code is not that simple and the past weeks were super busy for me.

@akien-mga akien-mga merged commit bc576af into godotengine:master Mar 17, 2022
@akien-mga
Copy link
Member

akien-mga commented Mar 17, 2022

Thanks! 🎉

@macryc
Copy link

macryc commented Mar 18, 2022

At last. Is this merge going into alpha 5? Assuming so, when's alpha 5 due out?

@macryc
Copy link

macryc commented Mar 24, 2022

The issue was closed on the basis that the y position issue of the root bone during blending between animations got fixed.
It's fixed, but pos was only a third of the issue. Rot/scl is still messed up in alpha 5.

The only commit so far that fixes the issue properly is this one:
https://github.com/godotengine/godot/pull/54205/commits

@lyuma
Copy link
Contributor

lyuma commented Mar 25, 2022

@macryc would it be possible for you to provide a reproduction case / demo project possibly as a separate bug so it can be tracked more easily.

Take note that Godot's skeleton bone visualization is extremely confusing because rather than drawing bones, they draw lines connecting each bone to its parent, so the "scale" and rotation of each joint will appear to change as they move, especially noticeable on root motion bones.

@TokageItLab TokageItLab deleted the fix-blending branch May 23, 2022 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment