Skip to content
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 Skeleton2D - Squished #47872

Conversation

TwistedTwigleg
Copy link
Contributor

This PR and commit adds a new IK system for 2D with the Skeleton2D node that adds several new IK solvers, a way to control bones in a Skeleton2D node similar to that in Skeleton3D. It also adds additional changes and functionality.

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! 🎉

@pouleyKetchoupp
Copy link
Contributor

The error in different builds is (this one is the windows version):

.\scene/2d/physical_bone_2d.h(82): error C3668: 'PhysicalBone2D::get_configuration_warning': method with override specifier 'override' did not override any base class methods

But I'm not sure what changed. Maybe a rebase went wrong?

@TwistedTwigleg
Copy link
Contributor Author

TwistedTwigleg commented Apr 14, 2021

The error in different builds is (this one is the windows version):

.\scene/2d/physical_bone_2d.h(82): error C3668: 'PhysicalBone2D::get_configuration_warning': method with override specifier 'override' did not override any base class methods

But I'm not sure what changed. Maybe a rebase went wrong?

Possibly something went wrong with the rebase, but the non-squished version also had errors with the CI and it was likely the same thing. The only thing that changed though from the last successful CI build was changing the looking_at function, which shouldn't have influenced this at all.

Looking at how other nodes override the function in their header, it uses the same syntax and formatting as they do, so I'm not sure why it isn't working here when it works there.

Edit: Just build this PR locally and it compiles fine, no errors or anything. I'm not sure why the CI is getting different results.

@pouleyKetchoupp
Copy link
Contributor

It's actually due to #43180, there's no conflict but you would have the same warnings if you rebased locally.

This method now returns an array of strings instead of one string with line breaks.

@TwistedTwigleg TwistedTwigleg force-pushed the GSOC_2020_Working_Branch_2D_IK_SQUISHED branch from e6d653f to 1b8e38f Compare April 15, 2021 02:32
@TwistedTwigleg
Copy link
Contributor Author

The CI issue should be fixed. I rebased the PR with the latest version of master and fixed the issue with the get_configuration_warning function.

However, there is an issue with the physical_bone_2d node that now that causes a segmentation fault. According to the stack trace, it is caused by the is_node_in_tree function that is called when calling get_bone on the Skeleton2D node. I tried to do some quick fixes but they did not work. I will look into the issue further once I am able.

@TwistedTwigleg TwistedTwigleg force-pushed the GSOC_2020_Working_Branch_2D_IK_SQUISHED branch from 1b8e38f to 4c92971 Compare April 15, 2021 19:00
@TwistedTwigleg
Copy link
Contributor Author

TwistedTwigleg commented Apr 15, 2021

The segmentation fault issue has been fixed! Turns out it was an issue where the PhysicsBone2D was positioning itself at the Bone2D position in the Godot editor at start up. I removed the notification and it works fine. Removing it also shouldn't change anything functionally, the node can just now be moved around in the editor but it will snap itself into place when running.

Now everything should be fixed and good to go.
Edit: Had to make a minor format fix.


There is one minor thing, but I'm not sure it is related to this PR, is that the PhysicsBone2D spits out a bunch of the following error when run:

ERROR: Condition "det == 0" is true.
   at: affine_invert (core/math/transform_2d.cpp:49)

Which seems to be caused by line 70 in the _position_at_bone2d function, specifically in the set_global_transform function call. My only guess is that the parent item has a determinate of 0, but I'm not sure why it is spitting an error out when it didn't before.
I am not really sure what I can do in PhysicsBone2D to fix the issue without altering the set_global_transform function or the affine_inverse function in Transform2D.

@TwistedTwigleg TwistedTwigleg force-pushed the GSOC_2020_Working_Branch_2D_IK_SQUISHED branch from 4c92971 to 2ba72f1 Compare April 15, 2021 19:06
@pouleyKetchoupp
Copy link
Contributor

About the affine_invert error:

I'm able to reproduce it when running this scene from your test project:
https://github.com/TwistedTwigleg/Godot_GSOC_2020_2D_Project/tree/master/Scenes/Test_09

