Malcolm, I wasn't able to build the docs at home even on OSX 10.6.
Would you try the master repo on your machine again (post merge) to
verify it works for you?

-- Jeremy

Looks good to me. I did the merge, but disabled doc generation in the
top-level makefile by default. When you add the explicit target i'll
just roll that in too.

Thanks!

-- Jeremy

Version 0.7.4 (Jan 4 2011):
* Added Context, allowing for 'per-shot' Transforms
* Misc API Cleanup: removed old functions + fixed const-ness
* Added config.sanityCheck() for validation
* Additional Makefile configuration options, SONAME, etc.

Coming soon...
* Docs / Examples
* "Looks"
* IIF Profile

Yah, these conflict in:
export/OpenColorIO/OpenColorIO.h
src/pyglue/CMakeLists.txt

Looks easy to address though. Why don't you checkin the extra build
option to make the docs optional on your master, and then i'll fix it
to work on the new trunk? (or you can do the merge too if you'd like)
;)

-- Jeremy

On Tue, Jan 4, 2011 at 3:37 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

LGTM,

This might slightly conflict with the docs commit just see how you go.

.malcolm

On 05/01/2011, at 10:00 AM, Jeremy Selan wrote:

Commits:
http://github.com/jeremyselan/OpenColorIO/commit/f5a80ee9e46eab956de7d618abda866af81de0e7
http://github.com/jeremyselan/OpenColorIO/commit/9859c936bd4c17651ec2a4ae33fa23c6476fc6d2

These checkins expose additional build options on the command-line
(needed for custom facility install scripts).
* SOVERSION can now be exposed, and overridden, on the command-line (a
la openimageio)
* The python so can be built with, or without, the "lib" prefix.

This checkin does *not* change any build default values. Default
behavior should be identical.

-- Jeremy

LGTM,

This might slightly conflict with the docs commit just see how you go.

.malcolm

LGTM

Commits:
http://github.com/jeremyselan/OpenColorIO/commit/f5a80ee9e46eab956de7d618abda866af81de0e7
http://github.com/jeremyselan/OpenColorIO/commit/9859c936bd4c17651ec2a4ae33fa23c6476fc6d2

These checkins expose additional build options on the command-line
(needed for custom facility install scripts).
* SOVERSION can now be exposed, and overridden, on the command-line (a
la openimageio)
* The python so can be built with, or without, the "lib" prefix.

This checkin does *not* change any build default values. Default
behavior should be identical.

-- Jeremy

On 05/01/2011, at 9:20 AM, Jeremy Selan wrote:

Whoa!!

These are beautiful docs, thanks for the hard work! The pdf is
awesome, as is the html. I think this will inspire me to keep all
docstrings up to date from now on.

;) Thats good.

When I run with your checkin, I get the following build error.

[ 40%] Building pdf doc
This is pdfeTeX, Version 3.141592-1.21a-2.2 (Web2C 7.5.4)
kpathsea: Running mktexfmt pdflatex.fmt
fmtutil: format directory `/usr/share/texmf/web2c' is not writable.
I can't find the format file `pdflatex.fmt'!
make[2]: *** [docs/CMakeFiles/pdf] Error 1
make[1]: *** [docs/CMakeFiles/pdf.dir/all] Error 2
make: *** [all] Error 2

Unfortunately on my machine I'm not able to make that directory have
write permissions. (and you shouldnt need root to do a 'make') What's
it trying to do? Can we have it do its work locally in the cmake build
dir? I also dont have pdflatex.fmt. in that directory.

It should be trying to do it all in the build/ directory, so this is a little strange.

Does this require users to have additional libraries on their system?

I have included all the additional libs for the html build, and install of Latex is the only external dep needed for the tex -> pdf build.

Should we make the docs building optional? (If your system doesnt have
the right libraries, it skips?)

It should skip the pdf build if Latex isn't found, but it seems it's found on your system and doesn't work.

Can you print out ${PDFLATEX_COMPILER} in the docs/CMakeLists.txt? Is this on OSX or Linux? It worked out of the box on debian 'testing' and centos.

On osx I have latex installed as part of the MacTex disto http://www.tug.org/mactex/ (1.6G eek?) To get cmake to pick it up I just set the PATH env to include '/usr/local/texlive/2010/bin/universal-darwin'

