-
-
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
New and improved IK in Skeleton3D #39353
New and improved IK in Skeleton3D #39353
Conversation
Reserved, in case the opening post gets too big |
e95e821
to
a979dbd
Compare
Updated to the latest version of the master branch, hence the force push Edit 06/11/2020 - Updated branch to use the latest changes in #39126 (requiring a force push) and pushed initial CCDIK implementation |
a979dbd
to
bcadecd
Compare
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 haven't looked deeply into style but the changes look good overall. We need Juan's input on a couple of things, namely the new Quat methods and the fact that skeleton modifications are a Resource.
I think the Resource usage is fine, but better ask now than have the work rejected at the end ;)
core/math/quat.cpp
Outdated
|
||
return; | ||
} | ||
} |
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.
This needs Juan's approval. While the functions are useful, they may be not general enough to be added in Quat
itself. If that's the case we will have to move them to where they are used (local "hacks" have priority over global bloat).
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've been using these definitions from my peer. Hope it's helpful.
Vector<IKQuat> IKQuat::get_swing_twist(Vector3 p_axis) {
Vector3 euler = get_euler();
const real_t d = Vector3(
euler.x,
euler.y,
euler.z)
.dot(Vector3(p_axis.x, p_axis.y, p_axis.z));
set(p_axis.x * d, p_axis.y * d, p_axis.z * d, w);
normalize();
if (d < 0) {
operator*(-1.0f);
}
IKQuat swing = IKQuat(-x, -y, -z, w);
swing = operator*(swing);
Vector<IKQuat> result;
result.resize(2);
result.write[0] = swing;
result.write[1] = IKQuat(x, y, z, w);
return result;
}
void IKQuat::clamp_to_quadrance_angle(real_t p_cos_half_angle) {
real_t new_coeff = 1.0f - (p_cos_half_angle * p_cos_half_angle);
real_t current_coeff = y * y + z * z + w * w;
if (new_coeff > current_coeff) {
return;
} else {
x = x < 0.0 ? -p_cos_half_angle : p_cos_half_angle;
real_t composite_coeff = Math::sqrt(new_coeff / current_coeff);
y *= composite_coeff;
z *= composite_coeff;
w *= composite_coeff;
}
}
void IKQuat::clamp_to_angle(real_t p_angle) {
real_t cos_half_angle = Math::cos(0.5f * p_angle);
clamp_to_quadrance_angle(cos_half_angle);
}
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.
Thanks @fire!
editor/editor_properties.cpp
Outdated
spin[8]->set_value(p_transform.basis[2][2]); | ||
spin[9]->set_value(p_transform.origin[0]); | ||
spin[10]->set_value(p_transform.origin[1]); | ||
spin[11]->set_value(p_transform.origin[2]); | ||
setting = false; | ||
} | ||
|
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.
There is a shared commit between this PR and #39126. I don't know if that will cause merging issues but it's something to be aware of. Maybe @akien-mga knows if git will play along nice with that or not.
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'll rebase and remove the commit from this PR once #39126 is merged, so there will be no conflicts. I just wanted it in this PR so I could use the helper functions.
return true; | ||
} | ||
#endif //_3D_DISABLED | ||
|
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.
As a convention, properties use lower case snake_case
.
void SkeletonModificationStack3D::_get_property_list(List<PropertyInfo> *p_list) const { | ||
for (int i = 0; i < modifications.size(); i++) { | ||
p_list->push_back( | ||
PropertyInfo(Variant::OBJECT, "Modifications/" + itos(i), |
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.
Same here, properties should be lower case.
} | ||
|
||
void SkeletonModificationStack3D::execute() { | ||
if (!is_setup || skeleton == nullptr || !enabled || is_queued_for_deletion()) { |
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.
May be worth adding an ERR_FAIL_COND_MSG()
explaining what is going on. Unless it's expected to happen often and it's not really an error. Same with all the state checks in this file.
return instantly_apply_modification; | ||
} | ||
|
||
// CCDIJ joint data functions |
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.
CCIDK
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.
Whoops! I'll fix that 😅
core/math/quat.cpp
Outdated
@@ -273,8 +273,7 @@ void Quat::rotate_from_vector_to_vector(const Vector3 p_from, const Vector3 p_to | |||
real_t dot = v0.dot(v1); | |||
|
|||
if (dot >= 1.0 || dot <= 0) { | |||
// cannot do anything! Print an error and return |
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 guess this makes sense, I don't see why this should be an error
GDCLASS(SkeletonModification3D_LookAt, SkeletonModification3D); | ||
|
||
private: | ||
String bone_name; |
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.
Useful to be StringName for performance reasons of a pointer compare vs string compare?
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.
bone_name
is just used to get the bone index using the find_bone
function in Skeleton3D, and in a couple conditions. The function returns a String, so I figured I should store it as such. Would storing it as a StringName be more performant?
I tested with switching it to a StringName and it compiled okay, but it wasn't properly getting the bone index from the find_bone
function. To be fair though, I only changed the type to a StringName and fixed a couple errors, so it is possible that something needs adjusting for it to work as a StringName and I missed that.
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.
Do you need to compare the whole thing or character by character?
For comparison of whole strings, a StringName is faster.
You probably need to cast back to a string.
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.
Looking at the code, it looks like the only comparison is for checking if it is an empty string (if bone_name == ""
or equivalent), and this is only in one of the two modifications (LookAt).
Thinking about it, I can probably remove the condition altogether from the modifications, making it just for storage and use in the find_bone
function.
edit: I removed the condition and everything seems to be working fine. Now bone_name
is just being used for data storage and for the find_bone
function.
|
||
private: | ||
String bone_name; | ||
int bone_idx; |
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.
Not variable constructor initialized which is the new Godot Engine master style.
core/math/quat.cpp
Outdated
@@ -248,3 +248,51 @@ void Quat::set_axis_angle(const Vector3 &axis, const real_t &angle) { | |||
cos_angle); | |||
} | |||
} | |||
|
|||
// Decomposes a Quaterian into a swing and twist, where twist is the rotation around the |
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.
Typo. Quaternion.
I agree that this (get_twist_quat and get_swing_quat) should be in the quaternion class and not as a subclass.
doc/classes/Skeleton3D.xml
Outdated
</argument> | ||
<description> | ||
Takes the given bone pose/transform and converts it to a world transform, relative to the [Skeleton3D] node. | ||
|
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.
Spaces are different in documentation. I think this causes an extra space. Please check documentation text.
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.
6060855
to
42f0295
Compare
Rebased with the latest version of master, which removes the overlap with #39126. |
8dc3b49
to
359b6b2
Compare
You can use ERR_PRINT_ONCE. |
Godot's PI constant is Math_PI in Edited: Had to replace PI and PI_2 with Math_PI and Math_PI / 2.0. |
Godot Engine coding style uses PascalCase.
There's an extra underscore. |
Yeah, that might not be a bad idea for now. Ideally, I just need to fix the issue that causes that condition to fail, but until then, having an error print couldn't hurt.
Awesome! That should fix AppVeyor not building then. I'll make the necessary adjustments.
Yeah. The only issue I have with using PascalCase with the modifications is that it makes it hard to read. For example:
That said, I probably should change it, to stay consistent. I might leave it for a bit, just to make sure I do not make too many big (potentially breaking) changes at once. I will add renaming the classes to PascalCase to the TODO list though! |
Missed one :D .\scene/resources/skeleton_modification_3d.h(190): error C2065: 'M_PI': undeclared identifier |
Good eye! I'll fix it in the next commit :) |
Good news, only formatting changes! Not errors. CICD green go-go. |
Great! Now I just need to get FABRIK fixed (right now it's not working), implement two bone IK, fix the naming issue, and then I can open this PR up and start fixing bugs 😄 Edit: FABRIK should be fixed now! The jiggle modification was also broken, and now it should also be fixed. I'm not entirely sure what broke or why, but undoing a few things and implementing (and using) |
I've been using the term effector to mean tip. Rotation in a 360 fashion is named rotation While rotation on the axis is called twist. Naming things gets confusing with all the terms. |
* SkeletonModification3DTwoBoneIK: Fixed issue that was preventing two bone IK from working without a tip bone set. * SkeletonModification3DTwoBoneIK: Fixed issue that caused the joint to not always correctly take the pole vector node into account when rotating. * Skeleton3D: Fixed issue where the modification stack looses reference to the Skeleton3D when saving. Now it works in the editor, even when saving (yay!). * Removed uneeded TODOs from the code in SkeletonModification3D.
* SkeletonModification3DFABRIK: Fixed issue that caused major jittering issue when the tip bone was too close to the origin bone. This also made FABRIK chains with just two bones impossible. This is fixed now, and shouldn't be an issue anymore for chains of any length. * While a minor change, pinning down what was causing the issue and fixing it took hours... I'm glad its fixed though! * SkeletonModification3DFABRIK: Removed the forward pass function and code, as it wasn't really doing anything. This is because the rotation application function, rotation_apply, basically handles the forward pass by setting the bone's local poses back to origin. * This should give a slight performance boost with no change visually. * Technically it should be called BRIK (backwards-reaching-inverse-kinematics) now though ;)
* BoneAttachment3D: Rewrote the class and its implementation so it does not have to be so tightly coupled with the Skeleton3D node, making it more standalone. * This is done by using a callable/signal, which the Skeleton3D emits when the pose changes. * A new virtual function was added, on_bone_pose_override, which can be overriden in GDScript. This allows users to have more control over how the node works, if they desire, like adding offsets for example. * BoneAttachment3D: Added and exposed the bone index. Now you can select the bone you want to use either through the dropdown or by setting the bone index directly. * There should also be a *slight* performance boost, as the bone attachment no longer needs to call find_bone every time it binds/unbinds. * Skeleton3D: Removed all of the functions relating to binding nodes to children bones. * I thought about trying to make deprecated/transition functions, but there really isn't any great way to do so that I could think of. However, I do not think many users are using these features anyway, and if they are, transitioning should be fairly seamless. * Skeleton3D: Now a signal, bone_pose_changed, is emitted when the bone pose is calculated. This is needed for the BoneAttachment node, so it always stays up to date (required for IK like CCDIK and FABRIK). * By exposing this as a signal though, it means developers can also use this signal if they need, which could be handy. * SceneStringNames: Added new signal name called bone_pose_updated. This signal is needed to decouple the BoneAttachment from the Skeleton3D. * PhysicsBody3D: Removed the single line of code that used unbind_child_node_from_bone (which didn't seem to be needed, as removing it and not replacing it didn't do anything to functionality that I could see)
* BoneAttachment3D: Added an override property and several override modes. These allow the user to set the bone poses from the BoneAttachment node. * This makes the BoneAttachment node more useful overall, allowing for modifying the bone when needed. * This property is disabled by default, so all projects already using the BoneAttachment3D node can migrate without any issues or modifications needed. * The new property may help eliminate confusion, as it is more obvious that the BoneAttachment node does NOT override the bones by default because the property is disabled. * The modes are the following: global_pose_override, local_pose_override, and custom_pose. Modes for bone_pose and bone_rest could be added in the future, if wanted/needed. * When in local_pose_override or custom_pose mode, the BoneAttachment node will correctly position its transform when parent bones change. When set to global_pose_override mode, the BoneAttachment3D will not react to parent bone transform changes.
* BoneAttachment3D: Added the option to use an external skeleton! Now BoneAttachment3D nodes do not have to be a child of a Skeleton3D node to function. * Because of this, scenes instanced from 3D files do not necessarily have to loose all their BoneAttachment3Ds on reimport, as the BoneAttachment3D nodes can now be outside of the instanced scene. * The property that controls whether an external skeleton is used is turned off by default, so existing BoneAttachment3D nodes are not affected. * BoneAttachement3D: Moved the override properties and the external skeleton properties to the property list, so their properties dynamically appear/disappear as the options are used. * BoneAttachement3D: The signal/callable binding is now a deferred call, as it would crash when reparenting otherwise.
* RegisterSceneTypes: SkeletonModification3D is now registered as a normal class, instead of a virtual one. This was required for extending the class via GDScript. * SkeletonModificationStack3D: Exposed the get_skeleton function to GDScript. This is required for making custom SkeletonModification3D resources from GDScript. * SkeletonModification3D: Added functions to get the modification stack it is associated with and to get whether the modification is setup. * SkeletonModification3D: Made adjustments so the SkeletonModification3D can actually be extended and used in GDScript. Now it is fully possible to make GDScript SkeletonModification3D resources that work with the rest of the IK changes. * I tested this by making a GDScript modification in my 3D GSOC test project, test scene nine. It works great, though there are other issues I need to fix before the example will fully work. * These changes will need to be ported to the 2D IK PR. * BoneAttachment3D: Fixed issues where override poses were not being properly applied on scene load. * BoneAttachment3D: Removed code that synced BoneAttachment3D node transforms when set to override the bone pose. Instead, BoneAttachment3D nodes that are intended to follow parent bones should be children of a BoneAttachment3D that is following the parent bone. * This change simplified the code, and fixed a lot of the issues pose overrides were having. * Using a parent BoneAttachment3D is also more in line with how the Bone2D and RemoteTransform3D nodes work.
* SkeletonModification3DTwoBoneIK: Added roll/twist support for both joints. This is needed to get nice results, as TwoBoneIK does not take the bone's current roll/twist into account when solving. * SkeletonModification3DTwoBoneIK: Moved joint properties into their own sub-category, so its similar to the other IK modifications and so there is less visual clutter. * BoneAttachment3D: When set to use an external Skeleton3D, the BoneAttachment3D node will attempt to automatically use the its parent BoneAttachment3D's Skeleton (assuming its parent is a BoneAttachment3D). This makes setting up BoneAttachment3D nodes easier. * BoneAttachment3D: The BoneAttachment3D will attempt to automatically bind and position itself when set to use a external Skeleton3D node.
* Skeleton3D and SkeletonModificationStack3D: When duplicated, the Skeleton3D makes a unique SkeletonModificationStack3D and the modification stack makes unique SkeletonModification3Ds. This allows for instancing scenes using IK. * SkeletonModification3D: Added a property that defines whether the modification will execute in _process or _physics_process. * Now you can mix and match process modes! This is helpful for when you want physics aware modifications to mix with non-physics aware modifications. Currently this really only applies to the Jiggle modification. * Skeleton3D and SkeletonModificationStack3D: Changed how modifications are executed to allow for per-modification execution modes * SkeletonModification3DStackHolder: added a modification that allows for adding additional modification stacks. This allows for mixing and matching modification stacks together, if needed/wanted. * I'm not sure how much use it will get, but for those who want to use multiple modification stacks, now it should be possible via this modification. * Known limitation: the stack holder can only execute in a single mode, process or physics process, and therefore only modifications in that mode will be executed by the scene holder. Mixing and matching execution modes with the modification stack holder is currently not supported due to the complexity it would bring. * RegisterSceneTypes: Registered the SkeletonModification3DStackHolder class.
* VariantCall.cpp: rotate_to_align in Basis is now properly exposed to GDScript. * Basis: The class reference for the rotate_to_align has been added. * All SkeletonModification3D resources: Error and warning printing has been changed so it doesn't spam the console when something goes wrong. The changes are a port of the code made in the 2D IK PR.
* SkeletonModification3DCCDIK: Rewrote CCDIK from the ground up, for the third time. * Fixes the issue where CCDIK was not working at all due to something changing. * Likely still needs adjustments to work with angle constraints (have not tested yet) * This time, I'm hoping it will continue to work moving forward. It seems to be working right now at least. Fingers crossed that this is the last rewrite. * SkeletonModification3DCCDIK: Removed rotation modes, since they are not really needed, they are not actually CCDIK, and they are not really compatible with the way CCDIK works now. * SkeletonModification3DCCDIK: Removed the option to use a custom rotation axis for CCDIK. This was adding code complexity and with the way CCDIK is written now, it would be very hard to support. * Custom rotation axis support is something I haven't seen any CCDIK solver support anyway.
* SkeletonModification3DFABRIK: Added the forward pass back. Turns out, removing it lead to the wiggle instability issue! Now that it is back, FABRIK doesn't wiggle around when the target is getting out of range * SkeletonModification3DFABRIK: Moved chain_apply outside of the while loop when solving FABRIK, since rotation needs to only be applied once. This should help improve performance without any difference, helping offset the slight performance loss of adding the forward pass back. * SkeletonModification3DCCDIK: Fixed angle constraints and now it is working as expected. * Turns out an assumption I was making (or something changed under the hood) was causing the issue. I assumed the get_axis_angle function would return an angle in the range -180 to 180, but this is not the case. The angle returned is always in the range 0 to 180, with the negative value in the axis as the indicator that the angle is over 180 degrees. With this worked around, the angle constraints work as expected. * SkeletonModification3D: Changed how get_modification_stack is defined in SkeletonModification3D. Now it returns a Ref<SkeletonModificationStack3D> and that allows the CI tests to run successfully. * SkeletonModification3D: _print_execution_error now does not print an error to the console if the modification is not setup. * All 3D SkeletonModification resources: Now uses _print_execution_error when checking if setup, so there are not a ton of "errors" when loading just because the resource has not been setup by the Skeleton3D node at start.
* BoneAttachment3D: Now when an external Skeleton is set, it properly updates to show the bone name selection without needing to reselect the node. * BoneAttachment3D: Added a configuration warning to show when the BoneAttachment node is not properly setup. * SkeletonModification3DCCDIK, SkeletonModification3DFABRIK, and SkeletonModification3DJiggle: Joint function names now use the following convention "set_joint_property" instead of "joint_set_property".
…sion of master branch
* Fixed issue where some function parameters did not have p_ at the start of the argument name * Changed the execute and setup function names to _execute and _setup_modification to fit the code base naming convention used throughout the code * Fixed formatting and other code issues based on the reviews from the GSOC 2D IK pull request
…ed year on a few header files
608dcec
to
5d81d17
Compare
Thanks @fire! The PR has been rebased (CI builds pending) and I adjusted the code based on the review feedback 👍 Edit: Reran doctool to fix documentation issues and adjust the new documentation to current styles. |
…ents based on review feedback. Edit: Ran doctool to fix documentation issues
5d81d17
to
08bc048
Compare
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'm having a hard time finding problems, this is good!
- needs to be merged into one pr
- For all of the todos, can you do them? You can also remove them and leave the work for a enhancement proposal? Wanted to resolve them before merging.
I'll run the code through some of the uses and report back. I don't think any demos work anymore.
Thanks. For the single PR, I’ll look at trying to get it into a squashed PR next weekend. For the TODOs, I’d need to look and see, but probably I won’t be able to tackle them and they’ll need to be handled at a later date and/or by someone else. I can at very least pull them out, remove them from the PR and make an enhancement proposal. I’ll look at trying to get this processing going next weekend as well. |
Closing this PR, as I made a squashed version here: #51368 Edit: I culled out the TODOs, updated the proposal including the enhancements, and opened the PR for potential merging. |
This pull request lays the groundwork for the new and improved IK in Godot. This includes behind the scene changes, as well as the IK code itself.
The first 7 commits are squashed changes from the verbose log, bringing the commit count from 91 to just 7. Commits after commit 7 are new changes added on top of the changes in the verbose archive linked above.
Below is a list of everything that has been added, removed, or otherwise changed. The list is in no particular order.
Changes:
clamp_angle
function, taken from the 2D IK PR. This is used by CCDIK to constrain the angles. Potentially, it may be possible to use with other modifications._process
or_physics_process
. Some modifications have options that only are available in_physics_process
.|
|
|
|
|
|
|
|
local_pose_override
.local_pose_override
transform works just like theglobal_pose_override
transform, but the transform is assumed to be a transform relative to the parent rather than a global position.global_pose_override
, for consistency.pose
,rest
, andlocal_pose_override
) to a global bone pose, and vice versa. These functions make working with the bone transforms much easier, especially when used in conjunction with the helper functions added in Skeleton and Skeleton inspector low-level changes #39126.global_pose_override
, calledmodification_pose
, but I found this led to dislocated bones that floated in the air. Additionally, it basically was just anotherglobal_pose_override
. Using the newlocal_pose_override
fixed the dislocation issue and the code is just as usable.global_pose_override
, just uses a local bone pose instead of a global one. Has the same functions and capabilities.|
|
process_list
functions and related code have been removed.|
bone_pose_changed
, which is emitted when the bone changes. From a end-user perspective, the node should function exactly the same as before.on_bone_pose_update
, is virtual. This allows GDScript users to extend it if needed. The reason behind this is that it allows users to offset BoneAttachment3D nodes via GDScript, something I have seen request a few times.bind_child_node_to_bone
, in Skeleton3D have been removed! This is because they are no longer needed, were already marked as deprecated, and were not really used anywhere in the code base. For projects that use or need these functions, the new signal and a tiny bit of code should be usable to replicate the functionality. Technically this is compatibility breaking, but I do not believe very many projects used the binding functions directly.global_pose_override
,local_pose_override
, andcustom_pose
. The remaining poses could be added in the future if wanted/needed with relative ease.local_pose_override
, you can actually directly modify the results of IK in the editor! This gives an interesting level of control over how IK operates that was entirely accidental, but its a cool added bonus feature.|
|
rotate_to_align
to Basisrotate_from_vector_to_vector
function I previously added in Quat, but uses a Basis instead.|
|
Todo:
Convert the SkeletonIK node's code to a SkeletonModification3DNot needed! The SkeletonIK node will likely need to stick around for awhile so projects can be migrated.Note the list above is in no particular order.
Known bugs/errors/problems:
Saving a scene with SkeletonModification3D resources attached to a Skeleton cause them to no longer work in the editor. Functionality returns upon reloading the scene, and this only occurs in the editor.Fixed/Workaround-found!@tool
mode in the Godot editor, as you cannot assign them. If you remove@tool
, you can assign it correctly but it will not work in the editor until you add@tool
, in which case it will work from that point on.The test project I am using can be found here. This project will evolve as I continue to work.
Disclaimer: This work is part of the GSOC 2020 program.