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

USDShader : Add new node for loading shaders from pxr::SdrRegistry #5384

Merged
merged 3 commits into from
Jul 18, 2023

Conversation

johnhaddon
Copy link
Member

This provides the ability to load and assign the various UsdPreviewSurface shaders, and will form the foundations for loading UsdLuxLights in a future PR.

image

We have been debating whether or not we should be using the SdrRegistry or the SchemaRegistry for loading this stuff, and I came down on the side of the SdrRegistry for now because :

  • The UsdPreviewSurface definitions are only registered with the SdrRegistry, not the SchemaRegistry.
  • After building arnold-usd, I couldn't find any useful "bolt on" schemas that would be useful for extending a UsdLuxLight for Arnold.
  • Nothing about the implementation of UsdShader is exposed publicly, so we can switch to loading from the schema registry in future if we want, without any visible change.

@johnhaddon johnhaddon self-assigned this Jul 7, 2023
Copy link
Contributor

@murraystevenson murraystevenson left a comment

Choose a reason for hiding this comment

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

Thanks John! A couple of minor comments and questions inline, but otherwise is looking good to me.

Changes.md Outdated Show resolved Hide resolved
python/GafferUSDUI/USDShaderUI.py Show resolved Hide resolved
python/GafferUSDUI/USDShaderUI.py Show resolved Hide resolved
src/GafferUSD/USDShader.cpp Outdated Show resolved Hide resolved
src/GafferUSD/USDShader.cpp Outdated Show resolved Hide resolved
@johnhaddon
Copy link
Member Author

Thanks for the review - I've pushed fixups and replied to each of your comments. The initial commits are as they were before, but have changed SHA just because I rebased to get Changes.md in sync with the 1.3 release. It should be sufficient to only re-review the fixups.

@murraystevenson
Copy link
Contributor

Thanks John, fixups look good to me! Should be good to go after ensuring we're targeting 1.3_maintenance rather than main.

What we'd like to do here is drive everything with stuff we query from the SdrRegistry, but this road doesn't seem very well travelled as of yet, so for now we have to provide a lot of metadata of our own.
@johnhaddon johnhaddon changed the base branch from main to 1.3_maintenance July 18, 2023 07:54
@johnhaddon johnhaddon merged commit 82b6b62 into GafferHQ:1.3_maintenance Jul 18, 2023
4 checks passed
@johnhaddon johnhaddon deleted the usdShader branch July 18, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants