Add special UDIM handling when creating usdz

[ablev-dwa]

Description of Change(s)

Adds support for including UDIM texture files when creating usdz

[jtran56]

Filed as internal issue #USD-6614

[spiffmon]

This is now the third codesite where we do UDIM substitution, though I'm not too concerned about logic duplication here because this is the simplest (and only such) glob-style subst, and it's clearly needed! But the OCD coder in me asks if you could follow the pattern that hdSt and usdImaging do in declaring a const to avoid duplicating the string literal four times? E.g.

static const char UDIM_PATTERN[] = "<UDIM>";

[spiffmon]

...and, having said above I wasn't concerned about this simpler logic, I now realize that we need to do the same kind of search that e.g. _ResolveAssetAttribute does, because TfGlob() will only work, even on filesystems, for anchored or absolute paths. So we need to find one actual UDIM file before we can run TfGlob()

That's not a trivial amount of code, and I really don't advocate duplicating it. I'm not sure UsdUtils is the ideal place for introducing UDIM-support-logic, but it's not horrible, and it is at least accessible to UsdImaging. So I would advocate, for now, refactoring that logic out of materialParamUtils.cpp to a public helper here. If there are folks at Pixar who think we can find a better long-term home for it, we'll take on any further refactoring.

[spiffmon]

Ahh.... OK, I wasn't that familiar with this code, but I think I believe now that this substitution will happen before the Glob matching above happens, so we don't need to replicate the logic from usdImaging. A few requests, though?

1. For clarity, can you make a a teeny helper, IsUDIMIdentifier() and use it in both places you look for UDIMS?
2. Can you pull the code block below up here into a contiguous block, so the UDIM logic is less diffused, and you can move baseName inside the block?
3. Can you add a test? There's a pretty good pattern already in place that you can easily copy/add.

Thanks, @ablev-dwa !

[ablev-dwa]

Will do!

[lbiasco-dwa]

Hi @spiffmon, I'm the original author of these changes. Thanks for the review so far. For (2) I'm having some difficulty finding a way to combine those changes without duplicating the sandwiched resolve and fetch checks since baseName can't be included for those. Do you have any suggestions, or should I just duplicate them for the sake of keeping all UDIM logic to a block?

[spiffmon]

It seemed to me that only the Resolve() needed to be duplicated, though maybe I got that wrong. But yes, I do think that small amount of duplication is worth it to centralize the special logic. Thanks!

[lbiasco-dwa]

I'm not very knowledgeable about how resolvers could implement FetchToLocalResolvedPath since ours does the same automatic passthrough as the default, so I just assumed some could encounter issues with a <UDIM> token in the name. If you still think the FetchToLocalResolvedPath is fine after the resolved dir and UDIM basename have been recombined, I'll gladly do that.

[spiffmon]

Right... GitHub was hiding that from me. So, FetchToLocalResolvedPath() is going to be deprecated eventually with Ar 2.0, since it is entirely counter to a big part of the point of the new API's (to be able to work with non-filesystem-based assets). Also, the concept of "directory" is one we have explicitly refrained from defining in Ar, because not all datastores support it. For anything that needs to be fetched locally, your TfGlob is going to fail anyways.

How about you pull the FetchToLocalResolvedPath clause into an else clause of the if (IsUdimPath() block (with an additional check that the resolvedRefFilePath is not empty ), and leave the if (resolvedRefFilePath.empty()) check afterwards, to be encountered in both cases? Does that make sense?

[lbiasco-dwa]

I think your suggestion makes sense, or at least I can see a path forward now. Regardless, I'll get these changes out and we'll see if I'm on the right track! Thanks for all the info.

[lbiasco-dwa]

Sorry for the delay, I got wrapped up in other work. I attempted to make the requested changes, and I really should have guessed it, but @spiffmon's original suggestion (moving this code to common utility UDIM functions) turns out to be the likely best option. The reason why is that the current implementation mostly does well with the tests, but it specifically fails when having to compute dependencies of a usdz file that contains UDIM textures (because TfGlob of course won't work for usdz-internal data).

Anyway, even with the consolidation option there are some concerns, so I'd like to run them by this PR. First, regardless of the other concerns, the new implementation will mean that UsdImaging will need a new dependency on UsdUtils. I'm assuming this is okay, for now? (I'm also assuming the UDIM code in HdSt should remain untouched since that package getting a dependency on UsdUtils feel very ill-advised?)

The second concern is that the implementation of the UDIM code for UsdImaging is sort of unique. After it resolves a single UDIM tile, it attempts to resolve any symlinks and then reinsert the <UDIM> pattern to the path. This symlink resolution makes the code diverge a bit in a way that increases code complexity (it prevents a general UsdUtilsResolveUdimPath function that takes and returns a path with <UDIM> from being helpful), but more importantly it seems to me that the symlink resolution might break on some more zany setups (files being symlinked from separate locations, etc). Would it be okay to remove the symlink resolution part of this process?

I think there will be 2-3 new utility functions that come out of this implementation: UsdUtilsIsUdimPath, UsdUtilsResolveUdimPath (these two could potentially be combined), and UsdUtilsGetUdimFiles.

[spiffmon]

Thanks, @lbiasco-dwa ! Yes, totally fine with UsdImaging depending on UsdUtils - we'll leave hdSt and its UDIM code alone.

Unfortunately we can't just remove the symlink resolving logic, because it actually makes a huge savings in GPU memory from texture deduplication, in our pipeline, at least. However, from my inspection, it seems very amenable to an optional FilterResolvedPath callback that UsdUtilsResolveUdimPath and UsdUtilsGetUdimFIles would accept, that has the same signature and use as _ResolveSymlinks , so that UsdImaging can supply it in its usage?

[spiffmon]

Hi @lbiasco-dwa ,
I'm unsure this account is still even active, given that you have changed studios, but I wanted to send a ping here, as we're running up against this ourselves, now, and if you had done most of the work for the solution we'd been discussing last year, seems a shame for it to go to waste.

Cheers,
--spiff

[lbiasco]

Sorry for not responding to your reply, @spiffmon! As noted, I switched studios so this flew under my radar until a related discussion came up on the ASWF Slack recently.

And sorry for the massive delay on this PR, which was due to a combination of changes at the studio around the time and my own inattentiveness. I took a look at my old stuff and cleaned it up a bit, and I do have something I can share! ...but related to my studio move, I no longer have access to @lbiasco-dwa and can't actually do anything to this PR.

So first, this PR should probably be closed. Then after I get a thumbs-up on the CLA I just submitted, I can make a new PR with my updates and link to this one for history sake.

[spiffmon]

Closing, as this PR is effectively defunct.