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

Add partial and optional support for OCIO to MaterialX #1917

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

JGamache-autodesk
Copy link
Contributor

@JGamache-autodesk JGamache-autodesk commented Jul 3, 2024

The default color management system in MaterialX has support for a very limited set of color spaces. Some commonly used spaces such as linear Rec.2020 or sRGB-encoded AP1 are not available. MaterialX is often used in applications that already use OCIO for color management and so artists are sometimes confused as to why the color spaces in a menu that is available for textures in another part of the application are not available for textures in MaterialX. In some cases, a color space may even be mathematically the same as one in the MaterialX set, but it is simply named differently. This may lead to the artist picking the wrong color space and wasting time. In situations like this, the application is able to offer a better user experience by offloading the interpretation of color space names to OCIO. This allows artists to use the same color spaces with all textures rather than having a different set for MaterialX.
However, not all OCIO color spaces are appropriate for use in a MaterialX document and this PR does not offer support for full OCIO. Supported color spaces include any of those that are mathematically compatible with one of the default MaterialX spaces.
This PR does not change where the implicit color conversions happen in a MaterialX shader, and it does not provide any additional functionality to creators of MaterialX documents, other than expanding the set of allowed color space names. The MaterialX API already allows a client to override the default mx::ColorManagementSystem, this PR illustrates how that may be done in order to take advantage of a subset of OCIO.
It should be noted that only minimal changes would be needed to adapt this PR for use with either OCIO or the nanoColor library currently under development.
This integrates the OCIO shadergen into the MaterialX shadergen to provide on-the-fly color management for OpenGL, Vulkan, Metal, and OSL.
Targeting 1.39.1. Definitely not 1.39.0.
Tested with GLSL and OSL on Linux.

Copy link
Contributor

@ld-kerley ld-kerley left a comment

Choose a reason for hiding this comment

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

This looks like really great work - when I get some cycles I'd be happy to kick the tires on the MSL side (might be next week at this point). I left a few observations below.

