Page MenuHome

Negate Shear in Copy Rotation
Needs ReviewPublic

Authored by Joseph Brandenburg (TheAngerSpecialist) on Sep 17 2020, 3:39 AM.

Details

Summary

This patch negates the effects of shear in the Copy Rotation constraint, which causes incorrect results and even flipping on rare occasions.

Here is the bug report associated with the issue: T80970
This bug report has an example of the problem in a .blend file.

This patch will change existing .blend files if a copy rotation constraint is used with a target that has shearing -- instead of pointing in the "wrong" direction (due to the shear), the constrained object will copy the final rotation of the target, with the shearing applied.

Proposed Solution: Correct the matrix of the constraint target for shear.

Limitations of proposed solution:

  • It's conceivable that someone has used the "incorrect behaviour" in a rig... although I doubt this. It may be necessary to create an "allow shear" checkbox that is disabled by default to maintain the legacy behaviour.

No UI changes are needed for this patch :)

Diff Detail

Repository
rB Blender
Branch
rotlike_skip_decompose (branched from master)
Build Status
Buildable 10751
Build 10751: arc lint + arc unit

Event Timeline

@Wayde Moss (GuiltyGhost) I mentioned this in blender.chat earlier. This is what I meant.

The rig I encountered the problem with was for a production, I don't think I can share the file. I'm gonna try to make an example, but it was a rare glitch in a rather complex rig.

Edit: I'm having trouble reproducing the issue. Maybe it was even caused by something else, although changing the rotation order of the constraint fixed it (and I have since implemented a workaround). Still, I think the idea behind this commit is solid - skip the complex math if you don't have to, use the more reliable, simple math when you can. The fact that the other code path can rarely lead to errors is almost irrelevant.

Edit: I've run into the issue again, will post the file as soon as I can remove all the unnecesary stuff. My patch improves the behavior but doesn't seem to completely fix it.

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Sep 19 2020, 1:11 PM

I made a bug report about the rare incorrect results, and identified the cause of the problem: T80970

Here is the .blend file:

Here's how my little patch changes it:
Before:


After:

So you can see, there's still more to do, but my patch does something to correct the flipping, at least. Ideally, the bone in the middle will follow the long bone, as the one in the bottom does. (I used a workaround trick to force it to behave correctly).

OK, I have corrected the shearing and implemented inversion, and I've set the new code to only run when Euler Order is "default".

Joseph Brandenburg (TheAngerSpecialist) retitled this revision from Skip matrix decomposition in Copy Rotation if all axes are selected and none are inverted. to Fix shear, Skip matrix decomposition in Copy Rotation if all axes and all or none are inverted..Sep 19 2020, 11:23 PM
Joseph Brandenburg (TheAngerSpecialist) edited the summary of this revision. (Show Details)

Here's the new "after":

Maybe I should split this into two patches, one for the shear fix and one for the new optimized code path? The shear fix is a single line in my patch.

Finally this commit will change the behavior of the Copy Rotation constraint, although I doubt anyone is depending on the effects of shearing (which are unpredictable). It's conceivable that someone is using the Euler decomposition when inverted, but it's hard to imagine.My patch retains the old behavior if the Euler order is explicitly chosen.

So, I have a few ideas about how to solve that problem, but I don't know how to do any of them:

a) Versioning that sets the Euler Order to the value that was being chosen by "Default" before.
b) An option to "Do Not Decompose Matrix" that would then hide the Euler Order and the XYZ and invert XYZ and replace them with an "invert" checkbox. This seems simple enough.
c)Don't touch Copy Rotation, instead - modify Copy Transform to give the user the ability to disable Location, Rotation, Scale, Shear... and then add invert options. This seems like a bigger change than is necesary.

Anyways, I am happy because my code does what I want it to!

Fix formatting, thanks Lazy Dodo :)

Alexander Gavrilov (angavrilov) requested changes to this revision.Sep 20 2020, 2:42 PM
Alexander Gavrilov (angavrilov) added inline comments.
source/blender/blenkernel/intern/constraint.c
1828

This bit of code is C-like, so put this at the start of the scope with the other vectors and matrices.

1839

This condition is completely broken, and should be rewritten to correctly check bitwise masks. Also, it should check for the Add and Offset modes, as those can't be done without decomposing.

1842

The only meaningful way to 'invert' without decomposing is to invert the matrix, and I have actually been considering whether to add that option or not here and in the Copy Transforms constraint, since currently the only way to invert a matrix is with multiple bones and parenting. This on the other hand makes no sense whatsoever.

1846

What is the point of quaternions here? Just converting mat4 to mat3 is much simpler and efficient, as it is just copying values between two arrays of different dimensions.

This revision now requires changes to proceed.Sep 20 2020, 2:42 PM

This condition is completely broken, and should be rewritten to correctly check bitwise masks. Also, it should check for the Add and Offset modes, as those can't be done without decomposing.

Does this mean using a binary operator against a number that has the right bits set? Something like data->flag & 13? You'll have to forgive me here, I'm not very familiar with C. Anyways, at this point in the code I only care that all of the invert flags are the same, and this gives me the correct results as far as I can tell. I check later on if *one* is set, since if they are all the same, if one is set, all of them are.

The only meaningful way to 'invert' without decomposing is to invert the matrix, and I have actually been considering whether to add that option or not here and in the Copy Transforms constraint, since currently the only way to invert a matrix is with multiple bones and parenting. This on the other hand makes no sense whatsoever.

Inverting the matrix is the first thing I tried, but it seemed to negate the rotation... at any rate, the behavior was not correct. I agree that my solution is a hack. I'm going to poke at this a little more. Could inverting the matrix lead to scaling? I want to avoid that.

Edit: Here are some pictures of the weird behavior I am talking about:
World <--> World


Local <--> Local

While playing around with the code earlier I noticed that bones have some extra information in the last row of the matrix (The displacement from head to tail, I think). Could this be affecting things, and how do I correctly account for this?

What is the point of quaternions here? Just converting mat4 to mat3 is much simpler and efficient, as it is just copying values between two arrays of different dimensions.

I'm a little embarrassed about this one. I was getting weird behavior when I tried this, at the time I didn't understand the math library - the first argument is the variable that is modified! I'm used to mathutils, so this threw me off. I expected the last variable to be the one that gets modified. I was trying to use copy_m4_m3 instead of copy_m3_m4.

Anyways what did you think about correcting for shear? I don't even really care about the rest :P The shearing was what caused the problem that motivated me to try this patch. That's a much more important issue to me than optimizing the one case.

Thank you. It'll probably take me a few patches before I can wrap my head around Blender's code base and style -- but before long I won't need too much hand-holding! I'm looking forward to contributing.

  • Adressing comments: Improve the comparison and add comparison

for Add and Offset, correct some math, get ready for fixing matrix inversion

Currently the matrix inversion is not correct. I don't know why, so I would
appreciate any advice about this.

@Wayde Moss (GuiltyGhost) I mentioned this in blender.chat earlier. This is what I meant.fix it.

I'm likely not the person you should be asking help or advice from. I'm not a core developer nor have I done significant work in this area.

Sybren A. Stüvel (sybren) requested changes to this revision.Mon, Oct 12, 12:07 PM

@Joseph Brandenburg (TheAngerSpecialist) Thanks for the patch. Could you maybe update the patch description so that it's a bit more according to the Ingredients of a Patch guideline? I know that the patch is older than that list ;-) It would be nice to at least have some example file which can be used to reproduce the described problem, and a description of how (or not) this patch influences existing blend files.

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

Actually, the requirement of C code to declare all variables at the top of the scope is gone, and it's preferred to declare them close to the actual use.

1837–1839

Never use magic numbers. What are 7 and 119?

This revision now requires changes to proceed.Mon, Oct 12, 12:07 PM

It would be nice to at least have some example file which can be used to reproduce the described problem, and a description of how (or not) this patch influences existing blend files.

Thanks Dr. @Sybren A. Stüvel (sybren), I've updated the patch description. The example file is in the bug report that I made about the problem (which is awaiting triage :P ).

Actually, the requirement of C code to declare all variables at the top of the scope is gone, and it's preferred to declare them close to the actual use.

OK, will keep that in mind and fix!

Never use magic numbers. What are 7 and 119?

That is the bit-mask that has the flags, in response to @Alexander Gavrilov (angavrilov) 's comment: "This condition is completely broken, and should be rewritten to correctly check bitwise masks"... I am a Python programmer, primarily, so I did the best I knew how, and it looks to me like this is how bit-wise masks are checked elsewhere in Blender's code. Is there a better way?

Finally, the most important thing in the patch is actually the part that fixes the shear. I am not particularly attached to the part that skips the decomposition. It's probably faster, and it's *hypothetically* more accurate, but a) it's still missing matrix inversion because I don't know the correct way to do this (invert_m3 gave me weird results), and b) it's more of an optimization than a fix for the actual problem I had encountered. I started working on this before I had really narrowed down the problem that I was facing (shear). So I am OK with removing all of that and only including the bit that fixes shear... the rest can be tackled in a different patch! I really need feedback about this, which is why I haven't been updating this. I originally solved the wrong problem! But I think both of these are good ideas, only one is more important than the other.

Thanks!

Never use magic numbers. What are 7 and 119?

That is the bit-mask that has the flags

As I said, never use magic numbers. If you want specific bitflags, use the named constants/enum values for this. Maybe that's also what @Alexander Gavrilov (angavrilov) is referring to as "broken"?

Finally, the most important thing in the patch is actually the part that fixes the shear. I am not particularly attached to the part that skips the decomposition.

In that case please split it up into two parts.

Joseph Brandenburg (TheAngerSpecialist) edited the summary of this revision. (Show Details)
  • Removed code that skips decomposing the matrix, that should be handled elswhere.

Still to do:

  • ensure that the constraint behaves correctly for all combinations of axes and inversion
  • ensure that the constraint behaves correctly for bones and objects.
  • ensure ... when the influence is changed.
  • ensure ... when spaces are changed

I have notices that disabling the Z axis *really* messes things up, and I think this is related to the axis that the orthogonalize matrix function is using. Anyhow, this is obviously a problem that I need to solve! Inversions seem OK, though.

Restart the patch from the current master, it seems that I made a mistake
with brackets that caused the weird Z axis stuff. Now it's OK!

So, something that concerns me here is this: The patch actually removes shear if the constrained object has shear, unless the owner or target space is Local Space. Although... that's what LTS does, too! So I don't think it's a problem. To clarify: my patch corrects the constraint target's matrix so that the rotation values reflect the shearing. It doesn't do anything with the matrix once it's computed to affect the shearing -- all it does is fix the input value of the calculation.

Joseph Brandenburg (TheAngerSpecialist) retitled this revision from Fix shear, Skip matrix decomposition in Copy Rotation if all axes and all or none are inverted. to Negate Shear in Copy Rotation.Tue, Oct 20, 1:36 AM

It's conceivable that someone has used the "incorrect behaviour" in a rig... although I doubt this.

Why do you doubt this? It's very common for people to try things out, see how they behave, and accept how it behaves as "correct".

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

mat isn't the most expressive of names (although it's used a lot, I don't like it). Maybe orthogonal_input_matrix is better?

Why do you doubt this? It's very common for people to try things out, see how they behave, and accept how it behaves as "correct".

Well, one argument is, nobody likes shear to appear in the first place. Second, I bet euler decomposition math is utterly broken if given a non-orthogonal matrix so it's basically unspecified behavior.

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

It would be a completely different naming style to all the surrounding code, which doesn't look good. If you keep the same style it would be something like tgtmat or inmat.

Well, one argument is, nobody likes shear to appear in the first place.

That argument is only valid after asking everybody.

Second, I bet euler decomposition math is utterly broken if given a non-orthogonal matrix so it's basically unspecified behavior.

There is no such thing as "unspecified behaviour" when it comes to Blender. If Blender consistently behaves in a certain way, that's what people expect & build their rigs on.

Having said that, I can understand the reasoning that a Copy Rotation constraint should only copy rotations, and that other effects (translation, scale, shear) are thus bugs.

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

Assuming that tgt means "target", thank you for making my point. The matrix in question is not the input/target matrix. It's an orthogonalised copy of it.

One of my goals for the Animation & Rigging module is to get code that's easier to understand. Clear (instead of overly abbreviated) names are certainly appreciated, even if that causes some different styles to be used in an area.

I'd love to test the patch as it is if possible - I'd also like to see us fix bugs like this in the constraint system, but I'm aware of compatibility issues.

  • 'good' riggers avoid problems is by using 'too many' bones, to isolate the issue, for instance, adding a bone with a tracking constraint, or adding more constraints on top to fix the problem, or by finding alternate solutions
  • these workarounds should continue to work regardless of the fix.
  • in some cases, rigs make it to animation with the shear behavior still present, and the animator 'did stuff' to make it work, in this case it is possible that fixing the bug will break the animations.

Maybe in that last case introducing a 'compatibility' checkbox - something like legacy mode - might help

Test builds are available for Windows and Linux

If you need a different platform, either trigger a 'custom branch' build yourself at https://builder.blender.org/admin/#/builders or tell me which platform you need and I'll do it.

Having looked at the example case, I feel like this is a good candidate to fix in 2.92; the case is somewhat rare, and as pointed out above, will often result in the rigger coding a workaround (I sometimes have an un-animated bone that I copy scale from, plus an extra child bone as a target, for many cases and I feel perhaps this one)
working rigs will have the workaround, and not be affected by fixing the bug (workarounds should still work while becoming un-needed) - broken rigs in this case will mainly be too broken to animate, so I doubt we'll break many (any?) files in the wild with this bugfix.
I've been opening existing animation files with it, so far so good. I encourage any testers to do the same (open it on your files with existing rigs and animations)

Thanks for your help @bassam kurdali (bassamk) !
Anyhow:

If Blender consistently behaves in a certain way, that's what people expect & build their rigs on.

I think this is precisely the reason why it's OK to proceed - this bug results in inconsistent and unpredictable behavior and replaces it with consistent, predictable behavior. Now, I'd be 110% OK with careful testing on this... I haven't had a chance to research creating unit tests but I might be able to do so for this and other things at some point (if there isn't one already).

I agree it's good to proceed.

@Joseph Brandenburg (TheAngerSpecialist) in T80970 you write:

I made a diff, D8915, which attempts to solve this problem. It corrects the flipping, but I don't think it accounts for the shear correctly, and it only works in one case.

Is this still accurate? If so, what do you need to get this resolved properly?