Add BKE_object_defgroup_flip_map_unlocked which excludes locked groups from the flip-map.
Details
Details
- Reviewers
Daniel Salazar (zanqdo) Campbell Barton (campbellbarton) Sybren A. Stüvel (sybren) - Maniphest Tasks
- T96787: Edit mode normalize fails to respect locked groups
- Commits
- rB87a45db522e8: Edit mode normalize fails to respect locked groups
rB43b1624eee17: Fix T96787: Edit mode normalize fails to respect locked groups
rBfb07bbb75122: License headers: use SPDX identifiers
rB274dc024f62c: Cleanup: format, trailing space
rB0c9749093b95: Cleanup: spelling in comments
Diff Detail
Diff Detail
- Repository
- rB Blender
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Okay, dumping my findings here.
Walking down the method call stack: ED_mesh_defvert_mirror_update_em -> editbmesh_get_x_mirror_vert -> mesh_defvert_mirror_update_internal
// object_vgroup.c
(inside mesh_defvert_mirror_update_internal) def_nr is the index of the vertex group (vgroup), and a value of -1 indicates that all vgroups should be mirrored. This doesn't account for locked groups & non deforming groups.
Since it calls out to BKE_object_defgroup_flip_map (a Blender Kernel method) to get the flipped map before syncing, it suggests to me that locked / non-deforming groups aren't being respected from a low level.
Comment Actions
Discussed how this might be resolved with Nate and our conclusion was to make a flip_map that excludes locked groups.
At least I think this approach is more general but needs to be tested - along with checking all uses of BKE_object_defgroup_flip_map to check which uses of BKE_object_defgroup_flip_map should include locked groups and which should not - something that should be documented as part of a patch that introduces this distinction.
| source/blender/blenkernel/BKE_deform.h | ||
|---|---|---|
| 61 | Prefer name BKE_object_defgroup_flip_map_unlocked (auto-completes more usefully). | |
| source/blender/blenkernel/intern/deform.c | ||
| 622–675 | This is a reasonable amount of code duplication, suggest to have a static function object_defgroup_unlocked_flip_map_ex(ob, flip_map_len, use_default, use_only_unlocked). Then there there should only need to be minor differences without having to duplicate logic. If including the unlocked logic in-line complicates things too much, it would also be fine to clear locked groups in a second loop when use_only_unlocked is set. Even though that incurs redundant calculation, this particular function I don't see it causing any differences users are going to notice. | |
Comment Actions
Changed the title to make this a smaller patch fixing only the locking groups and separated the non deforming groups issue to a new task T100187
| source/blender/blenkernel/BKE_deform.h | ||
|---|---|---|
| 61 | Was unsure of the name, I like this better. Thanks! | |
| source/blender/blenkernel/intern/deform.c | ||
| 622–675 | I 100% agree there is too much duplication of logic (was throwing this up for some people to test). I do like the idea of abstracting the core logic into its own method. I'll go ahead and do that. | |
Comment Actions
Minor request, otherwise LGTM.
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 581 | flip_map_len can be passed in as a return argument & calculated in this funciton. if (use_only_unlocked) {
/* count unlocked. */
}
else {
/* use `defbase_tot` */
}Avoids 2x calls to BLI_listbase_count for the common code-path too (while not a problem, it's not necessary either). | |
Comment Actions
Checking over the patch and there is infact incorrect logic.
flip_num = BKE_object_defgroup_name_index(ob, name_flip);
if (flip_num != -1) {
map[defgroup] = flip_num;
map[flip_num] = defgroup;
}flip_num uses the full range of defbase_tot.
I think the correct solution here is to keep the map the same size and just not map any groups which are locked. (keep the value -1 if either are locked).
An alternative could be to create a map between both values but this doesn't seem worthwhile.
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 583–594 | There is no need to count unlocked groups now as flip_map_len doesn't change anymore for unlocked groups. | |
| 589–590 | Assigning an allocation to an int should warn, e.g: source/blender/blenkernel/intern/deform.c: In function ‘object_defgroup_unlocked_flip_map_ex’:
source/blender/blenkernel/intern/deform.c:602:21: warning: initialization of ‘int’ from ‘void *’ makes integer from pointer without a cast [-Wint-conversion]
602 | int i, flip_num = MEM_mallocN(*flip_map_len * sizeof(int), __func__);
| ^~~~~~~~~~~ | |
Comment Actions
Te patch makes some unnecessary changes, besides that think it's fine.
| source/blender/blenkernel/intern/deform.c | ||
|---|---|---|
| 581 | Prefer to keep defbase_tot, accessing the return argument everywhere reads worse and makes the patch noisier. | |