Page MenuHome

Fix T67454: Blender crash on rapid undo and select
ClosedPublic

Authored by Sergey Sharybin (sergey) on Thu, Jul 25, 4:38 PM.

Details

Summary

Speculation here is that sometimes undo and selection operators are sometimes handled in the same event loop iteration, which leaves non-evaluated dependency graph.

This sounds like a deeper problem, so this change is going over all access to the dependency graph and ensures graph is fully evaluated prior to using it (unless access is generic, where actual evaluation might happen later).

Some general rules:

  • Drawing is expected to happen from a fully evaluated dependency graph. There is now a function to access it, which will in the future control that depsgraph is actually evaluated.
  • All operators which needs to access evaluated state must use CTX_data_ensure_evaluated_depsgraph(), since it is not known at which state the operator is called and whether the data in it is safe to be read.
  • All cases which needs to know depsgraph pointer, but which doesn't want to actually evaluate it can use old-style function CTX_data_depsgraph_pointer(), assuming that underlying code will ensure depsgraph is evaluated prior to accessing it.
  • The new functions are replacing OPTYPE_USE_EVAL_DATA, so now it is explicit and local about where depsgraph is being ensured. This commit also contains some fixes of wrong usage of evaluation functions on original objects. Ideally should be split out, but in reality with all the APIs being renamed is quite tricky.

Diff Detail

Repository
rB Blender

Event Timeline

The risk here is that evaluating the dependency graph can invalidate evaluated data pointers.

There are also functions like paint_draw_cursor and meshobject_foreachScreenVert using this, where it becomes hard to guarantee it's safe to evaluate the dependency.

Is it practical to remove depsgraph from ED_view3d_viewcontext_init, and instead check all the case that use it and use CTX_data_evaluated_depsgraph as needed?

I think we also really need a review of all cases calling CTX_data_depsgraph, to me it seems there are a lot of possible case where it could crash due to using an outdated depsgraph.

Brecht Van Lommel (brecht) requested changes to this revision.Thu, Jul 25, 6:18 PM
This revision now requires changes to proceed.Thu, Jul 25, 6:18 PM

A lot of updates (tm)

Now operators which needs evaluated data are ensuring the depsgraph
is evaluated.

Can confirm this fixes both T67973 and T67902 as well...

Brecht Van Lommel (brecht) requested changes to this revision.Wed, Jul 31, 10:54 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/animation/anim_filter.c
415 ↗(On Diff #16622)

This function is used in drawing code like graph_main_region_draw, I don't think it's safe to do here.

source/blender/editors/include/ED_anim_api.h
90 ↗(On Diff #16622)

insserting -> inserting

source/blender/editors/mesh/editmesh_tools.c
423–424

Better to make the depsgraph a function parameter then, otherwise it's quite hidden.

source/blender/editors/object/object_constraint.c
872–873

Make depsgraph a parameter of this function and get it at the start of childof_set_inverse_exec. Things are not obvious if this happens in deeper functions.

950–951

Same comment.

source/blender/editors/object/object_data_transfer.c
159

I don't think we should ever have depsgraph evaluation in an _itemf function, though I guess it won't be a problem in this case.

source/blender/editors/object/object_modes.c
188–189

This function is not used anywhere, remove it.

source/blender/editors/physics/particle_edit.c
3522

update_world_cos doesn't even use the depsgraph parameter, maybe just remove it?

source/blender/editors/physics/physics_fluid.c
344–345

Make the depsgraph a parameter to this function and ensure evaluated at the start of the operator, it's not obvious what happens between this and when it starts executing the operator.

521–522

Same comment.

source/blender/editors/sculpt_paint/paint_image_proj.c
6237

Do this at the start of the function? Otherwise edits to the image can cause an additional depsgraph evaluation when that's not what we need it for.

source/blender/editors/sculpt_paint/paint_vertex.c
1173–1174

This function is not used anywhere, better to just remove it.

1178–1179

This function is not used anywhere, better to just remove it.

source/blender/editors/sculpt_paint/sculpt.c
6154–6155

Function is not used anywhere, remove it.

6200–6201

Same comment as enter.

source/blender/editors/sculpt_paint/sculpt_undo.c
151

I don't think depsgraph evaluation should ever happen in undo code?

462–463

Same comment.

1135

Same comment.

source/blender/editors/transform/transform_conversions.c
6814

I don't think this should be done inside a loop over bones, otherwise might get evaluated for every bone?

source/blender/editors/transform/transform_gizmo_3d.c
736

Probably better to use expect_evaluated? I don't really trust evaluating the depsgraph at this point.

source/blender/editors/util/ed_util.c
151

This is going to lead to dependency re-evaluation multiple times on file load in some cases, as it loops through the objects and edits them.

We calll wm_event_do_depsgraph before ED_editors_init. I think we should put expect_evaluated at the start of this function, outside the object loop.

source/blender/makesrna/intern/rna_modifier.c
1156

Simple reading of an RNA property should not cause depsgraph evaluation. I guess this is a bit of a weird case for the modifier UI drawing and not general Python API usage, maybe it's ok to to use expect_evaluated. here.

source/blender/python/intern/bpy_rna_anim.c
340 ↗(On Diff #16622)

I wouldn't do ensure evaluated in a Python API like this. Otherwise if you insert a bunch of keyframes, it will re-evaluate the depsgraph every time.

376 ↗(On Diff #16622)

Same comment.

This revision now requires changes to proceed.Wed, Jul 31, 10:54 AM
Sergey Sharybin (sergey) marked 18 inline comments as done.Wed, Jul 31, 4:01 PM
Sergey Sharybin (sergey) updated this revision to Diff 16723.

Think all feedback has been addressed in one way or another.

source/blender/editors/animation/anim_filter.c
415 ↗(On Diff #16622)

The ac->depsgraph has been removed by D5379.

source/blender/editors/include/ED_anim_api.h
90 ↗(On Diff #16622)

depsgraph has been removed by D5379, no longer relevant.

source/blender/editors/object/object_data_transfer.c
159

As long as this specific function relies on mesh_get_eval_final there is no other way.

source/blender/editors/sculpt_paint/sculpt_undo.c
151

With the current code in BKE_sculpt_update_object_for_edit() this is unavoidable.

462–463

Same answer.

1135

Same answer (ED_object_sculptmode_enter_ex -> sculpt_init_session -> BKE_sculpt_update_object_for_edit).

source/blender/editors/transform/transform_conversions.c
6814

IS no longer needed anyway (insert_keyframe does no longer need depsgraph),

source/blender/makesrna/intern/rna_modifier.c
1156

It should not cause object evaluation either. It is not guaranteed that object has been evaluated with CD_MASK_MLOOPUV.

This code is already "dangerous", but as long as it uses mesh_get_eval_final you can not be safe here unless use CTX_data_ensure_evaluated_depsgraph.

source/blender/python/intern/bpy_rna_anim.c
340 ↗(On Diff #16622)

No longer needed since D5379.

376 ↗(On Diff #16622)

Same answer,

This revision is now accepted and ready to land.Wed, Jul 31, 4:14 PM
This revision was automatically updated to reflect the committed changes.