Paths

Table of Contentst

Dont add temporary IK constraints twice
AbandonedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Sep 14 2020, 11:28 PM.

Details

Reviewers

Sebastian Parborg (zeddb)
Sybren A. Stüvel (sybren)
Alexander Gavrilov (angavrilov)
Sergey Sharybin (sergey)

Maniphest Tasks

T80437: Auto IK Double Generates IK constraints

Summary

Caused by rB9a7f5f1bb422.

If using Auto IK (or targetless IK and Auto IK together), two temporary
constraints were added.

from pose_grab_with_ik_add (Auto IK checkbox)
from add_pose_transdata

Since both both do similar things, but cannot work in tandem (with
possibly different chainlengths for example), we have to decide which
type to prefer over the other (as in: do not create a constraint for the
other).
It seems better to ignore the 'Auto IK' option on bones that already
have targetless IK set up for them specificallly [e.g. defining special
chainlength]. This way you can still work with 'Auto IK' ON generally
[with interactive chainlength control], but also have specific bones that
need their own custom chainlength.

ref T80437

Diff Detail

Repository

rB Blender

Branch

T80437_doubleconstraints_b (branched from master)

Build Status

Buildable 10212
Build 10212: arc lint + arc unit

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Sep 14 2020, 11:28 PM

Harbormaster completed remote builds in B10188: Diff 28853.Sep 14 2020, 11:28 PM

Philipp Oeser (lichtwerk) requested review of this revision.Sep 14 2020, 11:28 PM

Philipp Oeser (lichtwerk) mentioned this in T80437: Auto IK Double Generates IK constraints.Sep 14 2020, 11:32 PM

Philipp Oeser (lichtwerk) added a project: Animation & Rigging.Sep 14 2020, 11:34 PM

Philipp Oeser (lichtwerk) added reviewers: Sebastian Parborg (zeddb), Sybren A. Stüvel (sybren), Alexander Gavrilov (angavrilov).

prefer targetless IK over Auto IK (if both are used) and skip adding temporary constraints for Auto IK earlier

Harbormaster completed remote builds in B10212: Diff 28884.Sep 15 2020, 5:29 PM

Philipp Oeser (lichtwerk) edited the summary of this revision. (Show Details)Sep 15 2020, 5:31 PM

@Sergey Sharybin (sergey) : since you were reviewing D7378: Fix T67232: Multiples targetless IKs in a chain gives weird behaviour (known as FakeIK for FK posing) originally, mybe you want to have a look here as well?

This seems to work as described. From my quick testing, I do not have any complaints beside the comment.

I think you don't really have to refer to the commit hash. It doesn't really matter that it worked differently before.
Simply explaining that this code is here because of <reason> is enough.

So you can just drop the Since 9a7f5f1bb422, imo.

Other than that, LGTM.

Not really my area, so would trust you tested original and current files.

Also please address Sebastian's comment: the code is a "snapshot". It is not build of changes on top of each other, the code implements some design. And it is not a specific commit which defines the design.

remove reference to a specific commit from the comment

Harbormaster completed remote builds in B10226: Diff 28920.Sep 16 2020, 10:43 AM

Sebastian Parborg (zeddb) accepted this revision.Sep 16 2020, 11:44 AM

This revision is now accepted and ready to land.Sep 16 2020, 11:44 AM

Actually, one thing I still want to double-check is the conversion of protectflag to the bPoseChannel ikflag here again (since that part is not done when adding the temp constraints from add_pose_transdata and is thus skipped in this patch)

From looking at code this should be respected with targetless IK, zero chainlength, but this also doesnt work in master, nor prior to rB9a7f5f1bb422, it does work with AutoIK though, so will investigate a bit further.

So, question is if temporary IK should obey standard rotation locks on bones in the chain (not just the the IK locks -- these are always respected) too?

if using AutoIK alone, these are respected (with or without this patch -- regardless of chainlength)
if using targetless IK only, these were never respected (with or without this patch -- regardless of chainlength)
if using targetless IK and AutoIK together, these will be ignored with this patch, with rB9a7f5f1bb422 these were respected regardless of chainlength, prior to rB9a7f5f1bb422 these were only respected with chainlength zero.

Afraid I am not the best person to make this decision (because I dont know how this is most likely used in rigs).
To make it somewhat consistent, I would suggest respecting these if chainlength is zero when adding temporary constraints in add_pose_transdata (regardless of AutoIK being ON or OFF)

@Sidney Moraes Jr (mangojambo): do you have an opinion here maybe?

In D8894#218794, @Philipp Oeser (lichtwerk) wrote:

To make it somewhat consistent, I would suggest respecting these if chainlength is zero when adding temporary constraints in add_pose_transdata (regardless of AutoIK being ON or OFF)

This would be it (on top of this Diff)

P1638 D8894_proposal

