Details

Reviewers

Maniphest Tasks

T90535: USDPreviewSurface materials in Animal Logic's ALab are missing textures

Summary

Added support for importing UDIM textures.
Added Material Name Collision USD import menu option, to specify the behavior when USD materials in different namespaces have the same name.
If no materials with generic purpose (allPurpose) are assigned to the USD primitive, the importer will now fall back on converting assigned materials with purpose preview.
Minor cleanup in the assign_materials() utility function used by USDMeshReader, removing an unnecessary variable and fixing comments.

The Material Name Collision menu options are

Modify: Create a unique name for the imported material.
Skip: Keep the existing material and discard the imported material.

Previously, the default behavior was Skip. 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.

One of the design challenges in implementing the Material Name Collision 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. Instead, I decided to add a map to the ImportSettings struct that the USDMeshReader classes can update to record and share this information. This works, but I'm open to suggestions for improving this design.

These changes are committed to the temp-T90535-usd-alab-material-import branch. A build of this branch is available for testing on the experimental builds page.

Repro steps for testing:

Download and extract the "USD ALab scene" from animallogic.com/usd-alab.

In Blender:

File > Import > Universal Scene Description
Experimental > Import USD Preview
Import entry.usda

It might be helpful to toggle off Include > Proxy to visualize the render geometry more clearly.

Diff Detail

Repository

rB Blender

Branch

temp-T90535-usd-alab-material-import (branched from master)

Build Status

Buildable 19596
Build 19596: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Michael Kowalski (makowalski) edited the summary of this revision. (Show Details)Nov 20 2021, 4:36 PM

kursad k (kursadk) awarded a token.Nov 21 2021, 2:42 AM

Jesse Yurkovich (deadpin) mentioned this in T92696: UDIM: Support virtual filenames for tile sets.Dec 1 2021, 2:23 AM

Evan Wilson (EAW) added a subscriber: Evan Wilson (EAW).Dec 2 2021, 7:58 PM

Evan Wilson (EAW) added inline comments.

source/blender/io/usd/intern/usd_reader_material.cc
158

Thank you for commenting your code so well! Just a couple of spelling mistakes I noticed.

source/blender/io/usd/intern/usd_reader_material.cc
132

Jesse Yurkovich (deadpin) mentioned this in D13057: UDIM: Support virtual filenames.Dec 14 2021, 1:05 AM

USD import: fixed material reader comments.

Harbormaster completed remote builds in B19596: Diff 46323.Dec 22 2021, 11:44 PM

@Evan Wilson (EAW) thank you for catching the mistakes! These should be fixed now.

Hey Michael, some good news/bad news/good news.

The good news is I've made progress getting blender to support UDIM virtual filepaths directly. This will greatly simplify both your reader/writer patches and make it less duplicative.
The bad news is I'm unsure if I can get final review for it for 3.1. It may not make it but I don't know yet (1 of 2 reviewers signed off)
The last bit of good news is that I've already adapted your reader/writer code anyhow. So whoever "wins" the race to checkin it shouldn't matter much. If mine gets in first, I will send you the small diffs to update your patch. If you win, I'll still be ready to go.

Is it your intention to get these USD patches in for 3.1?

General code review on the UDIM loading portion below:

I think a lot of Sybren's feedback from the Writer patch also applies here. Style wise, using modern c++ concepts, etc.
Some inline comments below (with the caveat that many of these locations will be changed/removed once my patch is checked in)

source/blender/io/usd/intern/usd_reader_material.cc
137	`r_tile_indices` should just be a reference here instead of a pointer. This implies the vector could be null which isn't a useful distinction for what this function actually does with it.
235–237	for (int tile_number : indices) { BKE_image_add_tile(image, tile_number, nullptr); }
767	The return value for `get_udim_tiles` is ignored. Besides that, it's a bit odd to pass in `file_path` to both the 1st and 2nd parameters here, where one is const and the other is secretly modified later. This also means that the next statement that checks for `file_path.empty()` probably won't work as you'd expect since file_path is never set to empty explicitly anywhere in the case of not being able to resolve the image asset.

A lot more local variables & parameters can become const (style guide).

It is preferred to use Blender's container types, instead of STL. Unless there is a good reason not to, replace std::vector with blender::Vector etc.

