Fix: USD import: deform group name collision #111747

Open
Michael Kowalski wants to merge 7 commits from makowalski/blender:usd_deform_group_name_collision into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

This addresses a bug when importing USD skeleton data, where
deform groups are misnamed due to name collisions with existing
mesh attributes. The incorrectly named deform groups fail to
bind to the armature animation.

The issue is that the call to BKE_object_defgroup_add_name()
assigns a unique name to the deform group if an attribute
with the same name already exists in the imported mesh.

The solution implemented in this commit is to remove attributes
with colliding names before creating the deform groups.

To reproduce, import the attached arm_test.usda, which contains
skeletal joints Hand, Shoulder and Elbow as well as mesh
attributes with the same names. Note that the imported deform
groups are renamed Hand.001, Shoulder.001 and Elbow.001 and that
the mesh fails to bind to the armature animation.

This issue would be prone to happening on round trips
Blender -> USD -> Blender, because the Blender deform groups would
be exported as both mesh attributes and USD skeleton binding data.

This addresses a bug when importing USD skeleton data, where deform groups are misnamed due to name collisions with existing mesh attributes. The incorrectly named deform groups fail to bind to the armature animation. The issue is that the call to `BKE_object_defgroup_add_name()` assigns a unique name to the deform group if an attribute with the same name already exists in the imported mesh. The solution implemented in this commit is to remove attributes with colliding names before creating the deform groups. To reproduce, import the attached `arm_test.usda`, which contains skeletal joints `Hand`, `Shoulder` and `Elbow` as well as mesh attributes with the same names. Note that the imported deform groups are renamed `Hand.001`, `Shoulder.001` and `Elbow.001` and that the mesh fails to bind to the armature animation. This issue would be prone to happening on round trips Blender -> USD -> Blender, because the Blender deform groups would be exported as both mesh attributes and USD skeleton binding data.
Michael Kowalski added 1 commit 2023-08-31 16:48:12 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
ea9a66b0f6
Fix: USD import: deform group name collision.
This addresses a bug when importing USD skeleton data, where
deform groups are misnamed due to name collisions with existing
mesh attributes. The incorrectly named deform groups fail to
bind to the armature animation.

The issue is that the call to BKE_object_defgroup_add_name()
assigns a unique name to the deform group if an attribute
with the same name already exists in the imported mesh.

The solution implemented in this commit is to remove attributes
with colliding names before creating the deform groups.

To reproduce, import the attached arm_test.usda, which contains
skeletal joints Hand, Shoulder and Elbow as well as mesh
attributes with the same names.  Note that the imported deform
groups are renamed Hand.001, Shoulder.001 and Elbow.001 and that
the mesh fails to bind to the armature animation.

This issue would be prone to happening on round trips
Blender -> USD -> Blender, because the Blender deform groups would
be exported as both mesh attributes and USD skeleton binding data.
Michael Kowalski added this to the USD project 2023-08-31 16:49:41 +02:00
Michael Kowalski added the
Interest
USD
Interest
Pipeline, Assets & IO
labels 2023-08-31 16:50:01 +02:00
Michael Kowalski requested review from Hans Goudey 2023-08-31 16:52:31 +02:00
Author
Member

@blender-bot build

@blender-bot build
Member

The solution implemented in this commit is to remove attributes colliding names before creating the deform groups.

Hmm, I'm a bit skeptical of this. I'd usually try to be very careful removing data or replacing it, especially in something as automated as an importer/exporter. For example, this basically implies that vertex groups (USD skeleton binding data) is "more important" than attributes. That's a bit hard to justify IMO.

because the Blender deform groups would be exported as both mesh attributes and USD skeleton binding data.

Based on the description here, it sounds like this might be the more fundamental problem? Vertex groups are accessed with the attribute API, mainly for geometry nodes, but if they are already exported separately for USD, they probably shouldn't be exported as attributes too.

>The solution implemented in this commit is to remove attributes colliding names before creating the deform groups. Hmm, I'm a bit skeptical of this. I'd usually try to be very careful removing data or replacing it, especially in something as automated as an importer/exporter. For example, this basically implies that vertex groups (USD skeleton binding data) is "more important" than attributes. That's a bit hard to justify IMO. >because the Blender deform groups would be exported as both mesh attributes and USD skeleton binding data. Based on the description here, it sounds like this might be the more fundamental problem? Vertex groups are accessed with the attribute API, mainly for geometry nodes, but if they are already exported separately for USD, they probably shouldn't be exported as attributes too.
Author
Member

