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

Improve widget hints for PxrUsdIn.py. #218

Closed
wants to merge 1 commit into from

Conversation

aferrall
Copy link

@aferrall aferrall commented Jun 5, 2017

Description of Change(s)

Very small tweak to PxrUsdIn.py so that its "fileName" parameter uses an assetIdInput widget, allowing type-filtered file browsing.

Fixes Issue(s)

@aferrall
Copy link
Author

aferrall commented Jun 6, 2017

I realize this is similar to pull request #216, but the "assetIdInput" provides greater flexibility compared with "fileInput", since it allows customization of the browser based on local Asset plugin implementations. This is also consistent with existing file loaders like the shipping "Alembic_In" node.

@jtran56
Copy link

jtran56 commented Jun 7, 2017

Filed as internal issue #147290.

@sunyab
Copy link
Contributor

sunyab commented Jun 9, 2017

Hi @aferrall, thanks for the pull request! We ask all contributors to sign a CLA before contributing code to USD. Could you take a look at https://graphics.pixar.com/usd/docs/Contributing-to-USD.html and follow the instructions at the top of the page? Once we've received your CLA, we can have a look at merging in your changes. Thanks!

@aferrall
Copy link
Author

aferrall commented Jun 9, 2017

Hey @sunyab, I've actually already gone through that process and the ILM legal folks told me that everything is setup for the corporate CLA. I assume that if you have an ILM CLA on file, you don't have my GitHub username associated with it.

Do I need that to come through the same formal channel which sent along the original CLA? Thanks!

@sunyab
Copy link
Contributor

sunyab commented Jun 9, 2017

Ah, let me double check about the corporate CLA and get back to you -- I think if we've got that on file, you should be all set. In the meantime, I'll queue this change up for review.

@sunyab
Copy link
Contributor

sunyab commented Aug 7, 2017

Hi @aferrall, I'd like to close this out in favor of commit 283f715, which was coincidentally submitted shortly after your pull request and uses an assetIdInput widget like your change.

However, your change adds .abc files as a supported file type, while the above commit does not. I don't think we should unconditionally add this file type, since USD only supports .abc files if users have built the Alembic plugin. Ideally, we'd only add the file type in if the plugin were available, but I'm not sure how complicated it would be to do such a check.

Is having .abc files listed here important for your use case? If not, I propose we close this out in favor of the above commit. What do you think?

@aferrall
Copy link
Author

aferrall commented Aug 8, 2017

Hey @sunyab - I'm sure we'll be able to make do without the .abc filetype option. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants