Page MenuHome

Fix T81854: crash when undoing switch between sculpt and edit mode.
ClosedPublic

Authored by Bastien Montagne (mont29) on Oct 19 2020, 4:11 PM.

Details

Summary

To reproduce, from startup scene, switch cube to sculpt mode, then edit,
then sculpt again, and undo twice.

Root of the issue is that ED_object_sculptmode_enter_ex frees
sculptsession, then calls sculpt_init_session which ends up using
evaluated object that still has that sculptsession pointer in it, since
depsgraph is not updated at all in the mean time.

This is the simplest fix, however am quiet not sure it is a proper one,
I suspect the issue to go deeper.

Bug comes actually from BKE_sculpt_update_object_for_edit, which calls
mesh_get_eval_final outside of any depsgraph evaluation, and without
even ensuring it is up to date.

I would rather see BKE_object_get_evaluated_mesh being used here? But
code in calling ED_object_sculptmode_enter_ex actually frees object
cache, so that would always return NULL here.

Diff Detail

Repository
rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Oct 19 2020, 4:11 PM

Tagging and forcing an update for something what is needed 5 levels deeper in the callstack does not sound correct at all.

The sculpt pointer is supposed to be used from the original object, as it is the source of truth. So the fact that evaluated object points to a possible outdated one is weird.
I guess this is caused by mesh_get_eval_final which actually re-evaluates the object, which is wrong on its own. Why is it re-evaluating the object? Should the depsgraph be "acquired" from the context by CTX_data_ensure_evaluated_depsgraph(C), leaving away the need of BKE_object_free_derived_caches and mesh_get_eval_final ?

Tagging and forcing an update for something what is needed 5 levels deeper in the callstack does not sound correct at all.

The sculpt pointer is supposed to be used from the original object, as it is the source of truth. So the fact that evaluated object points to a possible outdated one is weird.
I guess this is caused by mesh_get_eval_final which actually re-evaluates the object, which is wrong on its own.

Yes that's also what I thought (just don't have enough experience in that area to be sure that that was actually bad).

Why is it re-evaluating the object? Should the depsgraph be "acquired" from the context by CTX_data_ensure_evaluated_depsgraph(C), leaving away the need of BKE_object_free_derived_caches and mesh_get_eval_final ?

But this code gets called from many different places, so will take some time to check whether we can actually use BKE_object_get_evaluated_mesh all the time, or if we need to handle both cases.
Another problem is that ED_object_sculptmode_enter_ex gets called from within undo code of sculpt, don't think we can expect having an evaluated depsgraph from there.

Currently non-fully-formalized rule of thumb is to do ensure_evaluated at the top level, and pass evaluated depsgraph. The function must be clear whether it expects things to be evaluated or not. The reason for this is to avoid re-evaluations o nthe "bottom" levels when, say, operator does modify a lot of objects. So general flow is such:

  • An operator ensures depsgraph is evaluated in the beginning, which gives all the input data for it to operate on
  • Operator modifies data, tags for update

This is a bit of a sidetracking, but I think it is good to state this to ensure the understanding of what and how is expected to be is same on our both sides.

The ED_object_sculptmode_enter_ex is called from called from sculpt_undosys_step_decode, which does ensure the depsgraph is evaluated. So the depsgraph is evaluated, unless undo system sues some other code pass which is not immediately clear to me.

Maybe proper approach would be to:

  • Replace mesh_get_eval_final with BKE_object_get_evaluated_mesh
  • Remove the BKE_object_free_derived_caches call
  • Track top level calls where depsgraph pointer is passed, and make sure it is evaluated

Maybe also add DEG_id_assert_evaluated() macro in DEG_debug.h, which will assert that the given ID is fully evaluated. Could help tracking cases when the object which is moving into the sculpt mode is not yet evaluated.

source/blender/editors/sculpt_paint/sculpt.c
8201–8202

To follow old logic there needs to be evaluation here. Without such evaluation it seems that sculpt session will be wrongly initialized if, say, multires sculpt level is different from viewport level.

source/blender/editors/sculpt_paint/sculpt_undo.c
1488–1492

Does the ED_object_mode_generic_exit require evaluated despgraph? If so, it needs to be commented as such. Otherwise move the Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C); line next to the ED_object_sculptmode_enter_ex call.

Not sure what is the point of assert here, but you probably was looking into CTX_data_depsgraph_pointer.

Bastien Montagne (mont29) marked an inline comment as done.

Update from review.

source/blender/editors/sculpt_paint/sculpt_undo.c
1488–1492

It seems ED_object_mode_generic_exit does not need an evaluated depsgraph. It seems to be only used when leaving sculpt mode, which may end up calling SCULPT_dynamic_topology_disable_ex, which forces update of the depsgraph. This logic sounds very weird and not valid to me either, be this should be addressed in another patch anyway.

But an evaluated depsgraph is also need at the end of this function, when calling the sculpt_undosys_step_decode_undo/_redo functions.

This whole sculpt code seems to be doing very unsafe things with depsgraph.

1491

Will replace with BKE_scene_graph_evaluated_ensure infact, we already have our depsgraph here...

1492

Will remove, not really needed indeed.

A bit messy.
But lets fix crash, and leave sanitation of the dataflow here for later.

This revision is now accepted and ready to land.Tue, Oct 27, 12:49 PM