And I'm getting this callstack in debug:

 godot.windows.tools.64.exe!Transform2D::affine_invert() Line 49	C++
 godot.windows.tools.64.exe!Transform2D::affine_inverse() Line 63	C++
 godot.windows.tools.64.exe!PinJoint2DSW::setup(float p_step) Line 142	C++
 godot.windows.tools.64.exe!Step2DSW::_setup_island(LocalVector<Constraint2DSW *,unsigned int,0> & p_constraint_island, float p_delta) Line 70	C++
 godot.windows.tools.64.exe!Step2DSW::step(Space2DSW * p_space, float p_delta, int p_iterations) Line 218	C++
 godot.windows.tools.64.exe!PhysicsServer2DSW::step(float p_step) Line 1263	C++
 godot.windows.tools.64.exe!PhysicsServer2DWrapMT::step(float p_step) Line 76	C++
 godot.windows.tools.64.exe!Main::iteration() Line 2488	C++
 godot.windows.tools.64.exe!OS_Windows::run() Line 622	C++

So if it's related to the case you spotted, it's due to setting up a joint between a kinematic body and a static body (infinite inertia and mass are leading to a zeroed matrix). I'm not sure why this would have changed, but maybe the error wasn't spotted on version 3.2. Also it looks like the error wouldn't be raised in release_debug.

In any case, I think this can be solved outside of this PR. It probably makes sense for the physics server to handle this case by just ignoring the joint, so I'll open a PR for that.

@akien-mga
Copy link
Member

Welp, looks like GitHub closes the whole PR when merging another one that fixes a comment.

@akien-mga akien-mga reopened this Apr 16, 2021
@akien-mga
Copy link
Member

It was good in the end that this got closed and reopened as it restarted CI, and there's a new issue to handle: https://github.com/godotengine/godot/pull/47872/checks?check_run_id=2359849305

We merged #47414 yesterday so we now have @qarmin's RegressionTestProject running on CI and it seems to get a crash with this PR:

#################### SkeletonModification2DPhysicalBones ####################
SkeletonModification2DPhysicalBones.set_physical_bone_chain_length
Parameters [100]
SkeletonModification2DPhysicalBones.get_physical_bone_chain_length
Parameters []
SkeletonModification2DPhysicalBones.set_physical_bone_node
Parameters [100, ]
ERROR: Joint index out of range!
   at: set_physical_bone_node (scene/resources/skeleton_modification_2d_physicalbones.cpp:259)
SkeletonModification2DPhysicalBones.get_physical_bone_node
Parameters [100]
ERROR: Joint index out of range!
   at: get_physical_bone_node (scene/resources/skeleton_modification_2d_physicalbones.cpp:265)
