Details

Reviewers

Campbell Barton (campbellbarton)
Bastien Montagne (mont29)

Maniphest Tasks

T81060: Vertex Slide and Loop Cut and Slide tools breaks the UVs and Vertex Colors after topology changes [new or deleted] (if CustomSplitNormals are present)

Commits

rBaafd71a8a160: Fix T81060: CustomData Correction sometimes breaks UVs and Vertex Colors

Summary

In the tranform customdata correction code, the BM_loop_interp_from_face
function is called to interpolate customdata.

However, this function expects f_src (where customdatas are read) to
come from the same bmesh as l_dst.

This is seen in the CustomData_bmesh_interp where the same CustomData
decryptor (in this case, bm->ldata) is used for both blocks.

The problem is that f_src in this case comes from a bmesh with parameters
copied through BM_mesh_copy_init_customdata and this function skips
layers of customdata that are flagged with` CD_FLAG_NOCOPY` for example.

So the ldata of both is different.

The solution in this patch is to force the copy of the CustomData used.

Ref T81060

Diff Detail

Repository

rB Blender

Branch

master

Build Status

Buildable 10653
Build 10653: arc lint + arc unit

Event Timeline

Germano Cavalcante (mano-wii) created this revision.Oct 9 2020, 4:20 AM

Harbormaster completed remote builds in B10644: Diff 29765.Oct 9 2020, 4:20 AM

Germano Cavalcante (mano-wii) requested review of this revision.Oct 9 2020, 4:20 AM

The general approach seems fine.

This seems like something we might run into again, so this could be moved into a utility function.

BM_mesh_copy_init_customdata_all_layers(bm_dst, bm_src, BM_LOOP | BM_FACE, allocsize);

With an extra argument for the types of layers to initialize.

Also, I wouldn't worry about counting the number of items - as this is only a minor gain in the case of mempool. Passing in allocsize as NULL is fine too, it can use bm_mesh_allocsize_default.

This revision now requires changes to proceed.Oct 9 2020, 8:17 AM

Oleg (DotBow) awarded a token.Oct 9 2020, 10:05 AM

Create and use utility function BM_mesh_copy_init_customdata_all_layers
Optimization: Don't copy Faces customdata (they are not really being changed).
Rebase

Harbormaster completed remote builds in B10653: Diff 29786.Oct 9 2020, 3:11 PM

Assert the correct order of the elements

Harbormaster completed remote builds in B10654: Diff 29787.Oct 9 2020, 3:20 PM

Campbell Barton (campbellbarton) accepted this revision.Oct 10 2020, 8:11 AM

Campbell Barton (campbellbarton) added inline comments.

source/blender/bmesh/intern/bmesh_construct.c
609	There should be an explanation why all layers are sometimes needed, instead of using `BM_mesh_copy_init_customdata`.

This revision is now accepted and ready to land.Oct 10 2020, 8:11 AM

Ankit Meel (ankitm) added a subscriber: Ankit Meel (ankitm).Oct 10 2020, 8:41 AM

Ankit Meel (ankitm) added inline comments.

source/blender/bmesh/intern/bmesh_construct.c
609	Other functions in that file `source/blender/bmesh/intern/bmesh_construct.h` might also need this change: `r_` prefix for return value arguments. https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Naming
630–632	Always initialize https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-always
633	`const int size`
636	Generates warnings: `Passing 'const CustomData ' (aka 'const struct CustomData ') to parameter of type 'struct CustomData *' discards qualifiers`
638	`Passing 'const CustomData ' (aka 'const struct CustomData ') to parameter of type 'struct CustomData *' discards qualifiers`

From review:

Explain the difference between BM_mesh_copy_init_customdata_all_layers and BM_mesh_copy_init_customdata
Fix warning about misuse of const qualifier
Always initialize an object (ES.20 of C++ Core Guidelines)
Use const where possible