Colorspace: Resolve OCIO template #970
Conversation
iLLiCiTiT
changed the title
| colorspace_template = instance.data["colorspaceConfig"] | ||
| try: | ||
| additional_color_data["colorspaceTemplate"] = remap_source( | ||
| colorspace_template, anatomy) | ||
| colorspace_template = resolve_colorspace_config_template( | ||
| colorspace_template, |
There was a problem hiding this comment.
Isn't it weird that we pass it a template to begin with? Should we update the input variable name to ocio_path or whatever the variable represents at that point?
There was a problem hiding this comment.
I just changed used function...
There was a problem hiding this comment.
Yup understand - I'm commenting it mostly from code clarity - should we refactor that so it reads as something that actually makes sense :D
| @@ -879,6 +879,42 @@ def _get_global_config_data( | |||
| } | |||
|
|
|||
|
|
|||
| def _is_host_ocio_enabled_n_overriden(host_name, project_settings): | |||
There was a problem hiding this comment.
What's the n?
Plus typo: overridden
| def _is_host_ocio_enabled_n_overriden(host_name, project_settings): | |
| def _is_host_ocio_enabled_overridden(host_name, project_settings): |
| colorspace_template = instance.data["colorspaceConfig"] | ||
| try: | ||
| additional_color_data["colorspaceTemplate"] = remap_source( | ||
| colorspace_template, anatomy) | ||
| colorspace_template = resolve_colorspace_config_template( |
There was a problem hiding this comment.
Side note:
This usually runs over MANY instances - it may run for each renderlayer over EACH aov that is written as a separate output product (if not multilayer exr). We may be looking at 50+ or 100+ instances.
We should make sure the resolving we're doing does perform somewhat optimal if it doesn't.
There was a problem hiding this comment.
I don't know why it happens here? We might do it before this function, feel free to add enhancement.
| for profile in imageio_global["ocio_config_profiles"]: | ||
| profile_type = profile["type"] | ||
| if profile_type == "custom_path": | ||
| custom_paths.append(profile["custom_path"]) | ||
| continue | ||
|
|
||
| # | ||
| if profile_type != "builtin_path" or builtin_tried: | ||
| continue | ||
|
|
||
| template = profile["builtin_path"] | ||
| try: | ||
| path = template.format(**os.environ) | ||
| except Exception: | ||
| continue | ||
| if os.path.realpath(path) == os.path.realpath(config_path): | ||
| return template |
There was a problem hiding this comment.
Some of these may not apply to the current host though? The profile should be validated?
There was a problem hiding this comment.
Is that relevant? You have "some" ocio path, and you want to get template so others can get the same path.
| # Try to fill custom paths only with 'os.environ' | ||
| failed_custom_paths = [] | ||
| for custom_path in custom_paths: | ||
| try: | ||
| path = custom_path.format(**os.environ) | ||
| except Exception: | ||
| failed_custom_paths.append(custom_path) | ||
| continue | ||
|
|
||
| if os.path.realpath(path) == os.path.realpath(config_path): | ||
| return custom_path | ||
|
|
||
| # If none of custom paths formatting failed it means we don't need to try | ||
| # with entities template data. | ||
| if not failed_custom_paths: | ||
| raise ValueError("Failed to resolve OCIO config template.") | ||
|
|
||
| # Prepare all entities for template data | ||
| if project_entity is None: | ||
| project_entity = ayon_api.get_project(project_name) | ||
| if folder_path and not folder_entity: | ||
| folder_entity = ayon_api.get_folder_by_path( | ||
| project_name, folder_path | ||
| ) | ||
| if task_name and folder_entity and not task_entity: | ||
| task_entity = ayon_api.get_task_by_name( | ||
| project_name, folder_entity["id"], task_name | ||
| ) | ||
| template_data = get_template_data( | ||
| project_entity, | ||
| folder_entity, | ||
| task_entity, | ||
| host_name, | ||
| project_settings, | ||
| ) | ||
| template_data.update(os.environ.items()) | ||
|
|
||
| # Try to fill custom paths with template data passed to function | ||
| for custom_path in failed_custom_paths: | ||
| try: | ||
| path = custom_path.format(**template_data) | ||
| except Exception: | ||
| continue | ||
| if os.path.realpath(path) == os.path.realpath(config_path): | ||
| return custom_path | ||
|
|
||
| raise ValueError("Failed to resolve OCIO config template.") |
There was a problem hiding this comment.
Why do we need all this "custom" formatting and matching compared to what the OCIO 'setup' does elsewhere in the codebase? It feels a bit duplicated (and easily going out of sync in code in the future) between this check and what get_imageio_config or get_imageio_config_preset does. Why can't we use that function and make sure we run the same logic as intended?
It also feels way too complex?
There was a problem hiding this comment.
Because get_imageio_config and get_imageio_config_preset does return path based on current settings and current context. This does look into all options that might be used.
If we do care about the ocio path being set based on current settings and related context, then we should have validator that checks if the path is set to correct value. In that case we can just use whatever get_imageio_config returns and not fill it based on colorspaceConfig collected from scene, but use settings all the time.
There was a problem hiding this comment.
Because
get_imageio_configandget_imageio_config_presetdoes return path based on current settings and current context. This does look into all options that might be used.
Exactly - and those should be the ones we should only consider, right? We shouldn't consider the ones from another profile not related to current context?
There was a problem hiding this comment.
Then we need the validator to check if scene colospace matches config from settings... And in that case the logic happening in create_instances_for_aov does not have to happen there, because all you need to to collect config data for the context in a collector.
|
So after some discussion with @antirotor . Better solution would be to collect the path and template from settings, and use |
| @@ -879,6 +879,42 @@ def _get_global_config_data( | |||
| } | |||
|
|
|||
|
|
|||
| def _is_host_ocio_enabled_n_overridden(host_name, project_settings): | |||
There was a problem hiding this comment.
The n for and just feels like a typo, either write it in full or just _get_host_ocio_state or _get_host_ocio_settings or whatever. A docstring can should elaborate what it returns too.
The function name gave me no idea that it returned a tuple - I thought it would return a single bool based of the function name on whether it was enabled and had overrides.
|
The issue will be resolved with different approach. |
Changelog Description
Implemented helper function to resolve ocio config template from path.
Additional info
The previous logic used only anatomy roots to find out template of the path, but if builtin configs (from ayon-config) are used, the path leads to appdirs, which are not in any project root. That means it passes full path to farm job, leading to crash on the target machine.
The new function goes through all possible templates from settings and tries them one by one. The order is from fastest approach to slowest. First tries to check if the path is builtin path, then tries to use Anatomy to find rootless path, then tries to look into custom paths from settings, and from host settings (if availabe) and fill them with env variables and template data based on passed in context.
Testing notes:
"colorspaceData".Resolves #969