1
2
3	diff --git a/source/blender/blenkernel/BKE_action.h b/source/blender/blenkernel/BKE_action.h
4	index e08604f02ad..810cd5af5a6 100644
5	--- a/source/blender/blenkernel/BKE_action.h
6	+++ b/source/blender/blenkernel/BKE_action.h
7	@@ -182,6 +182,9 @@ void BKE_pose_ikparam_init(struct bPose *pose);
8	/* initialize a bItasc structure with default value */
9	void BKE_pose_itasc_init(struct bItasc *itasc);
10
11	+/* Sets ik flags based on standard rotation locks (for temporary IK constraints). */
12	+void BKE_pose_channel_ikflag_set_from_protectflag(struct bPoseChannel *pchan);
13	+
14	/* Checks if a bone is part of an IK chain or not */
15	bool BKE_pose_channel_in_IK_chain(struct Object ob, struct bPoseChannel pchan);
16
17	diff --git a/source/blender/blenkernel/intern/action.c b/source/blender/blenkernel/intern/action.c
18	index 64a8ae15fd3..41535ac8628 100644
19	--- a/source/blender/blenkernel/intern/action.c
20	+++ b/source/blender/blenkernel/intern/action.c
21	@@ -924,6 +924,20 @@ static bool pose_channel_in_IK_chain(Object ob, bPoseChannel pchan, int level)
22	return false;
23	}
24
25	+void BKE_pose_channel_ikflag_set_from_protectflag(bPoseChannel *pchan)
26	+{
27	+ /* XXX: careful with quats/axis-angle rotations where we're locking 4d components. */
28	+ if (pchan->protectflag & OB_LOCK_ROTX) {
29	+ pchan->ikflag \|= BONE_IK_NO_XDOF_TEMP;
30	+ }
31	+ if (pchan->protectflag & OB_LOCK_ROTY) {
32	+ pchan->ikflag \|= BONE_IK_NO_YDOF_TEMP;
33	+ }
34	+ if (pchan->protectflag & OB_LOCK_ROTZ) {
35	+ pchan->ikflag \|= BONE_IK_NO_ZDOF_TEMP;
36	+ }
37	+}
38	+
39	bool BKE_pose_channel_in_IK_chain(Object ob, bPoseChannel pchan)
40	{
41	return pose_channel_in_IK_chain(ob, pchan, 0);
42	diff --git a/source/blender/editors/transform/transform_convert_armature.c b/source/blender/editors/transform/transform_convert_armature.c
43	index 6de9ca89425..c1bd7995f18 100644
44	--- a/source/blender/editors/transform/transform_convert_armature.c
45	+++ b/source/blender/editors/transform/transform_convert_armature.c
46	@@ -345,17 +345,7 @@ static short pose_grab_with_ik_add(bPoseChannel *pchan)
47
48	/* we only include bones that are part of a continual connected chain */
49	do {
50	- /* here, we set ik-settings for bone from pchan->protectflag */
51	- // XXX: careful with quats/axis-angle rotations where we're locking 4d components
52	- if (pchan->protectflag & OB_LOCK_ROTX) {
53	- pchan->ikflag \|= BONE_IK_NO_XDOF_TEMP;
54	- }
55	- if (pchan->protectflag & OB_LOCK_ROTY) {
56	- pchan->ikflag \|= BONE_IK_NO_YDOF_TEMP;
57	- }
58	- if (pchan->protectflag & OB_LOCK_ROTZ) {
59	- pchan->ikflag \|= BONE_IK_NO_ZDOF_TEMP;
60	- }
61	+ BKE_pose_channel_ikflag_set_from_protectflag(pchan);
62
63	/* now we count this pchan as being included */
64	data->rootbone++;
65	@@ -680,6 +670,15 @@ static void add_pose_transdata(TransInfo t, bPoseChannel pchan, Object *ob, Tr
66	/* Add a temporary auto IK constraint here, as we will only temporarily active this
67	* targetless bone during transform. (Targetless IK constraints are treated as if they are
68	* disabled unless they are transformed). */
69	+
70	+ /* if no chain length has been specified,
71	+ * just make things obey standard rotation locks too */
72	+ if (data->rootbone == 0) {
73	+ for (bPoseChannel *pchan_iter = pchan; pchan_iter; pchan_iter = pchan_iter->parent) {
74	+ BKE_pose_channel_ikflag_set_from_protectflag(pchan_iter);
75	+ }
76	+ }
77	+
78	add_temporary_ik_constraint(pchan, data);
79	Main *bmain = CTX_data_main(t->context);
80	update_deg_with_temporary_ik(bmain, ob);
81	@@ -1325,6 +1324,15 @@ static void pose_transform_mirror_update(TransInfo t, TransDataContainer tc, O
82	/* TODO(germano): Realitve Mirror support */
83	}
84	data->flag \|= CONSTRAINT_IK_AUTO;
85	+
86	+ /* if no chain length has been specified,
87	+ * just make things obey standard rotation locks too */
88	+ if (data->rootbone == 0) {
89	+ for (bPoseChannel *pchan_iter = pchan; pchan_iter; pchan_iter = pchan_iter->parent) {
90	+ BKE_pose_channel_ikflag_set_from_protectflag(pchan_iter);
91	+ }
92	+ }
93	+
94	/* Add a temporary auto IK constraint here, as we will only temporarily active this
95	* target-less bone during transform. (Target-less IK constraints are treated as if they are
96	* disabled unless they are transformed) */

Taking another step back here, I think we can male it a bit more straightforward and put all temporary constraint creation in one place: pose_grab_with_ik

Sounds like a plan! Hope it goes well

Took another step back and there is even more refactoring possible, but for now lets go with the more straightforward fix in D8930: Fix T80437: Auto IK Double Generates IK constraints

Revision Contents
Changeset List

Path

Size

source/

blender/

editors/

transform/

transform_convert_armature.c

38 lines

Diff 28884

View Options

source/blender/editors/transform/transform_convert_armature.c

Dont add temporary IK constraints twiceAbandonedPublicActions