Array modifier: Do not merge vertex groups with different names
AbandonedPublic

Authored by Philip Holzmann (Foaly) on Mar 2 2018, 11:30 PM.

Diff Detail

Repository
rB Blender

Generally OK, although this should be a reusable utility function, it can be added to BKE_object_deform.h call:

  • int *BKE_object_defgroup_index_map_create(Object *ob_src, Object *ob_dst, int *r_map_len)
  • void BKE_object_defgroup_index_map_apply(MDeformVert *dvert, int dvert_len)

Edit: there is code for this in join_mesh_single, think it would be good to make the utility functions and call from both places.

This code doesn't remove invalid indices but it should.

source/blender/modifiers/intern/MOD_array.c
356

It should be freed, since order isn't important, it can be swap with the last item, then realloc'd to a smaller array after remapping is done for this vertex.

424–429

Can use single call to: defgroup_name_index

Campbell Barton (campbellbarton) requested changes to this revision.Mar 3 2018, 10:01 AM
This revision now requires changes to proceed.Mar 3 2018, 10:01 AM

Vertex Group Merging is now two functions in BKE_object_deform.h.
Used by Array Modifier and join_mesh_single.

Generally looks good, I'd like to get feedback from @Bastien Montagne (mont29) on clamping.

Are people joining meshes, then adding vertex groups later that use the indices which were previously outside the range?

Wondering if this is a kind of hidden feature users will miss.

source/blender/blenkernel/intern/object_deform.c
483

map can be const, also code style re: * placement.

493

Can make this a single check by doing: (uint)def_nr < map_len.

498

Code style: spaces around operators.

source/blender/modifiers/intern/MOD_array.c
324

Convention for naming is dvert

This revision is now accepted and ready to land.Mar 3 2018, 12:16 PM
Campbell Barton (campbellbarton) requested changes to this revision.Mar 3 2018, 12:20 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/object_deform.c
462

Best do an early exit, if either objects have no groups (BLI_listbase_is_empty)

This revision now requires changes to proceed.Mar 3 2018, 12:20 PM
Philip Holzmann (Foaly) marked 5 inline comments as done.Mar 3 2018, 1:06 PM
source/blender/blenkernel/intern/object_deform.c
509

Is MEM_reallocN_id the right function for reallocation here?

Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/object_deform.c
509

Just use MEM_reallocN

This revision is now accepted and ready to land.Mar 3 2018, 1:32 PM

MEM_reallocN instead of MEM_reallocN_id.

Philip Holzmann (Foaly) marked 2 inline comments as done.Mar 3 2018, 1:42 PM
Bastien Montagne (mont29) requested changes to this revision.Mar 4 2018, 6:50 PM

@Campbell Barton (campbellbarton) I guess by clamping you mean the removal of 'invalid' vgroups? I think it’s fine to completely nuke them, I would really not like users relying on such a bad nasty dirty hack to do anything! But as pointed in comments below, I think there is something odd/inconsistent in proposed code regarding this, anyway…

source/blender/blenkernel/intern/object_deform.c
498–506

I have two questions here:

  1. How could def_nr be above map_len, since map_len is supposedly created from same mesh? So I assume this should only be for invalid -1 case?
  2. Which leads to, map is created from defgroup_name_index(), which returns -1 in case vgroup name is not found. So we might be adding new invalid vgroups entries to this vertex, while removing some others…

All this sounds a bit inconsistent to me? Or am I missing something here?

509

totweight may now be 0, shouldn’t we rather use MEM_SAFE_FREE in that case? Especially since, given how our guardedalloc works, we’d then alloc a non-zero memory with zero usable space (just control structs)…

source/blender/modifiers/intern/MOD_array.c
316

should be int *remap

This revision now requires changes to proceed.Mar 4 2018, 6:50 PM
Philip Holzmann (Foaly) marked an inline comment as done.Mar 4 2018, 7:49 PM

Demonstration File to show that def_nr can be out of bounds, created with Boolean Modifier "Union" in 2.79:


Add a vertex group to the object and go into edit mode. Click "Select" to see the vertices assigned to the group. The right cube will be selected.

source/blender/blenkernel/intern/object_deform.c
498–506

1:
def_nr is not guaranteed to be in bounds. I can (in the old code) create a mesh with def_nr greater than the number of vertex groups (exactly by merging two meshes with the Array Modifier). If that should not have happened at all (and no other code creates def_nrs out of bounds), the check can be removed of course.

(I just checked, apparently the Boolean Modifier exhibits the same behavior, maybe some others too.)

2:
Right, instead of mapping to -1 it should be removed.

@Philip Holzmann (Foaly), for now it might be simplest to use old behavior (don't throw away groups which are out of range).
Changes to behavior can backfire so it's sometimes best not to mix them into more general refactoring.

Realize I suggested to do this in the first place - and think it's still worth considering.
If this patch doesn't change behavior from mesh-join we can apply it immediately.

@Campbell Barton (campbellbarton) I'm not sure how to do that then.
If we want to keep vertex groups that are out of range, there are different possibilities:

  • Map all invalid indices to -1: That could result in a vertex having multiple MDeformWeights with the same def_nr. Also, I don't know if these weights can ever be accessed again.
  • Keep all invalid indices: Weights that do not have a mapping to a vertex group in the new mesh keep their def_nr. This means some vertex groups might be merged.
  • Keep invalid indices only if they are out of range: Weights that do not have a mapping and are >= totweight are kept. This should not cause problems. But it has to be decided what to do with the weights with valid indices but no mapping, so they don't merge. (Move above totweight or map to -1?)

I would argue that removing invalid weights is the better solution.

Concerning the mesh-join: This patch should already have the same behavior, if all def_nrs are in bounds.
And as far as I can tell, the old code resulted in out-of-bounds array access, if they are not not.

@Philip Holzmann (Foaly) - there is no need to remove them. Just keep the weights that are out of bounds, this is how booleans and object join already work.

As a separate patch we can check on clearing unused vertex groups.

BKE_object_defgroup_index_map_apply now removes all weights with invalid def_nr.
If totweight is zero, it will be freed.

Philip Holzmann (Foaly) marked an inline comment as done.Mar 5 2018, 4:38 PM