ynput / OpenPype

OpenPype has been surpassed by AYON and is now read only.
https://ayon.ynput.io
MIT License
286 stars 129 forks source link

Global: colorspace `parse_colorspace_from_filepath` simplification #4348

Open jakubjezek001 opened 1 year ago

jakubjezek001 commented 1 year ago

Would this function make more sense when simplified/rewritten as just finding the first relevant color space from the list of color spaces of the config? It'd make it more versatile in use instead of it internally always needing to call get_ocio_config_colorspaces.

It feels to me that parsing from filenames like this might often be frequently done close after each other - e.g. parsing multiple files. Quering the color space files rules will be relatively slow since it requires accessing the OCIO config file each time.

colorspaces = get_ocio_config_colorspaces(config_path)
for filepath in filepaths:
    colorspace = parse_colorspace_from_filepath(filepath, colorspaces=colorspaces)

This also looks to me like a much more friendly API to the user with much less arguments to pass. Plus it'd be much faster and a simpler function code body.

As a general recommendation I'd try to see if there are other areas where functions could be simplified like this. Needing to pass project name, host, project settings to all functions where they end up just parsing an OCIO config path I'd say should be rewritten to just take the config path.

_Originally posted by @BigRoy in https://github.com/ynput/OpenPype/pull/4195#discussion_r1070448372_

[cuID:OP-4770]

jakubjezek001 commented 1 year ago

This is also need to address following issue from @BigRoy comment here https://github.com/ynput/OpenPype/pull/4195#discussion_r1080980267