Skip to content

Commit

Permalink
Fix: Incorrect BMesh to Mesh attribute copying
Browse files Browse the repository at this point in the history 
The existing logic to copy `BMesh` custom data layers to `Mesh`
attribute arrays was quite complicated, and incorrect in some cases
when the source and destinations didn't have the same layers.
The functions leave a lot to be desired in general, since they have
a lot of redundant complexity that ends up doing the same thing for
every element.

The problem in #104154 was that the "rest_position" attribute overwrote
the mesh positions since it has the same type and the positions weren't
copied. This same problem has shown up in boolean attribute conversion
in the past. Other changes fixed some specific cases but I think a
larger change is the only proper solution.

This patch adds preprocessing before looping over all elements to
find the basic information for copying the relevant layers, taking
layer names into account. The preprocessing makes the hot loops
simpler.

In a simple file with a 1 million vertex grid, I observed a 6%
improvement animation playback framerate in edit mode with a simple
geometry nodes modifier, from 5 to 5.3 FPS.

Fixes #104154, #104348

Pull Request #104421
  • Loading branch information
Hans Goudey committed Feb 13, 2023
1 parent 0dfc102 commit dfacaf4
Show file tree
Hide file tree
Showing 3 changed files with 162 additions and 155 deletions.
20 changes: 3 additions & 17 deletions source/blender/blenkernel/BKE_customdata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ bool CustomData_has_referenced(const struct CustomData *data);
* implemented for mloopuv/mloopcol, for now.
*/
void CustomData_data_copy_value(int type, const void *source, void *dest);
void CustomData_data_set_default_value(int type, void *elem);

/**
* Mixes the "value" (e.g. mloopuv uv or mloopcol colors) from one block into
Expand Down Expand Up @@ -506,6 +507,8 @@ void CustomData_clear_layer_flag(struct CustomData *data, int type, int flag);

void CustomData_bmesh_set_default(struct CustomData *data, void **block);
void CustomData_bmesh_free_block(struct CustomData *data, void **block);
void CustomData_bmesh_alloc_block(struct CustomData *data, void **block);

/**
* Same as #CustomData_bmesh_free_block but zero the memory rather than freeing.
*/
Expand All @@ -517,23 +520,6 @@ void CustomData_bmesh_free_block_data_exclude_by_type(struct CustomData *data,
void *block,
eCustomDataMask mask_exclude);

/**
* Copy custom data to/from layers as in mesh/derived-mesh, to edit-mesh
* blocks of data. the CustomData's must not be compatible.
*
* \param use_default_init: initializes data which can't be copied,
* typically you'll want to use this if the BM_xxx create function
* is called with BM_CREATE_SKIP_CD flag
*/
void CustomData_to_bmesh_block(const struct CustomData *source,
struct CustomData *dest,
int src_index,
void **dest_block,
bool use_default_init);
void CustomData_from_bmesh_block(const struct CustomData *source,
struct CustomData *dest,
void *src_block,
int dest_index);

