Page MenuHome

Limit Rotation: add an Euler Order option and orthogonalize the matrix.
ClosedPublic

Authored by Alexander Gavrilov (angavrilov) on Nov 22 2020, 12:27 PM.

Details

Summary

Since Limit Rotation is based on Euler decomposition, it should allow
specifying the order to use for the same reasons as Copy Rotation does,
namely, if the bone uses Quaternion rotation for its animation channels,
there is no way to choose the order for the constraint.

In addition, add a call to orthogonalize the matrix before processing
for the same reasons as D8915, and an early exit in case no limits are
enabled for a bit of extra efficiency.

Since the constraint goes through Euler decomposition, it would remove
shear even before the change, but the rotation won't make much sense.

Diff Detail

Repository
rB Blender
Branch
temp-angavrilov-limit-rotation (branched from master)
Build Status
Buildable 11384
Build 11384: arc lint + arc unit

Event Timeline

Alexander Gavrilov (angavrilov) requested review of this revision.Nov 22 2020, 12:27 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 22 2020, 12:29 PM
This revision was automatically updated to reflect the committed changes.

Resubmitting due to a weird glitch (closed by unrelated commit).

Playing the animation in this file can show the difference between doing euler decomposition as is and orthogonalizing first. Without orthogonalizing the rotation is wrong, and the end of the bone follows a peanut-like shape instead of an ellipse:

Alexander Gavrilov (angavrilov) retitled this revision from Fix unclear tooltip for the Affect Transform option of constraints. to Limit Rotation: add an Euler Order option and orthogonalize the matrix..Nov 22 2020, 12:40 PM
Alexander Gavrilov (angavrilov) edited the summary of this revision. (Show Details)

Although I don't understand the math part, pressing play in that file before/after the patch shows pretty clearly the benefit of orthogonalizing the matrix, since the Limit Rotation constraint with Limit XYZ all turned off should have no effect at all, and that is apparently not the case in current master. That part really just seems like a bugfix, so that gets a +1 from me.

For the new Order input in the constraint though, I tried coming up with a case where there is a difference between the different rotation modes, but couldn't:


This file looks identical before/after the patch, no matter what rotation order I choose.

And in the file attached in T83908, the rotation order always seems to be left as Default, so that doesn't show how it can be useful either.

Euler angles are meaningless without a rotation order, and the order does matter a lot. You need to use big angles on multiple axes to see the difference obviously. The reason for the option is explained in the description, and Copy Rotation already have an order option.

What is obvious to you is not obvious to me at all! I have admitted to you many times before and will a thousand times again if I have to, that I am just a fraud who clicks buttons and somehow gets rigs out of them without understanding literally the simplest piece of maths that's going on under the hood. You could put two 2x2 integer matrices in front of me and tell me to multiply them, and I would stare at you with hopeless dead eyes, not knowing how to do it without googling it first.

And I think patches will get into Blender a lot smoother if contributors assume that everyone is as dumb as I am, lol. In this case for example, if you hit me with an example of where it matters, me or a code reviewer don't have to spend time trying to figure it out by ourselves, if you can slap a blend file together in 2 minutes, even if it is something totally obvious.

This demo file of mine from 2 years ago demonstrates decomposing the same rotation into different euler orders. The one you see immediately after opening the file has two similar outputs and one very different; and in different orientations the similar and dissimilar groups change.

Euler angles is the same as 3 parent objects, each of which rotates only on one axis. Changing the order of parenting affects the end result.

While I think that file is really awesome at visualizing euler rotations, it only shows why result is supposed to be different in the case of Limit Rotation. And I actually thought I understood this part, until the moment when I actually changed the rotation order in the actual Limit Rotation constraint with the patch and I saw no difference, and I was actually surprised. This is why I'm asking for a file where changing the rotation order with the patch actually changes the result of the Limit Rotation constraint.

Sorry to keep asking for further and further clarification in not only this but other patches, but the usefulness of new features must be crystal clear, especially since this area of Blender is not that well known by most who are available to review patches (afaik).

Well, this file demonstrates empties animated in quaternion space and clamped with Limit Rotation using two different euler orders. The file requires a build with this patch applied.

Without the patch the order used by the constraint is locked to the one used by the animation channels, or only XYZ if quaternion.

Thank you, that's perfect! When changing the order, the empty snaps to different orientations, so it's crystal clear that it's doing something.

So as far as I'm concerned, the files in D9626#238958 and D9626#289830 show perfectly why this patch is useful, so now it just needs code review. I'll help kicking devs for that :)

[...] the Limit Rotation constraint with Limit XYZ all turned off should have no effect at all

This is not the case with this code, as the existence of the constraint will already perform sheer removal, even when all Limit XYZ options are disabled. I think that's good, though. I suspect it would be unexpected for artists to have the constraint only remove sheer when one of the limits is enabled; if that would be the case, enabling a limit on the X axis could also unexpectedly influence the Z axis (because of the sheer removal).

source/blender/blenkernel/intern/constraint.c
1556

I had some doubts about this at first, because I don't like the "this can be used for something else that's not obvious from the name" approach. I'd be much more in favour of a separate "remove sheer" constraint. However, in this case I think it would be unexpected for artists to have the constraint only remove sheer when one of the limits is enabled; if that would be the case, enabling a limit on the X axis could also unexpectedly influence the Z axis (because of the sheer removal).

1559–1562

This doesn't directly have anything to do with the added option, so it shouldn't be part of this patch. The change itself is good, though, so as far as I'm concerned it can be committed separately.

This revision is now accepted and ready to land.Jun 4 2021, 11:11 AM