Page MenuHome

Fix T80464: Crash deleting bone constraints when the armature layer is not active
ClosedPublic

Authored by Philipp Oeser (lichtwerk) on Sep 4 2020, 4:40 PM.

Details

Summary

Caused by rB608d9b5aa1f1: UI: Add shortcuts for constraint panels

Prior to rB608d9b5aa1f1, the constraint was gotten using context [CTX_data_pointer_get_type(C, "constraint", &RNA_Constraint) -- which is valid for bones on hidden layers].
After rB608d9b5aa1f1, the constraint is found (or isnt) using edit_constraint_property_get [this is not valid for bones on hidden layers because internally BKE_pose_channel_active checks if the bone is on an active layer].

Some observations:

  • Every operator using edit_constraint_property_get doesnt work for bones on inactive layers [delete, moveup, movedown, move to index (drag n drop nowadays)]
    • moveup, movedown, move to index check if they could find a constraint beforehand though (dont crash)
    • delete crashes (doesnt check if a constraint could actually be found)
  • Every operator using edit_constraint_property_get for constraint data doesnt work for bones on inactive layers [stretchto_reset, limitdistance_reset, childof_set_inverse, ...]
    • these all check if they could find a constraint beforehand though (dont crash)

This is because the poll function is using context to get the constraint, the operators themselves use edit_constraint_property_get which leads to inconsistent/unexpected results.

Possible solutions were:

  • [1] let the delete operator just work with the context constraint again (like prior to rB608d9b5aa1f1) -- allows for deleting constraints on bones in inactive layers
  • [2] check if we could get a constraint -- prevents the crash, but does not allow for deleting constraints on bones in inactive layers
  • [3] make the poll edit_constraint_poll_generic be as strict as the operators -- dont use context to get the constraint, but something like edit_constraint_property_get
  • [4] make the operators be more graceful and let them act on bones on hidden layers -- let edit_constraint_property_get actually use context

Whatever we decide upon: this might be a candidate for 2.90.1?

This patch implements [4], so poll and operators are now in sync.

  • prevents reported crash
  • also enables operators for bone constraints on hidden layers
  • also enables drag and drop reordering of constraints on hidden layers

Note: Adding constraints also doesnt work for bones on inactive layers [that was the case in 2.79 as well -- it is also using BKE_pose_channel_active]
Note: edit_constraint_invoke_properties also uses said context

Diff Detail

Repository
rB Blender

Event Timeline

Philipp Oeser (lichtwerk) requested review of this revision.Sep 4 2020, 4:40 PM
Philipp Oeser (lichtwerk) created this revision.

Thanks for looking into this Philipp, it's a nuance I missed while adding the shortcuts.

As for making the operator poll and behavior more or less strict, I'm not really sure, but option 4 does seem to make sense.

I like containing this stuff in one place, so the new function is a plus. I'm not quite sure about how the naming relates to existing functions though, it's just a bit confusing.

For example,
ED_object_constraint_list_from_context takes just an object as an argument.
ED_object_constraint_list_from_context_no_layer_check actually takes the context as an argument.

What about these names?
ED_object_constraint_list_from_context -> ED_object_active_constraint_list
ED_object_constraint_list_from_context_no_layer_check -> ED_object_constraint_list_from_context (with a note in the comment that it doesn't check the active layer) This one is really long too, shortening it to this would be better IMO

I think that gets the naming closer to the actual functionality, but I might be missing something.

Sybren A. Stüvel (sybren) requested changes to this revision.Sep 7 2020, 11:18 AM

I'm not quite sure about how the naming relates to existing functions though, it's just a bit confusing.

I agree. IMO we shouldn't be using the word "context" unless there is a context object (like a bContext, or as in the USD exporter, a HierarchyContext).

What about these names?
ED_object_constraint_list_from_context -> ED_object_active_constraint_list

`ED_object_constraint_list_from_context() is called from ED_object_constraint_active_get(). Maybe ED_object_constraint_active_list() would be a good name? It also puts "constraint" between "object" and "active", making it less likely that people think it's about the active object. It helped me, anyway.

ED_object_constraint_list_from_context_no_layer_check -> ED_object_constraint_list_from_context (with a note in the comment that it doesn't check the active layer) This one is really long too, shortening it to this would be better IMO

I think the "does not check" is only understandable when you have a lot more context; comments that describe what doesn't happen always make me thing "yeah, so what?" I would flip it around, and describe what is returned (instead of what is not done). For example, add a note that the returned constraints may belong to a bone that's on an inactive layer. And be explicit what kind of layers this is about.

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

This boolean indicates that this function is actually two functions in disguise. Functions should do one thing, and if there is a boolean that makes it switch between two things, it should be two functions.

This revision now requires changes to proceed.Sep 7 2020, 11:18 AM

adress review comments:

  • hopefully better function naming
  • remove boolean from function args (second case was actually covered by existing func)
  • hopefully clarify comments
Sybren A. Stüvel (sybren) requested changes to this revision.Sep 7 2020, 3:37 PM
Sybren A. Stüvel (sybren) added inline comments.
source/blender/editors/object/object_constraint.c
83–85

This sentence is missing a verb.

84

The part between parentheses can be removed, or moved down into the function itself. It only means something when you know the exact implementation of the function.

106

"the context pose bone" is a weird concept. Probably clearer to write "the active pose bone". The fact that it comes from the context is already clear as that's the only parameter given.

110–117

This function is now too complex. The initialisation of constraints looks like it initialises a ListBase, but it's actually just setting constraints = NULL. The only case in which the function can actually return NULL is when pose_bone == NULL. This seems much simpler to me:

bPoseChannel *pose_bone = CTX_data_pointer_get(C, "pose_bone").data;
if (pose_bone == NULL) {
  return NULL;
}
return &pose_bone->constraints;
This revision now requires changes to proceed.Sep 7 2020, 3:37 PM

address more review comments (thx bearing with me @Sybren A. Stüvel (sybren)!)

LGTM.

I think it might be nice to split up the patch, because now it does some refactoring (like renaming ED_object_constraint_list_from_context to ED_object_constraint_active_list) as well as actual functional changes. Probably it's best to do the refactoring first, and then commit the functional changes separately. IMO that won't need another review pass, though, as long as the end result is what we discussed in this patch.

This revision is now accepted and ready to land.Sep 7 2020, 4:57 PM

Looks good to me too!