Inital Houdini lut support


Jeremy Selan <jeremy...@...>
 

Looks good to me!

I have a few comments, but these dont need to be addressed yet.

* In terms of coding style, you put the return value on a new line,
where as I tend to not. I'm actually fine either way, but I'd really
like to keep the codebase uniform in this regard. I'm going to be
putting together coding guidelines together in the next few days, and
once we pick what's preferred I'll do a single cleanup pass to bring
the whole codebase up to spec. (I'll also attack line lengths, and
#include order at the same time).

* Your commit, c7a1bfadeb..., (where you change the walk order to
match GLs ordering) was not strictly necessary. Lut3DOp.h defines two
3dlut index helpers:

int GetGLLut3DArrayOffset(...)
int GetAutodeskLut3DArrayOffset(...)

You could have used the second function, rather than the first, to
save yourself this trouble. (It is poorly named though, any better
idea on names?)

* We will be able to resolve ambiguously named formats (such as the
super ambiguous .lut) with a minor addition to FileTransform.cpp.
(This code is already in FileTransform:GetFile in code comment form).

The idea is inspired by image reading in Nuke. OCIO will use the
file extension to guess the format, and attempt to load it. (in the
case of multiple formats with the same extension, just pick one). If
this fails, then try to load all other formats with the same extension
until one succeeds. If this fails too, then go ahead and try all
known formats in case it's a misnamed file. The cool part of this
is that users don't have to worry about file format naming (assuming
the lut readers all are good about throwing exceptions early on
malformed luts), and performance doesnt suffer either as the format
results are cached so the rollover detection only happens once per
session.

I'll implement this behavior before the next .lut format is added.

-- Jeremy


Jeremy Selan <jeremy...@...>
 

Also - for future reference - you don't need to include the patch
file. It will be sufficient to just include the SHA1 hash for the top-
most commit that corresponds to the topic.

For example, in this case 9c2a6d47bb18ee6ca565e2ae1691486f8ae57c0b
(You can find this for old commits using gitk, in your repo).

Thanks!

-- Jeremy


Malcolm Humphreys <malcolmh...@...>
 

* In terms of coding style, you put the return value on a new line,
where as I tend to not. I'm actually fine either way, but I'd really
like to keep the codebase uniform in this regard. I'm going to be
putting together coding guidelines together in the next few days, and
once we pick what's preferred I'll do a single cleanup pass to bring
the whole codebase up to spec. (I'll also attack line lengths, and
#include order at the same time).
I'm happy to match what ever style you put forward, I mainly like doing
this as it makes it really clear what a function / method is returning. We
don't have function overloading but in code bases where you do I find
this helps.

My only other note would be about indenting all namespaces, I see you
want to limit to 80 char columns (+1) this with the namespace indenting
is going to make for some very compact coding.

ie.
--snip--
OCIO_NAMESPACE_ENTER
{
namespace
{

void foo();

}
}
OCIO_NAMESPACE_EXIT
--snip--

--snip--
OCIO_NAMESPACE_ENTER
{
namespace
{

void foo();

} // anonymous namespace
}
OCIO_NAMESPACE_EXIT
--snip--

* Your commit, c7a1bfadeb..., (where you change the walk order to
match GLs ordering) was not strictly necessary. Lut3DOp.h defines two
3dlut index helpers:

int GetGLLut3DArrayOffset(...)
int GetAutodeskLut3DArrayOffset(...)

You could have used the second function, rather than the first, to
save yourself this trouble. (It is poorly named though, any better
idea on names?)
int GetRGBLut3DArrayOffset(...)?

* We will be able to resolve ambiguously named formats (such as the
super ambiguous .lut) with a minor addition to FileTransform.cpp.
(This code is already in FileTransform:GetFile in code comment form).

The idea is inspired by image reading in Nuke. OCIO will use the
file extension to guess the format, and attempt to load it. (in the
case of multiple formats with the same extension, just pick one). If
this fails, then try to load all other formats with the same extension
until one succeeds. If this fails too, then go ahead and try all
known formats in case it's a misnamed file. The cool part of this
is that users don't have to worry about file format naming (assuming
the lut readers all are good about throwing exceptions early on
malformed luts), and performance doesnt suffer either as the format
results are cached so the rollover detection only happens once per
session.

I'll implement this behavior before the next .lut format is added.
Sounds good, like in nuke could your see us forcing a loader in the
profile format?

.malcolm