Maniphest T93087

Vertex groups with identical names can exist, and when they do, a lot of things break.
Confirmed, NormalDESIGN
Actions

Assigned To
None
Authored By
Nathan Vasil (vasiln)
Nov 15 2021, 7:13 AM
Tags
  • BF Blender
  • Animation & Rigging
Subscribers
Bastien Montagne (mont29)
Hans Goudey (HooglyBoogly)
Nathan Vasil (vasiln)
Philipp Oeser (lichtwerk)

Description

Renaming a bone of an armature also renames the matching vertex group of linked objects/obdata, if found.

Currently there is no check made for potential collisions in vgroups names, which can lead to two or more vgroups having the exact same name, which is considered as invalid data.

This could be solved in several ways:

  1. Rename the existing/clashing vertex group (and make sure all usages elsewhere are updated along)
  2. Change the specified bone name to be unique in a way that it does not conflict with any existing vertex group name.
  3. Merge the (renamed) vertex group with the already existing one with that new name.
  4. Refuse to rename the bone in case it would conflict with another existing vgroup name.

Original Report

System Information
Operating system: Windows-10-10.0.18362-SP0 64 Bits
Graphics card: GeForce GTX 1070/PCIe/SSE2 NVIDIA Corporation 4.5.0 NVIDIA 456.71

Blender Version
Broken: version: 2.93.5, branch: master, commit date: 2021-10-05 12:04, hash: rBa791bdabd0b2
Broken: version: 2.83.0, branch: master, commit date: 2020-06-03 14:38, hash: rB211b6c29f771
Didn't test earlier than that (thought 2.79 is too different to matter); didn't test intervening versions; 2.83.0 is just the oldest version since 2.80 that got saved in my Blender folder.

Short description of error
Renaming bones in an armature renames vertex groups of child mesh objects without dealing with name collision. When that happens, a lot (most?) functions involving vertex group weights no longer work correctly-- it appears that they assume vertex group names are unique identifiers.

Exact steps for others to reproduce the error

  1. Make a monkey. Give it a vertex group. It's named "Group".
  2. Create an armature. Parent the monkey to the armature with automatic weights. Monkey now has VGs "Group" and "Bone".
  3. Enter edit on the armature. In properties/bone/name, change the name of Bone to "Group".

Monkey now has 2 VGs named "Group".

And, when they get created, stuff breaks. I consider the real bug here to be, "Vertex groups can acquire identical names", but one could also consider the bug to be, "Multiple operations assume VG names to be unique identifiers when they are not necessarily unique."

Here's a quick screenshot of a bugged mesh (not one that I made.) Notice the sidebar list of vertex groups: we've got three different duplicated vertex groups in that list, by my count.

What breaks when this happens? The first obvious thing, apparent from the screenshot, is interpolation of vertex groups from a subdivision modifier. We can see that the selected vertex group is no longer interpolating smoothly between verts.

What else breaks? A "normalize all" operation no longer actually normalizes all vertex groups properly. Post normalization, these don't add up to 1:

I'm sure that there are other operations that would stop working as well-- those are just the quickies that I noticed. If subdivision interpolation doesn't work right, I wouldn't expect poke faces interpolation or inset faces interpolation to work any better, for example. But I haven't tested all that.

