Adsk Contrib - Improve file rules for v1 configs

Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com>

docs/guides/authoring/overview.rst

@@ -319,40 +319,40 @@ to use to control other types of tasks not listed below.
 
 * ``color_picking`` - colors in a color-selection UI can be displayed
   in this space, while selecting colors in a different working space
-  (e.g. ``scene_linear`` or ``texture_paint``)
+  (e.g. ``scene_linear`` or ``texture_paint``).
 
 * ``color_timing`` - color space used for applying color corrections,
   e.g. user-specified grade within an image viewer (if the application
   uses the ``DisplayTransform::setDisplayCC`` API method)
 
 * ``compositing_log`` - a log color space used for certain processing
   operations (plate resizing, pulling keys, degrain, etc). Used by the
-  OCIOLogConvert Nuke node
+  OCIOLogConvert Nuke node.
 
 * ``data`` - used when writing data outputs such as normals, depth
   data, and other "non color" data. The color space in this role should
   typically have ``data: true`` specified, so no color transforms are
-  applied
+  applied.
 
 * ``default`` - when ``strictparsing: false``, this color space is used
-  as a fallback. If not defined, the ``scene_linear`` role is used
+  as a fallback.
 
 * ``matte_paint`` - color space which matte-paintings are created in
   (for more information, :ref:`see the guide on baking ICC profiles
   for Photoshop <userguide-bakelut-photoshop>`, and
-  :ref:`config-spivfx`)
+  :ref:`config-spivfx`).
 
 * ``reference`` - the color space against which the other color spaces
-  are defined
+  are defined.
 
 .. note::
    The reference role has sometimes been misinterpreted as being the
    space in which "reference art" is stored in.
 
 * ``scene_linear`` - the scene-referred linear-to-light color space,
-  often the same as the reference space (see:ref:`faq-terminology`)
+  often the same as the reference space (see:ref:`faq-terminology`).
 
 * ``texture_paint`` - similar to ``matte_paint`` but for painting
   textures for 3D objects (see the description of texture painting in
-  :ref:`SPI's pipeline <config-spipipeline-texture>`)
+  :ref:`SPI's pipeline <config-spipipeline-texture>`).
 

docs/guides/authoring/rules.rst

@@ -115,7 +115,7 @@ determine the colorspace ``lnf`` (it being the right-most substring
 containing a colorspace name)
 
 However, if the colorspace cannot be determined and ``strictparsing:
-true``, it will produce an error.
+true``, it will return an empty string.
 
 If the colorspace cannot be determined and ``strictparsing: false``,
 the default role will be used. This allows unhandled images to operate

include/OpenColorIO/OpenColorIO.h

@@ -980,7 +980,8 @@ class OCIOEXPORT Config
      */
     void setFileRules(ConstFileRulesRcPtr fileRules);
 
-    ///  Get the color space of the first rule that matched filePath.
+    /// Get the color space of the first rule that matched filePath. (For v1 configs, this is
+    /// equivalent to calling parseColorSpaceFromString with strictparsing set to false.)
     const char * getColorSpaceFromFilepath(const char * filePath) const;
 
     /**
@@ -1009,7 +1010,7 @@ class OCIOEXPORT Config
      * * If strict parsing is disabled, return ROLE_DEFAULT (if defined).
      * * If the default role is not defined, return an empty string.
      */
-    OCIO_DEPRECATED("This is now deprecated, please use Config::getColorSpaceFromFilepath().")
+    OCIO_DEPRECATED("This is now deprecated starting at v2.0, please use Config::getColorSpaceFromFilepath().")
     const char * parseColorSpaceFromString(const char * str) const;
 
     bool isStrictParsingEnabled() const;
@@ -1178,6 +1179,10 @@ extern OCIOEXPORT std::ostream& operator<< (std::ostream&, const Config&);
  * Getters and setters are using the rule position, they will throw if the position is not
  * valid. If the rule at the specified position does not implement the requested property
  * getter will return NULL and setter will throw.
+ *
+ * When loading a v1 config, a set of FileRules are created with ColorSpaceNamePathSearch followed
+ * by the Default rule pointing to the default role. This allows getColorSpaceFromFilepath to emulate
+ * OCIO v1 code that used parseColorSpaceFromString with strictparsing set to false.
  */
 
 class OCIOEXPORT FileRules

src/OpenColorIO/Config.cpp

@@ -192,24 +192,6 @@ void GetColorSpaceReferences(std::set<std::string> & colorSpaceNames,
     }
 }
 
-static constexpr char AddedDefault[]{ "added_default_rule_colorspace" };
-
-void FindAvailableName(const ColorSpaceSetRcPtr & colorspaces, std::string & csname)
-{
-    int i = 0;
-    csname = AddedDefault;
-    while (true)
-    {
-        if (!colorspaces->hasColorSpace(csname.c_str()))
-        {
-            break;
-        }
-
-        csname = AddedDefault + std::to_string(i);
-        ++i;
-    }
-}
-
 // Views are stored in two vectors of objects, using pointers to temporarily group them.
 typedef std::vector<const View *> ViewPtrVec;
 
