Adsk Contrib - Enhanced methods to find an equivalent color space between configs#1788
Merged
doug-walker merged 27 commits intoJun 27, 2023
Conversation
…ce and AreProcessorsEquivalent. - Remove some include of Processor.h where it wasn't needed. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…onfig.cpp instead. Fixing identations issues. Fixing styling issues. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Moving the creation of the editable config into the wrapper of isColorSpaceLinear instead of doing it inside the function. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…onfig and one for builtin. Adding back isColorSpaceLinear inside Config.cpp since we need access to getProcessorWithoutCaching. Refactor some of config utils function to return index instead of string. Creating editable config in the two wrappers to prevent processor caching. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
doug-walker
reviewed
Apr 14, 2023
| // That should never happen. | ||
| return -1; | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Lines 1097-1504 were moved to ConfigUtils.cpp, without significant changes. This is the heuristics code and we wanted to move this out of Config.cpp, since this module is already so long and the heuristics are essentially their own somewhat self-contained set of functions.
doug-walker
reviewed
Apr 14, 2023
| getImpl()->setProcessorCacheFlags(flags); | ||
| } | ||
|
|
||
| ////////////////////////////////////////////////////////////////// |
Collaborator
There was a problem hiding this comment.
Lines 4740-4835 is simply a cut/paste move from farther down in this module. This code is from another one of our recent PRs and we decided it belongs better in this location.
Collaborator
|
Looks good to me, do we need Python bindings for the new methods? |
…pace methods. Adding a couple python unit tests to cover the new methods (same unit test as the C++ side, but not all of them) Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…ce and AreProcessorsEquivalent. - Remove some include of Processor.h where it wasn't needed. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…onfig.cpp instead. Fixing identations issues. Fixing styling issues. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Moving the creation of the editable config into the wrapper of isColorSpaceLinear instead of doing it inside the function. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…onfig and one for builtin. Adding back isColorSpaceLinear inside Config.cpp since we need access to getProcessorWithoutCaching. Refactor some of config utils function to return index instead of string. Creating editable config in the two wrappers to prevent processor caching. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
…pace methods. Adding a couple python unit tests to cover the new methods (same unit test as the C++ side, but not all of them) Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
…ttps://github.com/autodesk-forks/OpenColorIO into adsk_contrib/getProcessorToBuiltinCS-enhancements
…ments Signed-off-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com>
…ttps://github.com/autodesk-forks/OpenColorIO into adsk_contrib/getProcessorToBuiltinCS-enhancements
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
remia
approved these changes
May 15, 2023
Contributor
Author
|
The failed workflow is expected as there was an issue with that workflow on the main branch. We merged the fix into the main branch yesterday. |
Collaborator
|
Taking note here that I verified that this PR fixes a bug in GetProcessorFromBuiltinColorSpace reported by a user of OCIO 2.2.1. As a result, we will plan to include this branch in the upcoming 2.2.2 release rather than waiting for 2.3. |
zachlewis
approved these changes
May 24, 2023
Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com>
doug-walker
approved these changes
Jun 27, 2023
Collaborator
|
I'm going to go ahead and merge this since we need it for the config merge feature we're working on. The CI problem on macOS with docs=ON is unrelated to the code in this PR. |
doug-walker
deleted the
adsk_contrib/getProcessorToBuiltinCS-enhancements
branch
doug-walker
added a commit
to autodesk-forks/OpenColorIO
that referenced
this pull request
Dec 6, 2023
…ween configs (AcademySoftwareFoundation#1788) * - Implementation of identifyInterchangeSpace, identifyBuiltinColorSpace and AreProcessorsEquivalent. - Remove some include of Processor.h where it wasn't needed. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Moving code from Config.cpp to ConfigUtils.cpp and using wrapper in Config.cpp instead. Fixing identations issues. Fixing styling issues. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Typo and comments tweaks Moving the creation of the editable config into the wrapper of isColorSpaceLinear instead of doing it inside the function. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Splitting identifyInterchangeSpace into two methods, one for source config and one for builtin. Adding back isColorSpaceLinear inside Config.cpp since we need access to getProcessorWithoutCaching. Refactor some of config utils function to return index instead of string. Creating editable config in the two wrappers to prevent processor caching. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Tentative change to phase out isProcessorEquivalent. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Refactor code and add tests Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Remove unused method Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Remove unused method, tk 2 Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Tweak comments Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Python binding for IdentifyInterchangeSpace and IdentifyBuiltinColorSpace methods. Adding a couple python unit tests to cover the new methods (same unit test as the C++ side, but not all of them) Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * - Implementation of identifyInterchangeSpace, identifyBuiltinColorSpace and AreProcessorsEquivalent. - Remove some include of Processor.h where it wasn't needed. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Moving code from Config.cpp to ConfigUtils.cpp and using wrapper in Config.cpp instead. Fixing identations issues. Fixing styling issues. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Typo and comments tweaks Moving the creation of the editable config into the wrapper of isColorSpaceLinear instead of doing it inside the function. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Splitting identifyInterchangeSpace into two methods, one for source config and one for builtin. Adding back isColorSpaceLinear inside Config.cpp since we need access to getProcessorWithoutCaching. Refactor some of config utils function to return index instead of string. Creating editable config in the two wrappers to prevent processor caching. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Tentative change to phase out isProcessorEquivalent. Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Refactor code and add tests Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Remove unused method Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Remove unused method, tk 2 Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Tweak comments Signed-off-by: Doug Walker <doug.walker@autodesk.com> * Python binding for IdentifyInterchangeSpace and IdentifyBuiltinColorSpace methods. Adding a couple python unit tests to cover the new methods (same unit test as the C++ side, but not all of them) Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Fixing problem with the rebase Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> * Make sure the unit test is using the static method Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> --------- Signed-off-by: Cédrik Fuoco <cedrik.fuoco@autodesk.com> Signed-off-by: Doug Walker <doug.walker@autodesk.com> Signed-off-by: Cédrik Fuoco <105517825+cedrik-fuoco-adsk@users.noreply.github.com> Co-authored-by: Doug Walker <doug.walker@autodesk.com> Signed-off-by: Doug Walker <Doug.Walker@autodesk.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds several new methods to be used to identify a common color space between a pair of configs that may be used as an interchange space. If the official interchange roles are not present, heuristics are used to search for a suitable color space.
IdentifyBuiltinColorSpace-- Searches a user config to find the name of an equivalent color space to a color space in one of the Built-in configs. For example, "What is the name in this config for thesRGB - Texturecolor space from the default Built-in config?"IdentifyInterchangeSpace-- Identifies the pair of color space names to be used as the interchange space to allow conversion between color spaces in a user config and one of the built-in configs. This is the fundamental function that contains the heuristics for trying to identify known color spaces and is used by IdentifyBuiltinColorSpace and in the GetProcessorFrom/ToBuiltinColorSpace methods added in PR #1710.This improves upon and extends upon PR #1710. One piece of feedback on that PR is that it would be helpful to be able to use the heuristics to extract the names of the interchange color spaces rather than a Processor. This gives the application more flexibility in how that information is persisted and may reduce the number of times that any heuristics (that search through a config's color spaces) need to be run.
In addition, following up on the TODO mentioned in PR #1710, we refactored most of the heuristics code to move it out of the already crowded
Config.cppand into the newConfigUtils.cppmodule. The heuristics themselves were not significantly changed.A lot of additional unit tests were added to test more scenarios involving the heuristics.
Finally, an issue was fixed in
GetProcessorFromConfigsso that the returned Processor is a no-op if either of the color spaces in the two configs is a data space.