T90535: Handle material name collisions on USD import
ClosedPublic
Actions

Authored by Michael Kowalski (makowalski) on May 6 2022, 5:35 AM.

Details

Reviewers

Sybren A. Stüvel (sybren)

Commits

rBf6268f921ab0: USD import: Handle material name collisions

Summary

This is a partial fix for T90535. This code was originally reviewed in D13297, but is now split into a separate patch, as suggested by Sybren in that review.

Added Material Name Collision USD import menu option, to specify the behavior when USD materials in different namespaces have the same name.

The Material Name Collision menu options are

Make Unique: Import each USD material as a unique Blender material.
Reference Existing: If a material with the same name already exists, reference that instead of importing.

Previously, the default behavior was to always keep the existing material. This was causing an issue in the ALab scene, where dozens of different USD materials all have the same name, usdpreviewsurface1, so that only one instance of these materials would be imported.

The issue can be reproduced with the attached file, materialNameCollisionTest.usda, which contains two USD Preview Surface materials,
/World/Materials/PreviewSurface and /World/Looks/PreviewSurface. These materials have different diffuse colors (green and red) and are each assigned to a mesh. Without the fix, both meshes are assigned the red material only.

materialNameCollisionTest.usda5 KBDownload

Diff Detail

Repository

rB Blender

Branch

temp-T90535-usd-material-collision (branched from master)

Build Status

Buildable 22010
Build 22010: arc lint + arc unit

Event Timeline

Michael Kowalski (makowalski) requested review of this revision.May 6 2022, 5:35 AM

Michael Kowalski (makowalski) created this revision.

Harbormaster completed remote builds in B21993: Diff 51214.May 6 2022, 5:35 AM

Michael Kowalski (makowalski) retitled this revision from USD Import: Handle material name collision. to T90535: Handle USD material import name collisions..May 6 2022, 5:39 AM

Michael Kowalski (makowalski) edited the summary of this revision. (Show Details)

Michael Kowalski (makowalski) added a project: USD.

Michael Kowalski (makowalski) edited the summary of this revision. (Show Details)May 6 2022, 5:57 AM

Michael Kowalski (makowalski) added a reviewer: Sybren A. Stüvel (sybren).

Michael Kowalski (makowalski) retitled this revision from T90535: Handle USD material import name collisions. to T90535: Handle material name collisions on USD import.May 6 2022, 5:59 AM

Michael Kowalski (makowalski) mentioned this in D13297: Fix USD Preview Surface import bugs reported in T90535 (missing materials in Animal Logic's ALab)..May 6 2022, 6:12 AM

LGTM, found a few tiny things, see the inline notes.

Unique Name: Create a unique name for the imported material.
Reference Existing: Keep the existing material and discard the imported material.

About the labels: "Reference Existing" seems clear to me, but I think "Unique Name" could be better. The name isn't exactly relevant, the fact that each USD material is loaded as unique Blender material is the important bit. Assigning a unique name is just the means to that end. What would you think of changing the label to "Make Unique", with tooltip "Import each USD material as unique Blender material"?

Now that I think of it, "discard the imported material" is also a technicality that only makes sense if you know the USD material has been loaded and subsequently discarded. How about something like "Match materials by name, only importing from USD when no material with that name exists"?

source/blender/io/usd/intern/usd_reader_mesh.cc
85	I think `mat_name` can be `const`
source/blender/io/usd/intern/usd_reader_prim.h
39	typo
41	Why is this field mutable? Might be worth a comment.

This revision now requires changes to proceed.May 6 2022, 11:17 AM

USD import: rename material collision option.
USD import: material collision code fixes.

Harbormaster completed remote builds in B22010: Diff 51257.May 7 2022, 1:11 AM

Thanks for the review, @Sybren A. Stüvel (sybren). I made the changes.

Regarding the mutable ImportSettings::usd_path_to_mat_name field: As I now explain in the comment, this field is mutable because a const reference to the ImportSettings struct is provided to the reader classes, since these settings typically shouldn't be otherwise modified.

As I noted in the description to the original patch, one of the design challenges in implementing this feature was keeping track of the Blender material that is created for a given USD material, to avoid duplicating materials that are shared. I considered doing this by tagging the Blender materials with a custom property specifying the source USD material path, but this seemed like an intrusive approach that could introduce other problems. Instead, I decided to add this map to the ImportSettings struct that the USDMeshReader classes can update to record and share this information. But please let me know if you prefer a different approach.

Finally, regarding the help string for the Reference Existing menu option: in your original review, you had suggested making that string "If a material with the same name already exists, reference that instead of importing", which is what it currently is in the code. (I forgot to update the text in the patch description.) I like your original suggestion, but please let me know if you still want me to change this.

Thanks, again.

Michael Kowalski (makowalski) edited the summary of this revision. (Show Details)May 7 2022, 1:29 AM

Michael Kowalski (makowalski) mentioned this in T97362: USD image assets paths are not resolved in a cross platform manner.Jun 8 2022, 4:31 PM

In D14869#400266, @Michael Kowalski (makowalski) wrote:

Regarding the mutable ImportSettings::usd_path_to_mat_name field: As I now explain in the comment, this field is mutable because a const reference to the ImportSettings struct is provided to the reader classes, since these settings typically shouldn't be otherwise modified.

I still feel that this doesn't explain things yet. To me it reads like "other fields should be const when the struct is const", which of course makes total sense, but that doesn't say anything about usd_path_to_mat_name itself. How about something like this: "This field is mutable because it is used to keep track of what the importer is doing. This is necessary even when all the other import settings are to remain const."

As I noted in the description to the original patch, one of the design challenges in implementing this feature was keeping track of the Blender material that is created for a given USD material, to avoid duplicating materials that are shared. I considered doing this by tagging the Blender materials with a custom property specifying the source USD material path, but this seemed like an intrusive approach that could introduce other problems. Instead, I decided to add this map to the ImportSettings struct that the USDMeshReader classes can update to record and share this information. But please let me know if you prefer a different approach.

I think using the ImportSettings is fine for this, although it does mix concerns a little bit. This is reflected in the use of mutable. IMO it's better than using IDProperties for this, as those are typically for users & add-ons. We could revisit this at some point when there is a design for a proper way to keep track of import-time metadata.

Finally, regarding the help string for the Reference Existing menu option: in your original review, you had suggested making that string "If a material with the same name already exists, reference that instead of importing", which is what it currently is in the code. (I forgot to update the text in the patch description.) I like your original suggestion, but please let me know if you still want me to change this.

👍

The code LGTM, just update the comment before committing :)

This revision is now accepted and ready to land.Jun 9 2022, 10:39 AM