AcademySoftwareFoundation / OpenColorIO

A color management framework for visual effects and animation.
https://opencolorio.org
BSD 3-Clause "New" or "Revised" License
1.78k stars 452 forks source link

API won't write the environment section if config version is set to 1.0 #1903

Closed kenmcgaugh closed 10 months ago

kenmcgaugh commented 11 months ago

When serializing a config via the API, the environment section is omitted if the config's version is set to 1.0. However the environment section is supported in OpenColorIO-1.0.9 (the version currently used in our pipeline). I believe the offending line is here:

https://github.com/AcademySoftwareFoundation/OpenColorIO/blob/b94a184e4b66ca9dff72eca13fb1e7a3d0ce65f7/src/OpenColorIO/OCIOYaml.cpp#L4740

remia commented 11 months ago

We also faced the same issue, our workaround for now has been to simply add the environment back manually after the .ocio file is generated (which is trivial to do). I believe there was a discussion on Slack a while back about this issue but it is now long gone unfortunately. I remember mention of backward compatibility issues, @doug-walker do you remember anything?

kenmcgaugh commented 11 months ago

That is also my current workaround. I serialise to a string and then insert the environment settings before writing it to disk.

doug-walker commented 11 months ago

Here is a relevant section from OpenColorIO.h:

     * \brief The EnvironmentMode controls the behavior of loadEnvironment.
     *  * ENV_ENVIRONMENT_LOAD_PREDEFINED - Only update vars already added to the Context.
     *  * ENV_ENVIRONMENT_LOAD_ALL        - Load all env. vars into the Context.
     *
     * \note Loading ALL the env. vars may reduce performance and reduce cache efficiency.
     *
     * Client programs generally will not use these methods because the EnvironmentMode is
     * set automatically when a Config is loaded.  If the Config has an "environment"
     * section, the mode is set to LOAD_PREDEFINED, and otherwise set to LOAD_ALL.

Note that if the environment section exists, it must list all of the context vars used in the config.

For v2 configs, it is essentially always in LOAD_PREDEFINED mode because serialization always includes the environment section. We felt this was important for both performance and readability of configs.

We did not want to impose this on v1 configs and so the intent was to not write an empty environment section in that case. But unfortunately the code was sloppy and did not check to see if the environment had values in it, in which case it should be written. We will fix that. It was quite rare to use the environment section with v1 configs, which is probably why no one has flagged it so far.