Fix T91444: Edge Loop Preview fails with two Mirror Modifiers

The mirror modifiers merge option caused unnecessary re-ordering
to the vertex array with original vertices merging into their copies.

While this wasn't an error, it meant creating a 1:1 mapping from input
vertices to their final output wasn't reliable (when looping over
vertices first to last) as is done in
BKE_editmesh_vert_coords_when_deformed.

As merging in either direction is supported, keep the source meshes
vertices in-order since it allows the vertex coordinates to be extracted.

NOTE: Since this change introduce issues for some cases (e.g. bound
modifiers like SurfaceDeform), this change is only applied to newly
created modifiers, existing ones will still use the old incorrect merge
behavior.

Reviewed By: @brecht

Maniphest Tasks: T93321, T91444

Differential Revision: https://developer.blender.org/D13355

6 files changed, 69 insertions, 22 deletions

diff --git a/source/blender/blenkernel/BKE_mesh_mirror.h b/source/blender/blenkernel/BKE_mesh_mirror.h
index 7b230b04410..eaa479be867 100644
--- a/source/blender/blenkernel/BKE_mesh_mirror.h
+++ b/source/blender/blenkernel/BKE_mesh_mirror.h
@@ -43,10 +43,12 @@ void BKE_mesh_mirror_apply_mirror_on_axis(struct Main *bmain,
                                           const int axis,
                                           const float dist);
 
-struct Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(struct MirrorModifierData *mmd,
-                                                               struct Object *ob,
-                                                               const struct Mesh *mesh,
-                                                               const int axis);
+struct Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(
+    struct MirrorModifierData *mmd,
+    struct Object *ob,
+    const struct Mesh *mesh,
+    const int axis,
+    const bool use_correct_order_on_merge);
 
 #ifdef __cplusplus
 }
