Page MenuHome

Dont add temporary IK constraints twice
AbandonedPublic

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

Details

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) requested review of this revision.Sep 14 2020, 11:28 PM

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

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

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?

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)

1
2
3diff --git a/source/blender/blenkernel/BKE_action.h b/source/blender/blenkernel/BKE_action.h
4index 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
17diff --git a/source/blender/blenkernel/intern/action.c b/source/blender/blenkernel/intern/action.c
18index 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);
42diff --git a/source/blender/editors/transform/transform_convert_armature.c b/source/blender/editors/transform/transform_convert_armature.c
43index 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) */

Philipp Oeser (lichtwerk) planned changes to this revision.Sep 17 2020, 11:32 AM

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