Add get_texture_info_type to query TypeDesc of arbitrary texture metadata #3207
Conversation
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.