-
Notifications
You must be signed in to change notification settings - Fork 603
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
base: main
Are you sure you want to change the base?
Add get_texture_info_type to query TypeDesc of arbitrary texture metadata #3207
Conversation
There was a problem hiding this 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.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
virtual bool get_image_info_type(ustring filename, int subimage, | ||
int miplevel, ustring dataname, | ||
TypeDesc& datatype); | ||
virtual bool get_image_info_type(ImageCacheFile* file, |
There was a problem hiding this comment.
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.
|
||
ImageCacheStatistics& stats(thread_info->m_stats); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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.