Change subsurface_bsdf radius from vector3 to color3

[pablode]

This was changed in the 1.39 spec, but is not reflected in the code yet.

[jstone-lucasfilm]

@pablode We've now merged development work on MaterialX 1.39 back to the main branch of MaterialX, in preparation for the final weeks of development on the 1.39.0 release. When you have a chance, could you retarget this pull request back to the main branch as well?

[pablode]

Not sure why the CI is not running -- it should still fail. @jstone-lucasfilm @kwokcb any idea what's wrong with the implementation?

PS: I noticed that stdlib/pbrlib_defs.xml & co. still use 1.38 in their MaterialX version tags. Should they be updated?

[jstone-lucasfilm]

Thanks for updating the branch to main, @pablode, and I'll go ahead and start a new CI build with this latest work. We had some issues with GitHub Actions stability last week that required us to delete unsuccessful CI logs, but I believe everything is back up and running now.

For the version strings in the standard libraries, we usually leave these unmodified (i.e. at 1.38) until we're nearly ready to mark a new release, so that we get the maximum amount of testing for the upgrade path. For that reason, I'd recommend leaving all of the version strings in your PR at 1.38, and letting the document upgrade path handle any changes that need to occur.

Let me know if you need an assist in resolving the remaining build issues for your PR, and I'd be happy to take a closer look.

[pablode]

Thanks, I’ll revert the version string change in the test case then.

I’d appreciate it if you could have a look at the failures. I’ve already verified the upgrade logic, and it looks like all documents (including the Standard Surface node graph) are correctly upgraded.

[pablode]

Awesome! Thank you for catching these two cases in the upgrade logic 🙂

[jstone-lucasfilm]

Thanks for putting this change together, @pablode, and it should help to bring the specification and codebase into alignment for MaterialX 1.39.

I'm CC'ing @niklasharrysson and @portsmouth for their thoughts, to make sure the proposed change from vector3 to color3 radius matches the very latest discussions around the subsurface_bsdf node in MaterialX.

[niklasharrysson]

I'm not sure about this type change.. Looking at the 1.39 spec it's indeed a color3 now, and in the Standard Surface spec it's also a color3. However in the OpenPBR spec it's a vector3. @portsmouth is it correct to assume that a vector3 type is what you prefer for the subsurface radius / mfp parameter?

[jstone-lucasfilm]

@niklasharrysson @pablode This relates to the topic of physical versus perceptual colors in MaterialX, and I think it's a deep enough subject that it's worthwhile to discuss more broadly, so I've started a new thread on the MaterialX Slack:

https://academysoftwarefdn.slack.com/archives/C0230LWBE2X/p1719188320479269

[jstone-lucasfilm]

@niklasharrysson @pablode Summing up the MaterialX Slack thread, it sounds like representing physical colors as color3 values is preferred, as it provides better visual parity than using vector3 values. In the former case, physical colors are transformed imperfectly when rendering an asset in a different scene color space, but this is still better than not transforming them at all.

Given that additional context, I'm leaning towards including Pablo's change in MaterialX 1.39, and I'd be interested in your thoughts!

[jstone-lucasfilm]

Let's move forward with this change for MaterialX 1.39, since it aligns with our latest discussions around physical color representation. Thanks @pablode!