if (context.getShaderGenerator().getTarget() == "genglsl") {
shaderDesc->setLanguage(OCIO::GPU_LANGUAGE_GLSL_4_0);
} else if (context.getShaderGenerator().getTarget() == "genmsl") {
shaderDesc->setLanguage(OCIO::GPU_LANGUAGE_MSL_2_0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we could keep this version in sync somehow with

const string MslShaderGenerator::VERSION = "2.3";

from here

source/MaterialXGenShader/CMakeLists.txt Outdated Show resolved Hide resolved
#ifdef MATERIALX_BUILD_OCIO
try
{
auto config = OCIO::Config::CreateFromBuiltinConfig("ocio://studio-config-latest");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an OCIO expert - but does this pull some pre-baked config from inside OCIO. Will people need to provide custom OCIO configs at some point?

Choose a reason for hiding this comment

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

@ld-kerley , the PR supports use of arbitrary OCIO configs, but to keep things simple for the purposes of this unit test, Jerry is using one of the configs that's built into the OCIO library itself.

In the real world, apps could create the config using OCIO::Config::CreateFromFile(path) rather than CreateFromBuiltinConfig.

@JGamache-autodesk
Copy link
Contributor Author

Well, OSL was not as trivial as expected, but still manageable. Results are good:
OCIO_results
And we can see the OCIO code being injected in GLSL:

vec4 ocio_color_conversion_8182d4da8df3d64c153c5e4252dbbf(vec4 inPixel)
{
  vec4 outColor = inPixel;
  
  // Add Gamma 'monCurveFwd' processing
  
  {
    vec4 breakPnt = vec4(0.0392857157, 0.0392857157, 0.0392857157, 1.);
    vec4 slope = vec4(0.077380158, 0.077380158, 0.077380158, 1.);
    vec4 scale = vec4(0.947867274, 0.947867274, 0.947867274, 0.999998987);
    vec4 offset = vec4(0.0521326996, 0.0521326996, 0.0521326996, 9.99998974e-07);
    vec4 gamma = vec4(2.4000001, 2.4000001, 2.4000001, 1.00000095);
    vec4 isAboveBreak = vec4(greaterThan( outColor, breakPnt));
    vec4 linSeg = outColor * slope;
    vec4 powSeg = pow( max( vec4(0., 0., 0., 0.), scale * outColor + offset), gamma);
    vec4 res = isAboveBreak * powSeg + ( vec4(1., 1., 1., 1.) - isAboveBreak ) * linSeg;
    outColor.rgb = vec3(res.x, res.y, res.z);
    outColor.a = res.w;
  }

  return outColor;
}

and in the OSL code:

color4 ocio_color_conversion_8182d4da8df3d64c153c5e4252dbbf(color4 inPixel)
{
  color4 outColor = inPixel;
  
  // Add Gamma 'monCurveFwd' processing
  
  {
    vector4 breakPnt = vector4(0.0392857157, 0.0392857157, 0.0392857157, 1.);
    vector4 slope = vector4(0.077380158, 0.077380158, 0.077380158, 1.);
    vector4 scale = vector4(0.947867274, 0.947867274, 0.947867274, 0.999998987);
    vector4 offset = vector4(0.0521326996, 0.0521326996, 0.0521326996, 9.99998974e-07);
    vector4 gamma = vector4(2.4000001, 2.4000001, 2.4000001, 1.00000095);
    vector4 isAboveBreak = vector4((outColor.rgb.r > breakPnt.x) ? 1.0 : 0.0, (outColor.rgb.g > breakPnt.y) ? 1.0 : 0.0, (outColor.rgb.b > breakPnt.z) ? 1.0 : 0.0, (outColor.a > breakPnt.w) ? 1.0 : 0.0);
    vector4 linSeg = outColor * slope;
    vector4 powSeg = pow( max( vector4(0., 0., 0., 0.), scale * outColor + offset), gamma);
    vector4 res = isAboveBreak * powSeg + ( vector4(1., 1., 1., 1.) - isAboveBreak ) * linSeg;
    outColor.rgb = vector(res.x, res.y, res.z);
    outColor.a = res.w;
  }

  return outColor;
}

@JGamache-autodesk JGamache-autodesk changed the title MaterialXOCIO backported to MaterialX Add partial and optional support for OCIO to MaterialX Jul 9, 2024
@kwokcb
Copy link
Contributor

kwokcb commented Jul 11, 2024

It's quite possible that not all shading languages and code generation variants will be supported as this restricted to code generation built into OCIO and does not produce a shading language agnostic nodegraph. (*)(**)

A suggestion is to inherit OCIO from the base color transform and test for OCIO first and if it fails use the built in one (or check built in support first maybe better?).

Basically generation from an "OCIO" config will be called if it's not a supported transform in the "default". This would match more closely how targets and fallbacks work for code generation. I think this would also match now nanocolor is being proposed to be integrated for MaterialX -- i.e. if not supported by nanocolor then full OCIO can be used.

(*) The "default" color transforms in OCIO 2.0 are a superset of what's in MaterialX currently, but MaterialX has a super set of shading languages pre-supported with other variants possible.)
(**) I'm not sure whatever became of the discussion of OCIO providing operators which could map to MaterialX nodes but this would avoid "chasing" shading language support.

Just my 2 cents. :).

@doug-walker
Copy link

@jstone-lucasfilm, I wanted to ask about this during our meeting today but we ran out of time. Now that Jerry has implemented Bernard's request, is it possible to move forward with this PR? (Although this work is compatible with NanoColor, it meets a separate need, and thus seems like it could proceed without waiting for the conclusion of that initiative.)

@jstone-lucasfilm
Copy link
Member

@doug-walker Agreed that this pull request is still valuable, independent of the ultimate design of NanoColor, and I'm currently thinking that we should include it in our next MaterialX release, v1.39.2. Right now we're in the release candidate phase for v1.39.1, and this change deserves more time than we'd have to include it in that release.


vector4 __operator__mul__(vector4 v, color4 c)
{
return v * vector4(c.rgb.r, c.rgb.g, c.rgb.b, c.a);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use the commutative property here and write
return c*v;
and take advantage of the operator defined above.


vector4 __operator__add__(color4 c, vector4 v)
{
return vector4(c.rgb.r, c.rgb.g, c.rgb.b, c.a) + v;
Copy link
Contributor

Choose a reason for hiding this comment

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

similarly commutative here.
return v+c;


OpenColorIOManagementSystemPtr OpenColorIOManagementSystem::create(const OCIO::ConstConfigRcPtr& config, const string& target)
{
if (target != "genglsl" && target != "genmsl" && target != "genosl")
Copy link
Contributor

Choose a reason for hiding this comment

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

targets can also be inherited from each other, and I believe that maybe OpenUSD's Storm uses this concept. Instead of just having a fix list of base target names, we may perhaps need to respect inherited targets as well?

virtual ~OpenColorIOManagementSystem() { }

/// Create a new OpenColorIOManagementSystem
static OpenColorIOManagementSystemPtr create(const OCIO::ConstConfigRcPtr& config, const string& target);
Copy link
Contributor

Choose a reason for hiding this comment

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

How is a OCIO::ConstConfigRcPtr constructed?

This seems like the only instance where OCIO would be in the public MaterialX API, and it maybe be nice if could avoid that need.

Perhaps if we exposed wrappers around OCIO::Config::CreateFromEnv() OCIO::Config::CreateFromFile() OCIO::Config::CreateFromBuiltinConfig() ?


vector4 __operator__mul__(matrix m, vector4 v)
{
return vector4(v.x * m[0][0] + v.y * m[0][1] + v.z * m[0][2] + v.w * m[0][3],
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest this wrap the existing transform(m, v) function that exists in libraries/stdlib/genosl/lib/vector_4.h, but then when I was looking it looks like the math isn't actually the same.

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/libraries/stdlib/genosl/include/vector4.h#L414

I remember @jstone-lucasfilm mentioning something about extensive documentation about matrix vector multiplication in MaterialX but I can't find it right now - but I would like to propose that these two function at least agree with each other - or even better be a single shared implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Here's the documentation for matrix-vector multiplication in MaterialXCore, and our shading language implementations ought to follow this same pattern:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXCore/Types.h#L653

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants