PixarAnimationStudios / OpenUSD

Universal Scene Description
http://www.openusd.org
Other
5.99k stars 1.18k forks source link

UsdZipFileWriter does not use ArAsset / ArWritableAsset #2374

Open nvidia-jomiller opened 1 year ago

nvidia-jomiller commented 1 year ago

Description of Issue

An issue that we have been running into with the usdz file format is it always expects a file path(s) to read from and write to when creating the usdz package.

When dealing with an asset management backend such as an object store it becomes a burden to download all the paths that we plan to package to a temporary directory, write the usdz to a temporary file, then upload that to our object store. Now that we have Ar 2.0 where we have support for reading (ArAsset / OpenAsset) and writing (ArWritableAsset / OpenAssetForWrite) it makes sense that UsdZipFileWriter should use that API over Arch File System calls. This allows for the underlying ArResolver to deal with reading and writing the usdz content to it's backend.

I've submitted https://github.com/PixarAnimationStudios/USD/pull/2356 which updates UsdZipFileWriter to use OpenAssetForWrite and OpenAsset.

System Information (OS, Hardware)

Package Versions

Build Flags

spiffmon commented 1 year ago

Is this really a sufficient issue to address the object-store issue, @nvidia-jomiller? Ar intentionally does not get into the business of directory or asset organization, but that's something that is important for creating the proper asset identifiers to author into the assets going into the package-being-created. If the assets are not being "localized" into a staging ground, how do we address that issue?

nvidia-jomiller commented 1 year ago

Hey @spiffmon, are you referring to the structure that is created through the CreateUsdzPackage in usdzUtil.py? Where the script is iterating the directories and adding files to the package?

If so, yeah, I don't think that script would work out-of-the-box for something like an object store. But it would require creating a package and using the backing asset management systems organization to properly structure the usdz. For instance, if we have the following assets that we want to package up into a usdz:

schemea://root/foo/a1.usd schemea://root/foo/a2.usd schemea://root/bar/b1.usd schemea://root/bar/sub/c1.usd

And write the usdz to schemea://root/asset.usdz could be done with something like:

with Usd.ZipFileWriter.CreateNew("schemea://root/asset.usdz") as usdzWriter:
    usdzWriter.AddFile("schemea://root/foo/a1.usd", "foo/a1.usd")
    usdzWriter.AddFile("schemea://root/foo/a2.usd", "foo/a2.usd")
    usdzWriter.AddFile("schemea://root/bar/b1.usd", "bar/b1.usd")
    usdzWriter.AddFile("schemea://root/bar/sub/c1.usd", "bar/sub/c1.usd")

Internally, UsdZipFileWriter will use ArGetResolver().OpenAsset("schemea://root/foo/a1.usd") to read the asset and structure it within the package to "foo/a1.usd". UsdZipFileWriter would be using ArGetResolver().OpenAssetForWrite("schemea://root/asset.usdz") and writing to that asset.

The difference would be that usdzUtils.py could not be used since it relies on the file / folder structure to create the usdz. It would require a custom script that works with the asset management system's organization. But the important part is that UsdZipFileWriter is using the reading and writing APIs from Ar so it can read / write to something like an object store.

There is definitely something I might be missing but it seems like UsdZipFileWriter should be using Ar to read / write content

spiffmon commented 1 year ago

I do always forget about the low-level API's flexibility, but it still feels like you're giving someone a rug that might get yanked out from under them unpredictably, because if any of the "files" in your example contain references to any other files -- including each other -- using the object-store URI, those URI's will persist in the created package.

I'll concede though, that what's being proposed is a necessary precondition to providing full support. Just not sure if we want to deploy it by itself? (And I really am asking the question, not trying to render a verdict)

nvidia-jomiller commented 1 year ago

Sorry for the delayed response but I wanted to confirm a couple things before responding as I'm not as familiar with the usdzUtils.py / UsdUtilsCreateNewUsdzPackage utilities as I am with Ar. But I did open https://github.com/PixarAnimationStudios/USD/pull/2356 which is the minimal set of changes needed to support something like this in UsdZipFileWriter. I don't think it's comprehensive in regards to being completely detached from the filesystem but I do think it covers the necessary changes for UsdZipFileWriter to work with Ar 2.0 API. I think the biggest point I have here is that when dealing with assets Usd should be using the Ar 2.0 interface of ArAsset and ArWritableAsset instead of calls to Arch / Tf.

if any of the "files" in your example contain references to any other files -- including each other -- using the object-store URI, those URI's will persist in the created package.

This is a great point but, IMHO, I don't think it should limit how assets can be packaged into a package file format such as Usdz. From what I can tell, a lot of the internals of usdUtils are handling assets properly (by going through the Ar APIs) but there are a few places like _AssetLocalizer that always assume asset paths resolve to a normal file path on disk which is not always the case with Ar 2.0. The amazing thing about Ar 2.0 is it's ability to decouple USD from the filesystem, but what I think is important is that USD uses that API when appropriate. If something like usdz needs to use the file system that's totally understandable but it should respect the fact that not every asset resolves to a file on disk and should use things like ArFilesystemAsset / ArFilesystemWritableAsset. Perhaps in this scenario, it also requires _AssetLocalizer to use the reading and writing APIs from Ar to ensure that assets are localized to disk before creating the usdz package.

I'll concede though, that what's being proposed is a necessary precondition to providing full support. Just not sure if we want to deploy it by itself? (And I really am asking the question, not trying to render a verdict)

Good question and maybe we don't want to deploy it by itself. Maybe it's a combination of UsdZipFileWriter and _AssetLocalizer / dependencies.cpp which might require updates. I think what's difficult is hitting different parts of USD that work great with the underlying ArResolver implementation but others that forego Ar and make direct calls to TfAbsPath / TfNormPath / ArchOpenFile etc. But from a cursory glance it seems reasonable that UsdUtilsCreateNewUsdzPackage could handle converting asset paths such as URIs to their relative form when building the UsdZipFileWriter

sunyab commented 1 year ago

Filed as internal issue #USD-8199