Fix #129094: Sub-targets not symmetrized if from different armature #129169

Christoph Lendenfeld · 2024-10-17T18:42:52+02:00

2024-10-17 18:42:52 +02:00

When symmetrizing a bone (in edit mode) any constraint subtargets were name flipped,
if possible and they exist. This only worked if the target is the armature of the bone
being flipped.
This patch changes that so a subtarget flip is always attempted for targets that are an armature.

When symmetrizing a bone (in edit mode) any constraint subtargets were name flipped, if possible and they exist. This only worked if the target is the armature of the bone being flipped. This patch changes that so a subtarget flip is always attempted for targets that are an armature.

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-10-17 18:42:52 +02:00

Christoph Lendenfeld added 1 commit 2024-10-17 18:43:04 +02:00

the fix 9c7d5f0bf6

Christoph Lendenfeld added 1 commit 2024-10-17 18:46:51 +02:00

fix potential crash 8635f0c8e2

Sybren A. Stüvel requested changes 2024-11-01 11:55:20 +01:00

Sybren A. Stüvel left a comment

It would seem that there's now a chance that the same constraint may get flipped twice?

Before: updateDuplicateSubtarget() ensured that the flipping either happens when the subtarget is also flipped OR when lookup_mirror_subtarget is true.

After: if the subtarget bone is also flipped itself, both updateDuplicateSubtarget() and mirror_pose_bone() might flip the constraint subtarget.

Depending on the order of the calls, this may or may not be a problem. updateDuplicateSubtarget() will work fine regardless of the call order, but mirror_pose_bone() will always flip, so if it is called after updateDuplicateSubtarget(), the flipping might happen twice.

To me this feels a bit fragile, as there now seem to be two code paths in different functions that effectively do the same thing but differently.

I think it would be good to have a few test cases in a unit test, to cover the various use cases for this operator.

It would seem that there's now a chance that the same constraint may get flipped twice? Before: `updateDuplicateSubtarget()` ensured that the flipping either happens when the subtarget is also flipped OR when `lookup_mirror_subtarget` is `true`. After: if the subtarget bone is also flipped itself, both `updateDuplicateSubtarget()` and `mirror_pose_bone()` might flip the constraint subtarget. Depending on the order of the calls, this may or may not be a problem. `updateDuplicateSubtarget()` will work fine regardless of the call order, but `mirror_pose_bone()` will always flip, so if it is called after `updateDuplicateSubtarget()`, the flipping might happen twice. To me this feels a bit fragile, as there now seem to be two code paths in different functions that effectively do the same thing but differently. I think it would be good to have a few test cases in a unit test, to cover the various use cases for this operator.

source/blender/editors/armature/armature_add.cc Outdated

						
				@ -987,0 +974,4 @@

				  LISTBASE_FOREACH (bConstraint *, constraint, &pose_bone->constraints) {

				    ListBase targets = {nullptr, nullptr};

				    BKE_constraint_targets_get(constraint, &targets);

				    char name_flip[MAX_ID_NAME - 2];

Sybren A. Stüvel commented

2024-11-01 11:39:18 +01:00

I think name_flipped would be a better name for this variable.

I think `name_flipped` would be a better name for this variable.

👍 1

Christoph Lendenfeld commented

2024-11-05 17:44:17 +01:00

well I wrote a test, then realized there is one already in bl_rigging_symmetrize
The issue is that it's too generic and checks for equality on constraint variables which isn't always valid.
Will have to rewrite that a bit because those are of course broken now

well I wrote a test, then realized there is one already in `bl_rigging_symmetrize` The issue is that it's too generic and checks for equality on constraint variables which isn't always valid. Will have to rewrite that a bit because those are of course broken now

❤️ 1

Christoph Lendenfeld added 2 commits 2024-11-05 17:44:45 +01:00

Merge branch 'blender-v4.3-release' into fix_symmetrize_constraint_targets a6fe8fd0b5

unit tests for symmetrize operator d1dc9368a0

Christoph Lendenfeld changed target branch from blender-v4.3-release to main

2024-11-07 12:42:29 +01:00

Christoph Lendenfeld added 2 commits 2024-11-07 12:48:11 +01:00

test wip f70e8eaa92

Merge branch 'main' into fix_symmetrize_constraint_targets 31d84eb1b6

Christoph Lendenfeld added 1 commit 2024-11-07 12:51:18 +01:00

undo changes to armature test a038345857

Christoph Lendenfeld added 1 commit 2024-11-07 14:06:34 +01:00

new way to fix this

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

c6f3f8e005

Christoph Lendenfeld commented

2024-11-07 14:08:19 +01:00

@blender-bot build

I undid what I had done before. Now I just extended the existing function to use the bones of the target, instead of relying on an edit bone list that is passed in.
Also I added a unit test to explicitly test against symmetrizing of subtargets on different armatures

@blender-bot build I undid what I had done before. Now I just extended the existing function to use the bones of the target, instead of relying on an edit bone list that is passed in. Also I added a unit test to explicitly test against symmetrizing of subtargets on different armatures

Christoph Lendenfeld requested review from Sybren A. Stüvel 2024-11-07 14:08:26 +01:00

Sybren A. Stüvel approved these changes 2024-11-11 13:13:05 +01:00

Sybren A. Stüvel left a comment

LGTM, just one minor thing that can be handled when landing.

source/blender/editors/armature/armature_add.cc Outdated

						
				@ -417,3 +419,2 @@

				      }

				      oldtarget = get_named_editbone(editbones, ct->subtarget);

				      if (!oldtarget) {

				      Object *target = ct->tar;

Sybren A. Stüvel commented

2024-11-11 13:11:21 +01:00

Because oldtarget and newtarget are already variables, and they reference a bone, I feel that using target here is a bit confusing. Since there is no casting involved either, I think it's fine to either just keep repeating ct->tar (the compiler will likely optimise anyway) or name this variable target_ob or something.

Because `oldtarget` and `newtarget` are already variables, and they reference a bone, I feel that using `target` here is a bit confusing. Since there is no casting involved either, I think it's fine to either just keep repeating `ct->tar` (the compiler will likely optimise anyway) or name this variable `target_ob` or something.

Christoph Lendenfeld commented

2024-11-14 12:01:32 +01:00

renamed to target_ob
I like to have it as a variable because ct->tar is just really cryptic

renamed to `target_ob` I like to have it as a variable because `ct->tar` is just really cryptic

👍 1

dr.sybren marked this conversation as resolved

Christoph Lendenfeld added 2 commits 2024-11-14 12:02:56 +01:00

Merge branch 'main' into fix_symmetrize_constraint_targets 6e1ddac136

rename to target_ob 9912864b9c

Christoph Lendenfeld merged commit f025ff81fc into main

2024-11-14 12:05:17 +01:00

Christoph Lendenfeld deleted branch fix_symmetrize_constraint_targets