-
-
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
Fixed incorrect blending in Pos/Rot/Scl Track #54205
Fixed incorrect blending in Pos/Rot/Scl Track #54205
Conversation
6af53f5
to
36337c0
Compare
Okay, I checked the second video again and found that the rotation was broken, but I fixed it. It needed to include #53903 because it breaks the rotation. 2021-10-25.7.41.22-1.mov |
#53903 fixes inconsistent rotation blending order, but does not fix additive blending correctly, and more fixes is needed to get the animations to add correctly in NodeAdd2. For now, this PR will fix the inconsistent rotation order and fix the bug that causes weird blending. |
I think this PR is reviewable, but it doesn't solve the triangle nonlinear blending problem that marstaik mentioned in #34134, also this comment problem is don't so. They are already broken in master, regardless of this PR, and I feel that some overhauls of the blending algorithm is needed. In order to implement NodeAdd2 and NodeSub2 (BaseAnimation extraction) correctly, I think the quaternion process needs to be changed as needed. Probably the fundamental reason why the blend order is broken is that the number of times the blend value is multiplied in |
36337c0
to
d0bebf8
Compare
I can confirm that this PR fixed the issue! Before: animrot.mp4After: fixed.mp4 |
1f8a1cb
to
78f5ce0
Compare
@reduz Here is a summary of what this PR is meant to do. The initial Transform3D value of the blend is different for root motion and non-root motion.
Due to the PR in #53689, the initial values of all the blend animations were wrongly set to the same as the root motion; In other words, it never set the key correctly after the decision that it is not the root motion. As a result, the current blend is broken. This PR fixes it. |
78f5ce0
to
7e632c9
Compare
This also fixes the xfade time in the state machine for me. |
That may be a side effect of removing |
The current solution breaks root motion by initializing scale to zero except for tracks with scale |
@AThousandShips Are you talking about 4.0? That probably won't fix this bug. It is caused by a wrong initialization method other blends than root motion, so we need to implement the correct initialization method. |
@TokageItLab sorry meant this PR breaks root motion compared to the unchanged code, but correcting initialization seems necessary, thank you for clearing up it was noticed |
@AThousandShips Can you send a test project or video for root motion? If this PR breaks the root motion then it needs to be fixed. |
7e632c9
to
e6507df
Compare
e6507df
to
9af4d73
Compare
@AThousandShips Ah sure, I confirmed that it breaks, the root motion needs to initialize all the TRS values, so the previous initialization method seems correct for the root motion. |
@TokageItLab going over the updates and that's the solution I arrived at in my own usage of this PR before I joined, so I can confirm it has worked in my own usage, not familiar with the rest of the animation system but this appears to fix the blend issue while keeping root motion correct |
Is this ready? I'll kindly ask the relevant people. |
It has been ready! |
Pending; possibly, could be resolved in a more correct way by eliminating lerp() with reference to #46038 (comment). |
#46038 (comment) has a problem in Quaternion, I don't know if that's the problem reduz was saying in #34134 (comment), but I think we need to brush it up. Re-ready this once. |
Since #57675 is more stable now, this PR seems no longer necessary. |
Fixed #54407.
Fixed #55838.
Follow up of #53689. The process of
if (t->process_pass ! = process_pass)
needs to be separated between the root motion and others.Before:
2021-10-25.6.48.51-1.mov
After:
2021-10-25.6.44.44-1.mov