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.

In D7785#187763, @Sergey Sharybin (sergey) wrote:

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.

In D7785#187844, @Sergey Sharybin (sergey) wrote:

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.

Guys, could you either press the Accept button or tell me what needs changing?