@@ -591,56 +573,6 @@ class Config::Impl
 
     static ConstConfigRcPtr Read(std::istream & istream, const char * filename);
 
-    // Upgrade from v1 to v2.
-    void upgradeFromVersion1ToVersion2() noexcept
-    {
-        // V2 adds file_rules and these require a default rule. We try to initialize the default
-        // rule using the default role. If the default role doesn't exist, we look for a Raw
-        // ColorSpace with isdata true. If that is not found either, we add new ColorSpace
-        // named "added_default_rule_colorspace".
-
-        m_majorVersion = 2;
-        m_minorVersion = 0;
-
-        const char * rname = LookupRole(m_roles, ROLE_DEFAULT);
-        if (!hasColorSpace(rname))
-        {
-            std::string defaultCS;
-            bool addNewDefault = true;
-            // The default role doesn't exist so look for a color space named "raw"
-            // (not case-sensitive) with isdata true.
-            const int csindex = getColorSpaceIndex("raw");
-            if (-1 != csindex)
-            {
-                auto cs = m_allColorSpaces->getColorSpaceByIndex(csindex);
-                if (cs->isData())
-                {
-                    // "Raw" color space can be used for default.
-                    addNewDefault = false;
-                    defaultCS = cs->getName();
-                }
-            }
-
-            if (addNewDefault)
-            {
-                FindAvailableName(m_allColorSpaces, defaultCS);
-                auto newCS = ColorSpace::Create();
-                newCS->setName(defaultCS.c_str());
-                newCS->setIsData(true);
-                m_allColorSpaces->addColorSpace(newCS);
-                // Put the added CS in the inactive list to avoid it showing up in user menus.
-                if (!m_inactiveColorSpaceNamesConf.empty())
-                {
-                    m_inactiveColorSpaceNamesConf += ",";
-                }
-                m_inactiveColorSpaceNamesConf += defaultCS;
-                setInactiveColorSpaces(m_inactiveColorSpaceNamesConf.c_str());
-            }
-
-            m_fileRules->setColorSpace(m_fileRules->getNumEntries() - 1, defaultCS.c_str());
-        }
-    }
-
     // Validate view object that can be a config defined shared view or a display-defined view.
     void validateView(const std::string & display, const View & view, bool checkUseDisplayName) const
     {
@@ -1269,15 +1201,19 @@ void Config::upgradeToLatestVersion() noexcept
     {
         if (wasVersion == 1)
         {
-            m_impl->upgradeFromVersion1ToVersion2();
+            UpdateFileRulesFromV1ToV2(*this, m_impl->m_fileRules);
+
+            // The instance version is now 2.0
+            m_impl->m_majorVersion = 2;
+            m_impl->m_minorVersion = 0;
         }
+
         static_assert(LastSupportedMajorVersion == 2, "Config: Handle newer versions");
         setMajorVersion(LastSupportedMajorVersion);
         setMinorVersion(LastSupportedMinorVersion[LastSupportedMajorVersion - 1]);
     }
 }
 
-
 ConfigRcPtr Config::createEditableCopy() const
 {
     ConfigRcPtr config = Config::Create();
@@ -1757,26 +1693,17 @@ void Config::validate() const
 
     ///// FileRules
 
-    // All Config objects have a fileRules object, regardless of version. This object is
-    // initialized to have a defaultRule with the color space set to "default" (i.e., the default
-    // role). The fileRules->validate call will validate that all color spaces used in rules
-    // exist, or if they are roles that they point to a color space that exists. Because this would
-    // cause validate to improperly fail on v1 configs (since they are not required to actually
-    // contain file rules), we don't do this check on v1 configs when there is only one rule.
-    if (getMajorVersion() >= 2 || getImpl()->m_fileRules->getNumEntries() != 1)
+    try
     {
-        try
-        {
-            getImpl()->m_fileRules->getImpl()->validate(*this);
-        }
-        catch (const Exception & e)
-        {
-            std::ostringstream os;
-            os << "Config failed validation. File rules failed with: ";
-            os << e.what();
-            getImpl()->m_validationtext = os.str();
-            throw Exception(getImpl()->m_validationtext.c_str());
-        }
+        getImpl()->m_fileRules->getImpl()->validate(*this);
+    }
+    catch (const Exception & e)
+    {
+        std::ostringstream os;
+        os << "Config failed validation. File rules failed with: ";
+        os << e.what();
+        getImpl()->m_validationtext = os.str();
+        throw Exception(getImpl()->m_validationtext.c_str());
     }
 
     ///// Resolve all file Transforms using context variables.
@@ -4718,7 +4645,7 @@ void Config::Impl::checkVersionConsistency() const
 
     // Check for the file rules.
 
-    if (m_majorVersion < 2 && m_fileRules->getNumEntries() > 1)
+    if (m_majorVersion < 2 && m_fileRules->getNumEntries() > 2)
     {
         throw Exception("Only version 2 (or higher) can have file rules.");
     }

src/OpenColorIO/FileRules.cpp

@@ -11,6 +11,7 @@
 
 #include "CustomKeys.h"
 #include "FileRules.h"
+#include "Logging.h"
 #include "PathUtils.h"
 #include "Platform.h"
 #include "utils/StringUtils.h"
@@ -662,9 +663,23 @@ void FileRules::Impl::moveRule(size_t ruleIndex, int offset)
 
 void FileRules::Impl::validate(const Config & cfg) const
 {
-    for (auto & rule : m_rules)
+    // All Config objects have a fileRules object, regardless of version. This object is
+    // initialized to have a defaultRule with the color space set to "default" (i.e., the default
+    // role). The fileRules->validate call will validate that all color spaces used in rules
+    // exist, or if they are roles that they point to a color space that exists.
+    //
+    // Because this would cause validate to improperly fail on v1 configs (since they are not
+    // required to actually contain file rules), we don't do this check on v1 configs when there is
+    // only two rules. In some case (e.g. load a v1 config from disk), the two expected rules are
+    // the 'Default' and 'ColorSpaceNamePathSearch' ones.
+
+    if (cfg.getMajorVersion() >= 2
+            || (cfg.getMajorVersion() == 1 && m_rules.size() > 2))
     {
-        rule->validate(cfg);
+        for (auto & rule : m_rules)
+        {
+            rule->validate(cfg);
+        }
     }
 }
 
@@ -940,5 +955,96 @@ std::ostream & operator<< (std::ostream & os, const FileRules & fr)
     }
     return os;
 }
