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

Enhancement: Color management for Houdini workfiles #5858

Closed
wants to merge 53 commits into from

Conversation

MustafaJafar
Copy link
Contributor

@MustafaJafar MustafaJafar commented Nov 2, 2023

Changelog Description

This PR adds Houdini workfiles color management

  • Display: default display, it accepts Colon-separated list of displays, e.g ACES:P3, Read more Active Displays
  • View: default view, it accepts Colon-separated list of view names, e.g sRGB:DCDM Read more Active Views
  • Review Colorspace : Review output transform which exposes OCIO Colorspace parameter in opngl nodes, Read more Houdini OpenGL

Things to do:

  • Set default view and default display: In Houdini this is done via setting OCIO_ACTIVE_DISPLAYS OCIO_ACTIVE_VIEWS
  • make review creator and validator to use review color space value specified in the settings
  • update color space data while review extraction
  • update color space data while houdini image sequence extraction (P.S. It's the colorspace name assigned to scene_linear role.)
  • fix the conflict mentioned here Enhancement: Color management for Houdini workfiles #5858 (comment)

image

Testing notes:

  1. Test the hook functionality, case 1:
    • test : disable global color management, enable houdini color management, enable color management/workfile
    • result : Hook won't work.
  2. Test the hook functionality, case 2:
    • test : enable global color management, disable houdini color management, enable color management/workfile
    • result : Hook won't work.
  3. Test the hook functionality, case 3:
    • test : enable global color management, enable houdini color management, disable color management/workfile
    • result : Hook logs to console that workfile settings are disabled.
  4. Test the hook functionality, case 4:
    • test : enable global color management, enable houdini color management, enable color management/workfile
    • result : Hook sets the proper environment variables with values specified in the Houdini settings, it logs to console the environment variables and in houdini you are able to see the only color spaces specified in the settings.
  5. Test Review colospace, case 1:
    • test : leave Review colospace empty
    • result : default review colorspace will be used when
      • creating review instances
      • using Set Review Color Space action if Validate Review Colorspace failed
  6. Test Review colospace, case 2:
    • test : add you preferred Review colospace value
    • result : this value will be used when
      • creating review instances
      • Validate Review Colorspace will check if ociocolorspace parameter has that value
      • using Set Review Color Space action if Validate Review Colorspace failed

@ynbot
Copy link
Contributor

ynbot commented Nov 2, 2023

Task linked: OP-1017 houdini: colorspaces

@ynbot ynbot added size/S Denotes a PR changes 100-499 lines, ignoring general files type: enhancement Enhancements to existing functionality labels Nov 2, 2023
@MustafaJafar MustafaJafar linked an issue Nov 2, 2023 that may be closed by this pull request
2 tasks
@MustafaJafar
Copy link
Contributor Author

To me this issue #67 looks similar to #4836 ? Are they duplicates ? if yes, tell me to make this PR close it as well.

@BigRoy
Copy link
Collaborator

BigRoy commented Nov 2, 2023

To me this issue #67 looks similar to #4836 ? Are they duplicates ? if yes, tell me to make this PR close it as well.

@MustafaJafar That does seem like the same thing, but I do want to note that to 'get on par with nuke' and others likely we also need to test:

  • (Image) Outputs from Houdini should also submit along their colorspace data if they don't already do that, so we should make sure published data for e.g. published renders or COP2 networks include colorspace information if that publish itself can have that information from Houdini of course.
  • Potentially when loading say an image or anything it'd also automatically set the correct colorspace value - but purely checking out COP2 file nodes I don't see anything with regards to OCIO there at all?

@MustafaJafar
Copy link
Contributor Author

To me this issue #67 looks similar to #4836 ? Are they duplicates ? if yes, tell me to make this PR close it as well.

@MustafaJafar That does seem like the same thing, but I do want to note that to 'get on par with nuke' and others likely we also need to test:

  • (Image) Outputs from Houdini should also submit along their colorspace data if they don't already do that, so we should make sure published data for e.g. published renders or COP2 networks include colorspace information if that publish itself can have that information from Houdini of course.
  • Potentially when loading say an image or anything it'd also automatically set the correct colorspace value - but purely checking out COP2 file nodes I don't see anything with regards to OCIO there at all?

This is interesting!
Currently, I have no idea how to achieve that but let's find out!

Co-authored-by: Roy Nieterau <roy_nieterau@hotmail.com>
MustafaJafar and others added 4 commits November 2, 2023 17:18
Co-authored-by: Roy Nieterau <roy_nieterau@hotmail.com>
Co-authored-by: Roy Nieterau <roy_nieterau@hotmail.com>
Copy link
Member

@Minkiu Minkiu left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but code looks good.

Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

The review instance published looks okay.
image
It also validates the colorspace right when using the project setting to make sure ocio colorspace is in a certain parameter. But it would be great if there is doc(and comment) for users to understand the use of Review Colorspace. We can't guarantee there would be some users would just put sRGB or ACEScg into the setting. Will test it in composite later.
image

Copy link
Member

@moonyuet moonyuet left a comment

Choose a reason for hiding this comment

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

Tested with the composite instance.
Looks good with the global color enabled. It can be possibly merged after the code tweaks.
image

@MustafaJafar
Copy link
Contributor Author

MustafaJafar commented Jan 5, 2024

Quick Note regards review colorspace:

  1. Empty Review colorspace setting keeps review colorspace unmanaged which allows artists to pick any colorspace value
    image
  2. Set Default Review Color Space 🖥️ will set OCIO Colorspace parm in the open gl node to the same value of Review colorspace settings. and if setting is empty, it will fall to default corresponding to the default active view and display.

Tests in OP
Animation_17
Animation_18

Tests in Ayon
Animation_19
Animation_20
Animation_21

@fabiaserra
Copy link
Contributor

Tests in Ayon Animation_19 Animation_19 Animation_20 Animation_20 Animation_21 Animation_21

why is there a console window poping up when you click Set default review colorspace?

@MustafaJafar
Copy link
Contributor Author

why is there a console window poping up when you click Set default review colorspace?

because of get_default_display_view_colorspace().
These function were added in #5322

def get_default_display_view_colorspace():
"""Returns the colorspace attribute of the default (display, view) pair.
It's used for 'ociocolorspace' parm in OpenGL Node."""
prefs = get_color_management_preferences()
return get_display_view_colorspace_name(
config_path=prefs["config"],
display=prefs["display"],
view=prefs["view"]
)

def get_display_view_colorspace_name(config_path, display, view):
"""Returns the colorspace attribute of the (display, view) pair.
Args:
config_path (str): path string leading to config.ocio
display (str): display name e.g. "ACES"
view (str): view name e.g. "sRGB"
Returns:
view color space name (str) e.g. "Output - sRGB"
"""
if not compatibility_check():
# python environment is not compatible with PyOpenColorIO
# needs to be run in subprocess
return get_display_view_colorspace_subprocess(config_path,
display, view)
from openpype.scripts.ocio_wrapper import _get_display_view_colorspace_name # noqa
return _get_display_view_colorspace_name(config_path, display, view)

@mkolar
Copy link
Member

mkolar commented Feb 9, 2024

Because we're splitting OpenPype into ayon-core and individual host addons, this PR would have to be re-created to target one of those.

We're closing it down, but we'll he happy for a new PR to ynput/ayon-core or the host addon repository once it's up.

@mkolar mkolar closed this Feb 9, 2024
@ynbot ynbot added this to the next-patch milestone Feb 9, 2024
@jakubjezek001 jakubjezek001 removed this from the next-patch milestone Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Houdini port to AYON size/S Denotes a PR changes 100-499 lines, ignoring general files target: AYON type: enhancement Enhancements to existing functionality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Enhancement: Houdini publish existing caches/frames
9 participants