[Bf-blender-cvs] [bfdbc78466a] blender-v3.1-release: Fix T44415: Shape keys get out of sync when using undo in edit-mode
Campbell Barton
noreply at git.blender.org
Tue Feb 22 00:03:12 CET 2022
Commit: bfdbc78466ac14d45f353db9aa39cb21bb962701
Author: Campbell Barton
Date: Tue Feb 22 09:57:07 2022 +1100
Branches: blender-v3.1-release
https://developer.blender.org/rBbfdbc78466ac14d45f353db9aa39cb21bb962701
Fix T44415: Shape keys get out of sync when using undo in edit-mode
This is an alternate fix for T35170 since it caused T44415.
Having the undo system manipulate the key-block coordinates is error
prone as (in the case of T44415) there are situations when it's
important to apply the difference with the original shape key.
This reverts dab0bd9de65a9be5e8ababba0e2799f994d5d12f, and instead
avoids the problem by not using the data in `Mesh.key` as a reference
for updating shape-keys when exiting edit-mode.
The assumption that the `Mesh.key` in edit-mode won't be modified
until leaving edit-mode isn't always true. Leading to synchronization
errors. (details noted in code-comments).
Resolve this by using shape-key data stored in the BMesh.
Resolving both T35170 & T44415.
Details:
- Remove use of the original vertices when exiting edit mode.
- Remove use of the original shape-key coordinates when exiting
edit-mode (except as a last resort).
- Move shape-key synchronization into a separate function:
`bm_to_mesh_key`.
- Split the synchronization loop into two branches,
depending on the existence of BMesh shape-key coordinates.
- Always write shape-key values back to the BMesh CD_SHAPEKEY layers.
This was only done in some cases but is now necessary for all
shape-keys as these are used to calculate offsets where the `Mesh.key`
was previously used.
- Report a warning when the shape-key layer isn't found as this uses an
imperfect method of restoring coordinates which should only be used as
a last resort.
Reviewed By: mont29
Ref D14127
===================================================================
M source/blender/bmesh/CMakeLists.txt
M source/blender/bmesh/intern/bmesh_mesh_convert.cc
M source/blender/editors/mesh/editmesh_undo.c
===================================================================
diff --git a/source/blender/bmesh/CMakeLists.txt b/source/blender/bmesh/CMakeLists.txt
index e2ed005cf9e..715fc298a86 100644
--- a/source/blender/bmesh/CMakeLists.txt
+++ b/source/blender/bmesh/CMakeLists.txt
@@ -26,6 +26,7 @@ set(INC
../depsgraph
../makesdna
../../../intern/atomic
+ ../../../intern/clog
../../../intern/eigen
../../../intern/guardedalloc
../../../extern/rangetree
diff --git a/source/blender/bmesh/intern/bmesh_mesh_convert.cc b/source/blender/bmesh/intern/bmesh_mesh_convert.cc
index d6c642ff80b..41ca96b109e 100644
--- a/source/blender/bmesh/intern/bmesh_mesh_convert.cc
+++ b/source/blender/bmesh/intern/bmesh_mesh_convert.cc
@@ -68,6 +68,19 @@
*
* This has the effect from the users POV of leaving the mesh un-touched,
* and only editing the active shape key-block.
+ *
+ * \subsection other_notes Other Notes
+ *
+ * Other details noted here which might not be so obvious:
+ *
+ * - The #CD_SHAPEKEY layer is only used in edit-mode,
+ * and the #Mesh.key is only used in object-mode.
+ * Although the #CD_SHAPEKEY custom-data layer is converted into #Key data-blocks for each
+ * undo-step while in edit-mode.
+ * - The #CD_SHAPE_KEYINDEX layer is used to check if vertices existed when entering edit-mode.
+ * Values of the indices are only used for shape-keys when the #CD_SHAPEKEY layer can't be found,
+ * allowing coordinates from the #Key to be used to prevent data-loss.
+ * These indices are also used to maintain correct indices for hook modifiers and vertex parents.
*/
#include "DNA_key_types.h"
@@ -98,6 +111,10 @@
#include "bmesh.h"
#include "intern/bmesh_private.h" /* For element checking. */
+#include "CLG_log.h"
+
+static CLG_LogRef LOG = {"bmesh.mesh.convert"};
+
using blender::Array;
using blender::IndexRange;
using blender::Span;
@@ -567,8 +584,89 @@ static BMVert **bm_to_mesh_vertex_map(BMesh *bm, int ototvert)
return vertMap;
}
+/* -------------------------------------------------------------------- */
+/** \name Edit-Mesh to Shape Key Conversion
+ *
+ * There are some details relating to using data from shape keys that need to be
+ * considered carefully for shape key synchronization logic.
+ *
+ * Key Block Usage
+ * ***************
+ *
+ * Key blocks (data in #Mesh.key must be used carefully).
+ *
+ * They can be used to query which key blocks are relative to the basis
+ * since it's not possible to add/remove/reorder key blocks while in edit-mode.
+ *
+ * Key Block Coordinates
+ * =====================
+ *
+ * Key blocks locations must *not* be used. This was done from v2.67 to 3.0,
+ * causing bugs T35170 & T44415.
+ *
+ * Shape key synchronizing could work under the assumption that the key-block is
+ * fixed-in-place when entering edit-mode allowing them to be used as a reference when exiting.
+ * It often does work but isn't reliable since for e.g. rendering may flush changes
+ * from the edit-mesh to the key-block (there are a handful of other situations where
+ * changes may be flushed, see #ED_editors_flush_edits and related functions).
+ * When using undo, it's not known if the data in key-block is from the past or future,
+ * so just don't use this data as it causes pain and suffering for users and developers alike.
+ *
+ * Instead, use the shape-key values stored in #CD_SHAPEKEY since they are reliably
+ * based on the original locations, unless explicitly manipulated.
+ * It's important to write the final shape-key values back to the #CD_SHAPEKEY so applying
+ * the difference between the original-basis and the new coordinates isn't done multiple times.
+ * Therefore #ED_editors_flush_edits and other flushing calls will update both the #Mesh.key
+ * and the edit-mode #CD_SHAPEKEY custom-data layers.
+ *
+ * WARNING: There is an exception to the rule of ignoring coordinates in the destination:
+ * that is when shape-key data in `bm` can't be found (which is itself an error/exception).
+ * In this case our own rule is violated as the alternative is loosing the shape-data entirely.
+ *
+ * Flushing Coordinates Back to the #BMesh
+ * ---------------------------------------
+ *
+ * The edit-mesh may be flushed back to the #Mesh and #Key used to generate it.
+ * When this is done, the new values are written back to the #BMesh's #CD_SHAPEKEY as well.
+ * This is necessary when editing basis-shapes so the difference in shape keys
+ * is not applied multiple times. If it were important to avoid it could be skipped while
+ * exiting edit-mode (as the entire #BMesh is freed in that case), however it's just copying
+ * back a `float[3]` so the work to check if it's necessary isn't worth the overhead.
+ *
+ * In general updating the #BMesh's #CD_SHAPEKEY makes shake-key logic easier to reason about
+ * since it means flushing data back to the mesh has the same behavior as exiting and entering
+ * edit-mode (a more common operation). Meaning there is one less corner-case to have to consider.
+ *
+ * Exceptional Cases
+ * *****************
+ *
+ * There are some situations that should not happen in typical usage but are
+ * still handled in this code, since failure to handle them could loose user-data.
+ * These could be investigated further since if they never happen in practice,
+ * we might consider removing them. However, the possibility of an mesh directly
+ * being modified by Python or some other low level logic that changes key-blocks
+ * means there is a potential this to happen so keeping code to these cases remain supported.
+ *
+ * - Custom Data & Mesh Key Block Synchronization.
+ * Key blocks in `me->key->block` should always have an associated
+ * #CD_SHAPEKEY layer in `bm->vdata`.
+ * If they don't there are two fall-backs for setting the location,
+ * - Use the value from the original shape key
+ * WARNING: this is technically incorrect! (see note on "Key Block Usage").
+ * - Use the current vertex location,
+ * Also not correct but it's better then having it zeroed for e.g.
+ *
+ * - Missing key-index layer.
+ * In this case the basis key wont apply it's deltas to other keys and in the case
+ * a shape-key layer is missing, its coordinates will be initialized from the edit-mesh
+ * vertex locations instead of attempting to remap the shape-keys coordinates.
+ *
+ * \note These cases are considered abnormal and shouldn't occur in typical usage.
+ * A warning is logged in this case to help troubleshooting bugs with shape-keys.
+ * \{ */
+
/**
- * Returns custom-data shapekey index from a keyblock or -1
+ * Returns custom-data shape-key index from a key-block or -1
* \note could split this out into a more generic function.
*/
static int bm_to_mesh_shape_layer_index_from_kb(BMesh *bm, KeyBlock *currkey)
@@ -587,6 +685,196 @@ static int bm_to_mesh_shape_layer_index_from_kb(BMesh *bm, KeyBlock *currkey)
return -1;
}
+/**
+ * Update `key` with shape key data stored in `bm`.
+ *
+ * \param bm: The source BMesh.
+ * \param key: The destination key.
+ * \param mvert: The destination vertex array (in some situations it's coordinates are updated).
+ */
+static void bm_to_mesh_shape(BMesh *bm, Key *key, MVert *mvert)
+{
+ KeyBlock *actkey = static_cast<KeyBlock *>(BLI_findlink(&key->block, bm->shapenr - 1));
+
+ /* It's unlikely this ever remains false, check for correctness. */
+ bool actkey_has_layer = false;
+
+ /* Go through and find any shape-key custom-data layers
+ * that might not have corresponding KeyBlocks, and add them if necessary. */
+ for (int i = 0; i < bm->vdata.totlayer; i++) {
+ if (bm->vdata.layers[i].type != CD_SHAPEKEY) {
+ continue;
+ }
+
+ KeyBlock *currkey;
+ for (currkey = (KeyBlock *)key->block.first; currkey; currkey = currkey->next) {
+ if (currkey->uid == bm->vdata.layers[i].uid) {
+ break;
+ }
+ }
+
+ if (currkey) {
+ if (currkey == actkey) {
+ actkey_has_layer = true;
+ }
+ }
+ else {
+ currkey = BKE_keyblock_add(key, bm->vdata.layers[i].name);
+ currkey->uid = bm->vdata.layers[i].uid;
+ }
+ }
+
+ const int cd_shape_keyindex_offset = CustomData_get_offset(&bm->vdata, CD_SHAPE_KEYINDEX);
+ BMIter iter;
+ BMVert *eve;
+ float(*ofs)[3] = nullptr;
+
+ /* Editing the basis key updates others. */
+ if ((key->type == KEY_RELATIVE) &&
+ /* The shape-key coordinates used from entering edit-mode are used. */
+ (actkey_has_layer == true) &&
+ /* Original key-indices are only used to check the vertex existed when entering edit-mode. */
+ (cd_shape_keyindex_offset != -1) &&
+ /* Offsets are only needed if the current shape is a basis for others. */
+ BKE_keyblock_is_basis(key, bm->shapenr - 1)) {
+
+ BLI_assert(actkey != nullptr); /* Assured by `actkey_has_layer` check. */
+ const int actkey_uuid = bm_to_mesh_shape_layer_index_from_kb(bm, actkey);
+
+ /* Since `actkey_has_layer == true`, this must never fail. */
+ BLI_assert(actkey_uuid != -1);
+
+ const int cd_shape_offset = CustomData_get_n_offset(&bm->vdata, CD_SHAPEKEY, actkey_uuid);
+
+ ofs = static_cast<float(*)[3]>(MEM_mallocN(sizeof(float[3]) * bm->totvert, __func__));
+ int i;
+ BM_ITER_MESH_INDEX (eve, &iter, bm, BM_VERTS_OF_MESH, i) {
+ const int keyi = BM_ELEM_CD_GET_INT(eve, cd_shape_keyindex_offset);
+ /* Check the vertex existed when entering edit-mode (otherwise don't apply an offset). */
+ if (keyi != ORIGINDEX_NONE) {
+ float *co_orig = (float *)BM_ELEM_CD_GET_VOID_P(eve, cd_shape_offset);
+ /* Could use 'eve->co' or the destination #MVert.co, they're the same at this point. */
+ sub_v3_v3v3(ofs[i], eve->co, co_orig);
+ }
+ else {
+ /* If there are new vertices in the mesh, we can't propagate the offset
+ * because it will only work for the existing vertices and not the new
+ * ones, creating a mess when doing e.g. subdivide + translate. */
+ MEM_freeN(ofs);
+ ofs = nullptr;
+ break;
+ }
+ }
+ }
+
+ LISTBASE_FOREACH (KeyBlock *, currkey, &key->block) {
+ int keyi;
+ float(*currkey_data)[3];
+
+ const int currkey_uuid = bm_to_mesh_shape_layer_index_from_kb(bm, currkey);
+ const int cd_shape_offset
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list