diff --git a/source/blender/blenkernel/intern/mesh_mirror.c b/source/blender/blenkernel/intern/mesh_mirror.c
index b20d81e7b9c..2756629b440 100644
--- a/source/blender/blenkernel/intern/mesh_mirror.c
+++ b/source/blender/blenkernel/intern/mesh_mirror.c
@@ -137,7 +137,8 @@ void BKE_mesh_mirror_apply_mirror_on_axis(struct Main *bmain,
 Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(MirrorModifierData *mmd,
                                                         Object *ob,
                                                         const Mesh *mesh,
-                                                        const int axis)
+                                                        const int axis,
+                                                        const bool use_correct_order_on_merge)
 {
   const float tolerance_sq = mmd->tolerance * mmd->tolerance;
   const bool do_vtargetmap = (mmd->flag & MOD_MIR_NO_MERGE) == 0;
@@ -260,21 +261,51 @@ Mesh *BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(MirrorModifierData *mmd,
     mul_m4_v3(mtx, mv->co);
 
     if (do_vtargetmap) {
-      /* compare location of the original and mirrored vertex, to see if they
-       * should be mapped for merging */
-      if (UNLIKELY(len_squared_v3v3(mv_prev->co, mv->co) < tolerance_sq)) {
-        *vtmap_a = maxVerts + i;
-        tot_vtargetmap++;
-
-        /* average location */
-        mid_v3_v3v3(mv->co, mv_prev->co, mv->co);
-        copy_v3_v3(mv_prev->co, mv->co);
-      }
-      else {
+      /* Compare location of the original and mirrored vertex,
+       * to see if they should be mapped for merging.
+       *
+       * Always merge from the copied into the original vertices so it's possible to
+       * generate a 1:1 mapping by scanning vertices from the beginning of the array
+       * as is done in #BKE_editmesh_vert_coords_when_deformed. Without this,
+       * the coordinates returned will sometimes point to the copied vertex locations, see:
+       * T91444.
+       *
+       * However, such a change also affects non-versionable things like some modifiers binding, so
+       * we cannot enforce that behavior on existing modifiers, in which case we keep using the
+       * old, incorrect behavior of merging the source vertex into its copy.
+       */
+      if (use_correct_order_on_merge) {
+        if (UNLIKELY(len_squared_v3v3(mv_prev->co, mv->co) < tolerance_sq)) {
+          *vtmap_b = i;
+          tot_vtargetmap++;
+
+          /* average location */
+          mid_v3_v3v3(mv->co, mv_prev->co, mv->co);
+          copy_v3_v3(mv_prev->co, mv->co);
+        }
+        else {
+          *vtmap_b = -1;
+        }
+
+        /* Fill here to avoid 2x loops. */
         *vtmap_a = -1;
       }
+      else {
+        if (UNLIKELY(len_squared_v3v3(mv_prev->co, mv->co) < tolerance_sq)) {
+          *vtmap_a = maxVerts + i;
+          tot_vtargetmap++;
+
+          /* average location */
+          mid_v3_v3v3(mv->co, mv_prev->co, mv->co);
+          copy_v3_v3(mv_prev->co, mv->co);
+        }
+        else {
+          *vtmap_a = -1;
+        }
 
-      *vtmap_b = -1; /* fill here to avoid 2x loops */
+        /* Fill here to avoid 2x loops. */
+        *vtmap_b = -1;
+      }
 
       vtmap_a++;
       vtmap_b++;
diff --git a/source/blender/editors/object/object_remesh.cc b/source/blender/editors/object/object_remesh.cc
index 3bdf7e0d34d..719ffd36f03 100644
--- a/source/blender/editors/object/object_remesh.cc
+++ b/source/blender/editors/object/object_remesh.cc
@@ -821,7 +821,8 @@ static Mesh *remesh_symmetry_mirror(Object *ob, Mesh *mesh, eSymmetryAxes symmet
       mmd.flag = 0;
       mmd.flag &= MOD_MIR_AXIS_X << i;
       mesh_mirror_temp = mesh_mirror;
-      mesh_mirror = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(&mmd, ob, mesh_mirror, axis);
+      mesh_mirror = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(
+          &mmd, ob, mesh_mirror, axis, true);
       if (mesh_mirror_temp != mesh_mirror) {
         BKE_id_free(nullptr, mesh_mirror_temp);
       }
diff --git a/source/blender/makesdna/DNA_modifier_defaults.h b/source/blender/makesdna/DNA_modifier_defaults.h
index 5b2694f420b..78ab41031a6 100644
--- a/source/blender/makesdna/DNA_modifier_defaults.h
+++ b/source/blender/makesdna/DNA_modifier_defaults.h
@@ -429,6 +429,7 @@
     .uv_offset = {0.0f, 0.0f}, \
     .uv_offset_copy = {0.0f, 0.0f}, \
     .mirror_ob = NULL, \
+    .use_correct_order_on_merge = true, \
   }
 
 #define _DNA_DEFAULT_MultiresModifierData \
diff --git a/source/blender/makesdna/DNA_modifier_types.h b/source/blender/makesdna/DNA_modifier_types.h
index 631db64ddd3..f7a468264c3 100644
--- a/source/blender/makesdna/DNA_modifier_types.h
+++ b/source/blender/makesdna/DNA_modifier_types.h
@@ -374,7 +374,15 @@ typedef struct MirrorModifierData {
   short flag;
   float tolerance;
   float bisect_threshold;
-  char _pad[4];
+
+  /** Mirror modifier used to merge the old vertex into its new copy, which would break code
+   * relying on access to the original geometry vertices. However, modifying this behavior to the
+   * correct one (i.e. merging the copy vertices into their original sources) has several potential
+   * effects on other modifiers and tools, so we need to keep that incorrect behavior for existing
+   * modifiers, and only use the new correct one for new modifiers. */
+  uint8_t use_correct_order_on_merge;
+
+  char _pad[3];
   float uv_offset[2];
   float uv_offset_copy[2];
   struct Object *mirror_ob;
diff --git a/source/blender/modifiers/intern/MOD_mirror.c b/source/blender/modifiers/intern/MOD_mirror.c
index 7fd90c71c9f..bbac6589577 100644
--- a/source/blender/modifiers/intern/MOD_mirror.c
+++ b/source/blender/modifiers/intern/MOD_mirror.c
@@ -84,14 +84,17 @@ static void updateDepsgraph(ModifierData *md, const ModifierUpdateDepsgraphConte
 static Mesh *mirrorModifier__doMirror(MirrorModifierData *mmd, Object *ob, Mesh *mesh)
 {
   Mesh *result = mesh;
+  const bool use_correct_order_on_merge = mmd->use_correct_order_on_merge;
 
   /* check which axes have been toggled and mirror accordingly */
   if (mmd->flag & MOD_MIR_AXIS_X) {
-    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(mmd, ob, result, 0);
+    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(
+        mmd, ob, result, 0, use_correct_order_on_merge);
   }
   if (mmd->flag & MOD_MIR_AXIS_Y) {
     Mesh *tmp = result;
-    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(mmd, ob, result, 1);
+    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(
+        mmd, ob, result, 1, use_correct_order_on_merge);
     if (tmp != mesh) {
       /* free intermediate results */
       BKE_id_free(NULL, tmp);
@@ -99,7 +102,8 @@ static Mesh *mirrorModifier__doMirror(MirrorModifierData *mmd, Object *ob, Mesh
   }
   if (mmd->flag & MOD_MIR_AXIS_Z) {
     Mesh *tmp = result;
-    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(mmd, ob, result, 2);
+    result = BKE_mesh_mirror_apply_mirror_on_axis_for_modifier(
+        mmd, ob, result, 2, use_correct_order_on_merge);
     if (tmp != mesh) {
       /* free intermediate results */
       BKE_id_free(NULL, tmp);