/**
* Query info over types.
Expand Down
131 changes: 13 additions & 118 deletions source/blender/blenkernel/intern/customdata.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3654,7 +3654,7 @@ void CustomData_bmesh_free_block_data(CustomData *data, void *block)
}
}

static void CustomData_bmesh_alloc_block(CustomData *data, void **block)
void CustomData_bmesh_alloc_block(CustomData *data, void **block)
{
if (*block) {
CustomData_bmesh_free_block(data, block);
Expand Down Expand Up @@ -3689,19 +3689,23 @@ void CustomData_bmesh_free_block_data_exclude_by_type(CustomData *data,
}
}

static void CustomData_bmesh_set_default_n(CustomData *data, void **block, const int n)
void CustomData_data_set_default_value(const int type, void *elem)
{
int offset = data->layers[n].offset;
const LayerTypeInfo *typeInfo = layerType_getInfo(data->layers[n].type);

const LayerTypeInfo *typeInfo = layerType_getInfo(type);
if (typeInfo->set_default_value) {
typeInfo->set_default_value(POINTER_OFFSET(*block, offset), 1);
typeInfo->set_default_value(elem, 1);
}
else {
memset(POINTER_OFFSET(*block, offset), 0, typeInfo->size);
memset(elem, 0, typeInfo->size);
}
}

static void CustomData_bmesh_set_default_n(CustomData *data, void **block, const int n)
{
const int offset = data->layers[n].offset;
CustomData_data_set_default_value(data->layers[n].type, POINTER_OFFSET(*block, offset));
}

void CustomData_bmesh_set_default(CustomData *data, void **block)
{
if (*block == nullptr) {
Expand Down Expand Up @@ -3891,8 +3895,8 @@ void CustomData_data_copy_value(int type, const void *source, void *dest)
return;
}

if (typeInfo->copyvalue) {
typeInfo->copyvalue(source, dest, CDT_MIX_NOMIX, 0.0f);
if (typeInfo->copy) {
typeInfo->copy(source, dest, 1);
}
else {
memcpy(dest, source, typeInfo->size);
Expand Down Expand Up @@ -4067,115 +4071,6 @@ void CustomData_bmesh_interp(CustomData *data,
}
}

void CustomData_to_bmesh_block(const CustomData *source,
CustomData *dest,
int src_index,
void **dest_block,
bool use_default_init)
{
if (*dest_block == nullptr) {
CustomData_bmesh_alloc_block(dest, dest_block);
}

/* copies a layer at a time */
int dest_i = 0;
for (int src_i = 0; src_i < source->totlayer; src_i++) {

/* find the first dest layer with type >= the source type
* (this should work because layers are ordered by type)
*/
while (dest_i < dest->totlayer && dest->layers[dest_i].type < source->layers[src_i].type) {
if (use_default_init) {
CustomData_bmesh_set_default_n(dest, dest_block, dest_i);
}
dest_i++;
}

/* if there are no more dest layers, we're done */
if (dest_i >= dest->totlayer) {
break;
}

/* if we found a matching layer, copy the data */
if (dest->layers[dest_i].type == source->layers[src_i].type) {
int offset = dest->layers[dest_i].offset;
const void *src_data = source->layers[src_i].data;
void *dest_data = POINTER_OFFSET(*dest_block, offset);

const LayerTypeInfo *typeInfo = layerType_getInfo(dest->layers[dest_i].type);
const size_t src_offset = size_t(src_index) * typeInfo->size;

if (typeInfo->copy) {
typeInfo->copy(POINTER_OFFSET(src_data, src_offset), dest_data, 1);
}
else {
memcpy(dest_data, POINTER_OFFSET(src_data, src_offset), typeInfo->size);
}

/* if there are multiple source & dest layers of the same type,
* we don't want to copy all source layers to the same dest, so
* increment dest_i
*/
dest_i++;
}
}

if (use_default_init) {
while (dest_i < dest->totlayer) {
CustomData_bmesh_set_default_n(dest, dest_block, dest_i);
dest_i++;
}
}
}

void CustomData_from_bmesh_block(const CustomData *source,
CustomData *dest,
void *src_block,
int dest_index)
{
/* copies a layer at a time */
int dest_i = 0;
for (int src_i = 0; src_i < source->totlayer; src_i++) {
if (source->layers[src_i].flag & CD_FLAG_NOCOPY) {
continue;
}

/* find the first dest layer with type >= the source type
* (this should work because layers are ordered by type)
*/
while (dest_i < dest->totlayer && dest->layers[dest_i].type < source->layers[src_i].type) {
dest_i++;
}

/* if there are no more dest layers, we're done */
if (dest_i >= dest->totlayer) {
return;
}

/* if we found a matching layer, copy the data */
if (dest->layers[dest_i].type == source->layers[src_i].type) {
const LayerTypeInfo *typeInfo = layerType_getInfo(dest->layers[dest_i].type);
int offset = source->layers[src_i].offset;
const void *src_data = POINTER_OFFSET(src_block, offset);
void *dst_data = POINTER_OFFSET(dest->layers[dest_i].data,
size_t(dest_index) * typeInfo->size);

if (typeInfo->copy) {
typeInfo->copy(src_data, dst_data, 1);
}
else {
memcpy(dst_data, src_data, typeInfo->size);
}

/* if there are multiple source & dest layers of the same type,
* we don't want to copy all source layers to the same dest, so
* increment dest_i
*/
dest_i++;
}
}
}

void CustomData_file_write_info(int type, const char **r_struct_name, int *r_struct_num)
{
const LayerTypeInfo *typeInfo = layerType_getInfo(type);
Expand Down

0 comments on commit dfacaf4

Please sign in to comment.