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.
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.
Can use single call to: defgroup_name_index
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.
map can be const, also code style re: * placement.
Can make this a single check by doing: (uint)def_nr < map_len.
Code style: spaces around operators.
Convention for naming is dvert
@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…
I have two questions here:
All this sounds a bit inconsistent to me? Or am I missing something here?
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)…
should be int *remap
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.
(I just checked, apparently the Boolean Modifier exhibits the same behavior, maybe some others too.)
@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.