-
-
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
Modification stack to nodes #63588
Modification stack to nodes #63588
Conversation
0eabe5d
to
878a688
Compare
138698e
to
026d3bc
Compare
026d3bc
to
e109244
Compare
@lyuma is working on the pr, so the current code isn't accurate. The proposal is still broadly accurate. |
I don’t have the time to review the PR, but I agree with the general change (not that my agreement is needed - it’s all Godot code now 🙂 ). Moving it to nodes is MUCH closer to my original GSoC proposal and based on the work I did for my IK plugin for Godot 3.0, it is much more stable and easier to maintain as well. It should also make the workflow less confusing and reduce the number of clicks to get something working on screen. Now just need to make those Bone3D nodes and have the modification nodes use the Bone3D nodes to fully fit the original proposal 😉 In all seriousness though - I think the general change is good. It will make the modification stack completely unnecessary and I think that is where a lot of the pain came from. It also will untie it from the Skeleton3D node code, making the modifications standalone. Also, and I know I do not say this enough: Huge thanks to everyone who has continued to work on the IK stuff! I haven’t had any time and energy at all to work on much in spare time as of late and so I’m glad and thankful that others have continued to work on it and improve it. |
The git branches for this work is at https://github.com/lyuma/godot/tree/modification-stack-to-nodes-wip-full and https://github.com/lyuma/godot/tree/modification-stack-to-nodes-wip |
e109244
to
28012bb
Compare
@TwistedTwigleg I've updated this PR with a mostly feature-complete PR. This is now a two-commit PR. The reason is I don't know how to preserve the rename metadata if I make it all in one commit since so much code was rewritten, so it makes review easier to do it this way. I haven't tested anything at all. There is likely a lot that could have broken, but the existing implementation already has many features broken during alpha. As a future PR, there are still some methods that I would like to clean up in BoneAttachment3D related to update order and the pose_updated notification. I also find the external_skeleton feature to be confusing. Regarding what is in this update... I added documentation for all the new and renamed methods/members. @TokageItLab I addressed the biggest feedback items.
get_bone_rest_forward_vector and global_pose_z_forward_to_bone_forward were migrated to SkeletonModification3D as helper functions. I have made all calculations strictly based on I have removed bone_property_suffixes and used a |
28012bb
to
71756ad
Compare
c979e91
to
62d8e79
Compare
62d8e79
to
c398e5e
Compare
c398e5e
to
a886e81
Compare
I have decided to mark this as draft so that we can focus on other more pressing priorities for 4.0 beta, and fix some of the design issues with this that came up during testing. For now, I will submit a new PR that marks all of the existing SkeletonModification resources and SkeletonModificationStack as Deprecated (planned to move to modification nodes) and unexposes some skeleton APIs only being used by the modifications such as "global_pose_z_forward_to_bone_forward" so it will be easier to remove |
We're also marking SkeletonIK to Deprecated too? |
please add this issue too #54891 |
What's the status of this PR? It'd be great to see this before 4.0 stable if this level of API breaking is accepted. Right now IK is unusable. |
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 did some initial testing (after fixing merge conflicts) and the new system is a lot more intuitive to use and set up. Great work!
I only tested FABRIK in 3D so far, but it seems like the math is no longer correct, as the target node is not being reached anymore:
It looks like the algorithm results in global transform per bone, but is applied in local space (see how each bone rotates by 90°):
Let me know if you need a simple repro project.
if (!_cache_bone(fabrik_data_chain[i].bone_idx, fabrik_data_chain[i].bone_name)) { | ||
ret.append(vformat("Joint %d bone %s was not found.", i, fabrik_data_chain[i].bone_name)); | ||
} | ||
if (!_cache_target(fabrik_data_chain[i].tip_cache, fabrik_data_chain[i].tip_node, fabrik_data_chain[i].tip_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.
Not having any tip node/bone selected should not show a warning if automatic length calculation is successful.
origin_global_pose = origin_global_pose_trans; | ||
|
||
final_joint_idx = fabrik_data_chain.size() - 1; | ||
real_t target_distance = fabrik_transforms[final_joint_idx].origin.distance_to(target_global_pose.origin); |
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.
When creating a new FABRIK3D node, assigning skeleton, then assigning target node, I get following error + a crash on this line:
ERROR: Index p_index = 4294967295 is out of bounds (count = 0).
at: LocalVector<struct Transform3D,unsigned int,0,0>::operator [] (B:\Godot\core/templates/local_vector.h:168)
[1] SkeletonModification3DFABRIK::execute (B:\Godot\scene\3d\skeleton_modification_3d_fabrik.cpp:549)
I guess this will close #65265 unless it's an inspector-specific bug. (funny, on first attempt to send a message, I got the error |
I have repurposed this PR into #71137 and removed SkeletonModificationStack3D, so we will not port to nodes in time for 4.0. 2D modification stack works and will not be removed for now. 3D modification stack will hopefully return in a future 4.x in a different form. Stay tuned. |
Fixes: godotengine/godot-proposals#4863
Work in progress.
Using https://github.com/fire/godot-skeleton-modifier as the test project.
List of issue possible closed by completing this implementation (SkeletonModificationStack issues related to 4.0):
SkeletonModificationStack3D.set_modification_count
#54234bone_name
field goes backwards in SkeletonModification3DLookAt #62876