I'd consider this a really low priority bug. The problem doesn't actually show up that often (I'd estimate five times in five years for me.) And it's reasonably easy to work around, provided you're willing to throw your weights away-- you just delete all groups and start again. If unwilling to delete all groups, you can probably get what you want with some math on the VGs (with a VG mix modifier or with geo nodes) and then delete only the problem groups.

Unfortunately, the one big problem with this bug is that it's not very apparent when it happens, which can lead to extra wasted work.

Related Objects

Event Timeline

Nathan Vasil (vasiln) created this task.Nov 15 2021, 7:13 AM
Nathan Vasil (vasiln) updated the task description.
Philipp Oeser (lichtwerk) changed the task status from Needs Triage to Confirmed.Nov 15 2021, 11:16 AM
Philipp Oeser (lichtwerk) added projects: Modeling, Core.
Philipp Oeser (lichtwerk) added subscribers: Hans Goudey (HooglyBoogly), Bastien Montagne (mont29), Philipp Oeser (lichtwerk).

Can confirm in that file (do think this should be looked at though).

Just at a very quick glance, I can see that BKE_defgroup_duplicate / BKE_defgroup_copy_list does not do BKE_object_defgroup_unique_name

Used here:

  • mesh_copy_data (indirectly used when doing BKE_id_copy on a mesh)
  • BKE_mesh_copy_parameters_for_eval
  • BKE_mesh_nomain_to_mesh

Sourrounding code suggests though that this is handled properly (e.g. BKE_defgroup_copy_list clears before (BLI_listbase_clear)

Maybe this rings a bell with @Bastien Montagne (mont29) or @Hans Goudey (HooglyBoogly) ?

Will confirm this for now (even though clear reproductions steps have not been found -- feel free to set this back to Needs Information from User if this is totally required)

Bastien Montagne (mont29) changed the task status from Confirmed to Needs Information from User.Nov 15 2021, 2:40 PM
Bastien Montagne (mont29) removed a project: Core.

We indeed need a reproducible case, this should not happen, but there is no real way to investigate this just from the broken result blend file alone.

Nathan Vasil (vasiln) added a comment.Nov 15 2021, 5:14 PM
This comment was removed by Nathan Vasil (vasiln).
Nathan Vasil (vasiln) added a comment.EditedNov 15 2021, 6:26 PM

Okay, I figured out a way to reproduce this:

  1. Create a monkey. Give it a vertex group (named "Group" is fine.)
  2. Create an armature. Parent your monkey to the armature with automatic weights. Monkey now has VGs "Group" and "Bone".
  3. Enter edit on the armature. Rename Bone to Group. (I did this by typing a new name in the name field in properties/bone/name.)

Monkey now has two VGs, each named "Group".

I've edited the task description to reflect this.

Nathan Vasil (vasiln) updated the task description.Nov 15 2021, 6:50 PM
Philipp Oeser (lichtwerk) changed the task status from Needs Information from User to Confirmed.Nov 15 2021, 6:58 PM
Philipp Oeser (lichtwerk) claimed this task.
Philipp Oeser (lichtwerk) added projects: Core, Animation & Rigging.

Oh boy, that was almost too easy to break... thx for the repro steps.
Cant believe this has not been reported more often (this goes back to 2.79 even).
Will check on a fix.

Bastien Montagne (mont29) removed a project: Core.Nov 16 2021, 8:55 AM

@Philipp Oeser (lichtwerk) please do not add Core to all and everything. This is strictly anim/mesh edit related, core has nothing to do here (this is not ID management issue, but internal data of some specific IDs).

Philipp Oeser (lichtwerk) added a comment.Nov 16 2021, 9:21 AM
In T93087#1254268, @Bastien Montagne (mont29) wrote:

@Philipp Oeser (lichtwerk) please do not add Core to all and everything. This is strictly anim/mesh edit related, core has nothing to do here (this is not ID management issue, but internal data of some specific IDs).

Noted (I was assuming duplicate names were touching Core, but right, this is not about IDs per se), appologies.

Philipp Oeser (lichtwerk) changed the subtype of this task from "Report" to "Design".Nov 16 2021, 2:44 PM

So while it would be easy to rename a existing/clashing vertex group:

P2598 T93087_snippet
1
2
3diff --git a/source/blender/editors/armature/armature_naming.c b/source/blender/editors/armature/armature_naming.c
4index de1c14a15ce..e3b583d53b6 100644
5--- a/source/blender/editors/armature/armature_naming.c
6+++ b/source/blender/editors/armature/armature_naming.c
7@@ -266,10 +266,15 @@ void ED_armature_bone_rename(Main *bmain,
8 }
9
10 if (BKE_modifiers_uses_armature(ob, arm) && BKE_object_supports_vertex_groups(ob)) {
11+ /* In case of a name clash (equally named vertex group already exists), we give the existing one a unique name. */
12+ bDeformGroup *dg_clash = BKE_object_defgroup_find_name(ob, newname);
13 bDeformGroup *dg = BKE_object_defgroup_find_name(ob, oldname);
14 if (dg) {
15 BLI_strncpy(dg->name, newname, MAXBONENAME);
16 }
17+ if (dg_clash) {
18+ BKE_object_defgroup_unique_name(dg_clash, ob);
19+ }
20 }
21
22 /* fix modifiers that might be using this name */

I am not sure if this is the right solution.
This existing/clashing vertex group could be in use elsewhere (e.g. in a modifier) and since this is all name based the other place would then break (modifier would all of a sudden use a different vertex group without the user being aware -- which is equally bad).

Seems there is not a real system in place that handles name changes of vertex groups (in the sense that all usages of that vertex group's name would be updated).
(Part of ED_armature_bone_rename could be extracted int a separate function that takes care of this.)
And even if such a system would be there, it is always questionable what to rename in case of clashes, see also T71244: Renaming an ID through RNA does not consistently increment OTHER or CURRENT ID in case of name collision.

So I think there are these possibilities:

  • rename the existing/clashing vertex group (but make sure all usages elsewhere are updated along)
  • change the specified bone name to be unique in a way that it does not conflict with a possible clashing vertex group name)
  • would be good if there is another solution, because both of the above seem awkward

So I think this needs a design decision on how we handle name clashes like these, will leave up to module devs to decide and step down to not block others looking at this.

Philipp Oeser (lichtwerk) removed Philipp Oeser (lichtwerk) as the assignee of this task.Nov 16 2021, 2:46 PM
Bastien Montagne (mont29) removed a project: Modeling.Nov 16 2021, 3:00 PM

Think this indeed needs a design work & decision first, probably for the Animation & Rigging module. Will update the task accordingly.

Bastien Montagne (mont29) updated the task description.Nov 16 2021, 3:09 PM

Personally I think I would go for solution #2, or #4 if it is not easily doable.

Sybren A. Stüvel (sybren) moved this task from Backlog to Design on the Animation & Rigging board.Nov 26 2021, 10:55 AM