source/blender/editors/io/io_usd.c
85–86	"Modify" and "Skip" look a bit weird to me. "Discard" implies that the material was loaded and then thrown away, but "skip" means it wasn't loaded to begin with, so it's a bit fuzzy right now. "Modify" indeed modifies the name, but not the material itself. My proposal (feel free to change to your taste): Modify → Unique Name Skip → Reference Existing, with description "If a material with the same name already exists, reference that instead of importing"
source/blender/io/usd/intern/usd_reader_material.cc
116	Why is `Attribute` capitalised?
128	This comment could get more structure if written in Doxygen style (so with `\param`, `\return`, etc.)
137	I think it would be more preferable to return `std::optional<std::pair<std::string, blender::Vector<int>>>` instead, unless there is a good reason to append to an already-existing vector.
139–145	This may be too defensive. If there is no file path, why is this function called anyway? Missing pointers should not be checked like this -- if the caller can't even provide the right parameters, there is no guarantee that it'll check the return value. Just use `BLI_assert_msg()` for this, or just let Blender crash so that an error is actually noticed. Update: case-in-point: the actual call to this function in this patch already ignores the return value.
147–153	Same here: what's the reason to even call this function when the file path doesn't indicate UDIM usage?
163	typo
170–171	Too defensive, there is no need to set these. The array contents will be overwritten without considering the current content anyway. Looking further at the code, `base_head` and `base_tail` aren't even used, so they can simply be `NULL` parameters.
174–177	What guarantees that this 1001 comes from the `base_udim_path.replace(udim_token_offset, 6, "1001");` call and is not simply the first index in use?
194–196	Same as above: too defensive.
197	This shadows another variable.
217	There is no need to sort; the documentation fo this function makes no promise as to the order of the tile indices. If this is important, document it.
229–231	Too defensive. This function won't even be called when `indices` is empty or `image` is `NULL`.
767	This also shows that the `get_udim_tiles` function is doing too much. It is called without even knowing whether UDIM tiles are being referenced, and because of that the discrete reasons it'll return `false` are so diverse the return value basically becomes meaningless.
source/blender/io/usd/intern/usd_reader_mesh.cc
108–137	This function is getting too complex. Having such nested conditions with assignments to some variable and later a check on that variable is a sign that the code should be split up into various parts. That particular variable can then become the return value, and it's much easier to write simpler code.
720	Not for this patch, but this condition is unnecessary. A for-loop will happily skip iterating over an empty collection.
722–730	This could be its own function. `subset_api` is only used in these few lines, and it'll nicely abstract away the preview material fallback.
760–767	This seems to be a duplicate of that code that I pointed at earlier could be in its own function, so that function could be used here as well.
source/blender/io/usd/intern/usd_reader_prim.h
57	If it's not updated by readers during stage traversal, what else updates it? And why is that not documented?
source/blender/io/usd/usd.h
35–36	This should be documented.

This revision now requires changes to proceed.Dec 28 2021, 11:47 AM

@Jesse Yurkovich (deadpin) Hi Jesse.

My apologies for the delayed reply. Thanks for the updates and for your review comments! Sounds like you made very nice progress with the UDIM work, and I truly appreciate you taking care of the related updates to the USD IO.

My original hope was to get this patch in for 3.1 (because users have been asking for these fixes), but I realize this depends on other priorities and that the review schedule is very tight at this point.

Updates based on latest review comments.

Harbormaster completed remote builds in B19671: Diff 46452.Dec 29 2021, 3:56 AM

Thanks for the review, @Jesse Yurkovich (deadpin) and @Sybren A. Stüvel (sybren)! I've submitted an update to address your comments.

@Michael Kowalski (makowalski) @Sybren A. Stüvel (sybren)
My patch has now also been accepted but I wanted to give you both the heads up first before I checkin. Here's what the update for both import/export would look like on your side: P2688 Let me know if you have any questions. I can wait until I get at least a verbal ack from either of you.

In D13297#362446, @Jesse Yurkovich (deadpin) wrote:

@Michael Kowalski (makowalski) @Sybren A. Stüvel (sybren)
My patch has now also been accepted but I wanted to give you both the heads up first before I checkin. Here's what the update for both import/export would look like on your side: P2688 Let me know if you have any questions. I can wait until I get at least a verbal ack from either of you.