SkeletonModification2DPhysicalBones.fetch_physical_bones
Parameters []
scene/resources/skeleton_modification_2d_physicalbones.cpp:186:2: runtime error: member access within null pointer of type 'struct SkeletonModificationStack2D'
handle_crash: Program crashed with signal 11
Dumping the backtrace. Please include this when reporting the bug on godotengine/godot/issues
[1] bin/godot.linuxbsd.tools.64s() [0x1e697d8] (/home/runner/work/godot/godot/platform/linuxbsd/crash_handler_linuxbsd.cpp:54)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x46210) [0x7fd1ca5b0210] (??:0)
[3] SkeletonModification2DPhysicalBones::fetch_physical_bones() (/home/runner/work/godot/godot/scene/resources/skeleton_modification_2d_physicalbones.cpp:186)
[4] void call_with_variant_args_helper<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/home/runner/work/godot/godot/./core/variant/binder_common.h:202 (discriminator 4))
[5] void call_with_variant_args_dv<__UnexistingClass>(__UnexistingClass*, void (__UnexistingClass::*)(), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (/home/runner/work/godot/godot/./core/variant/binder_common.h:315)
[6] MethodBindT<>::call(Object*, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/./core/object/method_bind.h:283)
[7] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/core/object/object.cpp:784 (discriminator 1))
[8] Object::callv(StringName const&, Array const&) (/home/runner/work/godot/godot/core/object/object.cpp:709 (discriminator 1))
[9] void call_with_variant_args_ret_helper<__UnexistingClass, Variant, StringName const&, Array const&, 0ul, 1ul>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, Variant&, Callable::CallError&, IndexSequence<0ul, 1ul>) (/home/runner/work/godot/godot/./core/variant/binder_common.h:623 (discriminator 8))
[10] void call_with_variant_args_ret_dv<__UnexistingClass, Variant, StringName const&, Array const&>(__UnexistingClass*, Variant (__UnexistingClass::*)(StringName const&, Array const&), Variant const**, int, Variant&, Callable::CallError&, Vector<Variant> const&) (/home/runner/work/godot/godot/./core/variant/binder_common.h:399)
[11] MethodBindTR<Variant, StringName const&, Array const&>::call(Object*, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/./core/object/method_bind.h:452)
[12] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot/godot/modules/gdscript/gdscript_vm.cpp:1496 (discriminator 1))
[13] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/modules/gdscript/gdscript.cpp:1552)
[14] Object::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/core/object/object.cpp:765 (discriminator 1))
[15] Variant::call(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (/home/runner/work/godot/godot/core/variant/variant_call.cpp:938)
[16] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot/godot/modules/gdscript/gdscript_vm.cpp:1394)
[17] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/modules/gdscript/gdscript.cpp:1552)
[18] ScriptInstance::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (/home/runner/work/godot/godot/core/object/script_language.cpp:311)
[19] Node::_notification(int) (/home/runner/work/godot/godot/scene/main/node.cpp:147)
[20] Node::_notificationv(int, bool) (/home/runner/work/godot/godot/./scene/main/node.h:45 (discriminator 14))
[21] Object::notification(int, bool) (/home/runner/work/godot/godot/core/object/object.cpp:795)
[22] Node::_propagate_ready() (/home/runner/work/godot/godot/scene/main/node.cpp:189)
[23] Node::_set_tree(SceneTree*) (/home/runner/work/godot/godot/scene/main/node.cpp:2525)
[24] Node::_add_child_nocheck(Node*, StringName const&) (/home/runner/work/godot/godot/scene/main/node.cpp:1237)
[25] Node::add_child(Node*, bool) (/home/runner/work/godot/godot/scene/main/node.cpp:1250)
[26] void call_with_variant_args_helper<__UnexistingClass, Node*, bool, 0ul, 1ul>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool), Variant const**, Callable::CallError&, IndexSequence<0ul, 1ul>) (/home/runner/work/godot/godot/./core/variant/binder_common.h:202 (discriminator 4))
[27] void call_with_variant_args_dv<__UnexistingClass, Node*, bool>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (/home/runner/work/godot/godot/./core/variant/binder_common.h:315)
[28] MethodBindT<Node*, bool>::call(Object*, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/./core/object/method_bind.h:283)
[29] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (/home/runner/work/godot/godot/modules/gdscript/gdscript_vm.cpp:1496 (discriminator 1))
[30] GDScriptInstance::call(StringName const&, Variant const**, int, Callable::CallError&) (/home/runner/work/godot/godot/modules/gdscript/gdscript.cpp:1552)
[31] ScriptInstance::call(StringName const&, Variant const&, Variant const&, Variant const&, Variant const&, Variant const&) (/home/runner/work/godot/godot/core/object/script_language.cpp:311)
[32] Node::_notification(int) (/home/runner/work/godot/godot/scene/main/node.cpp:58)
[33] Node::_notificationv(int, bool) (/home/runner/work/godot/godot/./scene/main/node.h:45 (discriminator 14))
[34] CanvasItem::_notificationv(int, bool) (/home/runner/work/godot/godot/./scene/main/canvas_item.h:164 (discriminator 3))
[35] Control::_notificationv(int, bool) (/home/runner/work/godot/godot/./scene/gui/control.h:47 (discriminator 3))
[36] Object::notification(int, bool) (/home/runner/work/godot/godot/core/object/object.cpp:795)
[37] SceneTree::_notify_group_pause(StringName const&, int) (/home/runner/work/godot/godot/scene/main/scene_tree.cpp:809)
[38] SceneTree::process(float) (/home/runner/work/godot/godot/scene/main/scene_tree.cpp:443 (discriminator 2))
[39] Main::iteration() (/home/runner/work/godot/godot/main/main.cpp:2499)
[40] OS_LinuxBSD::run() (/home/runner/work/godot/godot/platform/linuxbsd/os_linuxbsd.cpp:261)
[41] bin/godot.linuxbsd.tools.64s(main+0x3ff) [0x1e67115] (/home/runner/work/godot/godot/platform/linuxbsd/godot_linuxbsd.cpp:60)
[42] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7fd1ca5910b3] (??:0)
[43] bin/godot.linuxbsd.tools.64s(_start+0x2e) [0x1e66c5e] (??:?)
-- END OF BACKTRACE --
Aborted (core dumped)
FATAL ERROR: Godot has been crashed.

It might not be immediate to reproduce locally though, here's the config for the CI build that finds this crash: https://github.com/godotengine/godot/blob/master/.github/workflows/linux_builds.yml#L129-L170

