From e670db4724f772a1c30c375e90dbbc56e8b9ee9e Mon Sep 17 00:00:00 2001 From: matthewcpp Date: Mon, 15 May 2023 22:06:33 -0700 Subject: [PATCH] Fix for Github #2074 This change fixes an issue with UsdUtils::ComputeAllDependencies in which the resoved Asset Path was being used to open layers as opposed to an AR identifier. This resulted in layer::GetIdentifier() returning an incorrect value (in this case the resolved path) Fixes #2074 (Internal change: 2276164) --- pxr/usd/usdUtils/CMakeLists.txt | 24 +++++ pxr/usd/usdUtils/dependencies.cpp | 30 +++--- pxr/usd/usdUtils/dependencies.h | 2 +- ...lsDependenciesCustomResolver_plugInfo.json | 19 ++++ ...UtilsDependenciesCustomResolver_plugin.cpp | 96 +++++++++++++++++++ .../testUsdUtilsDependenciesCustomResolver.py | 81 ++++++++++++++++ .../dependency.usda | 11 +++ .../main.usda | 14 +++ 8 files changed, 265 insertions(+), 12 deletions(-) create mode 100644 pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugInfo.json create mode 100644 pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugin.cpp create mode 100644 pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver.py create mode 100644 pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/dependency.usda create mode 100644 pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/main.usda diff --git a/pxr/usd/usdUtils/CMakeLists.txt b/pxr/usd/usdUtils/CMakeLists.txt index a8fc80de22..8a6902bcfa 100644 --- a/pxr/usd/usdUtils/CMakeLists.txt +++ b/pxr/usd/usdUtils/CMakeLists.txt @@ -73,6 +73,7 @@ pxr_test_scripts( testenv/testUsdUtilsConstantsGroup.py testenv/testUsdUtilsCreateNewUsdzPackage.py testenv/testUsdUtilsDependencies.py + testenv/testUsdUtilsDependenciesCustomResolver.py testenv/testUsdUtilsDependencyExtractor.py testenv/testUsdUtilsFlattenLayerStack.py testenv/testUsdUtilsIntrospection.py @@ -88,6 +89,18 @@ pxr_test_scripts( testenv/testUsdUtilsVarSelsSessionLayer.py ) +pxr_build_test_shared_lib(TestUsdUtilsDependenciesCustomResolver + CREATE_FRAMEWORK + INSTALL_PREFIX UsdUtilsPlugins + LIBRARIES + ar + sdf + tf + + CPPFILES + testenv/TestUsdUtilsDependenciesCustomResolver_plugin.cpp +) + pxr_build_test(testUsdUtilsCoalescingDiagnosticDelegateCpp LIBRARIES tf @@ -153,6 +166,11 @@ pxr_install_test_dir( DEST testUsdUtilsDependencies ) +pxr_install_test_dir( + SRC testenv/testUsdUtilsDependenciesCustomResolver + DEST testUsdUtilsDependenciesCustomResolver +) + pxr_install_test_dir( SRC testenv/testUsdUtilsDependencyExtractor DEST testUsdUtilsDependencyExtractor1 @@ -429,6 +447,12 @@ pxr_register_test(testUsdUtilsDependencies EXPECTED_RETURN_CODE 0 ) +pxr_register_test(testUsdUtilsDependenciesCustomResolver + PYTHON + COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdUtilsDependenciesCustomResolver" + EXPECTED_RETURN_CODE 0 +) + pxr_register_test(testUsdUtilsDependencyExtractor1 PYTHON COMMAND "${CMAKE_INSTALL_PREFIX}/tests/testUsdUtilsDependencyExtractor ascii.usda ascii-usda.txt" diff --git a/pxr/usd/usdUtils/dependencies.cpp b/pxr/usd/usdUtils/dependencies.cpp index 73c1cc4d87..85d1b176db 100644 --- a/pxr/usd/usdUtils/dependencies.cpp +++ b/pxr/usd/usdUtils/dependencies.cpp @@ -98,21 +98,24 @@ class _FileAnalyzer { const SdfLayerRefPtr &layer, const _DepType &depType)>; - // Opens the file at \p resolvedFilePath and analyzes its external - // dependencies. + // Attempts to open the file at \p referencePath and analyzes its external + // dependencies. Opening the layer using this non-resolved path ensures + // that layer metadata is correctly set. If the file cannot be opened then + // the analyzer simply retains the \p resolvedPath for later use. // // For each dependency that is detected, the provided (optional) callback // functions are invoked. // // \p processPathFunc is invoked first with the raw (un-remapped) path. // Then \p remapPathFunc is invoked. - _FileAnalyzer(const std::string &resolvedFilePath, + _FileAnalyzer(const std::string &referencePath, + const std::string &resolvedPath, _ReferenceTypesToInclude refTypesToInclude= _ReferenceTypesToInclude::All, bool enableMetadataFiltering = false, const RemapAssetPathFunc &remapPathFunc={}, const ProcessAssetPathFunc &processPathFunc={}) : - _filePath(resolvedFilePath), + _filePath(resolvedPath), _refTypesToInclude(refTypesToInclude), _metadataFilteringEnabled(enableMetadataFiltering), _remapPathFunc(remapPathFunc), @@ -121,15 +124,16 @@ class _FileAnalyzer { // If this file can be opened on a USD stage or referenced into a USD // stage via composition, then analyze the file, collect & update all // references. If not, return early. - if (!UsdStage::IsSupportedFile(_filePath)) { + if (!UsdStage::IsSupportedFile(referencePath)) { return; } TRACE_FUNCTION(); - _layer = SdfLayer::FindOrOpen(_filePath); + _layer = SdfLayer::FindOrOpen(referencePath); if (!_layer) { - TF_WARN("Unable to open layer at path @%s@.", _filePath.c_str()); + TF_WARN("Unable to open layer at path @%s@.", + referencePath.c_str()); return; } @@ -709,7 +713,8 @@ class _AssetLocalizer { auto &resolver = ArGetResolver(); - std::string rootFilePath = resolver.Resolve(assetPath.GetAssetPath()); + const std::string assetPathStr = assetPath.GetAssetPath(); + std::string rootFilePath = resolver.Resolve(assetPathStr); // Ensure that the resolved path is not empty. if (rootFilePath.empty()) { @@ -748,7 +753,8 @@ class _AssetLocalizer { seenFiles.insert(rootFilePath); std::string destFilePath = TfStringCatPaths(destDir, TfGetBaseName(rootFilePath)); - filesToLocalize.emplace(destFilePath, _FileAnalyzer(rootFilePath, + filesToLocalize.emplace(destFilePath, _FileAnalyzer( + assetPathStr, rootFilePath, /*refTypesToInclude*/ _ReferenceTypesToInclude::All, /*enableMetadataFiltering*/ enableMetadataFiltering, remapAssetPathFunc, processPathFunc)); @@ -855,7 +861,7 @@ class _AssetLocalizer { destDirForRef, remappedRef); filesToLocalize.emplace(destFilePathForRef, _FileAnalyzer( - resolvedRefFilePath, + refAssetPath, resolvedRefFilePath, /* refTypesToInclude */ _ReferenceTypesToInclude::All, /*enableMetadataFiltering*/ enableMetadataFiltering, remapAssetPathFunc, processPathFunc)); @@ -1121,9 +1127,11 @@ _ExtractExternalReferences( std::vector* references, std::vector* payloads) { + auto &resolver = ArGetResolver(); + // We only care about knowing what the dependencies are. Hence, set // remapPathFunc to empty. - _FileAnalyzer(filePath, refTypesToInclude, + _FileAnalyzer(filePath, resolver.Resolve(filePath), refTypesToInclude, /*enableMetadataFiltering*/ false, /*remapPathFunc*/ {}, [&subLayers, &references, &payloads]( diff --git a/pxr/usd/usdUtils/dependencies.h b/pxr/usd/usdUtils/dependencies.h index 95fb2a9f36..08d15c3379 100644 --- a/pxr/usd/usdUtils/dependencies.h +++ b/pxr/usd/usdUtils/dependencies.h @@ -148,7 +148,7 @@ UsdUtilsCreateNewARKitUsdzPackage( /// Any unresolved (layer and non-layer) asset paths are populated in /// \p unresolvedPaths. /// -/// The input vectors to be populated with the results are are *cleared* before +/// The input vectors to be populated with the results are *cleared* before /// any results are added to them. /// /// Returns true if the given asset was resolved correctly. diff --git a/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugInfo.json b/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugInfo.json new file mode 100644 index 0000000000..d67732b2c6 --- /dev/null +++ b/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugInfo.json @@ -0,0 +1,19 @@ +{ + "Plugins": [ + { + "Type": "library", + "Name": "TestUsdUtilsDependenciesCustomResolver", + "Root": "@TEST_PLUG_INFO_ROOT@", + "LibraryPath": "@TEST_PLUG_INFO_LIBRARY_PATH@", + "ResourcePath": "@TEST_PLUG_INFO_RESOURCE_PATH@", + "Info": { + "Types": { + "CustomResolver": { + "bases": ["ArResolver"], + "uriSchemes": ["test", "testresolved"] + } + } + } + } + ] +} diff --git a/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugin.cpp b/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugin.cpp new file mode 100644 index 0000000000..98b1673ecc --- /dev/null +++ b/pxr/usd/usdUtils/testenv/TestUsdUtilsDependenciesCustomResolver_plugin.cpp @@ -0,0 +1,96 @@ +// +// Copyright 2020 Pixar +// +// Licensed under the Apache License, Version 2.0 (the "Apache License") +// with the following modification; you may not use this file except in +// compliance with the Apache License and the following modification to it: +// Section 6. Trademarks. is deleted and replaced with: +// +// 6. Trademarks. This License does not grant permission to use the trade +// names, trademarks, service marks, or product names of the Licensor +// and its affiliates, except as required to comply with Section 4(c) of +// the License and to reproduce the content of the NOTICE file. +// +// You may obtain a copy of the Apache License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the Apache License with the above modification is +// distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the Apache License for the specific +// language governing permissions and limitations under the Apache License. +// +#include "pxr/pxr.h" + +#include "pxr/usd/ar/defaultResolver.h" +#include "pxr/usd/ar/defineResolver.h" +#include "pxr/usd/ar/filesystemAsset.h" + +#include + +PXR_NAMESPACE_USING_DIRECTIVE + +/// This bare bones resolver is specifically setup to use different URI schemes +/// for identifier creation and asset resolution. +/// Identifiers are in form of test:path +/// Resolved paths are in form testresolved:path +class CustomResolver + : public ArResolver +{ +public: + CustomResolver() + { + } + +protected: + std::string _CreateIdentifier( + const std::string& assetPath, + const ArResolvedPath& anchorAssetPath) const final + { + return assetPath; + } + + std::string _CreateIdentifierForNewAsset( + const std::string& assetPath, + const ArResolvedPath& anchorAssetPath) const final + { + return assetPath; + } + + ArResolvedPath _Resolve( + const std::string& assetPath) const final + { + const std::string resolved = "testresolved:" + + assetPath.substr(assetPath.find_first_of(":") + 1); + + return ArResolvedPath(resolved); + } + + ArResolvedPath _ResolveForNewAsset( + const std::string& assetPath) const final + { + return _Resolve(assetPath); + } + + std::shared_ptr _OpenAsset( + const ArResolvedPath& resolvedPath) const final + { + const std::string pathStr = resolvedPath.GetPathString(); + const std::string filesystemPath = + pathStr.substr(pathStr.find_first_of(":") + 1); + + return ArFilesystemAsset::Open(ArResolvedPath(filesystemPath)); + } + + std::shared_ptr + _OpenAssetForWrite( + const ArResolvedPath& resolvedPath, + WriteMode writeMode) const final + { + return nullptr; + } + +}; + +AR_DEFINE_RESOLVER(CustomResolver, ArResolver); diff --git a/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver.py b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver.py new file mode 100644 index 0000000000..986e17d0d4 --- /dev/null +++ b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver.py @@ -0,0 +1,81 @@ +#!/pxrpythonsubst +# +# Copyright 2023 Pixar +# +# Licensed under the Apache License, Version 2.0 (the "Apache License") +# with the following modification; you may not use this file except in +# compliance with the Apache License and the following modification to it: +# Section 6. Trademarks. is deleted and replaced with: +# +# 6. Trademarks. This License does not grant permission to use the trade +# names, trademarks, service marks, or product names of the Licensor +# and its affiliates, except as required to comply with Section 4(c) of +# the License and to reproduce the content of the NOTICE file. +# +# You may obtain a copy of the Apache License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the Apache License with the above modification is +# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the Apache License for the specific +# language governing permissions and limitations under the Apache License. +from pxr import UsdUtils, Plug, Sdf + +import os, unittest + + +class TestUsdUtilsDependenciesCustomResolver(unittest.TestCase): + @classmethod + def setUpClass(cls): + # Register test resolver plugins + # Test plugins are installed relative to this script + testRoot = os.path.join( + os.path.dirname(os.path.abspath(__file__)), 'UsdUtilsPlugins') + + pr = Plug.Registry() + + testURIResolverPath = os.path.join( + testRoot, 'lib/TestUsdUtilsDependenciesCustomResolver*/Resources/') + pr.RegisterPlugins(testURIResolverPath) + + def test_ComputeAllDependencies(self): + """Tests that ComputeAllDependencies correctly sets identifier and """ + """resolved paths when using a resolver with multiple uri schemes """ + + mainIdentifier = "test:main.usda" + expectedMainResolvedPath = "testresolved:main.usda" + dependencyIdentifier = "test:dependency.usda" + expectedDependencyResolvedPath = "testresolved:dependency.usda" + + layers, _, _ = UsdUtils.ComputeAllDependencies(mainIdentifier) + + self.assertEqual(len(layers), 2) + layer0 = layers[0] + self.assertEqual(layer0.identifier, mainIdentifier) + self.assertEqual(layer0.resolvedPath, expectedMainResolvedPath) + + layer1 = layers[1] + self.assertEqual(layer1.identifier, dependencyIdentifier) + self.assertEqual(layer1.resolvedPath, expectedDependencyResolvedPath) + + def test_findOrOpenLayer(self): + mainIdentifier = "test:main.usda" + expectedMainResolvedPath = "testresolved:main.usda" + dependencyIdentifier = "test:dependency.usda" + expectedDependencyResolvedPath = "testresolved:dependency.usda" + + UsdUtils.ComputeAllDependencies(mainIdentifier) + + layer = Sdf.Layer.FindOrOpen(mainIdentifier) + self.assertEqual(layer.identifier, mainIdentifier) + self.assertEqual(layer.resolvedPath, expectedMainResolvedPath) + + layer = Sdf.Layer.FindOrOpen(dependencyIdentifier) + self.assertEqual(layer.identifier, dependencyIdentifier) + self.assertEqual(layer.resolvedPath, expectedDependencyResolvedPath) + + +if __name__=="__main__": + unittest.main() diff --git a/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/dependency.usda b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/dependency.usda new file mode 100644 index 0000000000..f5880f24d0 --- /dev/null +++ b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/dependency.usda @@ -0,0 +1,11 @@ +#usda 1.0 +( + upAxis = "Y" + metersPerUnit = 0.01 + defaultPrim = "dependency" +) + +def XForm "dependency" +{ + +} \ No newline at end of file diff --git a/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/main.usda b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/main.usda new file mode 100644 index 0000000000..b1537b7e9c --- /dev/null +++ b/pxr/usd/usdUtils/testenv/testUsdUtilsDependenciesCustomResolver/main.usda @@ -0,0 +1,14 @@ +#usda 1.0 +( + upAxis = "Y" + metersPerUnit = 0.01 + defaultPrim = "World" + subLayers = [ + @test:dependency.usda@ + ] +) + +def XForm "main" +{ + +} \ No newline at end of file