-
-
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 Skeleton2D #40347
New and improved IK in Skeleton2D #40347
Conversation
877f5ab
to
7bdc233
Compare
cd1736c
to
195ef55
Compare
054bd07
to
78fcf91
Compare
2d7d309
to
3edb4f2
Compare
Alright, I think this PR is ready for review now! 🎉 |
6fa802e
to
da0f120
Compare
7123dcf
to
8cb4e88
Compare
Just an FYI to anyone looking - I need to update the 2D IK PR to the latest version of master to fix a merge conflict that has arisen. I was going to wait until the end of the GSOC, but because of the merge conflict, the CI tests are not running. Thankfully it appears everything is compatible going forward, with only PhysicalBone2D having some issues in the latest version of master. I'll rebase this PR soon and will update this comment once the PR is fully rebased. Edit: Rebased with the latest master build! It is a little unstable for PhysicalBone2D still, but the rest seems okay. I'm occasionally getting crashes, but it seems to be unrelated (and instead is input related?). Edit 2: Nevermind, it's just because 2D physics in Godot are broken right now. It is not related to the code in this PR! Edit 3: 2D physics are now fixed in Godot master, so all that's left is to rebase. I'll rebase this PR to master soon 👍 |
47fda96
to
92e810f
Compare
…rks. Edit: Fixed formatting issues with the static formatting checks Edit 2: Even more formatting fixes to hopeful solve static formatting check issues.
This was my fault, I didn't test the code very much and assumed it was working. The code works, but only on the very first joint and only if that first joint is never rotated by other IK modifiers or any other means. All other joins beyond the first joint do not work. I only discovered this while trying to write constrained FABRIK for Twisted IK 2 and found that the GSoC code was not working in the plugin, so I tested here with this code and confirmed it does not work. So, FABRIK will just be unconstrained now, unfortunately. If I find a solution to constrained IK for 2D FABRIK, I will document it so it can be potentially implemented into this system as well. Also: Fixed the header in physical_bone_2d.h not using the correct year.
…n the latest version of master
…rmal conditions for maximum compatability with how the code was written before. Changed errors about updating the cache to warnings. Fixed a typo. Added a compile warning about the Bone2D gizmo drawing code Added missing newlines on the header files for 2D IK modifications
… but it may be I accidentally deleted the line when fixing the merge conflicts Edit: Fixed code format issue in Skeleton_2d.cpp Edit 2: Fixed code format issue in register_scene_types.cpp
…e code in Skeleton2D, fix documentation issues
* Changed documentation wording * Removed the deprecated length functions from Bone2D and added compatibility through _set and _get * Changed parameters in functions to use p_parameter * (Maybe other changes? I don't remember. All changes are based on review feedback)
4566acb
to
6db3741
Compare
Alright, everything has been rebased and the review feedback taken into account. I messed up one part of the rebase a little bit, but since this PR isn't going to be merged and a new, single commit PR will be instead, I figured it was fine to leave it. Thanks again for the reviews everyone! Also, thanks for the link and example on the commit message @akien-mga, I will structure the commit on the new PR accordingly. |
void _popup_warning_temporarily(Control *p_control, const float p_duration); | ||
void _popup_warning_depop(Control *p_control); | ||
|
||
void _set_owner_for_node_and_children(Node *node, Node *owner); |
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 still lots of parameters that are missing the _p
prefix in several headers (coding style).
core/math/transform_2d.cpp
Outdated
@@ -158,6 +158,13 @@ bool Transform2D::is_equal_approx(const Transform2D &p_transform) const { | |||
return elements[0].is_equal_approx(p_transform.elements[0]) && elements[1].is_equal_approx(p_transform.elements[1]) && elements[2].is_equal_approx(p_transform.elements[2]); | |||
} | |||
|
|||
Transform2D Transform2D::looking_at(const Vector2 target) 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.
@TwistedTwigleg There are still lots of parameters that are missing the _p prefix in several headers.
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's still some coding style feedback not addressed from @akien-mga's review. Once it's done you can create a new PR with squashed commits.
Here's the list remaining issues that we need to keep track of:
- Editor code needs to be removed from scene node (might need proper support for 2D gizmos plugins).
- Use property binding macros instead of
_set
/_get
override - Review
NOTIFICATION_EDITOR_PRE_SAVE
&NOTIFICATION_EDITOR_POST_SAVE
notifications used to reset the skeleton while saving the scene.
Just to clarify, that's the list of issues we want to keep track of once this has been merged. They don't need to be addressed in this PR, as discussed previously. |
Thanks for the review, I will try to address the remaining code style feedback issues this weekend. |
… SkeletonModification2D pages. Syntax changes to canvas_item_editor_plugin, skeleton_2d, skeleton_modification_2d, and skeleton_modification_2d_jiggle
Alright, the syntax changes have been made and now every file should be good to go. If everyone is good with how everything in this PR, I'll make a new PR with the commits squashed into one. |
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! It should be good to go with this one last thing I missed before. I guess you can open the new PR at this point.
core/math/transform_2d.h
Outdated
@@ -100,6 +100,8 @@ struct Transform2D { | |||
Transform2D orthonormalized() const; | |||
bool is_equal_approx(const Transform2D &p_transform) const; | |||
|
|||
Transform2D looking_at(const Vector2 p_target) 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.
Transform2D looking_at(const Vector2 p_target) const; | |
Transform2D looking_at(const Vector2 &p_target) const; |
I'm not sure why the CI is failing, all I did was make the change suggested and it compiles fine on my machine. Regardless, I made the squished PR here: #47872 |
@TwistedTwigleg May I close this then? |
Let's close this PR since #47872 was opened instead. |
This pull request is the 2D work for the new and improved IK in Godot, as part of my Google Summer of Code proposal. This PR includes behind the scenes changes, like changes to Skeleton2D and Bone2D, as well as the IK code itself. You can find the 3D equivalent for this PR here: #39353
The first 8 commits are squashed changes from the verbose log, bringing the commit count from 34 to just 8. Commits after commit 8 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
_process
or_physics_process
! This is a per modification property.draw
functions!|
|
|
|
Has full angle constraint support that is taken into account when solving. Right now constraints only work in local-space (global-space support has not been added yet)Edit: angle constraints were not working. See comments below!|
_physics_process
mode. Uses raycasts to detect colliders.|
Has support for angle constraints, though these constraints are not taken into account by the algorithm.Removed!|
get_configuration_warning
function.|
|
|
local_pose_override
to Skeleton2D. Thelocal_pose_override
works just like it does for Skeleton3D, and can even be interpolated!It does require using RenderingServer to apply the override, but it works pretty well with only minimal changes required._process
or_physics_process
, based on the information in the SkeletonModificationStack2D.|
length
, which is the bone's current length.default_length
has been removed and its functions deprecated.length
can be set in the editor like normal, so there's no loss of functionality.bone_angle
.bone_angle
stores the angle that the bone has, which can be different than the transform's rotation.bone_angle
solves this issue by storing the angle of the bone. It is kinda hard to explain, but without it, all bone chains would have to be straight._draw
function.#ifdef TOOLS_ENABLED
, so no performance is lost on exported projects.|
Clear Bones
and the IK related options, as they are no longer needed or usable.Show Bones
option now toggles the editor gizmo visibility of any selected Bone2D nodes and any child Bone2D nodes for those selected.Create Custom Bone2D(s) from Node(s)
option will make Bone2D nodes using the selected Node2D nodes, and will re-parent the selected nodes to the created Bone2D nodes, making the creation process much easier and similar to how theCreate Custom Bone(s) from Node(s)
option worked.editor_selection
variable has been changed to a public variable in CanvasItemEditor, as otherwise there is no great way for Bone2D nodes to know if they have been selected.|
looking_at
function to Transform2D.|
NOTIFICATION_EDITOR_PRE_SAVE
andNOTIFICATION_EDITOR_POST_SAVE
.|
|
Todo:
Nothing for now!
Known bugs/errors/problems:
Setting bones before setting the target causes some modifications not to run.Can not reproduce anymore, so I'm going to assume this bug is fixed.Saving a scene with SkeletonModification2D resources attached to a Skeleton2D cause them to no longer work in the editor, though functionality returns upon reloading the scene. This is a known issue that also occurs in the 3D PR.Fixed! Used the same fix that I found in the 3D IK PR.The test project I am using can be found here.
Disclaimer: Some of the work done in this PR was part of the Google Summer of Code 2020 program!