search

AcademySoftwareFoundation / OpenImageIO

Reading, writing, and processing images in a wide variety of file formats, using a format-agnostic API, aimed at VFX applications.
https://openimageio.readthedocs.org
Apache License 2.0
1.97k stars 597 forks source link

MATRIX33 aggregate data type for TypeDesc? #1264

Closed Shootfast closed 8 years ago

Shootfast commented 8 years ago

Hi,

Just going through another round of missing metadata types in the OpenEXR input/output code, and I noticed that we don't have any support for 3x3 matricies. 4x4 have a pride of place in the TypeDesc implementation, would you foresee any problems with me adding in a MATRIX33 to the AGGREGATE enum?

I'll also be adding double support for the exrinput plugin as it looks like the output code already supports it.

lgritz commented 8 years ago

I don't have any objections to a MATRIX33.

I think there are really two separate issues.

  1. Should TypeDesc get MATRIX33 support. I certainly have no objections to this, sure. (Though I would tend to want to cut it off there, and not have a full zoo of mXn.)
  2. Whether 3x3 matrix metadata in image files should be communicated through the OIIO APIs as 3x3 matrices (proliferating "is it 3x3 or 4x4" tests through a lot of user code), or is it better to embed 3x3 matrices in 4x4 and have only one case? It's somewhat analogous to how we tend to use 'int' when communicating integer metadata, even if the file format uses 'short' underneath. (For pixels, we have the full compliment of types, but for metadata, we tend to push people to int/float/string except for the few cases where that clearly won't work.)
Shootfast commented 8 years ago

Sorry, I made this pull request before seeing your reply: https://github.com/OpenImageIO/oiio/pull/1265

Shootfast commented 8 years ago

Just commenting on your points. Point 1: Totally agree. 3x3 and 4x4 should be enough for anybody.

As to your point 2: What I would expect is that in the following usage 

oiiotool in.exr -o out.exr

out.exr should keep all of the metadata that in.exr had. As we have a TypeDesc that can describe these types, it makes sense to me to keep them as accurate as possible (and not have double become float in out.exr for example). I can agree with coercing the metadata types where the destination filetype does not support them, but I think EXR should be our baseline.

I'd also like to see things like the ImfChromaticitiesAttribute make it in there, and I'll probably add that soon, though that'd just be an addition to the VECSEMANTICS enum.

Cheers! Mark

lgritz commented 8 years ago

I'm not sure why chromaticities needs to have a separate vecsemantics. Why not just use a float array?

lgritz commented 8 years ago

Closing, because we committed a fix for this.