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

Added support for v3d and m44d #7

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

Conversation

Ambrosiussen
Copy link

No description provided.

@tiltX
Copy link

tiltX commented Dec 18, 2021

I just tried this patch and I got an error for m44d:

attribute_values = struct.unpack('d' * 32, exr_file.read(4 * 32))
                                       ^ this should still be 16 for a 4x4 matrix

Moreover, an EXR created by the latest Karma renderer (Houdini) failed due to an attribute of type "v2d" which I guess is just like v2i and v2f but twice the size?

@vvzen
Copy link
Owner

vvzen commented Dec 26, 2021

hi @Ambrosiussen !
sorry for the delay and thanks for the MR - I missed the notification for this one but lgtm! Do you mind linking the information that you used to write the implementation for the m44d ?
@tiltX: I haven't checked the white paper, but I suppose that v2d is a vector of doubles ?

Thanks all!

@tiltX
Copy link

tiltX commented Dec 27, 2021

@tiltX: I haven't checked the white paper, but I suppose that v2d is a vector of doubles ?

Yes, it works if I add a case for v2d by duplicating the code for v2f with this adjustment for struct.unpack()

            elif attribute_type == b'v2d':
                vector_2d_value = struct.unpack('dd', exr_file.read(8 * 2))

Unfortunately I don't know what the proper workflow would be to add to @Ambrosiussen 's pull request.
I haven't tried reading any of the test cases in the openexrs-images repository to see if they have such a v2d attribute. But both this addition and the fix for m44d above allowed me to parse EXR files written by the latest production build of Houdini's Karma renderer.

@vvzen
Copy link
Owner

vvzen commented Dec 27, 2021

Hey @tiltX !
the cleanest workflow would be to have @Ambrosiussen update his PR so that I can merge all changes in one place.
However - if you'd like to contribute, I'd really appreciate if you could make a PR that adds a few of these EXRs generated from Houdini that have these metadata (or you could just send them via email if that's easier for you). I could add them to the test image suite that is used when running the tests.
Thanks and happy holidays!

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.

3 participants