Fix: Incorrect BMesh to Mesh attribute copying #104421

Hans Goudey · 2023-02-07T19:24:52+01:00

Hans Goudey commented

2023-02-07 19:24:52 +01:00

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

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

Hans Goudey added 4 commits 2023-02-07 19:24:55 +01:00

a5fd198c9f Cleanup: Simplify mesh and point cloud conversion

Since mesh positions are stored as a generic attribute,
the attribute doesn't need special handling here.

fbfe01d5ee Mesh: Simplify BMesh Mesh conversion attribute copying

ef83c0002d Merge branch 'main' into bmesh-mesh-conversion-faster-hot-loops

7911dba433 Fix bad merge

Hans Goudey requested review from Campbell Barton 2023-02-09 15:43:24 +01:00

Hans Goudey changed title from ~~Mesh: Simplify BMesh Mesh conversion attribute copying~~ to Fix: Incorrect BMesh to Mesh attribute copying

2023-02-09 15:44:26 +01:00

Hans Goudey added 1 commit 2023-02-10 02:33:57 +01:00

ef27f6eb47 Merge branch 'main' into bmesh-mesh-conversion-faster-hot-loops

Campbell Barton requested changes 2023-02-10 07:42:56 +01:00

Campbell Barton left a comment

Generally this looks fine, noticed an issue (noted inline).

I'd also really prefer not use get/copy as prefixes - see patch: 3d82ba7ad4 ... it includes some additional code-comments too.

Generally this looks fine, noticed an issue (noted inline). I'd also really prefer not use get/copy as prefixes - see patch: https://projects.blender.org/ideasman42/blender/commit/3d82ba7ad44c87f17f38b075fe4c62b635dbb353 ... it includes some additional code-comments too.

source/blender/bmesh/intern/bmesh_mesh_convert.cc

						
				@ -153,0 +177,4 @@

				            CustomData_get_layer_index_n(&bm_data, type, per_type_index[type]) :

				            CustomData_get_named_layer_index(&bm_data, type, bm_layer.name);

				    if (mesh_layer_index == -1) {

				      infos.append({});

Campbell Barton commented

2023-02-10 07:38:45 +01:00

Appending a zeroed MeshToBMeshLayerInfo doesn't seem correct, the BMesh data should still be initialized so even if there is no source layer (where MeshToBMeshLayerInfo::mesh_data is NULL), the CustomData_data_set_default_value needs the bmesh_offset to be set. From what I can tell the other members should be set (type, elem_size) just not the mesh_data.

If I'm misunderstanding the logic, an explanation of why a zeroed MeshToBMeshLayerInfo is needed should be included.

It would also be good to have doc-strings for the functions that generate *LayerInfo vectors noting that Vector<BMeshToMeshLayerInfo> isn't mesh_data.totlayer aligned, were Vector<MeshToBMeshLayerInfo> is bm_data.totlayer aligned, it's not obvious why conversion in one direction needs to account for all layers while conversion in the other direction doesn't.

Appending a zeroed MeshToBMeshLayerInfo doesn't seem correct, the BMesh data should still be initialized so even if there is no source layer (where `MeshToBMeshLayerInfo::mesh_data` is NULL), the `CustomData_data_set_default_value` needs the `bmesh_offset` to be set. From what I can tell the other members should be set (type, elem_size) just not the mesh_data. If I'm misunderstanding the logic, an explanation of why a zeroed `MeshToBMeshLayerInfo` is needed should be included. ---- It would also be good to have doc-strings for the functions that generate `*LayerInfo` vectors noting that `Vector<BMeshToMeshLayerInfo>` isn't `mesh_data.totlayer` aligned, were `Vector<MeshToBMeshLayerInfo>` is `bm_data.totlayer` aligned, it's not obvious why conversion in one direction needs to account for all layers while conversion in the other direction doesn't.

Hans Goudey commented

2023-02-10 18:59:23 +01:00

Good point, the logic was bad there, thanks.

I added more comments about why some layers are skipped. I kept BMToMesh instead of BMeshToMesh since it's shorter and keeps statements on a single line which looks so much better.

I also added comments to the copy info struct elements.

Good point, the logic was bad there, thanks. --- I added more comments about why some layers are skipped. I kept `BMToMesh` instead of `BMeshToMesh` since it's shorter and keeps statements on a single line which looks so much better. I also added comments to the copy info struct elements.

Hans Goudey commented

2023-02-10 19:05:59 +01:00

I kept BMToMesh instead of BMeshToMesh since it's shorter and keeps statements on a single line which looks so much better.

Oops, this wasn't necessary. I made that change too.

> I kept BMToMesh instead of BMeshToMesh since it's shorter and keeps statements on a single line which looks so much better. Oops, this wasn't necessary. I made that change too.

ideasman42 marked this conversation as resolved

Hans Goudey added 16 commits 2023-02-10 19:05:15 +01:00

a8d951abdd Docs: remove malformed patterns for RNA mapping

The generator now skips these with a warning, they will need to be
corrected in the user manual.

This caused tests/python/bl_rna_manual_reference.py to fail looking
up URL's.