because the Blender deform groups would be exported as both mesh attributes and USD skeleton binding data.

Based on the description here, it sounds like this might be the more fundamental problem? Vertex groups are accessed with the attribute API, mainly for geometry nodes, but if they are already exported separately for USD, they probably shouldn't be exported as attributes too.

Thanks for the comment!

Yes, I see your point. When exporting to UsdSkel, I could add a test to skip exporting attributes that correspond to deform groups.

However, it still leaves open the possibility that a USD exported some other way would have mesh attributes that collide with bone names, and this would cause the same issue on import. I know such collisions are unlikely, but I feel we should handle them in some way.

I'm not sure the best way to deal with this. I think trying to rename the bones is not acceptable either.

I have a basic question. What's the consequence of allowing the deform groups to have the same name as existing attributes? Is allowing such name collisions acceptable in some cases? If so, can BKE_object_defgroup_add_name() be extended to optionally avoid renaming the group?

> >because the Blender deform groups would be exported as both mesh attributes and USD skeleton binding data. > > Based on the description here, it sounds like this might be the more fundamental problem? Vertex groups are accessed with the attribute API, mainly for geometry nodes, but if they are already exported separately for USD, they probably shouldn't be exported as attributes too. Thanks for the comment! Yes, I see your point. When exporting to UsdSkel, I could add a test to skip exporting attributes that correspond to deform groups. However, it still leaves open the possibility that a USD exported some other way would have mesh attributes that collide with bone names, and this would cause the same issue on import. I know such collisions are unlikely, but I feel we should handle them in some way. I'm not sure the best way to deal with this. I think trying to rename the bones is not acceptable either. I have a basic question. What's the consequence of allowing the deform groups to have the same name as existing attributes? Is allowing such name collisions acceptable in some cases? If so, can `BKE_object_defgroup_add_name()` be extended to optionally avoid renaming the group?
Member

In Blender the name conflicts are a fairly bad state (means things are hidden in geometry nodes, and leads to some other undefined behavior (which to choose?)) The conflicting names are only possible because we haven't gotten to unifying the storage of vertex groups and attributes, but that's planned.

I would suggest:

  • Modifying the exporter to not export both attributes and vertex groups redundantly
  • Keep this solution, but add a warning that some data was removed

Once vertex groups become attributes, not sure what will happen here. But I guess that's a problem for the future!

In Blender the name conflicts are a fairly bad state (means things are hidden in geometry nodes, and leads to some other undefined behavior (which to choose?)) The conflicting names are only possible because we haven't gotten to unifying the storage of vertex groups and attributes, but that's planned. I would suggest: - Modifying the exporter to not export both attributes and vertex groups redundantly - Keep this solution, but add a warning that some data was removed Once vertex groups become attributes, not sure what will happen here. But I guess that's a problem for the future!
Michael Kowalski added 2 commits 2023-09-02 23:15:03 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
2bbbbe4fc8
USD import: warn when deleting attributes.
Added warning when deleting attributes to avoid name collision
with deform groups.
Author
Member

@blender-bot build

@blender-bot build
Author
Member

I would suggest:

  • Modifying the exporter to not export both attributes and vertex groups redundantly
  • Keep this solution, but add a warning that some data was removed

That sounds good.

I added a warning when attributes are removed.

I also created pull request #111894 to not export attributes that correspond to deform groups.

> I would suggest: > - Modifying the exporter to not export both attributes and vertex groups redundantly > - Keep this solution, but add a warning that some data was removed That sounds good. I added a warning when attributes are removed. I also created pull request https://projects.blender.org/blender/blender/pulls/111894 to not export attributes that correspond to deform groups.
Hans Goudey changed title from Fix: USD import: deform group name collision. to Fix: USD import: deform group name collision 2023-09-05 04:31:58 +02:00
Hans Goudey requested changes 2023-09-05 04:39:29 +02:00
Hans Goudey left a comment
Member

Generally looks good, just left some inline comments.

Generally looks good, just left some inline comments.
@ -1005,2 +1006,4 @@
std::vector<bDeformGroup *> joint_def_grps(joints.size(), nullptr);
/* We must be careful about name collisions with existing attributes when creating
* deform groups, because in the call to BKE_object_defgroup_add_name() below
Member

Reference functions with a # prefix (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments)

Reference functions with a # prefix (https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments)
makowalski marked this conversation as resolved
@ -1008,2 +1016,4 @@
std::string joint_name = pxr::SdfPath(joints[idx]).GetName();
if (!BKE_object_defgroup_find_name(mesh_obj, joint_name.c_str())) {
/* If there is an attribute with the same name, remove it. */
bke::AttributeIDRef attr_id(joint_name);
Member

You can use if (attributes.remove(joint_name)), since it returns whether an attribute was deleted or not.

You can use `if (attributes.remove(joint_name))`, since it returns whether an attribute was deleted or not.
makowalski marked this conversation as resolved
@ -1009,1 +1017,4 @@
if (!BKE_object_defgroup_find_name(mesh_obj, joint_name.c_str())) {
/* If there is an attribute with the same name, remove it. */
bke::AttributeIDRef attr_id(joint_name);
if (attributes.contains(attr_id)) {
Member

You can rely on the implicit conversion to bke::AttributeIDRef, no need to create it on a separate line.

You can rely on the implicit conversion to `bke::AttributeIDRef`, no need to create it on a separate line.
makowalski marked this conversation as resolved
@ -1010,0 +1020,4 @@
if (attributes.contains(attr_id)) {
WM_reportf(
RPT_WARNING,
"%s: Removing attribute '%s' from mesh '%s', as it conflicts with a deform group "
Member

I think the message could be a bit more concise. Also not sure about adding the function name in there, that seems a bit "low level" for a warning message in the UI. How about something like this?

Attribute '%s' removed from mesh '%s` because of name conflict with vertex group

(vertex group is what "defgroup" is called in the UI, I guess the code was just never updated for a rename or something)

I think the message could be a bit more concise. Also not sure about adding the function name in there, that seems a bit "low level" for a warning message in the UI. How about something like this? ``` Attribute '%s' removed from mesh '%s` because of name conflict with vertex group ``` (vertex group is what "defgroup" is called in the UI, I guess the code was just never updated for a rename or something)
makowalski marked this conversation as resolved

Rather than removing the conflicting attribute, wouldn't it be possible to rename it? Am also fairly uncomfortable with forcefully removing data... even with a warning.

Once vertex groups become attributes, not sure what will happen here. But I guess that's a problem for the future!

Isn't this already the case? Since there is name conflict with attributes?

Rather than removing the conflicting attribute, wouldn't it be possible to rename it? Am also fairly uncomfortable with forcefully removing data... even with a warning. > Once vertex groups become attributes, not sure what will happen here. But I guess that's a problem for the future! Isn't this already the case? Since there is name conflict with attributes?
Member

Rather than removing the conflicting attribute, wouldn't it be possible to rename it?

Yeah, that would be possible, it raises the arbitrary question of what to rename it to though.

Isn't this already the case? Since there is name conflict with attributes?

Conceptually they are, but they aren't stored as attributes yet, they're stored as the separate CD_MDEFORMVERT type. The plan is to allow storing sparse attributes to unify the two systems internally.

>Rather than removing the conflicting attribute, wouldn't it be possible to rename it? Yeah, that would be possible, it raises the arbitrary question of what to rename it to though. >Isn't this already the case? Since there is name conflict with attributes? Conceptually they are, but they aren't stored as attributes yet, they're stored as the separate `CD_MDEFORMVERT` type. The plan is to allow storing sparse attributes to unify the two systems internally.
Author
Member

Rather than removing the conflicting attribute, wouldn't it be possible to rename it?

Yeah, that would be possible, it raises the arbitrary question of what to rename it to though.

I suppose I could append a numerical suffix to create a unique name for the attribute, rather than deleting it. Would that be preferable?

> >Rather than removing the conflicting attribute, wouldn't it be possible to rename it? > > Yeah, that would be possible, it raises the arbitrary question of what to rename it to though. I suppose I could append a numerical suffix to create a unique name for the attribute, rather than deleting it. Would that be preferable?
Member

That seems good to me. The warning could then give the old and new name of the attribute.

That seems good to me. The warning could then give the old and new name of the attribute.
Michael Kowalski added 2 commits 2023-09-12 04:54:09 +02:00
buildbot/vexp-code-patch-coordinator Build done. Details
9d75a9d9df
USD import: rename attributes on name collision.
Now renaming, rather than deleting, attributes that have the same
name as bone vertex groups.
Author
Member

I added a utility function to rename the attributes and I believe this addresses the requested changes.

I added a utility function to rename the attributes and I believe this addresses the requested changes.
Author
Member

@blender-bot build

@blender-bot build

Change seems fine to me now, at least logic of code sounds good.

however, there is an on-going issue with regard to that new 'do not allow same names for vertex groups and generic attributes' behavior, and it is likely to be reverted for 4.0... so not so sure whether that patch remain meaningful then?

Re. #112022, https://devtalk.blender.org/t/vertex-groups-generic-attributes-and-name-clashing/31073

Change seems fine to me now, at least logic of code sounds good. however, there is an on-going issue with regard to that new 'do not allow same names for vertex groups and generic attributes' behavior, and it is likely to be reverted for 4.0... so not so sure whether that patch remain meaningful then? Re. #112022, https://devtalk.blender.org/t/vertex-groups-generic-attributes-and-name-clashing/31073
Author
Member

however, there is an on-going issue with regard to that new 'do not allow same names for vertex groups and generic attributes' behavior, and it is likely to be reverted for 4.0... so not so sure whether that patch remain meaningful then?

Re. #112022, https://devtalk.blender.org/t/vertex-groups-generic-attributes-and-name-clashing/31073

Thanks for reviewing! That's a good point. Is it still better practice to prevent the name collision on import, to avoid potential issues, even if it's technically allowed? I defer to your judgement on this.

> however, there is an on-going issue with regard to that new 'do not allow same names for vertex groups and generic attributes' behavior, and it is likely to be reverted for 4.0... so not so sure whether that patch remain meaningful then? > > Re. #112022, https://devtalk.blender.org/t/vertex-groups-generic-attributes-and-name-clashing/31073 Thanks for reviewing! That's a good point. Is it still better practice to prevent the name collision on import, to avoid potential issues, even if it's technically allowed? I defer to your judgement on this.
Hans Goudey approved these changes 2023-09-12 23:07:38 +02:00
@ -327,6 +328,29 @@ void import_skeleton_curves(Main *bmain,
std::for_each(scale_curves.begin(), scale_curves.end(), recalc_handles);
}
/* If the given mesh has an attribute with the given name, rename the
Member

Use doxygen syntax:

/**
 * Text here...
 */
Use doxygen syntax: ``` /** * Text here... */
makowalski marked this conversation as resolved
Member

Is it still better practice to prevent the name collision on import, to avoid potential issues, even if it's technically allowed?

IMO: Yes, absolutely. That's also proposed by the latest item in that devtalk thread.

> Is it still better practice to prevent the name collision on import, to avoid potential issues, even if it's technically allowed? IMO: Yes, absolutely. That's also proposed by the latest item in that devtalk thread.
Michael Kowalski added 2 commits 2023-09-13 05:56:57 +02:00
Author
Member

@mont29 Are you okay with this change getting merged?

@mont29 Are you okay with this change getting merged?

I would rather wait for a final decision for vgroup topic first. Renaming randomly another generic attribute is not trivial, this can have completely unexpected consequences too (if other tools/data are referring specific attributes by names... afaik renaming attributes has no way to update these usages at all?).

I would rather wait for a final decision for vgroup topic first. Renaming randomly another generic attribute is _not_ trivial, this can have completely unexpected consequences too (if other tools/data are referring specific attributes by names... afaik renaming attributes has no way to update these usages at all?).
Author
Member

I would rather wait for a final decision for vgroup topic first. Renaming randomly another generic attribute is not trivial, this can have completely unexpected consequences too (if other tools/data are referring specific attributes by names... afaik renaming attributes has no way to update these usages at all?).

That makes sense. Thanks for the reviews @mont29 and @HooglyBoogly!

> I would rather wait for a final decision for vgroup topic first. Renaming randomly another generic attribute is _not_ trivial, this can have completely unexpected consequences too (if other tools/data are referring specific attributes by names... afaik renaming attributes has no way to update these usages at all?). > That makes sense. Thanks for the reviews @mont29 and @HooglyBoogly!
This pull request has changes conflicting with the target branch.
  • source/blender/io/usd/intern/usd_skel_convert.cc

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u usd_deform_group_name_collision:makowalski-usd_deform_group_name_collision
git checkout makowalski-usd_deform_group_name_collision
Sign in to join this conversation.
No reviewers
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#111747
No description provided.