Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure resolver to generate resolver context #1342

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

kxl-adsk
Copy link

Address issue reported by #1236. FWIW. We are still planning on exploring Ar 2.0 so things may change here.

@kxl-adsk kxl-adsk added the proxy Related to base proxy shape label Apr 16, 2021
@kxl-adsk
Copy link
Author

@dj-mcg @fabal @pmolodo Please review and raise concerns.

@kxl-adsk kxl-adsk force-pushed the kxl-adsk/configure_asset_solver_in_base_proxy branch from 53fae34 to 98a5aeb Compare April 16, 2021 20:30
@dolivares-spinvfx
Copy link

Copy link
Collaborator

@dj-mcg dj-mcg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

Copy link
Contributor

@fabal fabal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good thing to have 👍

Did you consider using ResolverContext objects too?
Typically in places like this where a call like ArGetResolver().CreateDefaultContextForAsset(fileString) has become the default standard in many places (usdview, solaris).

A step further would be to be able to apply arbitrary resolver contexts to proxyShapes.

@kxl-adsk
Copy link
Author

@fabal Thanks for bringing this up. I'm catching up on docs around this, it's unclear to me why we need this extra ArGetResolver().CreateDefaultContextForAsset(fileString) . A call to ConfigureResolverForAsset will update the default context. Why do we have to create it again explicitly instead of querying what was already configured?

Regardless, I updated the change to include suggestions raised in this review and matched the code to what is here.

@dolivares-spinvfx please give it a try since you raised this issue originally.

@fabal
Copy link
Contributor

fabal commented Apr 28, 2021

Admittedly, that explicit call to CreateDefaultContextForAsset is not needed in that particular case as Stage::Open does this by default but I guess it could be a good start towards explicit resolver contexts.
Over time, we steered clear of the default resolver context as it can lead to asset resolution "leaks" in complex scenarios (e.g. several proxyShapes, using dedicated resolver context). By "leaks", I don't mean a USD bug, I mean resolution overrides of one asset leaking into another one.
In relation to that, we also ended up guarding most of our calls to Sdf::Layer::FindOrOpen with a resolver context binder.
These comments are mainly for complex asset resolution use cases though, however I believe it is worth mentioning.

@kxl-adsk kxl-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 29, 2021
@kxl-adsk kxl-adsk merged commit 4ebce76 into dev Apr 29, 2021
@kxl-adsk kxl-adsk deleted the kxl-adsk/configure_asset_solver_in_base_proxy branch April 29, 2021 19:18
@dolivares-spinvfx
Copy link

Was just able to test this today due to a disk failure. Seems like asset resolution is working with this change. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proxy Related to base proxy shape ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants