Cmdline ar resolver #165

pmolodo · 2017-02-22T02:58:55Z

Description of Change(s)

Fixes to allow usage of unresolved asset paths with both the command-line tools and the katana plugin

Included Commit(s)

Fixes Issue(s)

None previously reported (that I know of...)

sunyab

Hey @elrond79, I had a couple of notes and questions down below. Thanks!

sunyab · 2017-03-10T22:02:12Z

pxr/usd/bin/usddiff/usddiff.py

+    # Sdf.Layer.FindOrOpen first
+    args = []
+    for arg in origArgs:
+        layer = Sdf.Layer.FindOrOpen(arg)


I think this would break usddiff in the case where you pass in directories to compare, since args (and origArgs) would contain directory names. You would wind up calling Sdf.Layer.FindOrOpen on a directory, which would fail and result in a ValueError getting raised.

See my response to the comment above...

sunyab · 2017-03-10T22:12:28Z

pxr/usd/bin/usddiff/usddiff.py

+    origArgs = args
+
+    # path may not be a "real" path due to custom resolvers... so use
+    # Sdf.Layer.FindOrOpen first


I'm concerned about the performance impact of using Sdf.Layer.FindOrOpen to try to deal with custom asset paths that may not have a corresponding file on the filesystem. It seems like we don't really care about loading the layer here, we just want to know whether a layer at that path actually exists. So instead of doing this, could we try to use Ar.Resolver.Resolve somehow to give us this information?

Hmm, good point. Essentially want we want to do here is just validate using the ArResolver. I actually posted a question on the mailing list about this (and some other resolver issues) - see the third question here: https://groups.google.com/forum/#!topic/usd-interest/E3huSatYrLI

Assuming that we can rely on getting back an empty string as a form of validation, I think maybe the best thing to do is to just replace the look that checks for existence with one which checks that either it resolves to non-empty, OR it's a dir (per your next note).

sunyab · 2017-03-10T22:15:06Z

third_party/katana/lib/usdKatana/cache.cpp

-        UsdStageRefPtr const& stage = UsdStage::Open(rootLayer, sessionLayer, 
-                ArGetResolver().GetCurrentContext(),
+        UsdStageRefPtr const& stage = UsdStage::Open(rootLayer, sessionLayer,
+                ArGetResolver().CreateDefaultContextForAsset(fileName),


What was the reason for changing this to use a new context rather than whatever context was currently bound?

Hmm, just confusion about best practices - see my first question here: https://groups.google.com/forum/#!topic/usd-interest/E3huSatYrLI

Going off of some other examples (ie, usdviewq/mainWindow.py, and _createPathResolverContext), it seemed like the "best" thing to do was, if you knew the path you were about to open, to create a context for it. Apologies if this isn't the proper thing to do - I will remove this change!

sunyab · 2017-03-10T22:19:17Z

third_party/maya/lib/usdMaya/usdImport.cpp

+        // absolute paths
+        std::string expandedPath = mayaFile.expandedFullName().asChar();
+        ArResolver& resolver = ArGetResolver();
+        resolver.ConfigureResolverForAsset(expandedPath);


I don't think we should reconfigure the resolver here, since that's a stateful change that potentially affects all subsequent consumers of ArResolver for path resolution. Can you describe what issue you were running into here?

Again, just confusion about best practices.

Your comment about this being a stateful change helps clarify things somewhat, though - on re-reviewing the existing code, it seems like everywhere that's calling ConfigureResolverForAsset is doing so from within "application level" code. I'll remove that line.

pmolodo · 2017-03-11T02:36:14Z

First - thanks for the code review, was very helpful!

Second - I'm making the changes noted... and realizing that using on-disk filepaths and unresolved asset paths interchangeably is tricky, once directories get involved. For simplicity, I'm going to make the following assumptions:

we first check an arg to see if it is a file/dir, and if so, assume it is NOT an unresolved asset path
if any of the directory forms are used, then none of the other args may be unresolved asset paths

I'll add a note to the parser help to this effect. Let me know if you'd rather proceed in a different manner.

pmolodo · 2017-03-15T15:59:44Z

So, I pushed a fresh branch where I've made the changes requested. Sorry for the delay - we're in the middle of migrating our commits over to topgit to make juggling our various patches more manageable. Anyway, this branch is a "clean" history - if you want to see all the incremental changes, I can push the topgit patch branches. Thanks again for the review!

pmolodo · 2017-05-04T21:55:38Z

Pushed a new version that fixed a test failure...

formerly failed testUsdDiffToolVariousFormats4 + 5 - _getFileFormat wasn't returning None for unrecognized file formats

jtran56 · 2017-06-02T01:10:52Z

Filed as internal issue #147131.

sunyab · 2017-06-13T17:12:57Z

Hi @elrond79, I was trying to merge this in but I still run into a test failure with testUsdDiffToolVariousFormats5:

Traceback (most recent call last):
File "/scratch/build_local/dev/src/inst/fedora-gcc64-opt/pxr/bin/usddiff", line 340, in
main()
File "/scratch/build_local/dev/src/inst/fedora-gcc64-opt/pxr/bin/usddiff", line 324, in main
results.flatten, results.noeffect):
File "/scratch/build_local/dev/src/inst/fedora-gcc64-opt/pxr/bin/usddiff", line 152, in _runDiff
baselineFileType = _getFileFormat(baseline)
File "/scratch/build_local/dev/src/inst/fedora-gcc64-opt/pxr/bin/usddiff", line 117, in _getFileFormat
return fileFormat.formatId
AttributeError: 'NoneType' object has no attribute 'formatId'

Do you guys see the same thing? Or is this something on my end?

pmolodo · 2017-06-13T22:30:42Z

Oops - sorry, yes, that is a problem we saw, and fixed a while ago... but apparently forgot to push a refreshed PR. The PR should be up-to-date now.

For your reference, here's the (non-collapsed) commit that fixed this:

LumaPictures@2f05bd7

ie, can now do "usdview custom://asset_bigGuy/costume_red/version_0023" (assuming you have a custom ArResolver to handle "custom://" paths) usdview, usdcat, usdcheck, and usddiff should now all work with unresolved paths; usdstitch already worked usdstitchclips and usdGenSchema are untested, due to not having appropriate inputs on hand; usdrender is untested, due to not having a OpenGL.raw._GLX module

pmolodo · 2017-11-16T23:42:29Z

Rebased the PR to resolve merge conflicts...

Cmdline ar resolver

…fix_frameRecorder_failure OGSMOD-2625: Fix test case "testUsdAppUtilsFrameRecorder" failure

sunyab requested changes Mar 10, 2017

View reviewed changes

sambvfx force-pushed the cmdline_ArResolver branch 2 times, most recently from 8f6c5be to 191720a Compare March 15, 2017 01:38

pmolodo force-pushed the cmdline_ArResolver branch from 191720a to d4a74d5 Compare March 15, 2017 02:24

pmolodo force-pushed the cmdline_ArResolver branch from d4a74d5 to dd7f456 Compare June 13, 2017 21:57

pmolodo added 3 commits November 16, 2017 15:39

[katana] fix for unresolved asset paths

b9cef7b

[usdMaya] make unresolved asset paths work with usdImport command

fdb3a78

pmolodo force-pushed the cmdline_ArResolver branch from 9586848 to fdb3a78 Compare November 16, 2017 23:40

sunyab assigned pixar-oss Jan 25, 2018

pixar-oss merged commit fdb3a78 into PixarAnimationStudios:dev Jan 29, 2018

pixar-oss added a commit that referenced this pull request Jan 29, 2018

Merge pull request #165 from elrond79/cmdline_ArResolver

cd040d6

Cmdline ar resolver

pmolodo deleted the cmdline_ArResolver branch March 12, 2019 23:02

Felix-Kit mentioned this pull request Feb 9, 2022

Stack-overflow bug found in sdfdump #1763

Closed

AdamFelt pushed a commit to autodesk-forks/USD that referenced this pull request Apr 16, 2024

Merge pull request PixarAnimationStudios#165 from autodesk-forks/fei/…

7f76e44

…fix_frameRecorder_failure OGSMOD-2625: Fix test case "testUsdAppUtilsFrameRecorder" failure

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmdline ar resolver #165

Cmdline ar resolver #165

pmolodo commented Feb 22, 2017 •

edited

Loading

sunyab left a comment

sunyab Mar 10, 2017

pmolodo Mar 11, 2017

sunyab Mar 10, 2017

pmolodo Mar 11, 2017 •

edited

Loading

sunyab Mar 10, 2017

pmolodo Mar 11, 2017

sunyab Mar 10, 2017

pmolodo Mar 11, 2017 •

edited

Loading

pmolodo commented Mar 11, 2017

pmolodo commented Mar 15, 2017 •

edited

Loading

pmolodo commented May 4, 2017 •

edited

Loading

jtran56 commented Jun 2, 2017

sunyab commented Jun 13, 2017

pmolodo commented Jun 13, 2017

pmolodo commented Nov 16, 2017

Cmdline ar resolver #165

Cmdline ar resolver #165

Conversation

pmolodo commented Feb 22, 2017 • edited Loading

Description of Change(s)

Included Commit(s)

Fixes Issue(s)

sunyab left a comment

Choose a reason for hiding this comment

sunyab Mar 10, 2017

Choose a reason for hiding this comment

pmolodo Mar 11, 2017

Choose a reason for hiding this comment

sunyab Mar 10, 2017

Choose a reason for hiding this comment

pmolodo Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

sunyab Mar 10, 2017

Choose a reason for hiding this comment

pmolodo Mar 11, 2017

Choose a reason for hiding this comment

sunyab Mar 10, 2017

Choose a reason for hiding this comment

pmolodo Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

pmolodo commented Mar 11, 2017

pmolodo commented Mar 15, 2017 • edited Loading

pmolodo commented May 4, 2017 • edited Loading

jtran56 commented Jun 2, 2017

sunyab commented Jun 13, 2017

pmolodo commented Jun 13, 2017

pmolodo commented Nov 16, 2017

pmolodo commented Feb 22, 2017 •

edited

Loading

pmolodo Mar 11, 2017 •

edited

Loading

pmolodo Mar 11, 2017 •

edited

Loading

pmolodo commented Mar 15, 2017 •

edited

Loading

pmolodo commented May 4, 2017 •

edited

Loading