Page MenuHome

Bugfix T60185: loops in graph produced unorderly clean-up
ClosedPublic

Authored by Jose C. Rubio (jrubio) on Jan 20 2019, 7:03 PM.

Details

Summary

Bug link: https://developer.blender.org/T60185

When having loops in a kinematic chain and also a IK constrain, the solve_cycle function seemed to remove graph relations necessary to ensure the cleanup operation was running at the very end. Due to his Blender was crashing when some operations (the bone constraints) accessed a pointer that was already freed.

Diff Detail

Repository
rB Blender

Event Timeline

Jose C. Rubio (jrubio) edited the summary of this revision. (Show Details)Jan 20 2019, 8:41 PM
Jose C. Rubio (jrubio) edited the summary of this revision. (Show Details)Jan 20 2019, 8:43 PM

This is effectively marking almost all of the relations as non-killable. This might be ok if those are done within a single bone channel only, but this definitely shouldn't be applied to inter-bone dependencies (i.e. from parent to solver). That will never be a reliable solution, and will fail sooner or later.

The proper solution here i guess is to add dependency from every bone node to its cleanup.

We can also tweak cycles solver, so it prefers to kill relation between different ID's first, if that's no possible then relation between different components. ANd that is still not possible, whatever relation.

P.S. If we are ending up marking majority of relaitons as GODMODE we are doing something wrong..

source/blender/depsgraph/intern/builder/deg_builder_relations_rig.cc
494

There is no such a thing as "updated", notes are always supposed to be up-to-date. For the history of notes use git history.

Yeah I was not really sure this was the right approach. I'll add relations bone-cleanup as you suggest. I guess tweaking the cycles solver wouldn't be a solution in the long term anyway as long as we allow arbitrary armature graphs.

This approach includes additional relations to avoid the unorderly cleanup rather than having unkillable relations. I had to add extra relations in bones as well as in constraints.

The patch (updated on - P934) seems to work (no crashes).
There are tons of dependency Cycles errors though (full list P935):

'OBArmature.Mario.POSE_IK_SOLVER()' depends on 'OBArmature.Mario.Spine.Bendy.BONE_READY()' through 'IK Chain Parent'               
'OBArmature.Mario.Spine.Bendy.BONE_READY()' depends on 'OBArmature.Mario.Spine.Bendy.BONE_CONSTRAINTS()' through 'Constraints -> Ready'
'OBArmature.Mario.Spine.Bendy.BONE_CONSTRAINTS()' depends on 'OBArmature.Mario.Spine.Tail.BONE_READY()' through 'Damped Track'          
'OBArmature.Mario.Spine.Tail.BONE_READY()' depends on 'OBArmature.Mario.Spine.Tail.BONE_CONSTRAINTS()' through 'Constraints -> Ready'
'OBArmature.Mario.Spine.Tail.BONE_CONSTRAINTS()' depends on 'OBArmature.Mario.Spine.Tail.BONE_POSE_PARENT()' through 'Pose -> Constraints Stack'
'OBArmature.Mario.Spine.Tail.BONE_POSE_PARENT()' depends on 'OBArmature.Mario.Spine.Base.BONE_DONE()' through 'Parent Bone -> Child Bone'
'OBArmature.Mario.Spine.Base.BONE_DONE()' depends on 'OBArmature.Mario.POSE_IK_SOLVER()' through 'IK Chain Result'

@Dalai Felinto (dfelinto), this change has nothing to do with solving cycles, it just makes blender to not crash when there is a cycle.
From reading the patch it seems fine to me.

This revision is now accepted and ready to land.Mar 13 2019, 9:39 AM

@Brecht Van Lommel (brecht), do you feel strong here? Can't think of a better solution than to introduce more relations.

I don't have a better local solution.

There may be a better overall design to handle this, but don't have immediate ideas for that.

Jose C. Rubio (jrubio) marked an inline comment as done.EditedMar 13 2019, 1:53 PM

The only alternative that comes to mind would be to improve the solve_cycles function so that the resulting graphs are ensured to perform orderly cleanup. But adding extra relations seems like a more clean/maintainable solution.

Needed to apply manually, and forgot to mention the revision in the comment.
Committed as rB76437b903de.

Thanks for the patch! Closing, as applied.