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

Add type information for needed attributes. #1650

Merged

Conversation

curtisblack
Copy link
Contributor

@curtisblack curtisblack commented Mar 6, 2023

Description

We can currently query which attribute names and scopes are requested in a shader. This PR extends that idea to also allow querying the attribute types. This now more closely matches the equivalent available queries for user data.

For example, this shader:

shader Shader
{
  color c;
  getattribute("user_color", c);
  
  string s;
  getattribute("user_string", s);
}

allows us to do this:

int nattr = 0;
if (shadingSys->getattribute(shaderRef.get(), "num_attributes_needed", nattr) && nattr)
{
  // this already works
  OIIO::ustring* names = nullptr;
  shadingSys->getattribute(shaderRef.get(), "attributes_needed", OSL::TypeDesc::PTR, &names);
  
  // this is the added feature
  OIIO::TypeDesc* types = nullptr;
  shadingSys->getattribute(shaderRef.get(), "attribute_types", OSL::TypeDesc::PTR, &types);
  
  // nattr: 2
  // names: ["user_color", "user_string"]
  // types: [OIIO::TypeColor, OIIO::TypeString]
}

The motivation behind this feature is to perform additional type validation at material compile time when determining what primvars can be provided to the shader.

One side-effect of this change to be aware of is that, as mentioned in the code comments, if the same name is requested multiple times with different types, it will now be reported multiple times. Hopefully this is not too much of an issue as the same behaviour also occurs when an attribute is requested in multiple scopes, or when user data is requested with multiple types.

Tests

I wasn't able to find any existing unit tests for the "attributes_needed" feature to use as a starting point, if they do already exist please let me know and I can make those changes. I did see a spot in testshade where it seemed appropriate to add a usage of this feature.

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 6, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: curtisblack / name: Curtis Black (34872ba)

Copy link
Contributor

@etheory etheory left a comment

Choose a reason for hiding this comment

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

LGTM.

@lgritz
Copy link
Collaborator

lgritz commented Mar 7, 2023

Looks like there are a few formatting issues to fix (look at the logs of the failed clang-format test), and also needs a DCO message attached to the commit. But the code LGTM.

@lgritz
Copy link
Collaborator

lgritz commented Mar 27, 2023

@curtisblack Ping! -- can you please push an update with the DCO and fixed formatting so I can merge this in time for the monthly patch release this week?

Signed-off-by: Curtis Black <curtis.w.black@gmail.com>
Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing.

@lgritz lgritz merged commit 6f1fedb into AcademySoftwareFoundation:main Apr 11, 2023
lgritz pushed a commit to lgritz/OpenShadingLanguage that referenced this pull request Apr 11, 2023
…n#1650)

We can currently query which attribute names and scopes are requested in a shader. This PR extends that idea to also allow querying the attribute types. This now more closely matches the equivalent available queries for user data.

For example, this shader:

```
shader Shader
{
  color c;
  getattribute("user_color", c);
  
  string s;
  getattribute("user_string", s);
}
```

allows us to do this:

```
int nattr = 0;
if (shadingSys->getattribute(shaderRef.get(), "num_attributes_needed", nattr) && nattr)
{
  // this already works
  OIIO::ustring* names = nullptr;
  shadingSys->getattribute(shaderRef.get(), "attributes_needed", OSL::TypeDesc::PTR, &names);
  
  // this is the added feature
  OIIO::TypeDesc* types = nullptr;
  shadingSys->getattribute(shaderRef.get(), "attribute_types", OSL::TypeDesc::PTR, &types);
  
  // nattr: 2
  // names: ["user_color", "user_string"]
  // types: [OIIO::TypeColor, OIIO::TypeString]
}
```

The motivation behind this feature is to perform additional type validation at material compile time when determining what primvars can be provided to the shader.

One side-effect of this change to be aware of is that, as mentioned in the code comments, if the same name is requested multiple times with different types, it will now be reported multiple times. Hopefully this is not too much of an issue as the same behaviour also occurs when an attribute is requested in multiple scopes, or when user data is requested with multiple types.

Signed-off-by: Curtis Black <curtis.w.black@gmail.com>
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.

None yet

3 participants