-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Conversation
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. |
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. |
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. |
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. |
I agree. I have no issues with the current proposed changes as I'm not a rotation calculation expert by any means. |
What results did slerpni() give? |
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. |
baceda1
to
6fdb7de
Compare
|
f62dc10
to
ed7a327
Compare
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.
Also, the accumulator probably does the similar thing as slerpni(), which could create unintended rotations. I finalized using slerp() as a tentative solution. |
8b63eb3
to
262be1e
Compare
3b3fddc
to
b06d253
Compare
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. |
Thanks! 🎉 |
At last. Is this merge going into alpha 5? Assuming so, when's alpha 5 due out? |
The issue was closed on the basis that the y position issue of the root bone during blending between animations got fixed. The only commit so far that fixes the issue properly is this one: |
@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. |
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.