@Jesse Yurkovich (deadpin) Thank you, Jesse. Your updates to the USD IO code look good (but I haven't tested them, of course). After you commit your patch, I can merge in your changes to the USD code and test fully.

Just so I understand, it looks like it's now valid to have the <UDIM> token in the im_file argument in the call

Image *image = BKE_image_load_exists(bmain_, im_file);

Is that right? (Before, I was replacing the token with an actual index.)

Also, it looks like in the USD material writer, you removed the code to explicitly add the <UDIM> token:

return (std::string(head) + "<UDIM>" + std::string(tail));

Is that because the image filepath is now guaranteed to have the <UDIM> token in it, if it's a tiled image?

But please don't let the answer to these questions hold up committing your patch. We can discuss this after you commit, as far as I'm concerned.

Thanks!

Yes, you're correct on both topics. Tiled image filepaths are expected to be tokenized now. Or said another way, tiled images need to use the virtual filepath; using a single, concrete, path will no longer work.

I've committed my patch a short while ago now. I'll respond to any questions you have as soon as I see them and am able.

Merged latest master and applied Jesse Yurkovich's patch to update
the UDIM tile loading code to conform to the new virtual filepath
specification requirements.

Harbormaster completed remote builds in B19792: Diff 46667.Jan 6 2022, 1:56 AM

Hi @Jesse Yurkovich (deadpin), I just applied your updates in the diff you provided and they appear to be working. Thank you for taking care of these changes!

Fixed warnings and macOS compile error.

Harbormaster completed remote builds in B19796: Diff 46671.Jan 6 2022, 3:18 AM

is this not ready for 3.1 too?

Michael Kowalski (makowalski) added inline comments.Jan 27 2022, 9:18 PM

source/blender/io/usd/intern/usd_reader_material.cc
128	I'm not sure, but I think this remark originally referred to the complex comment for `get_udim_tiles()`. Since this function has now been refactored to take a single parameter, the comment is much simpler. Please let me know if you'd still like me to change this to use the Doxygen style comments. (BTW, your point is well taken and I'll try to adopt the Doxygen style where appropriate for future submissions.)
137	The original comment was a good point, but is no longer relevant for the redesigned function, of course, which no longer has an `r_tile_indices` parameter.

Sybren A. Stüvel (sybren) added a task: T90535: USDPreviewSurface materials in Animal Logic's ALab are missing textures.Feb 18 2022, 2:53 PM

Is there a way to test this without having to download >15 GB of data?

... and this is at the Blender office with our 500 MBit/sec fibre connection. It takes 2 hours to just download the first 1 GB file, so the other 14.5 GB of textures is going to be a struggle.

In D13297#377504, @Sybren A. Stüvel (sybren) wrote:

Is there a way to test this without having to download >15 GB of data?

... and this is at the Blender office with our 500 MBit/sec fibre connection. It takes 2 hours to just download the first 1 GB file, so the other 14.5 GB of textures is going to be a struggle.

Hi @Sybren A. Stüvel (sybren). I've been testing with the low-res version here: https://animallogic.com/usd-alab/ (the first USD Alab Scene link, which is 1.0GB). It took only a couple of minutes for me to download on my 114 Mbps connection. That version has UDIM textures and I think was sufficient for testing. Is it this link that took 2 hours? If you are having issues downloading this for some reason, please let me know and I'll try to generate another test case for this. Thanks!

Roger B (rboxman) awarded a token.Mar 11 2022, 11:05 AM

Michael Kowalski (makowalski) mentioned this in T93904: USD import: investigate texture color space conversion issue.Mar 14 2022, 6:28 PM

This patch is introducing too many new things all at once. Support for importing UDIM should be its own patch. Material name collision handling as well. Falling back to another "material purpose" is yet another thing, and cleanups should also be separate from functional changes.

Even with this patch, there seem to be missing textures. At least, I think there are, as the house/shed itself isn't textured:

This revision now requires changes to proceed.Mar 22 2022, 12:41 PM

In D13297#385685, @Sybren A. Stüvel (sybren) wrote:

This patch is introducing too many new things all at once. Support for importing UDIM should be its own patch. Material name collision handling as well. Falling back to another "material purpose" is yet another thing, and cleanups should also be separate from functional changes.

Even with this patch, there seem to be missing textures. At least, I think there are, as the house/shed itself isn't textured:

Thanks for the remarks, @Sybren A. Stüvel (sybren). I'd like to address the last point first. I don't believe there are missing textures on the structure. This is proxy geometry that doesn't have materials assigned. You can see the same result when importing the USD in Houdini (viewed from the outside, in this case).

In D13297#385685, @Sybren A. Stüvel (sybren) wrote:

This patch is introducing too many new things all at once. Support for importing UDIM should be its own patch. Material name collision handling as well. Falling back to another "material purpose" is yet another thing, and cleanups should also be separate from functional changes.

Even with this patch, there seem to be missing textures. At least, I think there are, as the house/shed itself isn't textured:

Regarding splitting up the commits: Just to clarify, these changes were grouped together in one patch, as all three were required to load materials for the Alab scene, to fix the issues reported in the original task T90535. Without all three fixes in place, it's hard to tell whether any of them worked when testing with that particular test scene. If I were to split out the tasks, then I'll need to also create three new test cases that isolate the individual problems. Please let me know if you'd like me to do that.

I tried the patch on the latest master with the new USD libraries, and there are textures missing now.

About splitting up the patch, I can see your point. I still think it's better to separate some things out, such that for example UDIM support is added separately from material name collision handling.

In D13297#397653, @Sybren A. Stüvel (sybren) wrote:

I tried the patch on the latest master with the new USD libraries, and there are textures missing now.

About splitting up the patch, I can see your point. I still think it's better to separate some things out, such that for example UDIM support is added separately from material name collision handling.

Thanks for pointing out the textures issue, Sybren. I'll investigate this and will also provide a new patch for separating the changes.

The issue with textures not loading seems to have been caused by the change introduced in

https://developer.blender.org/rB98eb11156854e102a0405af1a64b2266eea22368

which added logic to BKE_image_get_tile_info()

https://developer.blender.org/diffusion/B/browse/master/source/blender/blenkernel/intern/image.cc$3148

to require that UDIM textures have at least two tiles. In the Alab scene, however, several models (for example, chemistry_beaker_02_geo) have UDIM textures with only one tile, and these are no longer recognized as UDIM textures and are failing to load.

To reproduce this, import

USD_ALab_0730\entity\chemistry_beaker02\chemistry_beaker02.usda

which fails to load the texture. This model is attempting to load textures from directory

USD_ALab_0730/fragment/look/surfacing/chemistry_beaker02/render_high/texture/look_surfacing_chemistry_beaker02_render_high_texture/

which only contains a single tile.

@Jesse Yurkovich (deadpin), are we sure that requiring at least two tiles for UDIM textures is the desired behavior? The Alab scene is widely used as an example for testing USD, and the textures load successfully in other DCCs (e.g. Houdini).

Hmm, ok, let me try getting this fixed up for you. This is a tricky one as we don't want files like frame-1042.png to be UDIM'd when the user attempts to load them normally. Tile sets do not have to start at 1001 either so the detection can't make the determination based on that.

Would an additional boolean parameter to BKE_image_get_tile_info to allow this case be ok?

If True, a tokenized filepath must be provided and the tile count can be 1 or more -- this signifies that the caller is aware of what they're doing (providing the tokenized path) and willing to handle any range of tiles returned
If False, any filepath could be provided but the tile count must be 2 or more -- this signifies that the caller is not aware of what the path is, it might be a UDIM or might not, be safe and search for at least 2

In D13297#399354, @Jesse Yurkovich (deadpin) wrote:

Hmm, ok, let me try getting this fixed up for you. This is a tricky one as we don't want files like frame-1042.png to be UDIM'd when the user attempts to load them normally. Tile sets do not have to start at 1001 either so the detection can't make the determination based on that.

Would an additional boolean parameter to BKE_image_get_tile_info to allow this case be ok?

If True, a tokenized filepath must be provided and the tile count can be 1 or more -- this signifies that the caller is aware of what they're doing (providing the tokenized path) and willing to handle any range of tiles returned

If False, any filepath could be provided but the tile count must be 2 or more -- this signifies that the caller is not aware of what the path is, it might be a UDIM or might not, be safe and search for at least 2

Thanks very much for the quick response, @Jesse Yurkovich (deadpin). Adding such a boolean parameter is certainly acceptable. I appreciate your help.

Kevin Collins (codeloadgame) added a subscriber: Kevin Collins (codeloadgame).May 5 2022, 12:09 AM

Jesse Yurkovich (deadpin) mentioned this in D14853: Re-fix T97366: Support single-file UDIMs if given a tokenized path.May 5 2022, 9:42 AM

In D13297#399354, @Jesse Yurkovich (deadpin) wrote:

Hmm, ok, let me try getting this fixed up for you. This is a tricky one as we don't want files like frame-1042.png to be UDIM'd when the user attempts to load them normally. Tile sets do not have to start at 1001 either so the detection can't make the determination based on that.

This is impossible to determine from the filename alone. Personally I would vastly prefer a solution that doesn't try to guess what users mean. After all, frame-1042.png also shouldn't be UDIM'd when frame-1043.png happens to exist.

Would an additional boolean parameter to BKE_image_get_tile_info to allow this case be ok?

If True, a tokenized filepath must be provided and the tile count can be 1 or more -- this signifies that the caller is aware of what they're doing (providing the tokenized path) and willing to handle any range of tiles returned

If False, any filepath could be provided but the tile count must be 2 or more -- this signifies that the caller is not aware of what the path is, it might be a UDIM or might not, be safe and search for at least 2

Unless I'm missing something (which is certainly possible), this keeps building on top of the false permise that "UDIM-ness" can be determined from the filename alone. Which other ways can we use to determine whether an artist wants to use UDIMs or not?

I'm also not a fan of boolean parameters, as they tend to lead to convoluted code in the function itself, as well as obscurity of the function call. Just create a function for each desired value of the boolean. This is especially important when that boolean parameter changes the semantics of the other parameters.

Michael Kowalski (makowalski) mentioned this in D14869: T90535: Handle material name collisions on USD import.May 6 2022, 5:39 AM

I split out the material name collision fix into patch D14869. I will likewise separate the UDIM import and material purpose fallback logic into separate commits.

Jesse Yurkovich (deadpin) mentioned this in rB86baf6e3edc8: Re-fix T97366: Support single-file UDIMs.May 28 2022, 7:15 AM

Michael Kowalski (makowalski) mentioned this in D15352: T90535: Handle material purpose on USD import.Jul 4 2022, 2:32 AM

I split out the material purpose fallback fix into patch D15352. This includes an additional improvement to handle purpose full as well as preview.

Michael Kowalski (makowalski) edited the summary of this revision. (Show Details)Jul 4 2022, 2:40 AM

Michael Kowalski (makowalski) mentioned this in D15379: T90535: import UDIM textures from USD.Jul 6 2022, 2:04 AM

I split out the UDIM texture import logic into patch D15379.

At this point, the changes originally in the present patch are now separated into individual patches, as requested by Sybren.

I'm closing this patch, as all the changes included in this revision have now been submitted in separate patches to master, as of this commit:

https://developer.blender.org/rB4655ddf3a2b7fee669f7d01f6f70d5f3711205c6

Fix USD Preview Surface import bugs reported in T90535 (missing materials in Animal Logic's ALab).
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 46323

source/blender/editors/io/io_usd.c

source/blender/io/usd/intern/usd_reader_material.cc

source/blender/io/usd/intern/usd_reader_mesh.cc

source/blender/io/usd/intern/usd_reader_prim.h

source/blender/io/usd/usd.h

Fix USD Preview Surface import bugs reported in T90535 (missing materials in Animal Logic's ALab).AbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 46323

source/blender/editors/io/io_usd.c

source/blender/io/usd/intern/usd_reader_material.cc

source/blender/io/usd/intern/usd_reader_mesh.cc

source/blender/io/usd/intern/usd_reader_prim.h

source/blender/io/usd/usd.h

Fix USD Preview Surface import bugs reported in T90535 (missing materials in Animal Logic's ALab).
AbandonedPublic
Actions

Revision Contents
Changeset List