Page MenuHome
Differential D5189

Fix T66080: "child of" constraint "set inverse" problematic with bones
ClosedPublic
Actions

Authored by Philipp Oeser (lichtwerk) on Jul 5 2019, 11:16 AM.

Details

Reviewers
Sergey Sharybin (sergey)
Brecht Van Lommel (brecht)
Maniphest Tasks
T66397: Bone Constraint Child Of Bone + Follow Path Bone
T66080: Bone vertex Child of Constraint problem
Commits
rB609e16339f13: Fix "child of" constraint "set inverse" problematic with bones
Summary

For bones, do this in the DEG evaluated domain.
I've split 'child_get_inverse_matrix' in two (depending on the
constraint owner type) to make it obvious one is working with evaluated,
whereas the other is working with original.
This seems to be working fine, but a second/third pair of eyes is really
welcome since I am (still) on shaky ground here.

also fixes T66397 afaict

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) created this revision.Jul 5 2019, 11:16 AM
Harbormaster completed remote builds in B3975: Diff 16212.Jul 5 2019, 11:16 AM
Sergey Sharybin (sergey) added a comment.Jul 5 2019, 11:36 AM

Formatting seems broken, make sure you use clang-format.

source/blender/editors/object/object_constraint.c
991

Why is it so?

1244

Again, why is it so?

Seems to be quite a duplicate from the case above, so totally worth moving the orig-to-eval queries to child_get_inverse_matrix_owner_bone().

1248

More clear would be to state: "Use evaluated bone to calculate matrix, and store it it the original constraint."

Philipp Oeser (lichtwerk) updated this revision to Diff 16213.Jul 5 2019, 12:38 PM
  • make format
  • cleanup comments
  • deduplicate call to ED_object_constraint_update(), wasnt necessary to call this on the evaluated object
Harbormaster completed remote builds in B3976: Diff 16213.Jul 5 2019, 12:38 PM
Philipp Oeser (lichtwerk) marked an inline comment as done.Jul 5 2019, 12:39 PM
Philipp Oeser (lichtwerk) added inline comments.Jul 5 2019, 12:46 PM
source/blender/editors/object/object_constraint.c
991

haha, was afraid of that question :)
assume this has to do with BKE_pose_where_is which seems to misbehave on non-evaluated object.
wild guess is that this has to do with flags being reset/set there and BKE_pose_where_is_bone, but I am still checking the internals...

You did this yourself [see your commit rB26d4a2a51694], what was the explanation there? ;)

Anyways, I'll take a deeper look...

1244

why? see above

I can move the queries to, but that would require to pass the op as well [to get the bConstraint in that function]

Philipp Oeser (lichtwerk) marked 2 inline comments as not done.Jul 5 2019, 12:46 PM
Philipp Oeser (lichtwerk) added a comment.Jul 5 2019, 1:44 PM

@Sergey Sharybin (sergey), @Brecht Van Lommel (brecht): seems I cant find the deeper reason in a reasonable time.

I can check further [would be quite good to get a deeper understanding], that might take some time though [to go over the whole shebang of BKE_pose_where_is, BKE_pose_where_is_bone, BKE_constraints_make_evalob and friends, checking pchan flags there etc]...
Just checking priorities here and want to avoid this getting a time-sink (if this is not desired)...

Should I dig deeper, or do you want to take over @Sergey Sharybin (sergey)?

Sergey Sharybin (sergey) added a comment.Jul 5 2019, 2:11 PM

@Philipp Oeser (lichtwerk), BKE_pose_where_is_bone() is indeed supposed to be used on evaluated bone, otherwise some dependencies will not be at a proper state.

The point was more about expanding comment a bit, mentioning that function relies on (re)evaluating parts of the scene and copying new evaluated stuff back to original.

Philipp Oeser (lichtwerk) updated this revision to Diff 16216.Jul 5 2019, 3:09 PM
  • move the orig-to-eval queries to child_get_inverse_matrix_owner_bone
  • update comments (used @sergeys wording here)
Harbormaster completed remote builds in B3979: Diff 16216.Jul 5 2019, 3:09 PM
Philipp Oeser (lichtwerk) marked 2 inline comments as done.Jul 5 2019, 3:13 PM
Philipp Oeser (lichtwerk) added inline comments.
source/blender/editors/object/object_constraint.c
991

added comment in child_get_inverse_matrix_owner_bone [used your wording :)]

1244

moved the queries to child_get_inverse_matrix_owner_bone

Sergey Sharybin (sergey) added a comment.Jul 5 2019, 3:18 PM

Generally fine. Just some last bit of feedback.

source/blender/editors/object/object_constraint.c
989

With the more comprehensive comment in child_get_inverse_matrix_owner_bone() this comment is not needed anymore. In fact, we don't access evaluated bone here. Think this exact comment can be remove.

1242

Same as above.

Philipp Oeser (lichtwerk) updated this revision to Diff 16218.Jul 5 2019, 3:34 PM
  • remove unneccesary comment
Harbormaster completed remote builds in B3980: Diff 16218.Jul 5 2019, 3:34 PM
Sergey Sharybin (sergey) accepted this revision.Jul 5 2019, 3:39 PM

Form reading the code seems fine to me.
Didn't have time to test it though, so relying this on you.. ;)

This revision is now accepted and ready to land.Jul 5 2019, 3:39 PM
Closed by commit rB609e16339f13: Fix "child of" constraint "set inverse" problematic with bones (authored by Philipp Oeser (lichtwerk)). · Explain WhyJul 5 2019, 4:02 PM
This revision was automatically updated to reflect the committed changes.
Philipp Oeser (lichtwerk) added a commit: rB609e16339f13: Fix "child of" constraint "set inverse" problematic with bones.

Revision Contents
Changeset List

PathSize
source/
blender/
editors/
object/
167 lines

Diff 16220

View Options

source/blender/editors/object/object_constraint.c

Loading...