Adsk Contrib - Improve File Rules support for v1 configs (#1417)

* Adsk Contrib - Improve file rules for v1 configs Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Adsk Contrib - Emergency GPU build fix (#1391) Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Adsk Contrib - Fix some bugs found by Maya and SonarCloud (#1403) * Adsk Contrib - Fix some bugs found by Maya and SonarCloud Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Fix a Windows warning Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * After Effects and Photoshop plug-in updates (#1373) * Set Mac OS deployment target to 10.10 Signed-off-by: Brendan Bolles <brendan@fnordware.com> * More 10.10 targets Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Ditch ADD_EXTRA_BUILTINS Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Getting strange callback when AE effect is pasted Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Use NDEBUG in AE project Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Disable radio buttons with no config Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Update version Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Move to AE 2021 SDK, set PF_OutFlag2_MUTABLE_RENDER_SEQUENCE_DATA_SLOWER Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Define OCIO_DEPRECATED in After Effects builds Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Verify change from color space menu Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Handle situation where color space has a '/' in it Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Windows family separator fix Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Invert everything Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Invert everything Windows Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Disable more controls Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Incorporate invert into LUT export Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Get rid of fullPaths stuff Signed-off-by: Brendan Bolles <brendan@fnordware.com> * Fix changed config settings retention Signed-off-by: Brendan Bolles <brendan@fnordware.com> Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Add OpenColorIOConfig.cmake generation (#1397) * Initial OpenColorIO Config CMake implementation Signed-off-by: Rémi Achard <remiachard@gmail.com> * Remove macro, improve documentation Signed-off-by: Rémi Achard <remiachard@gmail.com> Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Use CMake visibility flags (#1411) * Update install doc minimal version numbers Signed-off-by: Rémi Achard <remiachard@gmail.com> * Use CMake visibility presets variables Signed-off-by: Rémi Achard <remiachard@gmail.com> * Fix python package case Signed-off-by: Rémi Achard <remiachard@gmail.com> * Allow advanced user to override symbol visibility Signed-off-by: Rémi Achard <remiachard@gmail.com> Co-authored-by: Patrick Hodoul <patrick.hodoul@autodesk.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Fix Windows build Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Update include/OpenColorIO/OpenColorIO.h Co-authored-by: Michael Dolan <michdolan@gmail.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Update src/OpenColorIO/FileRules.cpp Co-authored-by: Michael Dolan <michdolan@gmail.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Hide dependency symbol visibility (#1409) (#1416) When creating libOpenColorIO.so, we lacked the linker commands that hide symbol visibility from the dependent libraries, which is necessary to prevent OCIO from exporting the symbols from Expat and the other dependencies that OCIO needs to use internally but is not trying intentionally to expose via its API. Failing to do this can result in symbol clashes and all sorts of subtle errors if OCIO is used in an app that also uses and is linked against a potentially different version of Expat (or any of the other deps). Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Adsk Contrib - Improve DX11 & Cg fragment shader language support (#1406) * Adsk Contrib - Improve DX11 support Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Improve Cg support Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Improve GPU comments Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> * Fix unit test failures Signed-off-by: Patrick Hodoul <Patrick.Hodoul@autodesk.com> Co-authored-by: Brendan Bolles <brendan@fnordware.com> Co-authored-by: Rémi Achard <remiachard@gmail.com> Co-authored-by: Michael Dolan <michdolan@gmail.com> Co-authored-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: doug-walker <43830961+doug-walker@users.noreply.github.com>
AcademySoftwareFoundation · Jul 6, 2021 · 76ddf44 · 76ddf44
1 parent a36a5be
commit 76ddf44
Show file tree

Hide file tree

Showing 10 changed files with 678 additions and 186 deletions.
diff --git a/docs/guides/authoring/overview.rst b/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>`).
 
diff --git a/docs/guides/authoring/rules.rst b/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

diff --git a/include/OpenColorIO/OpenColorIO.h b/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 was marked as deprecated starting in 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

diff --git a/src/OpenColorIO/Config.cpp b/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
     {
@@ -1274,15 +1206,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();
@@ -1762,26 +1698,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.
@@ -4723,7 +4650,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.");
     }

diff --git a/src/OpenColorIO/FileRules.cpp b/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,95 @@ std::ostream & operator<< (std::ostream & os, const FileRules & fr)
     }
     return os;
 }
-} // namespace OCIO_NAMESPACE
 
+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 falls back 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
diff --git a/src/OpenColorIO/FileRules.h b/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