Fixed the TwoBoneIK Skeleton Modification having unintuitive rolls at different target positions

[Shnazzy]

The documentation says that this modification is useful for things like arms and legs, so I modeled these changes with that in mind. Normally an elbow or a knee only bends in one axis, This is important to keep in mind when skinning is concerned because the knee or elbow mesh might accidentally get bent to opposite side if not taken into consideration. In other situations, the problem may occur where the second joint is rolled out of phase with the first joint, causing the topology of the mesh to twist into itself where the joints meet.

This fix addresses this by forcing the first joint to point towards the pole target. Most of the rest of the algorithm is kept intact. An additional fix included in this PR is to address the joints changing origin location seen in #59554. I'm using that issue's example project in these videos:

Current Godot 4 alpha (8):

Godot4_alpha8.mp4

With fixes described:

Godot4_pr.mp4

[Calinou]

Does this PR fully resolve #59554, or should it be kept open?

[Shnazzy]

This only addresses TwoBoneIK and doesn't touch FABRIK which is mentioned in that one, so I would say no, it likely doesn't completely resolve that issue.

[fire]

I'm curious what the change means?

The approximate series of checks matter for the math checks.

There a whole set of problems where it's near one of the singularities

[Shnazzy]

I'm curious what the change means?

The approximate series of checks matter for the math checks.

There a whole set of problems where it's near one of the singularities

Is this referring to the changes I pushed just now or just in regards to the whole PR?

The pushes today are meant to correct a few issues I noticed from what I originally proposed. One of those issues was to make the pole node behavior work on more edge cases such as when the pole node is very close to the first joint and/or target node. The other issue was just an issue that came from a human error not including the right change in the commit that got pushed.

[SaracenOne]

Sorry for not looking at this sooner. It does look like this is improving issues that the SkeletonModificaitonStack two-bone IK solver has. It seems way less inclined to freak out now. However, it's unclear if the intention was to solve situations where you might have a bone chain with non-uniform rolls, since that still appears to break the IK and cause mangling. Whatever the case, it does seem to be an improvement over the old implementation at least which was seemed very broken.

However, more importantly, it is worth noting that while this improves two-bone IK, it also appears to have caused regressions in the FABRIK IK solvers, both the newer skeleton modifier stack implementation (which completely freaks out now), and the old SkeletonIK3D node (which seems to behave consistently now regardless of bone rolls but is otherwise mangled. This one may end up being deprecated in the future, but I still don't think it should be knowingly broken). It looks like the other comments did mention FABRIK and how since this PR doesn't touch it directly, I'm guessing the changes to the rotate_to_align function must have caused the regression.

I've attached a sample project which demonstrates comparisons of some IK solvers to demonstrate the issues I described.
ik_test_2.zip
edit: fixed example project

[Shnazzy]

Yes, since publication, I've found some other edge cases that seem to be dependent on the bone rest forward vectors. There are a lot of weird things surrounding this that seem to be causing it. For example, say I have a skeleton in the following configuration:
Hip Center:
Left Leg
Right Leg
(suppose the Hip Center bone is mostly at the same elevation as the origin points of the left/right legs, which are positioned some x-distance from it)

If you check the code in update_bone_rest_forward_vector and find the line

bones.write[p_bone].rest_bone_forward_vector = bones[p_bone].rest.origin.normalized();

This is the line that's usually calculating the left and right legs' rest forward vectors, and since the left and right legs' origins (relative to their parent) are along the X axis, then the rest forward vector is calculated as such, and then in some other part of the code, for reasons I'm not familiar with, it's determining which axis the forward vector is mostly in.

I'm not sure what the purpose of the rest forward vectors was, so I don't want to propose changes to it. I think instead I need to rework this so that the rest forward vector isn't involved.

Another related issue is some confusion about the "Tip Node" parameter. It seems you're supposed to give it a node in the scene tree, but I don't know if that makes much sense. The only context that would sort-of make sense to me is if it's a bone attachment to the second joint.

What I think makes more sense is to designate the tip node as a child bone of the second joint, but I'd like to hear what others' thoughts are.

Lastly, regarding the issue about rotate_to_align, I'd like to know what the intended behavior is for it. What the name implies to me is that we are rotating the basis in some way, and how that rotation is happening is derived from the two vectors. Perhaps I'm misinterpreting that?

Edit: Also, I think it would improve the general stability of the modification stack idea if whatever modifications it made would only persist until the next time it ran (i.e. the next frame). The reason for that "freaking out" we're seeing is that the modification stack is executed every frame, and the results of that execution are based on what the skeleton looked like beforehand, even if part of that pose was because of a previous frame's modifications. Let me know if that explanation makes sense.

[akien-mga]

CC @TwistedTwigleg

[Shnazzy]

It's probably better to take a look at this updated commit which fixed some of the issues I noticed from my last one, however, it is likely an increase in scope since something this one is doing is resetting the modifications the stack does every time it updates (this is to avoid those freak outs from the bones as well as a situation I noticed where a bone's origin wouldn't follow its parent when moved.)

Link

[TwistedTwigleg]

I did a quick skim through the code and it looks good! It’s been so long I don’t remember all the details but I do remember that TwoBoneIK was very finicky. If this PR helps, then it’s good. 👍

I didn’t compile the code or do a detailed look - I don’t have a local dev setup for Godot work and do not have the time right now to re-familiarize myself with the IK code. It does look good though and I am confident it is an improvement.

[Shnazzy]

@TwistedTwigleg Is this referring to the current commit in the PR or the one I linked. There are a few issues I had noticed about the fixes I originally posted which are hard to explain quickly. I would very much suggest holding off on merging until the handful of topics I brought forward are discussed.

[TwistedTwigleg]

@TwistedTwigleg Is this referring to the current commit in the PR or the one I linked. There are a few issues I had noticed about the fixes I originally posted which are hard to explain quickly. I would very much suggest holding off on merging until the handful of topics I brought forward are discussed.

It is referring to this PR and not the linked one, though I think it is fine too. I left a minor comment on the linked one, but outside of what I brought up in that comment, I think both are good.
Thank you for making the changes! I have not had time to do IK stuff, so I am glad someone has looked at the code and is making fixes 🙂

[Shnazzy]

Sorry I haven't posted about this in so long. I can post the commit I linked last time as an updated commit reference in the PR branch, but regarding the issues brought up about the other IK solvers, I'm afraid I don't know what constitutes intended or unintended behavior for these.

[Shnazzy]

@SaracenOne Sorry I should probably have rebased before asking for review. Please ignore that review request until I do that.

[fire]

Let us know when it's ok to review.

[Shnazzy]

This recent update was a small change to work around the way that rotate_to_align() works without modifying it, meaning there should be less side effects.

Follow up issues:

1. I find it strange to use the verb "rotate" in the name when its effect is to call set_axis_angle(), a function that overwrites the struct entirely. One solution would be to return its value as a Basis instead of void, and then make the function static. Another might be to update rotate_to_align to use rotate() at the end instead of set_axis_angle() (this was how I originally had it but it was causing the side effects that were brought up.
2. Since the twoboneik method is based on the geometry of triangles, is it a fair assumption that the two bones in question are the direct parent/child of the other? If so, I think it would make sense to have the selection method for the second bone in the editor be filtered down to just the child bones of the first bone.
3. Lastly, I think the option to specify the pole as a bone index instead of a node might make things simpler. This would make it more like Blender and would allow those pole bones to be animated as part of the same motion data as the rest of the model, without needing to set up BoneAttachments

[Shnazzy]

Let us know when it's ok to review.

@fire Yes, it's ready for review. Cc @SaracenOne. Thank you for being patient.

[Shnazzy]

Okay so it seems the whole modification stack idea is deprecated (#71137), so I suppose instead I should just close this unless there was a plan to bring the same ideas over to whatever new solution is implemented.

Is 4.x planned to still have the same subtypes of IK like TwoBone, FABRIK, LookAt, etc?

[lyuma]

Hi Shnazzy,
Sorry I missed this PR when doing my review of bugs to the skeleton modification stack. I saw in #59554 that you were able to use Twisted IK successfully in 3.x so it's too bad we couldn't get 4.0 working the same. We have been running into lots of trouble with the modifications and so I recommended they be removed for 4.0 since we couldn't fix the bugs in time.

For now, in 4.0 we will try and make sure SkeletonIK3D works well (the node equivalent of SkeletonIK in 3.5, not the modification stack). and in a future 4.x release we will try to add some of the modifications back in (I hope to have TwoBone, LookAt, and some other more advanced solvers, though no promises about FABRIK itself), but possibly as nodes or in some other form. When that time comes, if we run into trouble again with TwoBoneIK we can reference the code in this PR.

Otherwise thanks for all the work looking into this issue, and I'm sorry we were not able to get the modifications ready in time for feature freeze with all the functionality working as desired. Stay tuned for a future 4.x release.

[Shnazzy]

From what I was reading in the other discussions it seemed like even ignoring the bugs, the modification stack doesn't feel to some like the right workflow. I'd love to participate in the redesign process when 4.x work starts. Where do those kinds of design discussions usually start?

[fire]

You can pick discussions or proposal at https://github.com/godotengine/godot-proposals/