Fix and cleanup: Pack to correct/Specified UDIMsoc-2021-uv-editor-improvements

After recent changes made in master branch, it is now possible to display
UDIM grid and tiled images simultaneously in the UV Editor.
This commit updates the pack islands to correct/specified UDIM
implementation to work with the new behavior. Also includes some code
and comment cleanups.

4 files changed, 177 insertions, 120 deletions

diff --git a/source/blender/editors/include/ED_uvedit.h b/source/blender/editors/include/ED_uvedit.h
index 269590197ee..cd89620fe64 100644
--- a/source/blender/editors/include/ED_uvedit.h
+++ b/source/blender/editors/include/ED_uvedit.h
@@ -247,10 +247,11 @@ struct UVPackIsland_Params {
   uint correct_aspect : 1;
 };
 void ED_uvedit_pack_islands_multi(const struct Scene *scene,
+                                  const struct SpaceImage *sima,
                                   Object **objects,
                                   const uint objects_len,
-                                  const struct SpaceImage *sima,
-                                  bool use_target,
+                                  const bool use_target_udim,
+                                  int target_udim,
                                   const struct UVPackIsland_Params *params);
 
 bool ED_uvedit_pack_islands_to_area_multi(const struct Scene *scene,
diff --git a/source/blender/editors/uvedit/uvedit_islands.c b/source/blender/editors/uvedit/uvedit_islands.c
index ef990080ec1..370e0176298 100644
--- a/source/blender/editors/uvedit/uvedit_islands.c
+++ b/source/blender/editors/uvedit/uvedit_islands.c
@@ -359,10 +359,11 @@ static int bm_mesh_calc_uv_islands(const Scene *scene,
  * \{ */
 
 void ED_uvedit_pack_islands_multi(const Scene *scene,
+                                  const SpaceImage *sima,
                                   Object **objects,
                                   const uint objects_len,
-                                  const SpaceImage *sima,
-                                  bool use_target,
+                                  const bool use_target_udim,
+                                  int target_udim,
                                   const struct UVPackIsland_Params *params)
 {
   /* Align to the Y axis, could make this configurable. */
@@ -370,18 +371,6 @@ void ED_uvedit_pack_islands_multi(const Scene *scene,
   ListBase island_list = {NULL};
   int island_list_len = 0;
 
-  const Image *image;
-  bool is_tiled_image = false;
-  int udim_grid[2] = {1, 1};
-
-  /* To handle cases where sima=NULL - Smart UV project */
-  if (sima) {
-    image = sima->image;
-    is_tiled_image = image && (image->source == IMA_SRC_TILED);
-    udim_grid[0] = sima->tile_grid_shape[0];
-    udim_grid[1] = sima->tile_grid_shape[1];
-  }
-
   for (uint ob_index = 0; ob_index < objects_len; ob_index++) {
     Object *obedit = objects[ob_index];
     BMEditMesh *em = BKE_editmesh_from_object(obedit);
@@ -423,26 +412,25 @@ void ED_uvedit_pack_islands_multi(const Scene *scene,
   BoxPack *boxarray = MEM_mallocN(sizeof(*boxarray) * island_list_len, __func__);
 
   int index;
-  /* Coordinates for the center of the all the selected islands */
-  float selection_center[2] = {0.0f, 0.0f};
-  float selection_min[2], selection_max[2];
-  INIT_MINMAX2(selection_min, selection_max);
+  float selection_min_co[2], selection_max_co[2];
+  INIT_MINMAX2(selection_min_co, selection_max_co);
 
   LISTBASE_FOREACH_INDEX (struct FaceIsland *, island, &island_list, index) {
 
-    /* Calculate bounding box of all selected islands */
-    float bounds_min[2], bounds_max[2];
-    INIT_MINMAX2(bounds_min, bounds_max);
-    for (int i = 0; i < island->faces_len; i++) {
-      BMFace *f = island->faces[i];
-      BM_face_uv_minmax(f, bounds_min, bounds_max, island->cd_loop_uv_offset);
-    }
-
-    selection_min[0] = MIN2(bounds_min[0], selection_min[0]);
-    selection_min[1] = MIN2(bounds_min[1], selection_min[1]);
-    selection_max[0] = MAX2(bounds_max[0], selection_max[0]);
-    selection_max[1] = MAX2(bounds_max[1], selection_max[1]);
+    /* Skip calculation if not using Specified UDIM option */
+    if (!use_target_udim) {
+      float bounds_min[2], bounds_max[2];
+      INIT_MINMAX2(bounds_min, bounds_max);
+      for (int i = 0; i < island->faces_len; i++) {
+        BMFace *f = island->faces[i];
+        BM_face_uv_minmax(f, bounds_min, bounds_max, island->cd_loop_uv_offset);
+      }
 
+      selection_min_co[0] = MIN2(bounds_min[0], selection_min_co[0]);
+      selection_min_co[1] = MIN2(bounds_min[1], selection_min_co[1]);
+      selection_max_co[0] = MAX2(bounds_max[0], selection_max_co[0]);
+      selection_max_co[1] = MAX2(bounds_max[1], selection_max_co[1]);
+    }
     if (params->rotate) {
       if (island->aspect_y != 1.0f) {
         bm_face_array_uv_scale_y(
@@ -475,9 +463,12 @@ void ED_uvedit_pack_islands_multi(const Scene *scene,
     }
   }
 
-  /* Calculate the center of the bounding box */
-  selection_center[0] = (selection_min[0] + selection_max[0]) / 2.0f;
-  selection_center[1] = (selection_min[1] + selection_max[1]) / 2.0f;
+  /* Center of the selected UV bounding boxes */
+  float selection_center[2];
+  if (!use_target_udim) {
+    selection_center[0] = (selection_min_co[0] + selection_max_co[0]) / 2.0f;
+    selection_center[1] = (selection_min_co[1] + selection_max_co[1]) / 2.0f;
+  }
 
   if (margin > 0.0f) {
     /* Logic matches behavior from #param_pack,
@@ -505,55 +496,109 @@ void ED_uvedit_pack_islands_multi(const Scene *scene,
   /* Tile offset */
   float base_offset[2] = {0.0f, 0.0f};
 
-  /* (sima = NULL) or (use_target = false) would skip the calculation of base_offset - Smart UV
-   * project */
-  if (use_target) {
-    const int specified_tile_index = scene->toolsettings->target_udim - 1001;
+  /* CASE: Specified UDIM */
+  if (use_target_udim) {
+    const int specified_tile_index = target_udim - 1001;
     /* Calculate offset based on specified_tile_index */
     base_offset[0] = specified_tile_index % 10;
     base_offset[1] = specified_tile_index / 10;
   }
 
-  /* If tiled image then constrain to correct/closest UDIM tile */
-  else if (sima && is_tiled_image && !use_target) {
-    int nearest_tile_index = BKE_image_find_nearest_tile(image, selection_center);
-    if (nearest_tile_index != -1) {
-      nearest_tile_index -= 1001;
-      /* Calculate offset based on nearest_tile_index */
-      base_offset[0] = nearest_tile_index % 10;
-      base_offset[1] = nearest_tile_index / 10;
+  /* CASE: Closest UDIM */
+  else {
+    const Image *image;
+    bool is_tiled_image = false;
+    int udim_grid[2] = {1, 1};
+
+    /* To handle cases where sima=NULL - Smart UV project in 3D viewport */
+    if (sima != NULL) {
+      image = sima->image;
+      is_tiled_image = image && (image->source == IMA_SRC_TILED);
+      udim_grid[0] = sima->tile_grid_shape[0];
+      udim_grid[1] = sima->tile_grid_shape[1];
     }
-  }
 
-  /* If no image present then constrain to correct/closest tile on UDIM grid*/
-  else if (sima && !image && !use_target) {
-    const float co_floor[2] = {floorf(selection_center[0]), floorf(selection_center[1])};
+    /* Check if selection lies on a valid UDIM grid tile */
+    bool is_valid_udim = false;
+    const float selection_co_floor[2] = {floorf(selection_center[0]), floorf(selection_center[1])};
     if (selection_center[0] < udim_grid[0] && selection_center[0] > 0 &&
         selection_center[1] < udim_grid[1] && selection_center[1] > 0) {
-      base_offset[0] = co_floor[0];
-      base_offset[1] = co_floor[1];
+      base_offset[0] = selection_co_floor[0];
+      base_offset[1] = selection_co_floor[1];
+      is_valid_udim = true;
     }
-    /* If Selected UVs lie outside the UDIM grid, constrain to closest tile on UDIM grid */
-    else {
+    /* Check if selection lies on a valid UDIM image tile */
+    else if (is_tiled_image) {
+      LISTBASE_FOREACH (const ImageTile *, tile, &image->tiles) {
+        const int tile_index = tile->tile_number - 1001;
+        const int target_x = (tile_index % 10);
+        const int target_y = (tile_index / 10);
+        if (selection_co_floor[0] == target_x && selection_co_floor[1] == target_y) {
+          base_offset[0] = selection_co_floor[0];
+          base_offset[1] = selection_co_floor[1];
+          is_valid_udim = true;
+        }
+      }
+    }
+    /* Probably not required since UDIM grid checks for 1001 */
+    else if (image && !is_tiled_image) {
+      if (is_zero_v2(selection_co_floor)) {
+        base_offset[0] = selection_co_floor[0];
+        base_offset[1] = selection_co_floor[1];
+        is_valid_udim = true;
+      }
+    }
+    /* If selection doesn't lie on any UDIM then compare both closest grid tile and image tile.
+     * Save the one that is closest */
+    if (!is_valid_udim) {
+      float nearest_image_tile_co[2] = {FLT_MAX, FLT_MAX};
+      if (image) {
+        int nearest_image_tile_index = BKE_image_find_nearest_tile(image, selection_center);
+        if (nearest_image_tile_index == -1) {
+          nearest_image_tile_index = 1001;
+        }
+        /* Calculate offset based on nearest_tile_index */
+
+        nearest_image_tile_co[0] = (nearest_image_tile_index - 1001) % 10;
+        nearest_image_tile_co[1] = (nearest_image_tile_index - 1001) / 10;
+        /* + 0.5f to get tile center coordinates */
+        nearest_image_tile_co[0] += 0.5f;
+        nearest_image_tile_co[1] += 0.5f;
+      }
+
+      float nearest_grid_tile_co[2] = {0.0f, 0.0f};
       if (selection_center[0] > udim_grid[0]) {
-        base_offset[0] = udim_grid[0] - 1;
+        nearest_grid_tile_co[0] = udim_grid[0] - 1;
       }
       else if (selection_center[0] < 0) {
-        base_offset[0] = 0;
+        nearest_grid_tile_co[0] = 0;
       }
       else {
-        base_offset[0] = co_floor[0];
+        nearest_grid_tile_co[0] = selection_co_floor[0];
       }
 
       if (selection_center[1] > udim_grid[1]) {
-        base_offset[1] = udim_grid[1] - 1;
+        nearest_grid_tile_co[1] = udim_grid[1] - 1;
       }
       else if (selection_center[1] < 0) {
-        base_offset[1] = 0;
+        nearest_grid_tile_co[1] = 0;
       }
       else {
-        base_offset[1] = co_floor[1];
+        nearest_grid_tile_co[1] = selection_co_floor[1];
       }
+      /* + 0.5f to get tile center coordinates */
+      nearest_grid_tile_co[0] += 0.5f;
+      nearest_grid_tile_co[1] += 0.5f;
+
+      float nearest_image_tile_dist = len_squared_v2v2(selection_center, nearest_image_tile_co);
+      float nearest_grid_tile_dist = len_squared_v2v2(selection_center, nearest_grid_tile_co);
+
+      base_offset[0] = (nearest_image_tile_dist < nearest_grid_tile_dist) ?
+                           (nearest_image_tile_co[0] - 0.5f) :
+                           (nearest_grid_tile_co[0] - 0.5f);
+      base_offset[1] = (nearest_image_tile_dist < nearest_grid_tile_dist) ?
+                           (nearest_image_tile_co[1] - 0.5f) :
+                           (nearest_grid_tile_co[1] - 0.5f);
     }
   }
 
diff --git a/source/blender/editors/uvedit/uvedit_unwrap_ops.c b/source/blender/editors/uvedit/uvedit_unwrap_ops.c
index 8387e09b7e8..3dbf9c69b02 100644
--- a/source/blender/editors/uvedit/uvedit_unwrap_ops.c
+++ b/source/blender/editors/uvedit/uvedit_unwrap_ops.c
@@ -1021,7 +1021,6 @@ static int pack_islands_exec(bContext *C, wmOperator *op)
 
   const Image *image = sima->image;
   const bool is_tiled_image = image && (image->source == IMA_SRC_TILED);
-  bool use_target = false;
 
   const UnwrapOptions options = {
       .topology_from_uvs = true,
@@ -1041,67 +1040,74 @@ static int pack_islands_exec(bContext *C, wmOperator *op)
     return OPERATOR_CANCELLED;
   }
 
+  /* Set RNA props */
   bool rotate = RNA_boolean_get(op->ptr, "rotate");
+  const bool use_target_udim = (RNA_enum_get(op->ptr, "packTo") == SPECIFIED_UDIM);
+  if (RNA_struct_property_is_set(op->ptr, "margin")) {
+    scene->toolsettings->uvcalc_margin = RNA_float_get(op->ptr, "margin");
+  }
+  else {
+    RNA_float_set(op->ptr, "margin", scene->toolsettings->uvcalc_margin);
+  }
 
-  /* Check if specified UDIM is valid - specified tile exists in the udim grid or tiled image */
-  if (RNA_enum_get(op->ptr, "packTo") == SPECIFIED_UDIM) {
-    if (RNA_struct_property_is_set(op->ptr, "target_udim")) {
-      int target_udim = RNA_int_get(op->ptr, "target_udim");
-
-      /* If no image in the UV editor then check if target_UDIM lies within the UDIM grid*/
-      if (!image) {
-        target_udim -= 1001;
-        const int target_x = (target_udim % 10) + 1;
-        const int target_y = (target_udim / 10) + 1;
-        if (target_x <= sima->tile_grid_shape[0] && target_y <= sima->tile_grid_shape[1]) {
-          scene->toolsettings->target_udim = RNA_int_get(op->ptr, "target_udim");
-        }
-        else {
-          RNA_int_set(op->ptr, "target_udim", scene->toolsettings->target_udim);
-        }
-      }
+  int target_udim = 1001;
+  if (use_target_udim) {
+    target_udim = RNA_int_get(op->ptr, "target_udim");
+    if (target_udim > 2000 || target_udim < 1001) {
+      /* Early exit since invalid UDIM was specified.
+       * Before exit, set RNA prop to the value specified when the operator was last used */
+      RNA_int_set(op->ptr, "target_udim", scene->toolsettings->target_udim);
+      MEM_freeN(objects);
+      return OPERATOR_CANCELLED;
+    }
 
-      /* If tiled image present then check if target_udim is valid */
-      else if (image && is_tiled_image) {
-        RNA_int_set(op->ptr, "target_udim", scene->toolsettings->target_udim);
-        LISTBASE_FOREACH (const ImageTile *, tile, &image->tiles) {
-          if (target_udim == tile->tile_number) {
-            scene->toolsettings->target_udim = target_udim;
-            RNA_int_set(op->ptr, "target_udim", target_udim);
-            break;
-          }
+    /* Check if specifed UDIM index is valid */
+    bool is_udim_valid = false;
+    const int target_x = ((target_udim - 1001) % 10) + 1;
+    const int target_y = ((target_udim - 1001) / 10) + 1;
+    if (target_x <= sima->tile_grid_shape[0] && target_y <= sima->tile_grid_shape[1]) {
+      scene->toolsettings->target_udim = target_udim;
+      is_udim_valid = true;
+    }
+    else if (is_tiled_image) {
+      LISTBASE_FOREACH (const ImageTile *, tile, &image->tiles) {
+        if (target_udim == tile->tile_number) {
+          scene->toolsettings->target_udim = target_udim;
+          is_udim_valid = true;
+          break;
         }
       }
-
-      /* If non-tiled image present then always pack to UDIM 1001 */
-      else if (image && !is_tiled_image) {
-        scene->toolsettings->target_udim = 1001;
-        RNA_int_set(op->ptr, "target_udim", 1001);
+    }
+    else if (image && !is_tiled_image) {
+      /* Non-tiled image. Always 0-1 UV space */
+      if (target_udim == 1001) {
+        scene->toolsettings->target_udim = target_udim;
+        is_udim_valid = true;
       }
     }
 
-    else {
-      scene->toolsettings->target_udim = RNA_int_get(op->ptr, "target_udim");
+    if (!is_udim_valid) {
+      if (RNA_struct_property_is_set(op->ptr, "target_udim")) {
+        /* Early exit since invalid UDIM was specified */
+        RNA_int_set(op->ptr, "target_udim", scene->toolsettings->target_udim);
+        MEM_freeN(objects);
+        return OPERATOR_CANCELLED;
+      }
+      else {
+        /* Fallback */
+        target_udim = 1001;
+        scene->toolsettings->target_udim = target_udim;
+        RNA_int_set(op->ptr, "target_udim", scene->toolsettings->target_udim);
+      }
     }
-    use_target = true;
-  }
-
-  else {
-    use_target = false;
-  }
-
-  if (RNA_struct_property_is_set(op->ptr, "margin")) {
-    scene->toolsettings->uvcalc_margin = RNA_float_get(op->ptr, "margin");
-  }
-  else {
-    RNA_float_set(op->ptr, "margin", scene->toolsettings->uvcalc_margin);
   }
 
   ED_uvedit_pack_islands_multi(scene,
+                               sima,
                                objects,
                                objects_len,
-                               sima,
-                               use_target,
+                               use_target_udim,
+                               target_udim,
                                &(struct UVPackIsland_Params){
                                    .rotate = rotate,
                                    .rotate_align_axis = -1,
@@ -1114,7 +1120,7 @@ static int pack_islands_exec(bContext *C, wmOperator *op)
   return OPERATOR_FINISHED;
 }
 
-static void pack_islands_draw(bContext *C, wmOperator *op)
+static void pack_islands_ui_draw(bContext *C, wmOperator *op)
 {
   uiLayout *layout = op->layout;
   uiLayout *col;
@@ -1124,7 +1130,7 @@ static void pack_islands_draw(bContext *C, wmOperator *op)
 
   col = uiLayoutColumn(layout, false);
 
-  /* Expose target UDIM property only if packing target is specified UDIM */
+  /* Expose target UDIM property only if packing target is set to Specified UDIM */
   uiItemR(col, op->ptr, "packTo", 0, NULL, 0);
   if (RNA_enum_get(op->ptr, "packTo") == SPECIFIED_UDIM) {
     uiItemR(col, op->ptr, "target_udim", 0, NULL, 0);
@@ -1136,7 +1142,7 @@ static void pack_islands_draw(bContext *C, wmOperator *op)
 
 void UV_OT_pack_islands(wmOperatorType *ot)
 {
-  static const EnumPropertyItem pack_to[] = {
+  static const EnumPropertyItem pack_target[] = {
       {CLOSEST_UDIM, "CLOSEST_UDIM", 0, "Closest UDIM", "Pack islands to closest UDIM"},
       {SPECIFIED_UDIM, "SPECIFIED_UDIM", 0, "Specified UDIM", "Pack islands to specified UDIM"},
       {0, NULL, 0, NULL, NULL},
@@ -1144,23 +1150,24 @@ void UV_OT_pack_islands(wmOperatorType *ot)
   /* identifiers */
   ot->name = "Pack Islands";
   ot->idname = "UV_OT_pack_islands";
-  ot->description = "Transform all islands so that they fill up the UV space as much as possible";
+  ot->description =
+      "Transform all islands so that they fill up the UV/UDIM space as much as possible";
 
   ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
 
   /* api callbacks */
   ot->exec = pack_islands_exec;
   ot->poll = ED_operator_uvedit;
-  ot->ui = pack_islands_draw;
+  ot->ui = pack_islands_ui_draw;
 
   /* properties */
-  RNA_def_enum(ot->srna, "packTo", pack_to, CLOSEST_UDIM, "Pack to", "");
+  RNA_def_enum(ot->srna, "packTo", pack_target, CLOSEST_UDIM, "Pack to", "");
   RNA_def_boolean(ot->srna, "rotate", true, "Rotate", "Rotate islands for best fit");
   RNA_def_int(ot->srna,
               "target_udim",
               1001,
               1001,
-              1100,
+              2000,
               "Target UDIM",
               "Pack islands to target UDIM",
               1001,
@@ -2366,6 +2373,8 @@ static int smart_project_exec(bContext *C, wmOperator *op)
 {
   Scene *scene = CTX_data_scene(C);
   ViewLayer *view_layer = CTX_data_view_layer(C);
+  /* sima=NULL cases are handled in ED_uvedit_pack_islands_multi() */
+  const SpaceImage *sima = CTX_wm_space_image(C);
 
   /* May be NULL. */
   View3D *v3d = CTX_wm_view3d(C);
@@ -2376,6 +2385,7 @@ static int smart_project_exec(bContext *C, wmOperator *op)
 
   const float project_angle_limit_cos = cosf(project_angle_limit);
   const float project_angle_limit_half_cos = cosf(project_angle_limit / 2);
+  const int target_udim = 1001; /* 0-1 UV space */
 
   /* Memory arena for list links (cleared for each object). */
   MemArena *arena = BLI_memarena_new(BLI_MEMARENA_STD_BUFSIZE, __func__);
@@ -2515,10 +2525,11 @@ static int smart_project_exec(bContext *C, wmOperator *op)
     /* Depsgraph refresh functions are called here. */
     const bool correct_aspect = RNA_boolean_get(op->ptr, "correct_aspect");
     ED_uvedit_pack_islands_multi(scene,
+                                 sima,
                                  objects_changed,
                                  object_changed_len,
-                                 NULL,
-                                 false,
+                                 true,
+                                 target_udim,
                                  &(struct UVPackIsland_Params){
                                      .rotate = true,
                                      /* We could make this optional. */
diff --git a/source/blender/makesdna/DNA_scene_types.h b/source/blender/makesdna/DNA_scene_types.h
index ef3d5b110d1..9f744a5614a 100644
--- a/source/blender/makesdna/DNA_scene_types.h
+++ b/source/blender/makesdna/DNA_scene_types.h
@@ -1404,8 +1404,8 @@ typedef struct ToolSettings {
   char uv_selectmode;
 
   float uvcalc_margin;
-  int target_udim;
-  int _pad3[3];
+  int target_udim; /* Can be extended for unwrap operator as well */
+  int _pad3[1];
 
   /* Auto-IK */
   /** Runtime only. */