Add view helper functions to Config class #2083

annieln · 2024-10-17T21:40:05Z

#2049

Signed-off-by: annie <[email protected]>

pulling from upstream Signed-off-by: annie <[email protected]>

Signed-off-by: annie <[email protected]>

doug-walker

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.

doug-walker · 2024-10-18T02:47:46Z

include/OpenColorIO/OpenColorIO.h

+    static bool viewsAreEqual(const ConstConfigRcPtr & first,
+                              const ConstConfigRcPtr & second,
+                              const char * dispName,
+                              const char * viewName);


Please use "ViewsAreEqual" since the names of all static methods are capitalized. Same for "VirtualViewsAreEqual".

doug-walker · 2024-10-18T03:03:31Z

include/OpenColorIO/OpenColorIO.h

@@ -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);



/// Clear all shared views. This will throw if any displays are still using the shared views.

Sorry, I'm just checking - is this meant for clearSharedViews instead of viewIsShared?

Yes, sorry, I put the comment on the wrong line.

doug-walker · 2024-10-18T03:12:55Z

include/OpenColorIO/OpenColorIO.h

@@ -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;



/** * \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. */

doug-walker · 2024-10-18T03:19:26Z

include/OpenColorIO/OpenColorIO.h

@@ -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;



/** * \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. */

doug-walker · 2024-10-18T03:23:31Z

src/OpenColorIO/Config.cpp

@@ -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.


Actually, despite what I wrote, you could remove the TODO, I think your fix is sufficient.

doug-walker · 2024-10-18T03:24:10Z

include/OpenColorIO/OpenColorIO.h

@@ -963,6 +975,9 @@ class OCIOEXPORT Config
     *
     */



Please add comments for the virtual case, similar to what I wrote above.

doug-walker · 2024-10-18T03:30:24Z

tests/cpu/Config_tests.cpp

@@ -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


You could remove the todo and make this a comment if you feel it clarifies the test.

Ah, that was a note I forgot to remove to help me understand what was going on in that test. Will be removing!

doug-walker · 2024-10-18T03:38:21Z

tests/python/ConfigTest.py

+        # cfg.removeSharedView('view5')
+        # cfg.removeSharedView('view6')
+        # cfg.removeSharedView('view1')
+        # cfg.removeSharedView('view2')


Remove commented code.

annieln added 6 commits September 27, 2024 22:11

Add view helper functions to Config class

a53bcee

Signed-off-by: annie <[email protected]>

Merge branch 'main' into viewHelpers

87dc1c5

pulling from upstream Signed-off-by: annie <[email protected]>

add view helper functions to Config class and unit tests

f3d3e9b

Signed-off-by: annie <[email protected]>

add view helper functions as python bindings

f4c3876

Signed-off-by: annie <[email protected]>

add unit tests for view helper py bindings

00dd835

Signed-off-by: annie <[email protected]>

Merge branch 'main' into viewHelpers

d73b61e

Signed-off-by: annie <[email protected]>

doug-walker requested changes Oct 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add view helper functions to Config class #2083

Add view helper functions to Config class #2083

annieln commented Oct 17, 2024

doug-walker left a comment

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

annieln Oct 18, 2024

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

doug-walker Oct 18, 2024

annieln Oct 18, 2024

doug-walker Oct 18, 2024

		@@ -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;

		@@ -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;

		@@ -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

Add view helper functions to Config class #2083

Are you sure you want to change the base?

Add view helper functions to Config class #2083

Conversation

annieln commented Oct 17, 2024

doug-walker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment