Page MenuHome

Spline IK: Correct the bone position calculation
ClosedPublic

Authored by Sebastian Parborg (zeddb) on Mar 29 2021, 12:27 PM.

Details

Summary

(I'm open for suggestions for a better subject title)

Previously the bone position outside of "fit to curve length" mode was incorrect.

It assumed that the curve was completely straight with no bends or turns.
This would lead to bones being scaled down as their final position would be servery underestimated in some cases.

The solution is to do a sphere -> curve intersection test to see where to put the bones while still preserving their length.
As we are using the tessellated curve data this essentially boils down to us doing a sphere -> line intersection check.

An old undocumented feature is that when the bone rolls off the curve, the constraint influence decreases until it can be full pose able again.

You can see this by applying the pose as rest pose (T77330, T81704).

Here is also an additional example file:

There is currently a patch for this already (D9244), but I felt it was a bit overly complicated and came up with an alternate solution.

I'm guessing that @Sybren A. Stüvel (sybren) wants me to do some additional cleanup in the file as the existing comments and variable names do not follow our code guidelines.
(Camel case, comments not starting with capital letter and ending with a period etc)

However for the initial review, I have not cleaned anything so all changes (except a clang format change at the end of the file) is only relevant to the fix.

TODO:

  1. Explain the "roll off" feature when bones end up entirely or partially outside of the curve. (Add this explanation to the manual as well.)

Diff Detail

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Mar 29 2021, 12:27 PM
Sebastian Parborg (zeddb) created this revision.
source/blender/blenkernel/intern/armature_update.c
257

Copy pasted code (code de-duplication)

664

Clang format automated change.

Sybren A. Stüvel (sybren) requested changes to this revision.Mar 29 2021, 2:38 PM

What does the patch do? I wouldn't mind seeing some pictures that explain how things are computed. These will help with the release notes and the manual as well.

Testing with the example file in the description shows good results, until the curve is shorter than the bones:

In this case I would expect the last bone to align with the curve, at least in some sensible way, like having the bone oriented such that

  • the end point of the curve aligns with the bone's Y-axis, or
  • the bone aligns with the part of the curve at its head (i.e. where it currently also touches the curve).

Either would be fine by me, but maybe riggers have a more specific desire here. The current orientation looks a bit arbitrary, though, and I don't expect this to be very useful for rigging (the improvements in general are, for sure, this is just about the "the curve is too small" situation).

I'm guessing that @Sybren A. Stüvel (sybren) wants me to do some additional cleanup in the file as the existing comments and variable names do not follow our code guidelines.

👍
It could also use a pass for constness of parameters/variables.

It should be fairly easy to take some code from splineik_evaluate_bone and split it off into smaller functions. Doesn't have to go over the entire function (plenty of old, untouched code), but I feel that some parts can easily be given a name and explict input/output parameters. I'm thinking about the contents of that if (ikData->yScaleMode != CONSTRAINT_SPLINEIK_YS_FIT_CURVE) { block, and then that function can be split up into smaller parts again.

source/blender/blenkernel/intern/armature_update.c
374–375

pchan->bone->length can be zero.

405–408

CLAMP_MAX(test_idx, max_idx)

416

Then why isn't this using where_on_path()?

418–421

eek, just write as two lines, instead of this ABBA rhyme.

427

is_outside_range expresses the booleanness better.

This revision now requires changes to proceed.Mar 29 2021, 2:38 PM
Sebastian Parborg (zeddb) marked 5 inline comments as done.
Sebastian Parborg (zeddb) added inline comments.
source/blender/blenkernel/intern/armature_update.c
277

Suggestions for better function name is welcome.

374–375

I fixed this by adding a check at the start of the function to handle zero length bones.

416

Because the where_on_path returns a single point.
I need to have the actual tessellated line segments to be able to do the sphere tests correctly with out potentially missing any intersection points.

IE I need the raw data from the curve, otherwise I can't make certain assumptions if I work on the interpolated data from where_on_path.

Thank you, Dr., for adding me as a subscriber. I am happy to test this patch, maybe it will be the push I need to finish my own patch! If this isn't just a better solution.
Is the aim of this Differential to solve a specific bug report, and if so, which one? Seems to fix T81704, but there's a hitch...

I would have to read this much more carefully to understand what you're doing, but it looks like you are doing the thing I tried with Sphere-intersecting each of the line-segments that make up the curve. IIRC I ran into problems when I tried this when the curve was very jagged. Here's a gif, using this patch, that demonstrates one of the problems we run into.


I was also running into a problem that crashed Blender when runtime-data was needed but unavailable. Your patch avoids this, it seems.

Also, this currently breaks Fit-To-Curve if the bones are too short in Edit Mode.
EDIT: this has been fixed:)

Eh, it looks like you updated this while I was writing. Will have another build and another look.
EDIT: fixed the fit-to-curve thing, but the gif is still accurate

I would have to read this much more carefully to understand what you're doing, but it looks like you are doing the thing I tried with Sphere-intersecting each of the line-segments that make up the curve. IIRC I ran into problems when I tried this when the curve was very jagged. Here's a gif, using this patch, that demonstrates one of the problems we run into.


I was also running into a problem that crashed Blender when runtime-data was needed but unavailable. Your patch avoids this, it seems.

I don't think there is anyway to avoid that if we truly want to preserve the bone lengths.

To me, it seems like your curve has too steep bends which makes it jump around like that as there is no valid solution that preserves the bone lengths without this discontinuity.
Here for example:

(The red line is the sphere in the first frame and the green line is the sphere in the second frame)

When you move the bone further up the curve, the only valid point the tail can be at is at a point much further along the curve.

Oh, for sure! I understand what's happening here, but I remember having a discussion with Dr. Stüvel and what he told me was that the discontinuous motion would be undesirable for animation... I'd like to have a perfect bone-length preservation option and an option that tries to preserve bone length but also moves continuously. My patch is over-complicated because it tries to do the latter -- it's a surprisingly difficult problem to solve. I still need to clean it up and finish it. IMO both behaviors are desirable to a rigger at different times.

At any rate, I'm really glad to see this getting attention from a Blender-Foundation team member. I think you'll produce a better patch than me :). But I'll be here to help test and stuff.

what he told me was that the discontinuous motion would be undesirable for animation...

That's still true. But if it's caused by some mathematical principle, they'll just have to live with it. It's the same with knee popping -- it's undersirable, but since it's caused by math being mathy, there is little we can do about it.

That's still true. But if it's caused by some mathematical principle, they'll just have to live with it. It's the same with knee popping -- it's undersirable, but since it's caused by math being mathy, there is little we can do about it.

In this case there is no way around it if we are going to preserve the bone lengths.
As you pointed out, if the user presents a problem that do not have a solution that is continuous, there is little we can do.

This could actually be used to see if a road/train track/canal has too sharp turns for a vessel of a certain length to be able to pass!
If the result is continuous, then it will be able to pass (at least in theory) ;)

I think it is useful to preserve bone-length exactly, too. As a rigger, it's nice being able to rely on something to behave predictably. For instance, if the bone-length is preserved exactly then it's fairly easy to calculate the length of the spline-ik-chain in a driver, provided all of the bones are initially the same length (the driver can use bone's original length * y-scale * number of bones in the chain to get the length). Moreover, I think that if you want continuous motion, just add more bones. Spline IK will take up to 255 bones. Dr. Stüvel mentioned knee-popping -- which is the result of "math being mathy" -- but almost every rigger will learn to use a "soft-IK" system to solve this. Workarounds exist for almost any problem. That being said -- it *is* possible to solve T81704 without introducing discontinuous motion. If we correct the way Blender calculates the joint-bindings, then we can adjust the entire spline IK chain instead of individual bones in the chain. This would be similar to the original behavior of the constraint, which has continuous motion, without the bug.
So really, I think that this solves the problem by introducing a new behavior. Maybe there should be a check-box that says "preserve bone scale exactly". Ideally we can choose between adjusting individual joints in the chain, or adjusting the ends of the chain and scaling the joint-bindings along with them.
Finally, I will suggest a situation where the original behavior is good, if only it were implemented correctly: sliding a bone along a chain of bones. Currently, you have to set up a convoluted system of constraints and drivers to do this. It's too heavy a solution for such a simple goal. Using Spline IK, however, you can slide a bone's end along a spline using a single constraint. One additional bone and constraint is enough to place a bone's head at an arbitrary, sliding point on the spline. If you remember Mr. Biped from Humane Rigging, he had a spine which used the former method. This may already be possible with this diff's result, so here's another example: say you have a character with tentacles that need to wiggle, bend, stretch and squash smoothly. Preserving the exact length of individual segments isn't very useful in this case. It's far better for the bones to move smoothly as the chain is scaled up and down. Finally, I worry that if a "pop" can occur in the spline's motion, then it may produce bad motion-blur if it occurs on a sub-frame unbeknownst to the animator. This kind of error can be especially frustrating.
I want to reiterate that I think this solution to the problem is useful and carries the benefit of simplicity. Regardless of where this differential goes, however, I intend to finish my patch since I think that both behaviors are useful.

Anyways, it seems there is still some anomalous behavior. I ran into this before when I tried this, I don't remember if I was able to figure out a solution.

That being said -- it *is* possible to solve T81704 without introducing discontinuous motion. If we correct the way Blender calculates the joint-bindings, then we can adjust the entire spline IK chain instead of individual bones in the chain. This would be similar to the original behavior of the constraint, which has continuous motion, without the bug.

You will have to explain this a bit better, I don't understand how that would work.

I can't see how it would be possible to solve that without introducing discontinuous motion.
If you want to preserve the length of the bones there is no way around it.
The only bone that will have a continuous deform is the first bone in the chain as only the tail will move along the curve.
This will only be true if the curve doesn't change shape and the start point doesn't change though...
Every other bone will have discontinuity issues if the curve has too sharp turns to allow them to slide back and forth smoothly.

I don't see how adjusting the IK chain will fix this.

If you don't preserve the length, then "apply as rest pose" will still have the issue of changing the bone length.

So really, I think that this solves the problem by introducing a new behavior. Maybe there should be a check-box that says "preserve bone scale exactly". Ideally we can choose between adjusting individual joints in the chain, or adjusting the ends of the chain and scaling the joint-bindings along with them.

If there is a use for it, we could indeed introduce a "new" option that instead places the bones at evenly spaced points along the curve.

But I don't think a "preserve bone scale exactly" is the way to go. The manual and tool tip of the current options "None" and "Bone Original" says that is should preserve the length, but it only does so if the curve itself is a straight line.
As it calculates the "distance traveled" along the curve of each bone and not the final "shortest path" from the head to the tail of the bone.

Having a "actually do what is says it should do" checkbox is not good in my opinion.

Anyways, it seems there is still some anomalous behavior. I ran into this before when I tried this, I don't remember if I was able to figure out a solution.

Nice catch! Thanks for testing.

This was because I didn't check if the first test segment was the same as the previous bone segment.
So it could go backwards in the chain if the the tessellated segments were longer than the bones.
I fixed this now! :)

However while testing I also noticed that the curve path does not match with the displayed path.
This was true before my patch and also with other constraints (like the object "follow path" constraint):

Fixing that will be for an other patch though.

I've actually already made a Bug Report for that: https://developer.blender.org/T81707

"Having a "actually do what is says it should do" checkbox is not good in my opinion." I agree, but the other thing that isn't good is naming things incorrectly to begin with, and then making changes to established behavior. to match the name.. although to be fair this is only as old as 2.80 and has always been broken. The only example of anyone using it that I've been able to find is a rig Alexander Gavrilov made for Rigify, which avoids the bug by using a perfectly straight curve. Well, I use it, but I can adapt 😄 I would be thrilled to have to adapt to a bugfix for this.

If you want to preserve the length of the bones there is no way around it.

Yes, this is true -- if you want to preserve the length of every individual bone. You can, however, preserve the length of the *entire chain* relative to the curve, which is what the current behavior is. Or, well, it tries. It has a fatal miscalculation, in that it doesn't take into account the shape of the curve and how, because bones are long and straight, the bones end up being shorter than the curve (which is continuous). This is generally true, and I think it's related to the problem with where_on_path giving different results than the viewport representation of the curve. Unfortunately, since where_on_path is very mathy, I don't understand it well enough to try and fix it myself :/ Well, I have written about it elsewhere.
Here's a mockup I made which uses my patch: https://developer.blender.org/T81704#1058662
And you should keep reading past that a bit to see the Dr.'s and Brecht's feedback. I think I explain what causes the bug a little higher up and also in the comments of my patch.

To explain further: The current algorithm stores "joint bindings" that say how far along the curve each joint should be (as a percentage of a curve, from 0.0-1.0). They are calculated, however, as a percentage of the length of the bone chain. So, in order to make up for the difference, the joint bindings are scaled by a bit, which IIRC is the length of the bone chain divided by the length of the curve. (Now we're at "None" y scale mode. The pose scale is also multiplied with this scaling factor to get to "Bone Original".) Anyhow, this ends up being wrong, because the bone's joint bindings should have been stored relative to the curve to begin with, and they shouldn't be scaled like that.
My working patch, IIRC, solved this problem by calculating the joint bindings relative to the curve and then scaling them only with the pose-scale. It might have been more involved than that, but the only thing it really needs is a way of getting some data from the curve in a way that is guaranteed to work (right now it uses the runtime data which can crash Blender if it isn't available). I probably wrote out my reasoning for everything in comments under the patch, though. In fact, I'd like for you to read a lot of it. I was careful to write it all down because I hoped it would help others (or me, since I tend to work in fits and starts). The problem is that there's a lot and I make a few incorrect guesses that are corrected later. So if you read it, maybe start at the end and read backwards until you have what you need. You'll also see a lot of the errors I make and correct, which will help you to know how to test your patch for corner cases that are hard to find. ( I stopped working on my patch when it reached a point where I could use it in my production in a pinch, and my focus shifted back to my production once the vacation ended. Oh, and I did a distro-hop since then! Perhaps you've motivated me to take it back up, though.)

EDIT:

This was true before my patch and also with other constraints (like the object "follow path" constraint):

IIRC where_on_path is only used by these two constraints and it's the culprit in both of them.
also, here is a link to a description of the way my patch works, so you don't have to search for it. https://developer.blender.org/D9244#243080

Updated so it applies to the current master branch after my anim_path changes.

@Joseph Brandenburg (TheAngerSpecialist) do you have time to test if everything still works as before?
I had to change the code a bit after my changes to anim_path. It seems to work as before to me, but you can never be too sure ;)

Accepting the patch; further improvements/extensions can be made separately from this fix.

This revision is now accepted and ready to land.Apr 13 2021, 6:25 PM