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 get_texture_info_type to query TypeDesc of arbitrary texture metadata #3207

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 is originally written for a pretty specific use-case outlined below, but hopefully it's generic enough to be useful.

It is sort-of a follow up to the maketx-patch that writes arbitrary length float arrays metadata in EXRs, and ties in with a patch on the OSL side: If we can query the TypeDesc of a metadata attribute, we can make more a lenient variant of OSL's gettextureinfo call that will fill arrays as long as the array on the OSL side is large enough.

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.

I've outlined suggested changes to the function signature (mostly for the sake of matching equivalent methods in other classes), and some missing error handling. But other than that, I like this idea and I think the rest are fine.

Missing in action, please add:

  • Documentation -- that is, do the usual Doxygen thing for the comments on these functions in the public headers texture.h and imagecache.h, to explain what they do. The docs for get_image_info/get_texture_info do most of the heavy lifting here, you can get away with a short explanation for how this differs, you obviously don't need to explain any of the specific queries it knows.
  • Testing. Something should be in the testsuite to verify this. I think that it's sufficient to simply modify testtex.cpp's test_gettextureinfo() to also do calls for the same queries to get_texture_info_type and verify it returns the expected value.

Comment on lines +2854 to +2865
ParamValue tmpparam;
const ParamValue* p = spec.find_attribute(dataname, tmpparam);

if (p) {
datatype = p->type();
return true;
}

// Does it make sense to set the type here, or should we just return
// false?
datatype.basetype = TypeDesc::UNKNOWN;
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these lines can simply be

return spec.getattributetype(dataname);

and the return type for get_image_info_type can just be TypeDesc (and the datatype parameter to this method can go away). The returned TypeDesc will already be UNKNOWN if it's not found.

This matches the way we've already done getattributetype() in ImageSpec, and also in ParamList.

Also, the ImageSpec::getattributetype works even if it's one of the "built-in" fields, and isn't restricted to the "extra" attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I take back that last sentence partially -- ImageSpec::getattributetype will only work for things that are in the ImageSpec (extra_args but also some other hard fields). But what this implementation of get_image_info_type won't be able to do is retrieve any of the other kinds of queries that get_image_info knows about that don't correspond to the ImageSpec, unless you are inclined to replicate much of that logic here.

Comment on lines +845 to +848
virtual bool get_image_info_type(ustring filename, int subimage,
int miplevel, ustring dataname,
TypeDesc& datatype);
virtual bool get_image_info_type(ImageCacheFile* file,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's just have these return the TypeDesc directly, since that's the way the equivalent works in other places we have a "get attribute type" kind of method. You don't need the bool return since there is already a special TypeDesc value (TypeUnknown) to indicate failure to find a matching item.

Comment on lines +2848 to +2849

ImageCacheStatistics& stats(thread_info->m_stats);
Copy link
Collaborator

Choose a reason for hiding this comment

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

See lines 2581ff, where get_image_info checks and gives appropriate errors and early exit for a null file, and lines 2672ff, where it handles non-existant subimage or miplevel. I think we need to do this here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm... that comment doesn't seem to have gotten associated with the right spot in your code. :-)
I was trying to point this out in get_image_info_type, line 2853 (of your changes), right before you dereference file and ask for the spec for this specific subimage and miplevel.

@lgritz
Copy link
Collaborator

lgritz commented Nov 30, 2021

Another thing worth mentioning: Because this breaks the ABI (by adding virtual methods to this class, we change he vtbl), we can't backport to 2.3.* and maintain binary compatibility, so this can only be in 2.4 and later. That's not a problem, just want to be sure it's understood.

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