-
Notifications
You must be signed in to change notification settings - Fork 606
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
ociodisplay gets an inverse control #3650
ociodisplay gets an inverse control #3650
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.
Thanks for adding this Larry!
@@ -1848,7 +1876,7 @@ bool | |||
ImageBufAlgo::ociodisplay(ImageBuf& dst, const ImageBuf& src, | |||
string_view display, string_view view, | |||
string_view from, string_view looks, bool unpremult, | |||
string_view key, string_view value, | |||
bool inverse, string_view key, string_view value, | |||
ColorConfig* colorconfig, ROI roi, int nthreads) | |||
{ |
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.
Don't know how to select the line 1872, but don't you want to pass inverse there too?
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.
yes, I think you're right
src/libOpenImageIO/color_ocio.cpp
Outdated
{ | ||
ImageBuf result; | ||
bool ok = ociodisplay(result, src, display, view, from, looks, unpremult, | ||
key, value, colorconfig, roi, nthreads); | ||
unpremult, key, value, colorconfig, roi, nthreads); |
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.
unpremult --> inverse
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.
ouch, yes
Fixes 3648
783b9bf
to
1b5b001
Compare
Updated the PRs to address @doug-walker's suggestions. |
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.
LGTM!
Because this isn't tested, I'm going to hold off on a merge while I do a little tinkering... I'm exploring more direct support for OpenColorIO 2.2 and its built-in configs, which will allow me to test display transforms (merely because it has some, unlike the sample config that's in the testsuite now). If minimal support of this is easy to achieve, I'll add it to this PR (even if the full range of tests only work when we're building against OCIO 2.2). But if that looks like a bigger job that doesn't make sense to append here, I'll merge this PR first (despite not being tested) and the follow up with a separate one with 2.2 support and more testing. |
I'm going to merge this now, because the follow-on has grown to be more complex. |
Fixes #3648