I was thinking of adding the 'doc' and 'pdf' targets just to the 'install' target so that they don't get triggered all the time when you call 'make'. You will still be able to call 'make doc | pdf' if your working on the doc strings.

I recon the 'pdf' target should be enabled by an option if it continues to be flaky.

I haven't added it yet but sphinx should be able to use autodoc to create the docs automagically for the python binding.

.malcolm

-- Jeremy


On Mon, Jan 3, 2011 at 11:14 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

I have added sphinx OCIO document generation using sphinx, there are demo's of the output here:

html - http://malcolmhumphreys.github.com/OpenColorIO/
pdf - http://malcolmhumphreys.github.com/OpenColorIO/OpenColorIO.pdf

There are some still some issues with the index generation which needs to be fixed in the sphinx c++ domain.

I have only spent a little time reformatting the docs and have spent most of the energy getting all the bits together. If you want the pdf target to build you need to install pdflatex.

I chose the 'haiku' for the html this can be customised easily, see: http://sphinx.pocoo.org/latest/theming.html

Message:
Added html and pdf documentation generation using sphinx (http://sphinx.pocoo.org/)

Added ExtractRstFromSource.py which exports specially tagged comments in headers for sphinx to pickup. eg.

--snip--
//!cpp:class:: this is some super informative
// docs
class SomeClass
{
public:
//!cpp:function:: the class namespace should still get set correctly
void foobar2();
};
--snip--

Also fixed the install crud which Closes [#50]

Commit:
https://github.com/malcolmhumphreys/OpenColorIO/commit/1c7b8aab4c9daa427ecd8edb690672dbb0bee6a9

.malcolm

Whoa!!

These are beautiful docs, thanks for the hard work! The pdf is
awesome, as is the html. I think this will inspire me to keep all
docstrings up to date from now on.

When I run with your checkin, I get the following build error.

[ 40%] Building pdf doc
This is pdfeTeX, Version 3.141592-1.21a-2.2 (Web2C 7.5.4)
kpathsea: Running mktexfmt pdflatex.fmt
fmtutil: format directory `/usr/share/texmf/web2c' is not writable.
I can't find the format file `pdflatex.fmt'!
make[2]: *** [docs/CMakeFiles/pdf] Error 1
make[1]: *** [docs/CMakeFiles/pdf.dir/all] Error 2
make: *** [all] Error 2

Unfortunately on my machine I'm not able to make that directory have
write permissions. (and you shouldnt need root to do a 'make') What's
it trying to do? Can we have it do its work locally in the cmake build
dir? I also dont have pdflatex.fmt. in that directory.

Does this require users to have additional libraries on their system?
Should we make the docs building optional? (If your system doesnt have
the right libraries, it skips?)

-- Jeremy


On Mon, Jan 3, 2011 at 11:14 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

I have added sphinx OCIO document generation using sphinx, there are demo's of the output here:

html - http://malcolmhumphreys.github.com/OpenColorIO/
pdf - http://malcolmhumphreys.github.com/OpenColorIO/OpenColorIO.pdf

There are some still some issues with the index generation which needs to be fixed in the sphinx c++ domain.

I have only spent a little time reformatting the docs and have spent most of the energy getting all the bits together. If you want the pdf target to build you need to install pdflatex.

I chose the 'haiku' for the html this can be customised easily, see: http://sphinx.pocoo.org/latest/theming.html

Message:
Added html and pdf documentation generation using sphinx (http://sphinx.pocoo.org/)

Added ExtractRstFromSource.py which exports specially tagged comments in headers for sphinx to pickup. eg.

--snip--
//!cpp:class:: this is some super informative
// docs
class SomeClass
{
public:
   //!cpp:function:: the class namespace should still get set correctly
   void foobar2();
};
--snip--

Also fixed the install crud which Closes [#50]

Commit:
https://github.com/malcolmhumphreys/OpenColorIO/commit/1c7b8aab4c9daa427ecd8edb690672dbb0bee6a9

.malcolm

I have added sphinx OCIO document generation using sphinx, there are demo's of the output here:

html - http://malcolmhumphreys.github.com/OpenColorIO/
pdf - http://malcolmhumphreys.github.com/OpenColorIO/OpenColorIO.pdf

There are some still some issues with the index generation which needs to be fixed in the sphinx c++ domain.

I have only spent a little time reformatting the docs and have spent most of the energy getting all the bits together. If you want the pdf target to build you need to install pdflatex.

I chose the 'haiku' for the html this can be customised easily, see: http://sphinx.pocoo.org/latest/theming.html

Message:
Added html and pdf documentation generation using sphinx (http://sphinx.pocoo.org/)

Added ExtractRstFromSource.py which exports specially tagged comments in headers for sphinx to pickup. eg.

--snip--
//!cpp:class:: this is some super informative
// docs
class SomeClass
{
public:
//!cpp:function:: the class namespace should still get set correctly
void foobar2();
};
--snip--

Also fixed the install crud which Closes [#50]

Commit:
https://github.com/malcolmhumphreys/OpenColorIO/commit/1c7b8aab4c9daa427ecd8edb690672dbb0bee6a9

.malcolm

The new 'Context' class is now exposed in python, and can be passed as
an optional kwarg in config.getProcessor().

Issue:
http://github.com/imageworks/OpenColorIO/issues#issue/47

Commit:
http://github.com/jeremyselan/OpenColorIO/commit/a7824a989b3db17fa59307b670763ba82c284f4b

-- Jeremy

LGTM

On 28/12/2010, at 8:57 AM, Jeremy Selan wrote:

Thanks, I've addressed all comments and rolled it into master.

'Default' seems a little redundant in this names. config.getDefaultContext() vs config.getCurrentContext()
'Config' also seems redundant in these names. config.getConfigRootDir() vs config.getWorkingDir()
'context' varies it's position in few of the argument list.

Fixed.

What would cause the g_fileExistsCache to be flushed? I could see this could be problematic if a client app was running for a while and files started to change on disk.

Currently, nothing causes it to be flushed. Adding a library-wide
"flush cache" call is at the top of my todo list. (It was already
written up as github issue #46)

In the future is the idea to add a section into the yaml which LoadEnvironmentVariables() can use to limit which variables it looks up?

When I started adding the code, there didnt seem to be a clear
advantage to adding complexity by requiring all env-vars to be
pre-declared. Any advantages you can think of? Only one that comes
to my mind would be a place to add a default value, if the value is
not otherwise defined in the environment.

I guess I was mainly thinking about speed, I have seen people create a hell of a lot of environment variables. But your right we can add that complexity later when / if that becomes a problem.

.malcolm

-- Jeremy

On Fri, Dec 24, 2010 at 3:31 AM, Malcolm Humphreys
<malcolmh...@...> wrote:

I really like most of this change, I have a few comments which are mostly cosmetic.

'Default' seems a little redundant in this names.
config.getDefaultContext() vs config.getCurrentContext()
config.getDefaultSearchPath() vs config.getSearchPath()
config.setDefaultSearchPath() vs config.setSearchPath()

'Config' also seems redundant in these names.
config.getConfigRootDir() vs config.getWorkingDir()
config.setConfigRootDir() vs config.setWorkingDir()

'context' varies it's position in few of the argument list. It would be nice if this was more consistent. ie. BuildColorSpaceOps(ops, config, context, src, dst) vs getProcessor(src, dst, context)

It feels better to me to change getProcessor to (context, src, dst) but either way would work.

Can we rename GetPreppedSearchPath to something like ResolveSearchPaths?

What would cause the g_fileExistsCache to be flushed? I could see this could be problematic if a client app was running for a while and files started to change on disk.

In the future is the idea to add a section into the yaml which LoadEnvironmentVariables() can use to limit which variables it looks up?
eg.
context:
JOB: None
SEQ: common
SHOT: None

.malcolm

On 24/12/2010, at 9:06 AM, Jeremy Selan wrote:

Commits:
1a54ec8d76

Resolves Issue #16:
http://github.com/imageworks/OpenColorIO/issues#issue/16

This commit enables color transforms to leverage environment variables
+ search paths to determine the lut location. This will enable
workflows where the looks changes across different
shots/sequences/etc. For apps which work in the context of a single
shot, no changes are required. And, for apps which span multiple
shots (such as a playback utility), a mechanism is provided to update
the variables at runtime.

We introduce a new class, OCIO::Context, that is responsible for all
of the work involved in turning a short filename into full path. (it
handles both envvar interpolation and search paths). Profiles, on
construction, automatically get a default context which contains the
full environment. New versions of getProcessor have been added that
take a context as an argument, allowing the default context to be
overridden at runtime. If no context is provided, the default is
used.

-- Jeremy

Issue:
http://github.com/imageworks/OpenColorIO/issues#issue/48

Commit:
http://github.com/jeremyselan/OpenColorIO/commit/a0fb960d0e8b083acc4bd18048d24ac8e49f99ad

All the classes that relied on the m_impl pattern were inadvertently
working around const correctness. This fixes the issue.

-- Jeremy

Thanks, I've addressed all comments and rolled it into master.

'Default' seems a little redundant in this names. config.getDefaultContext() vs config.getCurrentContext()
'Config' also seems redundant in these names. config.getConfigRootDir() vs config.getWorkingDir()
'context' varies it's position in few of the argument list.

Fixed.

What would cause the g_fileExistsCache to be flushed? I could see this could be problematic if a client app was running for a while and files started to change on disk.

Currently, nothing causes it to be flushed. Adding a library-wide
"flush cache" call is at the top of my todo list. (It was already
written up as github issue #46)

In the future is the idea to add a section into the yaml which LoadEnvironmentVariables() can use to limit which variables it looks up?

When I started adding the code, there didnt seem to be a clear
advantage to adding complexity by requiring all env-vars to be
pre-declared. Any advantages you can think of? Only one that comes
to my mind would be a place to add a default value, if the value is
not otherwise defined in the environment.

-- Jeremy

On Fri, Dec 24, 2010 at 3:31 AM, Malcolm Humphreys
<malcolmh...@...> wrote:

I really like most of this change, I have a few comments which are mostly cosmetic.

'Default' seems a little redundant in this names.
config.getDefaultContext() vs config.getCurrentContext()
config.getDefaultSearchPath() vs config.getSearchPath()
config.setDefaultSearchPath() vs config.setSearchPath()

'Config' also seems redundant in these names.
config.getConfigRootDir() vs config.getWorkingDir()
config.setConfigRootDir() vs config.setWorkingDir()

'context' varies it's position in few of the argument list. It would be nice if this was more consistent. ie. BuildColorSpaceOps(ops, config, context, src, dst) vs getProcessor(src, dst, context)

It feels better to me to change getProcessor to (context, src, dst) but either way would work.

Can we rename GetPreppedSearchPath to something like ResolveSearchPaths?

What would cause the g_fileExistsCache to be flushed? I could see this could be problematic if a client app was running for a while and files started to change on disk.

In the future is the idea to add a section into the yaml which LoadEnvironmentVariables() can use to limit which variables it looks up?
eg.
context:
 JOB: None
 SEQ: common
 SHOT: None

.malcolm

On 24/12/2010, at 9:06 AM, Jeremy Selan wrote:

Commits:
1a54ec8d76

Resolves Issue #16:
http://github.com/imageworks/OpenColorIO/issues#issue/16

This commit enables color transforms to leverage environment variables
+ search paths to determine the lut location.  This will enable
workflows where the looks changes across different
shots/sequences/etc.   For apps which work in the context of a single
shot, no changes are required.  And, for apps which span multiple
shots (such as a playback utility), a mechanism is provided to update
the variables at runtime.

We introduce a new class, OCIO::Context, that is responsible for all
of the work involved in turning a short filename into full path. (it
handles both envvar interpolation and search paths).  Profiles, on
construction, automatically get a default context which contains the
full environment.   New versions of getProcessor have been added that
take a context as an argument, allowing the default context to be
overridden at runtime.  If no context is provided, the default is
used.

-- Jeremy

I really like most of this change, I have a few comments which are mostly cosmetic.

'Default' seems a little redundant in this names.
config.getDefaultContext() vs config.getCurrentContext()
config.getDefaultSearchPath() vs config.getSearchPath()
config.setDefaultSearchPath() vs config.setSearchPath()

'Config' also seems redundant in these names.
config.getConfigRootDir() vs config.getWorkingDir()
config.setConfigRootDir() vs config.setWorkingDir()

'context' varies it's position in few of the argument list. It would be nice if this was more consistent. ie. BuildColorSpaceOps(ops, config, context, src, dst) vs getProcessor(src, dst, context)

It feels better to me to change getProcessor to (context, src, dst) but either way would work.

Can we rename GetPreppedSearchPath to something like ResolveSearchPaths?

What would cause the g_fileExistsCache to be flushed? I could see this could be problematic if a client app was running for a while and files started to change on disk.

In the future is the idea to add a section into the yaml which LoadEnvironmentVariables() can use to limit which variables it looks up?
eg.
context:
JOB: None
SEQ: common
SHOT: None

.malcolm

Commits:
1a54ec8d76

Resolves Issue #16:
http://github.com/imageworks/OpenColorIO/issues#issue/16

This commit enables color transforms to leverage environment variables
+ search paths to determine the lut location. This will enable
workflows where the looks changes across different
shots/sequences/etc. For apps which work in the context of a single
shot, no changes are required. And, for apps which span multiple
shots (such as a playback utility), a mechanism is provided to update
the variables at runtime.

We introduce a new class, OCIO::Context, that is responsible for all
of the work involved in turning a short filename into full path. (it
handles both envvar interpolation and search paths). Profiles, on
construction, automatically get a default context which contains the
full environment. New versions of getProcessor have been added that
take a context as an argument, allowing the default context to be
overridden at runtime. If no context is provided, the default is
used.

-- Jeremy

Thanks, I'm going to check this in then.

-- Jeremy

On Wed, Dec 22, 2010 at 3:07 PM, Malcolm Humphreys
<malcolmh...@...> wrote:

On 23/12/2010, at 5:38 AM, Jeremy Selan wrote:

Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks.  You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code.   Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally.   If we use an isValid call, would it return the tuple
"(bool, str)"?  People would probably expect it to just return a bool.

yeah I guess you would either need to do that or have some config.getErrorMsg() function.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird).  How about 'validate', instead?

No I think it's ok esp if people are used to the ilm code.

The ilm code does  call header.sanityCheck when using InputFile (), OutputFile(), TiledInputFile(), TiledOutputFile().

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.

I would say throwing inside sanityCheck is fine then.

If you think having a pointer to a profile which has problems is needed for something else then not throwing from in GetCurrentConfig() is also ok.

.malcolm

On 23/12/2010, at 5:38 AM, Jeremy Selan wrote:

Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks. You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code. Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally. If we use an isValid call, would it return the tuple
"(bool, str)"? People would probably expect it to just return a bool.

yeah I guess you would either need to do that or have some config.getErrorMsg() function.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird). How about 'validate', instead?

No I think it's ok esp if people are used to the ilm code.

The ilm code does call header.sanityCheck when using InputFile (), OutputFile(), TiledInputFile(), TiledOutputFile().

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.

I would say throwing inside sanityCheck is fine then.

If you think having a pointer to a profile which has problems is needed for something else then not throwing from in GetCurrentConfig() is also ok.

.malcolm

Either of our approaches will work, of course, but I'm still inclined
to go with the exception route.

A bunch of calls in OCIO can throw exceptions (the common one being
config->getProcessor(...) ), so most of the time you'll already have
try / catch blocks. You can usually thus add a "sanityCheck" call in
a single extra line.

If we instead added: bool isValid(const char * msg)
this requires a separate code block to explicitly check it, probably 3
or 4 lines of code. Which isn't a problem, just an observation.

I'm also not the biggest fan of functions with two return arguments.
Consider the python API... If we go the exception route, it ports
naturally. If we use an isValid call, would it return the tuple
"(bool, str)"? People would probably expect it to just return a bool.

Perhaps you object to the name 'sanityCheck'? (Which I admit is a bit
weird). How about 'validate', instead?

OCIO::GetCurrentConfig() will always return a valid profile object.
It currently will throw an exception only iff $OCIO is defined, but
points to a profile that has parse problems.

I'd prefer to not enforce 'auto validation' after loading at the library level.

-- Jeremy