Page MenuHome

Fix parenting objects to bones/vertices causes offset
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Feb 5 2019, 3:50 PM.

Details

Summary

This reverts part of rBbc5482337669.
Problem with above commit is that the evaluated object seems to not have
partype, par1, par2, par3 copied from the original (yet). Using original
object instead now.
Second issue (when parenting to 'Bone Relative') is that the bones
BONE_RELATIVE_PARENTING flag is set on the original, but not the
evaluated bone (yet), setting this on both now.

Fixes T60623 (and part of T59352)

Diff Detail

Repository
rB Blender

Event Timeline

Maybe @Sergey Sharybin (sergey) would know, too?
(about the fact that partype, par1, par2, par3 are not copied from the original)

Seems correct to me, when this function is called the dependency graph has not been updated yet. And so ob_eval does not have all these settings updated set.

This revision is now accepted and ready to land.Feb 5 2019, 6:29 PM

Just noticed there is now an offset with parenting Bone Relative...
Need to check this first before commiting...

Problem seems to be setting the parent to the evaluated object, but the BONE_RELATIVE_PARENTING flag on the bone is set on the original object.
Thus in ob_parbone() which is called twice (once from ED_object_parent_set, once from depsgraph BKE_object_eval_parent) we have this inconsistency...

Working on a fix...

Second issue (when parenting to 'Bone Relative') is that the bones
BONE_RELATIVE_PARENTING flag is set on the original, but not the
evaluated bone (yet), setting this on both now.

@Brecht Van Lommel (brecht): I know this was accepted already, but spotted a second issue (see above), could you check again?
[sloution of setting flag on both evaluated and original bone feels clumsy, but couldnt find another way...]

also noticed we have offsets when parenting to curve, but will do a separate report for that

Campbell Barton (campbellbarton) added inline comments.
source/blender/editors/object/object_relations.c
748–759

IIRC modifying the evaluated data should be avoided unless they're runtime structs to store cache, @Sergey Sharybin (sergey) is it acceptable in this case & is there some policy here?

source/blender/editors/object/object_relations.c
748–759

Evaluated data is to be evaluated by the dependency graph.

Solution which will follow the design is to modify original bone, tag object for update.

source/blender/editors/object/object_relations.c
748–759

Seems like we are doing BKE_object_workob_calc_parent() / BKE_object_where_is_calc(depsgraph, scene, workob) before depsgraph evaluation has done its thing... (so flags are not correct there [yet])
[Also see my/brechts comments above...]

Can do some more digging, but a bit out of ideas here I'm afraid...

Philipp Oeser (lichtwerk) marked an inline comment as not done.Feb 12 2019, 11:52 AM

I have reassigned T60623 and T59352 to @Sergey Sharybin (sergey) now (since I am not sure how to solve this in the desired way [using depsgraph alone]...)

Confusing thing to me is if the code did switch to use of an original object (which has the parent pointers assigned), then why do we need to modify evaluated pchan flags?
Would be good to add a note about this in the comment,

But even if it's needed, the code is actually appears to be correct: it does tag parent for transform (apparently), so is not like evaluated copies will run out of sync. So nothing really wrong here.

Also checked on other usages of BKE_object_workob_calc_parent. Seems there are areas where NOT doing this evaluated lookup is actually correct. Other areas i think will be fine, since all the possibly needed fields are copied from evaluated to original for an active dependency graph.

So don't really think i can do much different of a fix, so just go ahead with this one.

Confusing thing to me is if the code did switch to use of an original object (which has the parent pointers assigned), then why do we need to modify evaluated pchan flags?
Would be good to add a note about this in the comment,

  • getting the correct matrix is now using the original objects parent flags/pointers for the workobject [since these are apparently not copied over to the evaluated object yet]
  • getting the correct matrix is still using the evaluated parent for the workobject [thus the need to modify evaluated pchan flags (these are bones on the parent) -- since these apparently are not copied over yet either]

this is all still a bit confusing for me as well [at this point it seems to me no depsgraph update has happened, right?], so would a comment like this be enough?

But even if it's needed, the code is actually appears to be correct: it does tag parent for transform (apparently), so is not like evaluated copies will run out of sync. So nothing really wrong here.
Also checked on other usages of BKE_object_workob_calc_parent. Seems there are areas where NOT doing this evaluated lookup is actually correct. Other areas i think will be fine, since all the possibly needed fields are copied from evaluated to original for an active dependency graph.

not sure I understand correctly, so will this patch break elsewhere? which evaluated lookup?

So don't really think i can do much different of a fix, so just go ahead with this one.

hesitating... do I get a greenlight from @Brecht Van Lommel (brecht) (regarding the flagging of evaluated pchan) as well?

Seems fine to me.

This revision was automatically updated to reflect the committed changes.