Page MenuHome

Fix T94441: fix crash parenting object to a bone
ClosedPublic

Authored by Andrew Oates (aoates) on Sep 25 2022, 9:41 PM.

Details

Summary

This crash occurs when the bone is newly created. In certain
circumstances the depsgraph data for the armature is not updated,
causing pchan_eval to be NULL when the parent is updated. This causes
a segfault in ED_object_parent_set when the flags are updated.

This change fixes the underlying depsgraph bug, and also adds both an
assertion and NULL pointer check to ED_object_parent_set to better
handle this scenario if it recurs via another path.

Diff Detail

Repository
rB Blender

Event Timeline

I think it is important to understand why exactly pchan can be found in the original object but not in the evaluated object.

The beginning of the function seems a bit suspicious to me:

Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);
bPoseChannel *pchan = NULL;
bPoseChannel *pchan_eval = NULL;
Object *parent_eval = DEG_get_evaluated_object(depsgraph, par);

DEG_id_tag_update(&par->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY);

Ensuring the depsgraph is evaluated seems logical, so that matricies are all up-to-date and what not.
But why the tag of the parent is needed here? Setting parent modifies child, not parent. And why the order is done in the way it is done? It kinda feels that the code was intending to be

DEG_id_tag_update(&par->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY);
Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);
Object *parent_eval = DEG_get_evaluated_object(depsgraph, par);

But is still unclear why.

I think it is important to understand why exactly pchan can be found in the original object but not in the evaluated object.

...

But why the tag of the parent is needed here? Setting parent modifies child, not parent. And why the order is done in the way it is done? It kinda feels that the code was intending to be

DEG_id_tag_update(&par->id, ID_RECALC_TRANSFORM | ID_RECALC_GEOMETRY);
Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);
Object *parent_eval = DEG_get_evaluated_object(depsgraph, par);

Ah! So if I change the code to do it in that order, then both the crash and the error in my test are resolved. I don't understand Blender internals well enough to have an opinion on whether there's a reason not to do it in this order, but it seems to resolve the issue I'm seeing at least.

Ah! So if I change the code to do it in that order, then both the crash and the error in my test are resolved.

That's interesting. Are there any modifications to the parent object between the operator begin/invoke and the call ED_object_parent_set ?

That's interesting. Are there any modifications to the parent object between the operator begin/invoke and the call ED_object_parent_set ?

Just adding the new bone that I'm parenting to, nothing else. I've trimmed down the repro steps and posted the smallest version I have in T94441, but the script that causes the crash is as follows:

import bpy
import mathutils

arm = bpy.data.objects['Armature']
obj = bpy.data.objects['Cube']

eb = arm.data.edit_bones

name = 'TestBone'
assert(name not in arm.data.bones)
b = eb.new(name)
b.head = obj.location
b.tail = obj.location + mathutils.Vector((0, 0, 1))

bpy.ops.object.mode_set(mode='POSE')
obj.select_set(True)
pose_bone = arm.data.bones[name]
arm.data.bones.active = pose_bone

#b.select = True  # Prevents crash
bpy.ops.object.parent_set(type='BONE')

It's dependent on some sort of hidden state --- if, for example, you don't delete the existing bones before running the script, the crash doesn't occur.

So is the point you're making that while switching the deps graph update ordering here fixed the bug I'm seeing, the fact that the graph is not already up-to-date implies a bug _elsewhere_ in another operation that should be triggering an update, but isn't?

Interesting!

I don't know what that other operation would be, other than the bone creation itself. Even if the above is true, we'd want a defensive programming fix here as well to prevent a segfault, right? What would be the appropriate approach (log an internal error? or at least have an assertion failure rather than a segfault?). I'm new to the codebase so not sure what the norms are.

So is the point you're making that while switching the deps graph update ordering here fixed the bug I'm seeing, the fact that the graph is not already up-to-date implies a bug _elsewhere_ in another operation that should be triggering an update, but isn't?

Exactly. Perhaps the active bone assignment does not get propagated to the evaluated poses (based on just looking around). Can you check whether P3217 fixes the issue?

Thx looking into it.
Indeed, it seems ordering is better that way, tagging parent also seems to not be necessary anymore (at least I cannot reproduce T60623 anymore even without it)
Could this have changed with rB3566b81c8bfa: Refactor access to dependency graph?

The whole handling of pchan_eval is still necessary though it seems, this all still seems relevant (from the commit message of rB2894e75121d7: Fix parenting objects to bones/vertices causes offset) :

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.

Edit: was typing at the same time, checking P3217 now

Exactly. Perhaps the active bone assignment does not get propagated to the evaluated poses (based on just looking around). Can you check whether P3217 fixes the issue?

wait, is this meant to replace the parent tagging?
(as I said above, this doesnt seems necessary anymore anyways -- at least in my tests)

So is the point you're making that while switching the deps graph update ordering here fixed the bug I'm seeing, the fact that the graph is not already up-to-date implies a bug _elsewhere_ in another operation that should be triggering an update, but isn't?

Exactly. Perhaps the active bone assignment does not get propagated to the evaluated poses (based on just looking around). Can you check whether P3217 fixes the issue?

Just tested P3217, it fixes the bug on all three of my test cases!

wait, is this meant to replace the parent tagging?

Ideally I think so. But I did not dig into the history of the parent tag to see if there is some other issue which it was addressing. So more testing with the P317 and removed parent tag might be needed.

Just tested P3217, it fixes the bug on all three of my test cases!

Great to hear! And the result was all correct?
If so, mind updating this patch so we can apply it? :)

Andrew Oates (aoates) edited the summary of this revision. (Show Details)

Updated to include Sergey's P3217 and an assertion

Sergey Sharybin (sergey) added inline comments.
source/blender/editors/object/object_relations.c
574

We use C-style comments: /* ... */

This revision is now accepted and ready to land.Sep 29 2022, 10:35 AM