Details
Diff Detail
- Repository
- rB Blender
Event Timeline
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 | |
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 | |
| source/blender/blenkernel/intern/object_deform.c | ||
|---|---|---|
| 462 | Best do an early exit, if either objects have no groups (BLI_listbase_is_empty) | |
| source/blender/blenkernel/intern/object_deform.c | ||
|---|---|---|
| 509 | Is MEM_reallocN_id the right function for reallocation here? | |
| source/blender/blenkernel/intern/object_deform.c | ||
|---|---|---|
| 509 | Just use MEM_reallocN | |
@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:
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 | |
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: (I just checked, apparently the Boolean Modifier exhibits the same behavior, maybe some others too.) 2: | |
@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.