Basically it builds Godot with asan and ubsan, and uses this build to edit and run https://github.com/qarmin/RegressionTestProject/archive/4.0.zip (the SwiftShader stuff is a software Vulkan renderer for CI, it shouldn't be needed to reproduce the issue).

@qarmin
Copy link
Contributor

qarmin commented Apr 16, 2021

This last lines before crash:

SkeletonModification2DPhysicalBones.fetch_physical_bones
Parameters []

shows that probably this code is enough to reproduce crash

SkeletonModification2DPhysicalBones.new().fetch_physical_bones()

@TwistedTwigleg
Copy link
Contributor Author

Thanks guys, I'll take a look at the issue once I have a chance 👍

@fire
Copy link
Member

fire commented Apr 26, 2021

Merging is blocked by:


#################### SkeletonModification2DPhysicalBones ####################
SkeletonModification2DPhysicalBones.set_physical_bone_chain_length
Parameters [100]
SkeletonModification2DPhysicalBones.get_physical_bone_chain_length
Parameters []
SkeletonModification2DPhysicalBones.set_physical_bone_node
Parameters [100, ]
ERROR: Joint index out of range!
   at: set_physical_bone_node (scene/resources/skeleton_modification_2d_physicalbones.cpp:259)
SkeletonModification2DPhysicalBones.get_physical_bone_node
Parameters [100]
ERROR: Joint index out of range!
   at: get_physical_bone_node (scene/resources/skeleton_modification_2d_physicalbones.cpp:265)
SkeletonModification2DPhysicalBones.fetch_physical_bones
Parameters []
scene/resources/skeleton_modification_2d_physicalbones.cpp:186:2: runtime error: member access within null pointer of type 'struct SkeletonModificationStack2D'

@TwistedTwigleg TwistedTwigleg force-pushed the GSOC_2020_Working_Branch_2D_IK_SQUISHED branch 4 times, most recently from 7d60629 to 05ea296 Compare June 5, 2021 18:48
This PR and commit adds a new IK system for 2D with the Skeleton2D node
that adds several new IK solvers, a way to control bones in a Skeleton2D
node similar to that in Skeleton3D. It also adds additional changes
and functionality.

This work was sponsored by GSoC 2020 and TwistedTwigleg.

Full list of changes:
* Adds a SkeletonModifier2D resource
  * This resource is the base where all IK code is written and executed
  * Has a function for clamping angles, since it is so commonly used
  * Modifiers are unique when duplicated so it works with instancing
* Adds a SkeletonModifierStack2D resource
  * This resource manages a series of SkeletonModification2Ds
  * This is what the Skeleton2D directly interfaces with to make IK possible
* Adds SkeletonModifier2D resources for LookAt, CCDIK, FABRIK, Jiggle, and TwoBoneIK
  * Each modification is in its own file
  * There is also a SkeletonModifier2D resource that acts as a stack for using multiple stacks together
* Adds a PhysicalBone2D node
  * Works similar to the PhysicalBone3D node, but uses a RigidBody2D node
* Changes to Skeleton2D listed below:
  * Skeleton2D now holds a single SkeletonModificationStack2D for IK
  * Skeleton2D now has a local_pose_override, which overrides the Bone2D position similar to how the overrides work in Skeleton3D
* Changes to Bone2D listed below:
  * The default_length property has been changed to length. Length is the length of the bone to its child bone node
  * New bone_angle property, which is the angle the bone has to its first child bone node
  * Bone2D caches its transform when not modified by IK for IK interpolation purposes
  * Bone2D draws its own editor gizmo, though this is stated to change in the future
* Changes to CanvasItemEditor listed below:
  * Bone2D gizmo drawing code removed
  * The 2D IK code is removed. Now Bone2D is the only bone system for 2D
* Transform2D now has a looking_at function for rotating to face a position
* Two new node notifications: NOTIFICATION_EDITOR_PRE_SAVE and NOTIFICATION_EDITOR_POST_SAVE
  * These notifications only are called in the editor right before and after saving a scene
  * Needed for not saving the IK position when executing IK in the editor
* Documentation for all the changes listed above.
@TwistedTwigleg TwistedTwigleg force-pushed the GSOC_2020_Working_Branch_2D_IK_SQUISHED branch from 05ea296 to 8aa3c2f Compare June 5, 2021 19:20
@TwistedTwigleg
Copy link
Contributor Author

I fixed the build and documentation issues, as well as rebased with the latest version of master. 👍

@akien-mga akien-mga merged commit 94c31ba into godotengine:master Jun 5, 2021
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants