Skip to content
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

Colorspace: Resolve OCIO template #970

Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 176 additions & 38 deletions client/ayon_core/pipeline/colorspace.py
Original file line number Diff line number Diff line change
Expand Up @@ -879,6 +879,42 @@ def _get_global_config_data(
}


def _is_host_ocio_enabled_n_overriden(host_name, project_settings):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the n?
Plus typo: overridden

Suggested change
def _is_host_ocio_enabled_n_overriden(host_name, project_settings):
def _is_host_ocio_enabled_overridden(host_name, project_settings):

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n -> and.

imageio_global, imageio_host = _get_imageio_settings(
project_settings, host_name
)
# Global color management must be enabled to be able to use host settings
if not imageio_global["activate_global_color_management"]:
log.info("Colorspace management is disabled globally.")
return False, False

# Host 'ocio_config' is optional
host_ocio_config = imageio_host.get("ocio_config") or {}
# TODO remove
# - backward compatibility when host settings had only 'enabled' flag
# the flag was split into 'activate_global_color_management'
# and 'override_global_config'
host_ocio_config_enabled = host_ocio_config.get("enabled", False)

# Check if host settings group is having 'activate_host_color_management'
# - if it does not have activation key then default it to True so it uses
# global settings
activate_host_color_management = imageio_host.get(
"activate_host_color_management"
)
if activate_host_color_management is None:
activate_host_color_management = host_ocio_config_enabled

# Get config path from core or host settings
# - based on override flag in host settings
# TODO: in future rewrite this to be more explicit
override_global_config = host_ocio_config.get("override_global_config")
if override_global_config is None:
override_global_config = host_ocio_config_enabled

return activate_host_color_management, override_global_config


def get_imageio_config_preset(
project_name,
folder_path,
Expand Down Expand Up @@ -919,40 +955,22 @@ def get_imageio_config_preset(
if not project_settings:
project_settings = get_project_settings(project_name)

# Get colorspace settings
imageio_global, imageio_host = _get_imageio_settings(
project_settings, host_name
is_enabled, override_global_config = _is_host_ocio_enabled_n_overriden(
host_name, project_settings
)
# Global color management must be enabled to be able to use host settings
if not imageio_global["activate_global_color_management"]:
log.info("Colorspace management is disabled globally.")
return {}

# Host 'ocio_config' is optional
host_ocio_config = imageio_host.get("ocio_config") or {}
# TODO remove
# - backward compatibility when host settings had only 'enabled' flag
# the flag was split into 'activate_global_color_management'
# and 'override_global_config'
host_ocio_config_enabled = host_ocio_config.get("enabled", False)

# Check if host settings group is having 'activate_host_color_management'
# - if it does not have activation key then default it to True so it uses
# global settings
activate_host_color_management = imageio_host.get(
"activate_host_color_management"
)
if activate_host_color_management is None:
activate_host_color_management = host_ocio_config_enabled

if not activate_host_color_management:
if not is_enabled:
# if host settings are disabled return False because
# it is expected that no colorspace management is needed
log.info(
f"Colorspace management for host '{host_name}' is disabled."
)
return {}

# Get colorspace settings
imageio_global, imageio_host = _get_imageio_settings(
project_settings, host_name
)

project_entity = None
if anatomy is None:
project_entity = ayon_api.get_project(project_name)
Expand Down Expand Up @@ -993,14 +1011,12 @@ def get_imageio_config_preset(
# Add environment variables to template data
template_data.update(env)

# Get config path from core or host settings
# - based on override flag in host settings
# TODO: in future rewrite this to be more explicit
override_global_config = host_ocio_config.get("override_global_config")
if override_global_config is None:
override_global_config = host_ocio_config_enabled

if not override_global_config:
if override_global_config:
host_ocio_config = imageio_host["ocio_config"]
config_data = _get_host_config_data(
host_ocio_config["filepath"], template_data
)
else:
config_data = _get_global_config_data(
project_name,
host_name,
Expand All @@ -1010,10 +1026,6 @@ def get_imageio_config_preset(
folder_id,
log,
)
else:
config_data = _get_host_config_data(
host_ocio_config["filepath"], template_data
)

if not config_data:
raise FileExistsError(
Expand Down Expand Up @@ -1637,3 +1649,129 @@ def get_imageio_config(
template_data=anatomy_data,
env=env,
)


def resolve_colorspace_config_template(
config_path,
project_name,
folder_path,
task_name,
host_name,
project_settings=None,
anatomy=None,
project_entity=None,
folder_entity=None,
task_entity=None,
):
"""Try to resolve possible OCIO template.

First look if the config path is built-in, then try to use functionality
of anatomy to find rootless path ('find_root_template_from_path') and
last option is to use custom paths.

Args:
config_path (str): Path to OCIO config.
project_name (str): Project name.
folder_path (Optional[str]): Folder path.
task_name (Optional[str]): Task name.
host_name (str): Host name.
project_settings (Optional[Dict[str, Any]]): Project settings.
anatomy (Optional[Anatomy]): Project anatomy object.
project_entity (Optional[Dict[str, Any]]): Project entity data.
folder_entity (Optional[Dict[str, Any]]): Folder entity data.
task_entity (Optional[Dict[str, Any]]): Task entity data.

Returns:
str: Resolved template for OCIO config path.

Raises:
ValueError: Failed to resolve OCIO config template.

"""
if project_settings is None:
project_settings = get_project_settings(project_name)

imageio_global, imageio_host = _get_imageio_settings(
project_settings, host_name
)

builtin_tried = False
# Collect custom paths for last step
custom_paths = []
iLLiCiTiT marked this conversation as resolved.
Show resolved Hide resolved
# Add host path to custom paths
host_path = imageio_host.get("ocio_config", {}).get("filepath")
if host_path:
custom_paths.append(host_path)

for profile in imageio_global["ocio_config_profiles"]:
profile_type = profile["type"]
if profile_type == "custom_path":
custom_paths.append(profile["custom_path"])
continue

#
iLLiCiTiT marked this conversation as resolved.
Show resolved Hide resolved
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of these may not apply to the current host though? The profile should be validated?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that relevant? You have "some" ocio path, and you want to get template so others can get the same path.


if anatomy is None:
anatomy = Anatomy(project_name)

success, rootless_path = anatomy.find_root_template_from_path(config_path)
if success:
return rootless_path
BigRoy marked this conversation as resolved.
Show resolved Hide resolved

# 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.")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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?

Copy link
Member Author

@iLLiCiTiT iLLiCiTiT Oct 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

26 changes: 21 additions & 5 deletions client/ayon_core/pipeline/farm/pyblish_functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,14 @@
import ayon_api
import clique
from ayon_core.lib import Logger
from ayon_core.pipeline import get_current_project_name, get_representation_path
from ayon_core.pipeline import (
get_current_project_name,
get_representation_path,
)
from ayon_core.pipeline.create import get_product_name
from ayon_core.pipeline.farm.patterning import match_aov_pattern
from ayon_core.pipeline.publish import KnownPublishError
from ayon_core.pipeline.colorspace import resolve_colorspace_config_template


@attr.s
Expand Down Expand Up @@ -485,14 +489,26 @@ def create_instances_for_aov(instance, skeleton, aov_filter,
}

# Get templated path from absolute config path.
anatomy = instance.context.data["anatomy"]
context = instance.context
colorspace_template = instance.data["colorspaceConfig"]
try:
additional_color_data["colorspaceTemplate"] = remap_source(
colorspace_template, anatomy)
colorspace_template = resolve_colorspace_config_template(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why it happens here? We might do it before this function, feel free to add enhancement.

colorspace_template,
Comment on lines 493 to +496
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed used function...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup understand - I'm commenting it mostly from code clarity - should we refactor that so it reads as something that actually makes sense :D

context.data["projectName"],
instance.data.get("folderPath"),
instance.data.get("task"),
context.data["hostname"],
project_settings=context.data["project_settings"] ,
anatomy=context.data["anatomy"],
project_entity=context.data["projectEntity"],
folder_entity=instance.data.get("folderEntity"),
task_entity=instance.data.get("taskEntity"),
)

except ValueError as e:
log.warning(e)
additional_color_data["colorspaceTemplate"] = colorspace_template

additional_color_data["colorspaceTemplate"] = colorspace_template

# if there are product to attach to and more than one AOV,
# we cannot proceed.
Expand Down
Loading