-
Notifications
You must be signed in to change notification settings - Fork 477
Adsk Contrib - Include color spaces with no categories in menuHelper results by default #2189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adsk Contrib - Include color spaces with no categories in menuHelper results by default #2189
Conversation
Signed-off-by: Doug Walker <[email protected]> (cherry picked from commit 146e90b) Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]> (cherry picked from commit fbcd4d4) Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
From my point of view I think I prefer the alternative of defaulting to not including the uncategorised spaces. My reasoning is that as a config author I want to describe what spaces should appear under a category to give the users a good small set of approved choices, but then allow the application to apply a "modifier" of some form to show uncategorised, (and even a further option to show all spaces from any category). An example is how Baselight handles things. If we default to showing all correctly categorised and uncategorised spaces then users can rightly claim they did nothing wrong when they selected a less than idea choice. They will simply ask "why did you show me a choice which is incorrect?". This goes against the ideas of showing people best practice, it also assumes the config author does not know what they are doing, which again jars with our normal assumptions. |
Would the uncategorized color spaces show up at the top-level menu? That's kind of a bummer. What if we stuck all uncategorized spaces in an "uncategorized" category via AppHelpers instead? Sort of solves Kevin's problem, because I don't think folks will go digging in that kind of menu, and also gives users/apps knowledge of how menus are constructed and how to "fix" it. |
@KevinJW , what you describe can already be accomplished with the existing system (e.g., you could define both general and recommended categories). But either all color spaces should have the categories attribute, or none should. Omitting the categories to indicate a "general" level doesn't work because as soon as you add the "recommended" category, it would essentially no longer be in the general set. Furthermore, I think most people have the presumption that all color spaces in the config should show up in menus unless they are in the inactive list. @carolalynn , the placement of color spaces in hierarchical menus is controlled by the "family" attribute. I don't think it makes sense to have categories sometimes overriding that. The goal of this PR is simply to introduce a fallback that minimizes the difference in behavior between apps that use categories and those that do not. |
We had an extended conversation about this at the TSC meeting today. In summary, the decision was to go ahead and merge this for OCIO 2.5. Regarding Kevin's points, it is still possible to set up configs to have a "full set" and "recommended set" of color spaces. The change is that every color space needs to have a category attribute, rather than only the "recommended set". At a future point, it could be useful to add a flag to the config that would control the behavior (e.g., a "strict mode"). More discussion would be needed on the details. An issue was created for the idea of controlling category filtering via an env. var. Doug took an action item to have either Config::validate or ociocheck issue a warning if a config uses categories but not all color spaces have one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A very minor comment for a variable name, otherwise it looks good.
Signed-off-by: Doug Walker <[email protected]>
Signed-off-by: Doug Walker <[email protected]>
The categories attribute of color spaces allows config authors to filter the contents of application menus (for example, limit the contents of working color space menus to only those color spaces with the 'working-space' category). To enable this, applications may use the ColorSpaceMenuHelper class to build color space menus.
Currently, color spaces with no categories are always omitted from these menus, and this sometimes creates issues. For better or worse, we know that end-users often modify their configs by copy/pasting color spaces from other configs. This may mean either that a config that does not use categories now has a few color spaces that do use them; or alternatively that a config where all color spaces had a category attribute now has color spaces that do not.
In these scenarios, the newly added color spaces either don't show up, or alternatively, they cause all other color spaces to disappear. Either scenario is obviously confusing for end-users.
I think it would be less confusing if color spaces (or named transforms) without categories are always included in menus. In other words, to be excluded from the category filtering. This is a change to existing behavior, but I think it's a much better default and I don't think it should be very disruptive.
This PR implements this change and adds a new setTreatNoCategoryAsAny function on the ColorSpaceMenuParameters class. This defaults to true but applications may set it to false to obtain the previous behavior.
Lots of additional work was done on unit tests.
In addition, I added a --run_only option to OpenColorIOTestSuite.py to run a single Python test suite.