-
Notifications
You must be signed in to change notification settings - Fork 358
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
base: main
Are you sure you want to change the base?
Add partial and optional support for OCIO to MaterialX #1917
Conversation
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
#ifdef MATERIALX_BUILD_OCIO | ||
try | ||
{ | ||
auto config = OCIO::Config::CreateFromBuiltinConfig("ocio://studio-config-latest"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 (*) 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.) Just my 2 cents. :). |
Make the OCIO color manager derive from the default color management.
@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.) |
@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); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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], |
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
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.