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

Extend gettextureinfo to read arbitrary arrays and output their size … #1444

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

olegul
Copy link
Contributor

@olegul olegul commented Nov 30, 2021

This patch ties in with an OIIO patch: Add get_texture_info_type to query TypeDesc...

It extends the gettextureinfo call with an extra int output parameter, output int param_array_len;

int gettextureinfo(string texturename, string paramname, output int param_array_len, output type[] destination)

If the basetype of destination equals the basetype of the data under paramname, and the array length of destination is large enough to fit the data, it will store the data in destination and the array length of the data in param_array_len. This allows us
to be more flexible when working with arrays stored in texture metadata.

Shader example:

float dest[LARGEST_POSSIBLE_ARRAY];
int len = 0;
if (gettextureinfo(Filename, param, len, dest)) {
    printf("Array length of array %d\n", len);
    for (int i = 0; i < len; i++) {
        printf("%f ", dest[i]);
    }
}

…if matching basetype and fit inside the destination array.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 30, 2021

CLA Missing ID CLA Not Signed

@lgritz
Copy link
Collaborator

lgritz commented Nov 30, 2021

I like this, totally makes sense. And at first glance, the code looks fine.

Some things that are clearly needed to complete this work:

  • Docs -- can you take a stab at altering languagespec.tex to explain the new variant of gettextureinfo?
  • The exact call to TextureSystem::get_texture_info_type is going to change slightly, as discussed on the OIIO-side PR.
  • Remember that get_texture_info_type is not going to exist in all versions of OIIO that people might build against, so it's going to be crucial to have appropriate #if OIIO_VERSION_GREATER_EQUAL(2, 4, 0) surround this section, and some handling of the case when the OIIO version is not new enough to support this call. That's why all the CI tests are failing.
  • Needs testsuite coverage of this somehow. It can probably just be grafted onto testsuite/gettextureinfo/test.osl
  • You didn't have CLA coverage, but I just added you to SPI's CLA approval list. But you sill need to click the link in the CLA bot comment to activate it.
  • You didn't "DCO sign" your commits. That's easily fixed by just doing a git commit --amend -s.

@linux-foundation-easycla
Copy link

CLA Not Signed

@lgritz
Copy link
Collaborator

lgritz commented Nov 30, 2021

After I wrote that comment, I took the dog for a walk and had an important insight.

You don't technically need TextureSystem::get_texture_info_type at all (though it's nice to have and I still think we should add it to OIIO for symmetry with other classes).

TextureSystem::imagespec() returns a pointer to the ImageSpec, and you can call ImageSpec::getattributetype() to retrieve the type. No strict need for any new facility on the TextureSystem side. Or at least, that's certainly the way to handle the fallback for when you are building against an OIIO that is too old to have the new get_texture_info_type call.

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.

2 participants