Details

Reviewers

Sybren A. Stüvel (sybren)
Demeter Dzadik (Mets)

Group Reviewers

Animation & Rigging

Maniphest Tasks

T81541: Symmetrize Transform Constraint, Y rotational axis unexpected results.

Commits

rB623ff64a2789: Fix T81541: Symmetrize Transform Constraint, Y rotational axis unexpected…

Summary

The case where Y rotation is mapped to Y rotation was not handled.
This is now fixed.

It seems like @Demeter Dzadik (Mets) and I missed this case when testing this out previously.

Test file:

symm_test.blend1 MBDownload

(put it in lib/tests/animation/symm_test.blend)

Diff Detail

Repository: rB Blender

Event Timeline

Sebastian Parborg (zeddb) requested review of this revision.Oct 14 2020, 6:28 PM

Sebastian Parborg (zeddb) created this revision.

Sebastian Parborg (zeddb) added a task: T81541: Symmetrize Transform Constraint, Y rotational axis unexpected results..

Is this flip also necessary when the target bone is not part of the symmetrized bones? Or does it always need the flip?

At least in the example file, this is the case.

I also think that it should in general be true.
If the control bone is not duplicated, then the constraint result should probably still be mirrored.

Though I did notice that is says in a comment that we will skip this conversion of the target bone is not duplicated.
However, this is not the case, we always mirror the constraints even if the target bone was not duplicated.

So I guess I should perhaps fix this (so add the checking and abort code)?

Probably the best way to go about this is to create a unit test that actually tests various situations. This means that the test will fail on the current code, and then you can fix the code to pass the test and have the warm feeling inside :)

Updated with added unit test.
Test file:

symm_test.blend1 MBDownload

I added the test here as well but if this is accepted I will split this into two commits.
One for the fix and one for the test.

Sebastian Parborg (zeddb) added a project: Animation & Rigging.Jan 8 2021, 7:18 AM

bl_rigging_symmetrize.py doesn't adhere to the Python code style guidelines. For one, the lines are way too long.
It's also not necessary to repeat the tested values in the message, they should be reported by the unit test framework anyway.

In D9214#250382, @Sebastian Parborg (zeddb) wrote:

I added the test here as well but if this is accepted I will split this into two commits.
One for the fix and one for the test.

IMO the test code should be in the same Git commit as the fixed code. Of course the test file in SVN requires a second commit, so if those are the two commits you meant, sounds good to me.

I've added @Demeter Dzadik (Mets) as reviewer to think about the mirroring when not selected stuff. I wouldn't know what is desired in such a case.

This revision now requires changes to proceed.Jan 8 2021, 11:58 AM

Not sure 100% sure if I'm understanding the discussion correctly.

So I think the question is, in a scenario where "Ear.L" and "Ear.R" both exist, "Ear.L" is selected and we hit Symmetrize, should "Ear.R" retain its existing constraints or get the symmetrized constraints?

In that case I say it shouldn't retain anything. Symmetrize should be completely destructive to the target side (in this example the right side), such that the result of the operator is the same regardless of whether the target side had already existed or not. Put another way, this operator is effectively the stand-in for a Mirror Modifier for armatures (and does a pretty good job at that!).

But lemme know if I understood the question correctly!

Sebastian Parborg (zeddb) updated this revision to Diff 32647.Jan 11 2021, 11:30 PM

In D9214#250453, @Sybren A. Stüvel (sybren) wrote:

bl_rigging_symmetrize.py doesn't adhere to the Python code style guidelines. For one, the lines are way too long.

I've ran it through a pep8 formatter. However it seems like either the line are still too long or not indented just as it want to.
So I do a middle ground solution, hope it is ok.

It's also not necessary to repeat the tested values in the message, they should be reported by the unit test framework anyway.

Fixed!

In D9214#250501, @Demeter Dzadik (Mets) wrote:

But lemme know if I understood the question correctly!

It was basically if you had target bones that were not copied (a none .L or .R bone or simply left out of the operation) some comment said that the constraints would not be flipped.
But this was never the case in the code. I can't seem to find the comment anymore either, so I guess it was cleaned up after I made that comment.

I agree that things should always be synced, so I don't think there is anything left to discuss.

Ah, I see. In that case I have no strong feelings because my workflow always starts with the Select Pattern operator to select bones ending in .L, so I never have center bones selected. If someone thinks it makes sense to flip constraints in this case, go for it, otherwise, just leave it how it is :) It's just not a case that comes up in my workflow.

The test fails on my machine:

test 41
    Start 41: bl_rigging_symmetrize

41: Test command: /home/sybren/workspace/blender-git/build_linux/bin/blender "--background" "-noaudio" "--factory-startup" "--debug-memory" "--debug-exit-on-error" "--python-exit-code" "1" "--python" "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py" "--" "--testdir" "/home/sybren/workspace/blender-git/blender/../lib/tests/animation"
41: Environment variables: 
41:  LSAN_OPTIONS=exitcode=0:
41: Test timeout computed to be: 10000000
41: FSwitching to fully guarded memory allocator.
41: Blender 2.93.0 Alpha
41: Read blend: /home/sybren/workspace/blender-git/blender/../lib/tests/animation/symm_test.blend
41: 
41: ======================================================================
41: FAIL: test_symmetrize_operator (__main__.ArmatureSymmetrizeTest)
41: Test that the symmetrize operator is working correctly.
41: ----------------------------------------------------------------------
41: Traceback (most recent call last):
41:   File "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py", line 56, in test_symmetrize_operator
41:     self.assertEqualSymmetrize(arm, expected_arm)
41:   File "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py", line 198, in assertEqualSymmetrize
41:     bone_name, const_name, var))
41: AssertionError: 2.6555182933807373 != 2.6555185317993164 : Missmatching constraint value in pose.bones[DEF_arm.001.L].constraints[IK].pole_angle
41: 
41: ----------------------------------------------------------------------
41: Ran 1 test in 0.018s
41: 
41: FAILED (failures=1)
1/1 Test #41: bl_rigging_symmetrize ............***Failed    0.16 sec

Don't use self.assertEqual() for float comparisons, use assertAlmostEqual() instead.

This revision now requires changes to proceed.Jan 18 2021, 11:11 AM

Glad that it failed on your system so that we caught this in review! ^^

It still fails for me:

40: Test command: /home/sybren/workspace/blender-git/build_linux/bin/blender "--background" "-noaudio" "--factory-startup" "--debug-memory" "--debug-exit-on-error" "--python-exit-code" "1" "--python" "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py" "--" "--testdir" "/home/sybren/workspace/blender-git/blender/../lib/tests/animation"
40: Environment variables: 
40:  LSAN_OPTIONS=exitcode=0:
40: Test timeout computed to be: 10000000
40: ESwitching to fully guarded memory allocator.
40: Blender 2.93.0 Alpha
40: Read blend: /home/sybren/workspace/blender-git/blender/../lib/tests/animation/symm_test.blend
40: 
40: ======================================================================
40: ERROR: test_symmetrize_operator (__main__.ArmatureSymmetrizeTest)
40: Test that the symmetrize operator is working correctly.
40: ----------------------------------------------------------------------
40: Traceback (most recent call last):
40:   File "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py", line 56, in test_symmetrize_operator
40:     self.assertEqualSymmetrize(arm, expected_arm)
40:   File "/home/sybren/workspace/blender-git/blender/tests/python/bl_rigging_symmetrize.py", line 198, in assertEqualSymmetrize
40:     bone_name, const_name, var))
40:   File "/home/sybren/workspace/blender-git/build_linux/bin/2.93/python/lib/python3.7/unittest/case.py", line 897, in assertAlmostEqual
40:     if round(diff, places) == 0:
40: TypeError: 'str' object cannot be interpreted as an integer
40: 
40: ----------------------------------------------------------------------
40: Ran 1 test in 0.017s
40: 
40: FAILED (errors=1)

tests/python/bl_rigging_symmetrize.py
59	This function is far too complex, which is evident from its nesting depth. It should be split up into smaller functions.
131	Why would a `None` value not be compared to the expected value? If the expected value is not `None`, this would silently ignore an error case.
132	Don't use `type(value) == sometype`, use `isinstance(value, sometype)` instead.
146–148	There is no need to iterate three times over `bone_variables`, and it looks like you don't even need to construct a list. Just define a set of prefixes, then use a double generator expression, like: prefixes = ("ik_", "lock_ik", "use_ik") ik_bone_variables = ( var for var in bone_variables for prefix in prefixes if var.startswith(prefix) )

This revision now requires changes to proceed.Jan 26 2021, 5:55 PM

Sybren A. Stüvel (sybren) added inline comments.Jan 28 2021, 10:15 AM

tests/python/bl_rigging_symmetrize.py
146–148	My Python is getting rusty. str.startswith() supports tuples for prefixes, so you can use: prefixes = ("ik_", "lock_ik", "use_ik") ik_bone_variables = ( var for var in bone_variables if var.startswith(prefixes) )

Updated with the requested changes.

However for me the test passes before and after my new changes.

30: Test command: /home/zed/prog/blender/build_nodev/bin/blender "--background" "-noaudio" "--factory-startup" "--debug-memory" "--debug-exit-on-error" "--python-exit-code" "1" "--python" "/home/zed/prog/blender/tests/python/bl_rigging_symmetrize.py" "--" "--testdir" "/home/zed/prog/blender/../lib/tests/animation"
30: Environment variables:
30:  LSAN_OPTIONS=exitcode=0:
30: Test timeout computed to be: 10000000
30: Switching to fully guarded memory allocator.
30: Blender 2.93.0 Alpha (hash 78ff8526808a built 2021-02-02 23:42:46)
30: /run/user/1000/gvfs/ non-existent directory
30: Read blend: /home/zed/prog/blender/../lib/tests/animation/symm_test.blend
30: .
30: ----------------------------------------------------------------------
30: Ran 1 test in 0.063s
30:
30: OK
1/1 Test #30: bl_rigging_symmetrize ............   Passed    0.28 sec

So I'm not sure what it going on with your test failure.
It seems like a string was passed to assertEqualVector but I'm not sure why or where that would happen.
I also don't know why it passes for me and not you.

Sebastian Parborg (zeddb) updated this revision to Diff 47830.Feb 1 2022, 6:04 PM

Sebastian Parborg (zeddb) updated this revision to Diff 47831.Feb 1 2022, 6:06 PM

Sebastian Parborg (zeddb) edited the summary of this revision. (Show Details)