Page MenuHome

Fix T65148: Can't use shape key properties in driver when modifiers generate a new mesh
Needs ReviewPublic

Authored by Sybren A. Stüvel (sybren) on Tue, May 19, 12:21 PM.

Details

Summary

After mesh evaluation the shape keys are no longer necessary. Because of this, the key pointer was not copied. As drivers work on evaluated data, however, they do need this key pointer.

I put the new code in mesh_build_data(), as to me that looks like the appropriate spot. However, maybe mesh_build_extra_data() is more suitable? It's unclear to me what's the exact difference between the two.

I also tested with curves and lattices, and they don't show the problem described in T65148: Can't use shape key properties in driver when modifiers generate a new mesh.

Diff Detail

Repository
rB Blender
Branch
temp-shapekeys-T65148
Build Status
Buildable 8194
Build 8194: arc lint + arc unit

Event Timeline

Sybren A. Stüvel (sybren) requested review of this revision.Tue, May 19, 12:21 PM
Sybren A. Stüvel (sybren) edited the summary of this revision. (Show Details)
  • Added BLI_assert() that shows & checks the assigned shapekey is a CoW copy

While this might be acceptable solution, it must be stated as something to be watched out for.

There was an explicit reason why the key on result was kept at null pointer (which I think I've discussed with Brecht): to have evaluated mesh in a consistent state. Assigning pointer to the key makes it inconsistent: you might have different number of vertices in the mesh and the key.

I'm not sure if there is any areas (drawing, object-from-mesh, etc) which might run into such inconsistency, but this is definitely something to be checked before going this route.

source/blender/blenkernel/intern/DerivedMesh.c
1796

I would strongly advice to stop using this direct tags check and use DEG_is_evaluated_id().

Sybren A. Stüvel (sybren) marked an inline comment as done.Tue, May 19, 1:02 PM
  • Use DEG_is_evaluated_id()

Somehow Arcanist decided to base against master...

There was an explicit reason why the key on result was kept at null pointer (which I think I've discussed with Brecht): to have evaluated mesh in a consistent state. Assigning pointer to the key makes it inconsistent: you might have different number of vertices in the mesh and the key.

This is good to know, and I'll add a comment to this effect to the code.

I'm not sure if there is any areas (drawing, object-from-mesh, etc) which might run into such inconsistency, but this is definitely something to be checked before going this route.

There are some places that check for mesh->key, without any checks whether it's an evaluated or original datablock. Examples are functions used by BKE_mesh_nomain_to_mesh(), BKE_mesh_create_derived_for_modifier(), or BM_mesh_bm_from_me() and BM_mesh_bm_to_me(). I have tested with conversion between an evaluated mesh and a bmesh, and that works (i.e. doesn't crash and ASAN also doesn't complain), even when the evaluated mesh has a different topology.

There doesn't seem to be any drawing code directly using the shapekeys.

Without any checks whether it's an evaluated or original datablock

That was entire idea of ditching DerivedMesh and using Mesh everywhere: areas shouldn't care from where mesh specification came from.

BKE_mesh_nomain_to_mesh() is probably safe since there should be no CD_SHAPEKEY custom data layer.

BKE_mesh_create_derived_for_modifier() seems to be dangerous. It will copy minimum of shapekey and mesh vertices from the keyblock to the mesh. So imagine if you have mesh with shapekey and subsurf and then you click on Apply modifier. Quite sure there will be some unexpected result once the BKE_keyblock_convert_to_mesh codepath is hit.

Hard to tell what side effects in BM_mesh_bm_from_me() and BM_mesh_bm_to_me() will actually happen and how exactly you've tested this.

Also imagine there are some less obvious code paths where the change will have side effects.

I think perhaps the deeper design problem here is that unlike modifier settings, these shape key values are stored in this mesh / key datablock. If these shape key weight values were stored on the object we'd avoid this problem.

For the current design, I can't think of an elegant solution besides checking all those functions and ensuring they handle this case correctly. Maybe they need a test to see if the mesh datablock is evaluated, though that's messy of course.

  • Added note about potential topology inconsistencies
  • Fixed crash in BM_mesh_bm_from_me() by adding check for evaluatedness

BKE_mesh_create_derived_for_modifier() seems to be dangerous. It will copy minimum of shapekey and mesh vertices from the keyblock to the mesh. So imagine if you have mesh with shapekey and subsurf and then you click on Apply modifier.

Applying a modifier while the mesh has shapekeys is only allowed for non-geometric modifiers (i.e. mti->type == eModifierTypeType_NonGeometrical). These are the data transfer and UV/Vertex Weight modifiers. Applying those should not be an issue.

Hard to tell what side effects in BM_mesh_bm_from_me() and BM_mesh_bm_to_me() will actually happen and how exactly you've tested this.

I did some more testing on BM_mesh_bm_to_me(), and manage to break it even without my changes (see D7818). Apart from that, unfortunately it still need a check on whether the mesh is original or evaluated.

For BM_mesh_bm_to_me() to fail in a similar way, it should be passed an evaluated mesh to write to. This isn't allowed, and doesn't seem to have any effect either when I do this:

bm = (a bmesh created from a different mesh)
mesh_to = scene.objects['Suzanne'].evaluated_get(deg).data
bm.to_mesh(mesh_to)

The BKE_keyblock_convert_to_mesh() function is safe, as it takes the minimum of mesh & shapekey items (tot = min_ii(kb->totelem, me->totvert)).

It's probably fine approach within the current design of shapekeys.

source/blender/bmesh/intern/bmesh_mesh_conv.c
238

A bit annoying to have such check, but alternatives aren't less annoying.

Not only that, but shapekeys are also kind of "integrated" into the state of the evaluated mesh, So considering shapekeys here will kind of apply them twice.

  • Updated comment after Sergey's note.
Sybren A. Stüvel (sybren) marked an inline comment as done.Mon, May 25, 12:10 PM
  • Rebased on top of master