Add view helper functions to Config class #2083
Conversation
Signed-off-by: annie <[email protected]>
pulling from upstream Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
Signed-off-by: annie <[email protected]>
There was a problem hiding this comment.
Thanks Annie, this is excellent work!
I made a few suggestions below. If you could look at the comments I suggested for OpenColorIO.h and check that there is a test for each aspect of the documented functionality for each new function, that would be appreciated.
I notice that one of the Linux CI runs is failing due to a frozen Python docs problem. Currently the frozen docs system is broken and so we will handle that separately. As long as the Windows and Mac tests are passing, that's all you need to be concerned with for now.
| static bool viewsAreEqual(const ConstConfigRcPtr & first, | ||
| const ConstConfigRcPtr & second, | ||
| const char * dispName, | ||
| const char * viewName); |
There was a problem hiding this comment.
Please use "ViewsAreEqual" since the names of all static methods are capitalized. Same for "VirtualViewsAreEqual".
| @@ -852,13 +852,18 @@ class OCIOEXPORT Config | |||
| * \ref Config::validate will throw if the config does not contain | |||
| * the matching display color space. | |||
| */ | |||
|
|
|||
| bool viewIsShared(const char * dispName, const char * viewName) const; | |||
|
|
|||
| /// Will throw if view or colorSpaceName are null or empty. | |||
| void addSharedView(const char * view, const char * viewTransformName, | |||
| const char * colorSpaceName, const char * looks, | |||
| const char * ruleName, const char * description); | |||
| /// Remove a shared view. Will throw if the view does not exist. | |||
| void removeSharedView(const char * view); | |||
|
|
|||
There was a problem hiding this comment.
/// Clear all shared views. This will throw if any displays are still using the shared views.
There was a problem hiding this comment.
Sorry, I'm just checking - is this meant for clearSharedViews instead of viewIsShared?
There was a problem hiding this comment.
Yes, sorry, I put the comment on the wrong line.
| @@ -883,6 +888,11 @@ class OCIOEXPORT Config | |||
| int getNumViews(const char * display, const char * colorspaceName) const; | |||
| const char * getView(const char * display, const char * colorspaceName, int index) const; | |||
|
|
|||
There was a problem hiding this comment.
/**
* \brief Compare views in a pair of configs.
*
* Will return false if either of the views does not exist. This will return true even
* if the view is display-defined in one config and a reference to a shared view in the
* other config (both within the same display), as long as the contents match. The
* description text (if any) is ignored, since it is not a functional difference.
*/
| @@ -900,6 +910,8 @@ class OCIOEXPORT Config | |||
| /// Returns the description attribute of a (display, view) pair. | |||
| const char * getDisplayViewDescription(const char * display, const char * view) const noexcept; | |||
|
|
|||
There was a problem hiding this comment.
/**
* \brief Determine if a display and view exist.
*
* This returns false if either the display or view doesn't exist. It works regardless
* of whether the display or view are active, and it works regardless of whether the
* view is display-defined or if the display has this as a shared view. It will only
* check config-level shared views if dispName is null. It will not check config level
* shared views if dispName is not null.
*/
| @@ -3812,6 +3944,13 @@ const char * Config::getVirtualDisplayViewColorSpaceName(const char * view) cons | |||
| { | |||
| if (!view) return ""; | |||
|
|
|||
| // TODO: Remove the following work-around once bug for shared views is fixed. | |||
There was a problem hiding this comment.
Actually, despite what I wrote, you could remove the TODO, I think your fix is sufficient.
| @@ -963,6 +975,9 @@ class OCIOEXPORT Config | |||
| * | |||
| */ | |||
|
|
|||
There was a problem hiding this comment.
Please add comments for the virtual case, similar to what I wrote above.
| @@ -7338,7 +7349,10 @@ OCIO_ADD_TEST(Config, add_remove_display) | |||
|
|
|||
| // Add a (display, view) pair. | |||
|
|
|||
| // todo (annie): just a note here that this config will have two (display, view) pairs): sRGB and disp1 | |||
There was a problem hiding this comment.
You could remove the todo and make this a comment if you feel it clarifies the test.
There was a problem hiding this comment.
Ah, that was a note I forgot to remove to help me understand what was going on in that test. Will be removing!
| # cfg.removeSharedView('view5') | ||
| # cfg.removeSharedView('view6') | ||
| # cfg.removeSharedView('view1') | ||
| # cfg.removeSharedView('view2') |
There was a problem hiding this comment.
Remove commented code.
#2049