Fix #109565: Array modifier changes the Root vertex of Skin modifier #109627

Germano Cavalcante · 2023-07-02T23:53:46+02:00

2023-07-02 23:53:46 +02:00

The merging behavior of the Array, Mirror, Screw modifiers has changed
since 4369627e71.

Before, only the customdata of the destination vertex was kept. Source
vertices were completely removed.

With 4369627e71, all vertices in the merge group (whether source or
destination) now have their customdata interpolated.

But this can cause problems for example with the root vertex customdata
of the skin modifier, where if only one of the vertices of the group of
vertices has this flag, the resulting one will also have it.

This commit restores the behavior for the vertices customdata and does
not interpolate it if mix_data is false.

The merging behavior of the Array, Mirror, Screw modifiers has changed since 4369627e71. Before, only the customdata of the destination vertex was kept. Source vertices were completely removed. With 4369627e71, all vertices in the merge group (whether source or destination) now have their customdata interpolated. But this can cause problems for example with the root vertex customdata of the skin modifier, where if only one of the vertices of the group of vertices has this flag, the resulting one will also have it. This commit restores the behavior for the vertices customdata and does not interpolate it if `mix_data` is false.

Germano Cavalcante added the

Module

Modeling

label 2023-07-02 23:53:46 +02:00

Germano Cavalcante requested review from Hans Goudey 2023-07-02 23:54:06 +02:00

Germano Cavalcante force-pushed fix_109565 from a30e53dea1 to 4f247a36a5

2023-07-05 02:49:47 +02:00

Compare

Germano Cavalcante commented

2023-07-05 02:50:51 +02:00

Fix unnecessary array copy

- Fix unnecessary array copy

Hans Goudey requested changes 2023-07-05 20:38:43 +02:00

source/blender/geometry/GEO_mesh_merge_by_distance.hh

						
				@ -53,6 +53,9 @@ std::optional<Mesh *> mesh_merge_by_distance_connected(const Mesh &mesh,

				 *

				 * \param vert_dest_map_len: The number of non '-1' values in `vert_dest_map`. (not the size)

Hans Goudey commented

2023-07-05 20:38:41 +02:00

Add a description for mix_data, something like mentioning that mentions which element the final data comes from.

Add a description for `mix_data`, something like mentioning that mentions which element the final data comes from.

mano-wii marked this conversation as resolved

source/blender/geometry/intern/mesh_merge_by_distance.cc Outdated

						
				@ -1547,2 +1549,3 @@

				                                MutableSpan<int> vert_dest_map,

				                                const int removed_vertex_count)

				                                const int removed_vertex_count,

				                                bool mix_data)

Hans Goudey commented

2023-07-05 20:35:15 +02:00

bool -> const bool

Also, this only seems to control mixing for vertex data. Maybe the name should reflect that?

`bool` -> `const bool` Also, this only seems to control mixing for vertex data. Maybe the name should reflect that?

mano-wii marked this conversation as resolved

source/blender/geometry/intern/mesh_merge_by_distance.cc Outdated

						
				@ -1580,2 +1585,2 @@

				   * This map will be used to adjust edges and loops to point to new vertex indices. */

				  MutableSpan<int> vert_final_map = vert_group_map;

				  MutableSpan<int> vert_final_map;

				  Array<int> vert_final_map_stack;

Hans Goudey commented

2023-07-05 20:34:28 +02:00

"stack" isn't quite right, since the array will be allocated on the heap. I'd suggest _local instead of _stack.

"stack" isn't quite right, since the array will be allocated on the heap. I'd suggest `_local` instead of `_stack`.

mano-wii marked this conversation as resolved

source/blender/geometry/intern/mesh_merge_by_distance.cc Outdated

						
				@ -1605,2 +1619,4 @@

				        count = *(wgroup + 1) - *wgroup;

				      }

				      else {

				        src_indices = &i;

Hans Goudey commented

2023-07-05 20:37:09 +02:00

Would be better in my opinion to use a separate loop for the "no mixing" case. The goals are very different, and a performant implementation that copies a single index at a time would probably look quite different from one that mixes the data of multiple elements together.

Even if this causes some code duplication, that's okay I think.

Would be better in my opinion to use a separate loop for the "no mixing" case. The goals are very different, and a performant implementation that copies a single index at a time would probably look quite different from one that mixes the data of multiple elements together. Even if this causes some code duplication, that's okay I think.

Germano Cavalcante commented

2023-07-05 23:23:45 +02:00

Unfortunately the code duplication would be more than seems appropriate. But I improved the readability by:

Checking do_mix_vert_data instead of vert_groups_map.is_empty();
Using CustomData_copy_data instead of customdata_weld if do_mix_vert_data is false;
Moving vert_groups_map to WeldMesh (its usage becomes clearer).

Unfortunately the code duplication would be more than seems appropriate. But I improved the readability by: - Checking `do_mix_vert_data` instead of `vert_groups_map.is_empty()`; - Using `CustomData_copy_data` instead of `customdata_weld` if `do_mix_vert_data` is false; - Moving `vert_groups_map` to `WeldMesh` (its usage becomes clearer).

source/blender/geometry/intern/mesh_merge_by_distance.cc Outdated

						
				@ -1876,0 +1893,4 @@

				Mesh *mesh_merge_verts(const Mesh &mesh,

				                       MutableSpan<int> vert_dest_map,

				                       int vert_dest_map_len,

				                       bool mix_data)

Hans Goudey commented

2023-07-05 20:37:49 +02:00

bool -> const bool

`bool` -> `const bool`

mano-wii marked this conversation as resolved

Germano Cavalcante commented

2023-07-05 23:25:31 +02:00

Not sure if my reply got notified. So I'm copying it here:

Unfortunately the code duplication would be more than seems appropriate. But I improved the readability by:

Checking do_mix_vert_data instead of vert_groups_map.is_empty();

Using CustomData_copy_data instead of customdata_weld if do_mix_vert_data is false;

Moving vert_groups_map to WeldMesh (its usage becomes clearer).

Not sure if my reply got notified. So I'm copying it here: > Unfortunately the code duplication would be more than seems appropriate. But I improved the readability by: > - Checking `do_mix_vert_data` instead of `vert_groups_map.is_empty()`; > - Using `CustomData_copy_data` instead of `customdata_weld` if `do_mix_vert_data` is false; > - Moving `vert_groups_map` to `WeldMesh` (its usage becomes clearer).

Hans Goudey approved these changes 2023-07-06 02:25:49 +02:00

Hans Goudey left a comment

Just two things noted inline

source/blender/geometry/GEO_mesh_merge_by_distance.hh Outdated

						
				@ -54,1 +54,4 @@

				 * \param vert_dest_map_len: The number of non '-1' values in `vert_dest_map`. (not the size)

				 * \param do_mix_vert_data: If true, the groups of vertices in the `vert_dest_map_len`, defined by

				 * source vertices with the same target plus the target vertex, will have their custom data

				 * interpolated into the resulting vertex.If false, only the custom data of the target vertex will

Hans Goudey commented

2023-07-06 02:24:20 +02:00

Missing space after "vertex."

source/blender/geometry/intern/mesh_merge_by_distance.cc Outdated

						
				@ -1377,2 +1379,2 @@

				                                     MutableSpan<int> r_vert_group_map,

				                                     WeldMesh *r_weld_mesh)

				                                     WeldMesh *r_weld_mesh,

				                                     const bool do_vert_group)