PixarAnimationStudios / OpenUSD

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

Bug in Ar Resolver with nested packages #3162

Open fhechtAdobe opened 1 month ago

fhechtAdobe commented 1 month ago

Description of Issue

There is a small bug in the _GetPackageResolver() function in pxr/usd/ar/resolver.cpp: https://github.com/PixarAnimationStudios/OpenUSD/blob/release/pxr/usd/ar/resolver.cpp#L1362

When resolving a doubly nested package path like outer.foo[inner.bar[file.baz]] it will identify the inner.bar package as being of package type foo. It's an off by one error in terms of nesting.

The top of the function looks like this today:

ArPackageResolver*
_GetPackageResolver(const std::string& packageRelativePath) const
{
    const std::string innermostPackage = 
        ArSplitPackageRelativePathInner(packageRelativePath).first;
    const std::string format = GetExtension(innermostPackage);
    ...
}

but should be looking more like this:

ArPackageResolver*
_GetPackageResolver(const std::string& resolvedPackagePath) const
{
    const auto [outer, inner] =
        ArSplitPackageRelativePathInner(resolvedPackagePath);
    // If the package path has an inner most package, we want its
    // extension, otherwise use the outer and only package.
    const std::string& innermostPackage = inner.empty() ? outer : inner;
    const std::string format = GetExtension(innermostPackage);
    ...
}

Note, there are only two locations in resolver.cpp that call this function: _OpenAsset() and _ResolveHelper() and from that context it is clear that the function is called with a resolved package path, hence the suggestion to also change the name of the argument to make that clearer.

In the example above, this function would be called with outer.foo[inner.bar] and would determine the extension and hence the package type based on outer.foo and not inner.bar.

Steps to Reproduce

This is hard to reproduce unless you have two different package formats. This was noticed in the context of the Adobe SBSAR file format plugin, which is also a package resolver: https://github.com/adobe/USD-Fileformat-plugins/tree/main/sbsar

In our testing we put a .sbsar file inside of a .usdz file and then end up with a texture path like so package.usdz[material.sbsar[diffuse.magicHash.sbsarimage] and this path did not resolve properly. After some extensive debugging we noticed that it tried to find diffuse.magicHash.sbsarimage via the USDZ package resolver, instead of the SBSAR package resolver.

Note, that this can't be reproduced by nesting a .usdz file inside of a .usdz file, since the resolver type is the same in both cases

System Information (OS, Hardware), Package Versions, Build Flags

All systems and versions should be affected by this.

Please let me know if additional information or context would be helpful.

Thanks, Florian

jesschimein commented 1 month ago

Filed as internal issue #USD-9844