+
+void UpdateFileRulesFromV1ToV2(const Config & config, FileRulesRcPtr & fileRules)
+{
+    if (config.getMajorVersion() != 1)
+    {
+        return;
+    }
+
+    // In order to preserve the v1 behavior using Config:getColorSpaceFromFilepath() (i.e.
+    // mimic the Config::parseColorSpaceFromString() behavior) add the file path search
+    // rule to the list of file rules.
+
+    try
+    {
+        // Throws if the file rule does not exist.
+        fileRules->getIndexForRule(FileRules::FilePathSearchRuleName);
+    }
+    catch(const Exception & /* ex */)
+    {
+        fileRules->insertPathSearchRule(0);
+    }
+
+    // Now, double-check the default rule (which is using the default role) to find the
+    // right alternative if the default role is missing.
+
+    // In order to always return a valid color space, the algorithm for the default rule is:
+    //   1. Use the default role if it exists (i.e. that's the default implementation)
+    //   2. Use the "raw" color space (case insensitive) if it exists & is a 'data' color space
+    //   3. Use the first 'data' color space if one exists
+    //   4. Use the first active color space
+    //   5. finally, fallback to the first color space.
+
+    auto defaultCS = config.getColorSpace(ROLE_DEFAULT);
+
+    if (!defaultCS)
+    {
+        ConstColorSpaceRcPtr cs = config.getColorSpace("raw");
+        if (cs && cs->isData())
+        {
+            fileRules->setColorSpace(1, cs->getName());
+        }
+        else
+        {
+            const int numColorSpaces
+                = config.getNumColorSpaces(SEARCH_REFERENCE_SPACE_SCENE, COLORSPACE_ALL);
+
+            bool found = false;
+            for (int idx = 0; idx < numColorSpaces && !found; ++idx)
+            {
+                const char * csName 
+                    = config.getColorSpaceNameByIndex(SEARCH_REFERENCE_SPACE_SCENE,
+                                                      COLORSPACE_ALL,
+                                                      idx);
+                ConstColorSpaceRcPtr cs = config.getColorSpace(csName);
+
+                if (cs->isData())
+                {
+                    fileRules->setColorSpace(1, csName);
+                    found = true;
+                }
+            }
+
+            if (!found)
+            {
+                if (config.getNumColorSpaces() > 0)
+                {
+                    // Take the first active color space.
+                    fileRules->setColorSpace(1, config.getColorSpaceNameByIndex(0));
+                }
+                else
+                {
+                    static constexpr char msg[]
+                        = "The default rule creation fallbacks to the first color space because "\
+                          "no suitable color space exists.";
+
+                    LogWarning(msg);
+
+                    // Take the first available color space.
+                    const char * csName
+                        = config.getColorSpaceNameByIndex(SEARCH_REFERENCE_SPACE_SCENE,
+                                                          COLORSPACE_ALL,
+                                                          0);
+
+                    fileRules->setColorSpace(1, csName);
+                }
+            }
+        }
+    }
+}
+
+
 } // namespace OCIO_NAMESPACE
 

src/OpenColorIO/FileRules.h

@@ -69,6 +69,12 @@ class FileRules::Impl
     std::vector<FileRuleRcPtr> m_rules;
 };
 
+
+// Helper method to build valid v2 file rules from a v1 config. Note that it does not change
+// the config instance version.
+void UpdateFileRulesFromV1ToV2(const Config & config, FileRulesRcPtr & fileRules);
+
+
 } // namespace OCIO_NAMESPACE
 
 #endif