-
-
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
New and improved IK system for Skeleton3D - Squashed! #51368
New and improved IK system for Skeleton3D - Squashed! #51368
Conversation
8e99c0d
to
04f381b
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.
Approved, there are improvements for the user interface of the individual IK solvers to be done, but those are future improvements that require design work.
The goal is to get further testing to flush out the bugs in the IK modifier.
I'll try to get some demos created for the group, but isn't blocking this pr.
Is it fine to take the demos from your GSOC IK github repository and using them for demos?
<description> | ||
Returns the rest transform for a bone [code]bone_idx[/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.
global_pose_to_local_pose description disappeared?
04f381b
to
62e9e8f
Compare
Thanks guys! I adjusted the code based on the review feedback. @fire - feel free to use the demos from the IK repository for demos or whatever, it's all good 👍 |
As discussed with the Godot Engine meeting today. @AndreaCatania will do a final pass, review, approve and we can merge. |
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 are some nit here and there, but the code looks good.
Something to really fix before merge, other than the marked things, are all the float
, that should be real_t
instead: (If you use Visual Studio Code) you can just use crtl+h
to find and replace all those.
Another thing I advice you to fix, but it's not necessary and up to you, is to change the Vector
to LocalVector
, especially on the skeleton modifications: For example, all the SkeletonModification*
classes, are using Vector
, but they are doing heavy modifications on it, and not even using the pointer trick to just access the memory; so each access will result in complex operations. As example, you can check the function: void SkeletonModification3DJiggle::_execute_jiggle_joint(int p_joint_idx, Node3D *p_target, float p_delta) {
.
Refactor that part is not that complex however, you just need to Change Vector
to LocalVector
and remove .write
where you use it, all the other APIs are the same.
Thanks for the review @AndreaCatania! I will try to start incorporating the feedback and making changes as soon as I have the time, likely this weekend. |
36da712
to
08671b5
Compare
I had some spare time today, so I made the changes based on the review! All of the feedback was taken into account and the code modified accordingly. As of when this was written, there is now a few warnings since unsigned integers are being compared to integers due to the changes from LocalVector to Vector. I will see if I can resolve them though once I have a chance, but everything compiles. Edit: Fixed the issues. Now compiling does not present the errors. |
08671b5
to
b981368
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.
Many thanks for the LocalVector
usage, few floats
remained to be changed and few other changes on the return types are needed.
Please let me know if you have any question or you need help, feel free to ping me on Godot Chat.
scene/3d/skeleton_3d.cpp
Outdated
|
||
return bones[p_bone].parent; | ||
} | ||
|
||
void Skeleton3D::set_bone_rest(int p_bone, const Transform3D &p_rest) { | ||
ERR_FAIL_INDEX(p_bone, bones.size()); | ||
Vector<int> Skeleton3D::get_bone_children(int p_bone) const { |
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 function should now return const LocalVector<int> &
, otherwise you will convert a LocalVector
(bones[p_bone].child_bones
) to a Vector
, which is bad. If you can't convert that, I suggest to replace the bones[p_bone].child_bones
type and use Vector
again.
Of course, depending how you solve the issue you have to consider to also change all it's usage:
// From
Vector<int> child_bones = get_bone_children(p_bone);
// To
const LocalVector<int> &child_bones = get_bone_children(p_bone);
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 converted it back to a Vector<int>
, as something with exposing the LocalVector properties was causing issues. Here's what the errors looked like:
[ 86%] Compiling ==> scene/3d/skeleton_ik_3d.cpp
In file included from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/object/method_bind.h: In instantiation of 'Variant::Type MethodBindTR<R, P>::_gen_argument_type(int) const [with R = LocalVector<int>; P = {}]':
./core/object/method_bind.h:416:24: required from here
./core/object/method_bind.h:420:27: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
420 | return GetTypeInfo<R>::VARIANT_TYPE;
| ^~~~~~~~~~~~
./core/object/method_bind.h: In instantiation of 'PropertyInfo MethodBindTR<R, P>::_gen_argument_type_info(int) const [with R = LocalVector<int>; P = {}]':
./core/object/method_bind.h:424:23: required from here
./core/object/method_bind.h:430:41: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
430 | return GetTypeInfo<R>::get_class_info();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
./core/object/method_bind.h: In instantiation of 'GodotTypeInfo::Metadata MethodBindTR<R, P>::get_argument_meta(int) const [with R = LocalVector<int>; P = {}]':
./core/object/method_bind.h:440:34: required from here
./core/object/method_bind.h:444:27: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
444 | return GetTypeInfo<R>::METADATA;
| ^~~~~~~~
./core/object/method_bind.h: In instantiation of 'Variant::Type MethodBindTR<R, P>::_gen_argument_type(int) const [with R = LocalVector<int>; P = {int}]':
./core/object/method_bind.h:416:24: required from here
./core/object/method_bind.h:420:27: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
420 | return GetTypeInfo<R>::VARIANT_TYPE;
| ^~~~~~~~~~~~
./core/object/method_bind.h: In instantiation of 'PropertyInfo MethodBindTR<R, P>::_gen_argument_type_info(int) const [with R = LocalVector<int>; P = {int}]':
./core/object/method_bind.h:424:23: required from here
./core/object/method_bind.h:430:41: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
430 | return GetTypeInfo<R>::get_class_info();
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~
./core/object/method_bind.h: In instantiation of 'GodotTypeInfo::Metadata MethodBindTR<R, P>::get_argument_meta(int) const [with R = LocalVector<int>; P = {int}]':
./core/object/method_bind.h:440:34: required from here
./core/object/method_bind.h:444:27: error: incomplete type 'GetTypeInfo<LocalVector<int>, void>' used in nested name specifier
444 | return GetTypeInfo<R>::METADATA;
| ^~~~~~~~
In file included from ./core/object/method_bind.h:34,
from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/variant/binder_common.h: In instantiation of 'void call_with_variant_args_ret_helper(T*, R (T::*)(P ...), const Variant**, Variant&, Callable::CallError&, IndexSequence<Is ...>) [with T = __UnexistingClass; R = LocalVector<int>; P = {}; long unsigned int ...Is = {}]':
./core/variant/binder_common.h:453:35: required from 'void call_with_variant_args_ret_dv(T*, R (T::*)(P ...), const Variant**, int, Variant&, Callable::CallError&, const Vector<Variant>&) [with T = __UnexistingClass; R = LocalVector<int>; P = {}]'
./core/object/method_bind.h:454:32: required from 'Variant MethodBindTR<R, P>::call(Object*, const Variant**, int, Callable::CallError&) [with R = LocalVector<int>; P = {}]'
./core/object/method_bind.h:449:18: required from here
./core/variant/binder_common.h:648:8: error: no match for 'operator=' (operand types are 'Variant' and 'LocalVector<int>')
648 | r_ret = (p_instance->*p_method)(VariantCasterAndValidate<P>::cast(p_args, Is, r_error)...);
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./core/variant/callable_bind.h:35,
from ./core/object/object.h:44,
from ./core/variant/binder_common.h:35,
from ./core/object/method_bind.h:34,
from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/variant/variant.h:679:7: note: candidate: 'void Variant::operator=(const Variant&)'
679 | void operator=(const Variant &p_variant); // only this is enough for all the other types
| ^~~~~~~~
./core/variant/variant.h:679:32: note: no known conversion for argument 1 from 'LocalVector<int>' to 'const Variant&'
679 | void operator=(const Variant &p_variant); // only this is enough for all the other types
| ~~~~~~~~~~~~~~~^~~~~~~~~
In file included from ./core/object/method_bind.h:34,
from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/variant/binder_common.h: In instantiation of 'void call_with_ptr_args_ret_helper(T*, R (T::*)(P ...), const void**, void*, IndexSequence<Is ...>) [with T = __UnexistingClass; R = LocalVector<int>; P = {}; long unsigned int ...Is = {}]':
./core/variant/binder_common.h:501:43: required from 'void call_with_ptr_args_ret(T*, R (T::*)(P ...), const void**, void*) [with T = __UnexistingClass; R = LocalVector<int>; P = {}]'
./core/object/method_bind.h:463:40: required from 'void MethodBindTR<R, P>::ptrcall(Object*, const void**, void*) [with R = LocalVector<int>; P = {}]'
./core/object/method_bind.h:459:15: required from here
./core/variant/binder_common.h:253:21: error: 'encode' is not a member of 'PtrToArg<LocalVector<int> >'
253 | PtrToArg<R>::encode((p_instance->*p_method)(PtrToArg<P>::convert(p_args[Is])...), r_ret);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./core/variant/binder_common.h: In instantiation of 'void call_with_variant_args_ret_helper(T*, R (T::*)(P ...), const Variant**, Variant&, Callable::CallError&, IndexSequence<Is ...>) [with T = __UnexistingClass; R = LocalVector<int>; P = {int}; long unsigned int ...Is = {0}]':
./core/variant/binder_common.h:453:35: required from 'void call_with_variant_args_ret_dv(T*, R (T::*)(P ...), const Variant**, int, Variant&, Callable::CallError&, const Vector<Variant>&) [with T = __UnexistingClass; R = LocalVector<int>; P = {int}]'
./core/object/method_bind.h:454:32: required from 'Variant MethodBindTR<R, P>::call(Object*, const Variant**, int, Callable::CallError&) [with R = LocalVector<int>; P = {int}]'
./core/object/method_bind.h:449:18: required from here
./core/variant/binder_common.h:648:8: error: no match for 'operator=' (operand types are 'Variant' and 'LocalVector<int>')
648 | r_ret = (p_instance->*p_method)(VariantCasterAndValidate<P>::cast(p_args, Is, r_error)...);
| ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ./core/variant/callable_bind.h:35,
from ./core/object/object.h:44,
from ./core/variant/binder_common.h:35,
from ./core/object/method_bind.h:34,
from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/variant/variant.h:679:7: note: candidate: 'void Variant::operator=(const Variant&)'
679 | void operator=(const Variant &p_variant); // only this is enough for all the other types
| ^~~~~~~~
./core/variant/variant.h:679:32: note: no known conversion for argument 1 from 'LocalVector<int>' to 'const Variant&'
679 | void operator=(const Variant &p_variant); // only this is enough for all the other types
| ~~~~~~~~~~~~~~~^~~~~~~~~
In file included from ./core/object/method_bind.h:34,
from ./core/object/class_db.h:34,
from ./core/config/project_settings.h:34,
from ./scene/main/node.h:34,
from ./scene/3d/node_3d.h:34,
from scene/3d/skeleton_3d.h:36,
from scene/3d/skeleton_3d.cpp:31:
./core/variant/binder_common.h: In instantiation of 'void call_with_ptr_args_ret_helper(T*, R (T::*)(P ...), const void**, void*, IndexSequence<Is ...>) [with T = __UnexistingClass; R = LocalVector<int>; P = {int}; long unsigned int ...Is = {0}]':
./core/variant/binder_common.h:501:43: required from 'void call_with_ptr_args_ret(T*, R (T::*)(P ...), const void**, void*) [with T = __UnexistingClass; R = LocalVector<int>; P = {int}]'
./core/object/method_bind.h:463:40: required from 'void MethodBindTR<R, P>::ptrcall(Object*, const void**, void*) [with R = LocalVector<int>; P = {int}]'
./core/object/method_bind.h:459:15: required from here
./core/variant/binder_common.h:253:21: error: 'encode' is not a member of 'PtrToArg<LocalVector<int> >'
253 | PtrToArg<R>::encode((p_instance->*p_method)(PtrToArg<P>::convert(p_args[Is])...), r_ret);
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Based on the error, I think its a missing function or two that doesn't allow it to work with the properties list, from what I can tell. For now, I figure changing it back to a Vector<int>
is probably best since I'm not totally sure what would need to be changed to get it working. (Better to not break something accidentally 😅 )
scene/3d/skeleton_3d.cpp
Outdated
bool Skeleton3D::is_bone_enabled(int p_bone) const { | ||
ERR_FAIL_INDEX_V(p_bone, bones.size(), false); | ||
return bones[p_bone].enabled; | ||
Vector<int> Skeleton3D::get_parentless_bones() const { |
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 as above, this should return const LocalVector &
, since parentless_bones
is now LocalVector
, if you can't do the change, please make sure to make parentless_bones
a Vector
again.
Depending how you solve it, make sure to adjust its usage too.
Vector<int> bones; | ||
Vector<float> weights; | ||
LocalVector<int> bones; | ||
LocalVector<float> weights; |
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.
Nit, this should be: real_t
.
Vector3 v0 = grests[current_bone_idx].origin; | ||
Vector3 v1 = grests[child_bone_idx].origin; | ||
Vector3 d = skel->get_bone_rest(child_bone_idx).origin.normalized(); | ||
float dist = skel->get_bone_rest(child_bone_idx).origin.length(); |
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.
Nit, this should be: real_t
.
Vector3 d = skel->get_bone_rest(child_bone_idx).origin.normalized(); | ||
float dist = skel->get_bone_rest(child_bone_idx).origin.length(); | ||
|
||
// Find closest axis. | ||
int closest = -1; | ||
float closest_d = 0.0; |
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.
Nit, this should be: real_t
.
for (int j = 0; j < 3; j++) { | ||
float dp = Math::abs(grests[parent].basis[j].normalized().dot(d)); | ||
float dp = Math::abs(grests[current_bone_idx].basis[j].normalized().dot(d)); |
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.
Nit, this should be: real_t
.
@TwistedTwigleg Sorry about my previous review, I did a mistake.
These don't need to be |
1318736
to
ce05962
Compare
I changed the code based on the latest review. 👍 One thing I noticed: Now I cannot assign modifications to the SkeletonModificationStack3D when it's assigned to a Skeleton. The debugger doesn't show any errors in the Godot console or anything, it just doesn't add the new modification to the stack in the editor UI, making it impossible to configure the modifications and use them. |
Did you call the scan for new editor updates function? If not you'd have to look away from the godot editor and look back. I don't remember the exact name at this current moment. |
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.
Looks good, Good Job!
Thanks a lot! |
Awesome, thanks guys 😄 |
Would be lovely if you could also record a video (even short) to show the new features in action, and maybe post on twitter :) |
Definitely! I'll try to get something made and posted soon. |
After way too many attempts to post the videos, the tweets are finally up: https://twitter.com/TwistedTwigleg/status/1427430507168600067 Huge thanks to everyone for helping make this possible 👍 |
swapped = true; | ||
} | ||
} | ||
bonesptr[i].child_bones.clear(); |
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.
Doing the process in just one loop and removing the child bones for each bone is causing a regression when the bones are not sorted from root to leaves (if the root is last, all other bones are missing, since it removes its child bones).
Adding a first loop just for cleaning the child bones fixes the issue, but I wonder if there's a better solution (like sorting the bones beforehand).
You can reproduce the issue in @TokageItLab's project here: https://github.com/TokageItLab/3d-platform-test-for-godot4
Edit: tentative fix in #51798
This PR is causing a regression which the CC @fire 2021-08-18.10.11.19-1.mov |
The above broken gizmo problem fixed in #45699. At the same time, gizmo skew caused by translating and scaling is eliminated. |
Awesome! Thanks for fixing it and my apologies for accidentally breaking the old gizmo. I just ported/adjusted the code to work with the changes in the PR and didn’t realize it was broken (nor right off would I have an idea how to fix it even if I did notice). 😅 |
No problem! The octahedron drawing was unstable for a long time ago, so we needed to fix it anyway, and I hope that SkeletonGizmo will make IK easier to use :) |
I need constraints in IK I don't know if I should open this as feature proposal I opened issue here |
Opening an issue on the Godot proposal repository, like you have done, is probably the best way to track this feature request 👍 |
Hey lovely people! The modificationsstacks are really really great and very good to use :-D Thanks for the great work! I hope my proposals are correctly placed here. Proposal Nr. 1) My current workaround is that i am using a script, where i type the bone name to get the bone ID. Proposal Nr. 2) i made her also a little workaround - see this tweet: https://twitter.com/WuotanStudios/status/1554773231420248064?s=20&t=lrVZCkoUoJUFmySLBS_6mA You can find my "opensource project" called GodoDudeMannequin here on github: |
@WuotanStudios: Proposals should be made at https://github.com/godotengine/godot-proposals I think number 2 is a bug, btw. |
@Zireael07 i don't think that 2 is a bug at all. IMO it's more like a missing function. A real bug: ... btw, this was the main reason i made a workaround to find out the bone ID. |
Note: ModificationStack has many design issues and will be refactored as Node by Beta. |
This PR and commit adds a new IK system for 3D with the Skeleton3D node that adds several new IK solvers.
This work was sponsored by GSoC 2020 and TwistedTwigleg.
Once merged, the code in this PR will be up to the community to maintain. I will help advise and contribute small changes infrequently as best I can, but I cannot take the responsibility of maintaining and overseeing the code.
Huge thanks to everyone that has helped with this PR and getting the PR to this point! 🎉