-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
[3.x] Additive Animation Fix (and Sub2 node feature) #76310
base: 3.x
Are you sure you want to change the base?
Conversation
It is a known problem that Add2 in 3.x is broken, but I am skeptical that Sub2 is needed for 3.x because 3.x pose already stores the result of the subtracted by rest. In 4.x, this is no longer done. See also Animation data rework for 4.0. So, in 4.x, it adequate makes sense for Sub2 to exist for pre-calculation of Add2, but for 3.x, a filter may be sufficient. The reason this is necessary in 3.x is, as mentioned the above article, for skeletons that do not have a rest (they have a T-pose separate from the rest) such as the model exported Maya. Then, if you want to implement Sub2, we need to decide if Sub2 should be implemented in 4.x at first. |
Oops typo. I meant to write reference pose not rest pose. I've edited the article to remove the reference to rest pose and also included this helpful resource:
That is outside of my level of understanding. I've never used 4.x and am unfamiliar with that project. I am just trying to fix an old pain point in Godot 3, Im not sure what 4.x has to do with my commit. My promotion of Sub2:
|
Sub2 was discussed once in the following but it has been limbo. godotengine/godot-proposals#2700 (comment) However, in 3.x, it should not be necessary, at least in the case of importing a model with rest. I have explained the following matters several times:
And in fact, the animation stored in glTF has no concept of rest, and stores the same value as the Pose in Godot 4. But in Godot 3, if the skeleton bones have data that can be read as BoneRests, Godot 3 glTF importer subtracts the rests from the animation data and imports the processed values as BonePoses.
This means that Godot 3.x glTF importer has already done the work that Sub2 should have done. (But it may be useful in cases like Maya where you do not export data as a BoneRest to glTF ≈ the default pose and rest are different, or if you want to subtract additional data from it ..well the latter is a corner case) In any case, at this time, we need the step to implement the new features in Godot 4 first and then follow the procedure of backporting to Godot 3, so if you are in a hurry to implement Add2 Directly option, Sub2 should be removed from this commit PR. If not hurry, we need a PR to implement Sub2 to Godot 4. I think that it would make sense to implement it to Godot 4. (Although it looks like the amount parameter is lacking. And also implementing enum constants for Sub as a special case looks like a not good way) |
@TokageItLab I'm not sure what rest poses have to do with Sub2. You are correct rest poses are not at risk of being added twice. Let me know what you think. |
In other words, it is convenient to be able to use any pose as a reference, but in Godot 3, Sub2 should not be necessary if the desired pose is imported as a rest. I don't think there are many cases where a model has multiple reference poses and switches between them. In other words, in Godot 3, the case of needed it is basically a corner case where an incorrect rest is imported (means something is wrong in the glTF), which makes testing difficult. And, you might intend to animate and synchronize reference poses, but in the first place, Godot 3's blend calculation and synchronization breaks down with complicated chained AnimationNodes, so that is also difficult to test. However, as noted above, in Godot 4, the importer does not consider rests, so the expectation is that you basically need to use Sub2 to precompute Add2. This case would make the test result and what it want to do more clear. |
Sub2 gives users the ability to add as many reference poses as they like with as many Add2 nodes as they like without relying on the whims of whatever 3D file format or whichever 3D editing software.
I disagree that a character needing multiple reference (also called base) poses would be a corner case. In my example I use 1 reference pose which is the "Pistol Aim". That is, using one arm to aim a pistol and the other arm is posed on the characters hip. |
What do you want to subtract from each weapon pose? If you don't need the lower body animation, then the filter is sufficient, and if the addition of the upper body for each animation doesn't work correctly, then your glTF rest may be wrong. At least that should be the case with Godot 3. As I have said many times, the Bone Pose in Godot 3 is already the result of the Rest subtracted. If rest is equal to the reference pose, then the result of blending the weapon animation with the Directly option in Add2 to the reference pose will be equal to Blend2. The mainly need for Add2 is not for such weapon switching, but for inertial transitions such as adding a knockback animation to the original animation, or swinging the arm up when switching weapons, which would be unnatural with Blend2. However, the problem here is that when creating the animation of the weapon's recoil, the arms of each weapon face a different direction from the rest, and the difference between the rest and the direction of the arm holding the weapon will be added by Add2. That would be understandable. |
This is all I want to say. You may have misunderstood, I am not against adding of Sub2. Rather, I am in favor of adding it. But it is not possible to add it to Godot 3.x first. If the new feature is added to Godot 3.x first, the 3.x Project will be incompatible with the 4.0 Project and will generate a new issue of breaks compatibility. So, the only case to add a new feature directly to Godot 3.x should be if it is something that is only a problem in 3.x. The lack of Sub2 is still a problem in Godot 4, and Sub2 is rather more often needed in Godot 4 than in Godot 3.x.
In Godot 3, if the animation to be subtracted by Sub2 is not Rest, the Pose will contain a value other than Delta, which can cause problems. For example, if you want to take a Delta animation from an idle animation, even if the first frame of the idle animation is the same as Rest, the Delta animation after the second frame is not correct, which is a problem. Conversely, if the idle animation is always stationary and it is equal to Rest, it means that the Godot 3.x importer has already extracted the Delta animation. Godot 4 considers Rest to be just an initial value of Pose, so the importer imports the animation value without any processing. Concluded: Godot 3
Godot 4
Also, Godot 3.x cannot create complex AnimationTree because the calculation and Sync are completely broken before the need for Sub2 and AddDirectly. The broken calculations were not just an AddDirectly problem. For example, there were strange calculations that produced different results depending on how the nodes were connected, even though the same blend calculations were being performed. That fix includes huge breaking compatibility, so it can't be backported to 3.x. Also, sync was broken for some nodes, and broken Sync meant that the Delta animation calculation was also broken. For these reasons, it is difficult to perform complex AnimationTree tests with Godot 3.x. So Sub2 needs to be added to Godot 4 first. If you want, I will send a PR for Godot 4 later. |
I misunderstood, sorry. I understand the issue a little better now thank you.
That sounds good thanks. |
I sent #76616, please check it to know whether it satisfies what you intended it to do. |
#76616 has been merged and can be backported. However, this #76310 PR contains some distinctive implementations and will need to be as consistent with #76616 as possible. For example, the enum This means that in Godot 3.x, you need to implement the |
@TokageItLab Sorry I have been dragging my feet adding these changes. I did not do the math but now I've done my homework haha Here is my understanding for critique and documentation. 3.X GENERIC BLEND (CURRENT)t->loc = t->loc.linear_interpolate(loc, blend);
if (t->rot_blend_accum == 0) {
t->rot = rot;
t->rot_blend_accum = blend;
} else {
float rot_total = t->rot_blend_accum + blend;
t->rot = rot.slerp(t->rot, t->rot_blend_accum / rot_total).normalized();
t->rot_blend_accum = rot_total;
}
t->scale = t->scale.linear_interpolate(scale, blend); PROPOSED: ADD DIRECT// NOT the same as generic with positive blend
t->loc += loc * blend;
// same as a generic with positive blend
t->scale = t->scale.linear_interpolate(scale, blend);
// maybe same as generic with positive blend?
Quat q = Quat().slerp(rot.normalized(), blend).normalized();
t->rot = (t->rot * q).normalized(); PROPOSED: SUBTRACT// NOT the same as generic with negative blend
// same as ADD direction with -blend!
t->loc -= loc * blend;
// same as generic with negative blend
t->scale = t->scale.linear_interpolate(scale, blend);
// maybe same as generic with negative blend?
Quat q = Quat().slerp(rot.normalized().inverse(), blend).normalized();
t->rot = (t->rot * q).normalized(); Solution explanation:The location is the only variable that needs special attention for add direct and sub. We add a single flag. Called use_blend which if true will perform the generic 3.x location blend, the default:
// If use_blend is false we will use this proposed location blend:
PROPOSED UPDATED 3.X GENERIC BLENDif (use_blend) {
t->loc = t->loc.linear_interpolate(loc, blend);
} else {
t->loc += loc * blend;
}
if (t->rot_blend_accum == 0) {
t->rot = rot;
t->rot_blend_accum = blend;
} else {
float rot_total = t->rot_blend_accum + blend;
t->rot = rot.slerp(t->rot, t->rot_blend_accum / rot_total).normalized();
t->rot_blend_accum = rot_total;
}
t->scale = t->scale.linear_interpolate(scale, blend); I hope I have understood everything correctly. I think the most annoying part of all this is finding out a nice way to get the use_blend flag into the _process_graph function. The recursion hurts my head. EDIT:We also need to do rotation different. This becomes: if (as.use_blend) {
blend = Math::absf(blend); // If negative rot_total could be 0, causing divide by 0 errors, inf numbers.
t->loc = t->loc.linear_interpolate(loc, blend);
if (t->rot_blend_accum == 0) {
t->rot = rot;
t->rot_blend_accum = blend;
} else {
float rot_total = t->rot_blend_accum + blend;
t->rot = rot.slerp(t->rot, t->rot_blend_accum / rot_total).normalized();
t->rot_blend_accum = rot_total;
}
} else {
// Direct addition / subtraction
t->loc += loc * blend;
Quat q = Quat();
q = q.slerp(rot.normalized(), blend).normalized();
t->rot = (t->rot * q).normalized();
}
t->scale = t->scale.linear_interpolate(scale, Math::absf(blend)); |
0e29a7e
to
8bab987
Compare
scene/animation/animation_tree.cpp
Outdated
// If an animation node sets use_blend to false it must remain false | ||
// this is a way to make it pass down the tree. Without this | ||
// the next node, which will often set use_blend to true, will | ||
// override our request for use_blend false. | ||
if (p_use_blend) { | ||
// use self | ||
p_node->use_blend = this->use_blend; | ||
} else { | ||
// use parameter | ||
p_node->use_blend = p_use_blend; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate this hack. If anyone can find a better way, please make a PR / comment.
I am trying to pass a variable from an animation node to _process_graph, where the blending occurs.
How this works:
- sub/add2 animation node calls blend_input
- blend_input calls blend_node
- blend_node sets use_blend in the input animation node
- animation node leaf (animation/blendspace) adds animation state to the blend tree, adding its use_blend variable also.
The issue: In between steps 3. and 4. we may travel through many other nodes and the use_blend flag will be overwritten. This code segment will avoid overwriting use_blend with true if it is already false. Allowing our modified use_blend to pass down the tree and into _process_graph.
There has to be an easier way. I tried this way because I tried to modify the least amount of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead just use:
p_node->use_blend = p_use_blend && use_blend;
(No this
needed either)
Adds the Add2 option: Add directly. Default behaviour is current behaviour to preserve backwards compat. New node Sub2 subtracts pose. Doesnt change any existing nodes. This is a messy commit and news some polish but is is a good first draft. Use blender NLA to test what additive animations should look like :-)
Tested with a blend tree with blend spaces as leaves instead of animation nodes. This means we have to pass use_blend flag to animation state inside _blend_node instead of blend_input.
Oops. How do I fix it? |
You would need to do a rebase and drop the merge commit you made and instead rebase on top of 3.x, like so: If it doesn't work I can help in a bit but have to go |
521341e
to
1c6b820
Compare
There, fixed it up 🙂 |
scene/animation/animation_tree.cpp
Outdated
// If an animation node sets use_blend to false it must remain false | ||
// this is a way to make it pass down the tree. Without this | ||
// the next node, which will often set use_blend to true, will | ||
// override our request for use_blend false. | ||
if (p_use_blend) { | ||
// use self | ||
p_node->use_blend = this->use_blend; | ||
} else { | ||
// use parameter | ||
p_node->use_blend = p_use_blend; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead just use:
p_node->use_blend = p_use_blend && use_blend;
(No this
needed either)
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
Co-authored-by: A Thousand Ships <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use_blend
should not be added to AnimationNode
but to AnimationNodeAdd2
, AnimationNodeAdd3
and AnimationNodeSub2
each as a replacement for add_directly
.
@TokageItLab Explanation: The use is seen here: Personally I don't like using a variable like this because I avoid recursion and the code above seems hacky but currently I have not figured out a better way to set |
I cannot understand your intention at all, but it definitely should not be necessary to have both use_blend and add_directly. At least in your previous commit, only add_directly was there and it worked. By the way, Godot4 added a Deterministic option recently that reproduces Godot3's add2 (when Deterministic = false), so I think the best solution would be to backport that if you are possible. |
Godot 4 has a completely different animation system. It's solutions are not compatible with Godot 3. There is no commit where |
Yeah, that is confusing. Indeed, there have been additional members as arguments in past commits, but I'm not sure if it really needs to be an argument. Maybe one way to do it would be to add a Normalized property to the AnimationTree. Perhaps it could be migratable to some extent to the Deterministic option (as Deterministic = !Normalized). Without implementing Normalized option, the missing options for Add3 and Sub2 need to be fixed for consistency. |
Adds the Add2 node option: Add directly.
Default behavior is current behavior to preserve backwards compatibility.
Demonstration:
Issue premise:
Additive animation is where two animations are added together.
Normally animations are blended together which means interpolation between poses.
Additive animation adds poses. We can control how much we add by blending the pose being added
between the identity pose and itself.
Godot has had an Add2 node in its animation tree for years however this does not add poses, it blends them.
Why this matters:
Additive animation is how your animated character plays two animations at once.
For example:
In every shooter your character can look up and down, the body moves its aim up and down.
How this works under the hood is that we have an aiming forward pose and we are adding the aim up/down pose.
Why not blend? Imagine the player is running, and breathing, and reloading, while looking up. To accomplish this with blends
you could filter the blends per bone but that looks rubbish. In practice these animations are added, sometimes referred to as layered.
In our example we play the running animation and blend the torso into the aiming pose, we then add the aiming pose (Unreal calls these aim offsets), then we add the reload animation if it is playing, then we add the breathing loop.
All these adds combine to a single animation that is expressive, dynamic, and natural.
About this commit:
This commit adds a new option to the Add2 node allowing users to add directly which does add poses as expected.
It also adds a new animation tree node called Sub2, for the creation of delta animations, often used in conjunction with Add2.
delete_me.mp4
In this commit:
Why add a new animation node called Sub2?
If we directly add two animations we add every pose directly. So even poses that are the same in both animations get added!
Instead we add the delta between two animations by first subtracting a mutual pose from the animation being added.
Reference:
http://guillaumeblanc.github.io/ozz-animation/samples/additive/
EDIT: The term rest pose has a meaning already and I don't want to confuse the use of Sub2. I also added a helpful reference.
Notes:
edit: removed 'draft'.