Prman Spline Experiments

[danieldresser-ie]

Putting up an early prototype for discussion.

This is a pretty simple approach which allows rendering Prman OSL nodes with spline parameters, and also translating them to and from USD. ( Validated by rendering the resulting USD in USDView with the Prman hydra backend ).

For discussion:

• Why do we prefix shader types when writing USD? I assume that just deleting this code is not an actual solution, but it does seem to be required if we want to write shaders that are compatible with USDView.
• Is it OK to drive this just based on the shader name ( would be nice to at least at least check if the shader is an OSL shader ... that will be necessary if we need to support both OSL Pxr shaders and non-OSL Pxr shaders with different spline conventions ... but we seem to lose the shader type when round tripping to USD )?
• If we were going to drive this with a registry, there would be a bunch of questions about how to parameterize the registry - is the registry driven by a shader name prefix? Or does every shader name that contains splines need to get individually registered ( for greater flexibility )? Does the registry just return the suffixes for the 3 plugs that make up a spline, or do we allow for the possibility that a renderer might have a spline representation that doesn't directly map? Maybe all this is a good argument that we shouldn't create a registry yet, and wait until we have more data on what is required before we expose anything in the API.

If we're happy with this style of hardcoding, then potentially all that really needs to be done is adding in support for 3delight as well ( and potentially non-OSL Prman nodes ).

[danieldresser-ie]

I should follow up since I did notice the answer to one of my questions ... the interface for selecting the convention can't be quite as simple as the prototype lookupSplinePlugSuffixes, since 3delight converts the basis to an array of ints ( though that could just be one extra flag if we're OK with hardcoding things ).

[danieldresser-ie]

Oh, and right before bed, I finally put 2 and 2 together ... the reason why the shader type isn't round-tripping is probably because I broke that by removing the shader name prefix for USDView compatibility.

[johnhaddon]

Why do we prefix shader types when writing USD? I assume that just deleting this code is not an actual solution, but it does seem to be required if we want to write shaders that are compatible with USDView.

git blame wasn't of much use here, but I think the answer is that we were matching Autodesk's convention for Arnold shaders. We did all our initial testing with Arnold, so that would make sense.

In general, I guess anyone could choose any naming convention, because it's all down to how they register their shaders with RenderMan's Sdr registry? But we're currently working without that registry, to avoid having to ship everyone's USD plugins with Gaffer.

Is it OK to drive this just based on the shader name?

I think so. A quick glance at a mix of C++ and OSL Pxr shaders suggests that they all use the same convention, so I'm not sure we need to care about OSL vs C++, unless you've seen otherwise?

is the registry driven by a shader name prefix?

I'd suggest we use the same key as use for Gaffer's shader metadata registry - <type>:<name> as in ai:surface:standard_surface. Combined with wildcards that would let us register conversions for whole sets of shaders at once, and override for individual shaders.

Maybe all this is a good argument that we shouldn't create a registry yet, and wait until we have more data on what is required before we expose anything in the API.

That's fine with me. As things stand I think I'd be happier with the extra 3Delight-specific flag than I would a more complex mechanism, but a wait-and-see-approach seems wise anyway.

the reason why the shader type isn't round-tripping is probably because I broke that by removing the shader name prefix for USDView compatibility.

I don't think we've ever round-tripped shader type - see https://github.com/GafferHQ/gaffer/blob/main/python/GafferRenderManTest/RenderManShaderTest.py#L104-L134. I think this may be because we've been discussing removing the concept of type (as in surface vs shader etc) entirely, since OSL was moving away from it?

[boberfly]

Hey @danieldresser-ie not sure if you saw here but I did some initial testing here if it helps: #1450

What I've learned of the convention that Prman uses for splines is a common match to how Katana is doing them (maybe one influenced the other? Not sure) and KtoA tends to match this convention as well, despite the differently named inputs (and lack of interpolation arrays, it is just a constant remapped to the array). And yep C++ and OSL are 1:1 here as well.

[danieldresser-ie]

Coming back to this now that the Gaffer side is mostly working and I'm trying to get up more final PRs for both:

Re: Prefixing with shader type:
I think the answer is that we were matching Autodesk's convention for Arnold shaders. We did all our initial testing with Arnold, so that would make sense.

Does this mean that we should only prefix Arnold shaders with "arnold:", and leave off the prefix for everything else? I guess I need to run tests of how Arnold handles osl shaders ( does it require an "osl:" prefix )? Not sure what we'll do if Arnold only supports OSL with an "osl:" prefix, and PRMan only supports OSL without an "osl:" prefix.

I don't think we've ever round-tripped shader type - see https://github.com/GafferHQ/gaffer/blob/main/python/GafferRenderManTest/RenderManShaderTest.py#L104-L134. I think this may be because we've been discussing removing the concept of type (as in surface vs shader etc) entirely, since OSL was moving away from it?

It's true that we've been moving away from the surface vs shader distinction ( that test fails because ri:surface comes back in as ri:shader ). But with the change in this PR to match how the Hydra backend expects PRMan shaders, we lose the renderer prefix as well: "ri:surface" comes back as just "surface". Maybe that's OK too?

[johnhaddon]

Does this mean that we should only prefix Arnold shaders with "arnold:", and leave off the prefix for everything else?

I don't think so - as we discussed yesterday, not changing behaviour for current productions is of prime importance. We should change behaviour for RenderMan shaders only.

But with the change in this PR to match how the Hydra backend expects PRMan shaders, we lose the renderer prefix as well: "ri:surface" comes back as just "surface". Maybe that's OK too?

Are you talking about the shader type here, or the attribute name? We definitely should not change the attribute name. Ideally we'd keep the ri: in the type too.

[danieldresser-ie]

Updated this for discussion. The ShaderNetworkAlgo commit now deals reasonably with the 4th plug used by the PRMan spline convention.

I've also updated the IECoreUSD::ShaderAlgo commit with a possible way forward: pass in the attribute where we're assigning the shader, so we can skip the type prefix when we're outputting RenderMan shaders. Note that this doesn't help if an OSL shader is directly connected to a shader assignment - that results in assigning to the attribute "osl:surface", so it doesn't trip my check for "ri", and the shaders still get prefixed. But maybe we're "lucky" here, because I've never been able to get RenderMan to render an OSL shader directly - it seems to require an explicit surface shader as the last thing in the chain ( or maybe it would take an OSL shader that returns a closure? No one seems to be using those much? ).

To reply to some of your comments:

Are you talking about the shader type here, or the attribute name?

I had been a bit confused, but this only affects shader type. The attribute name is fine.

Ideally we'd keep the ri: in the type too.

Keeping "ri:" in the type definitely prevents USDView from rendering the shaders through PRMan. Unless you mean just keeping it in the shader type on the Gaffer side, and exploring other ways of storing it while in USD? I guess we could stuff it in some sort of metadata?

[johnhaddon]

Thanks for the update Daniel. As well as considering my inline comments, we'll need to add some test coverage for the unprefixing of Prx/Lama names, the RenderMan spline convention and the Count suffix.

[danieldresser-ie]

Addressed inline comments, added two more commits with tests for IECoreUSD and ShaderNetworkAlgo

[johnhaddon]

Thanks for updates Daniel - let's get this squashed and merged. Looks like we need to retarget the PR to RB-10.6 as well, and update the changelog - I can do that last bit if you prefer.

[johnhaddon]

For future consideration, this would suggest that it works for Cycles, although it's not clear if it's what they prefer or just something they tolerate : https://github.com/blender/cycles/blob/main/src/hydra/material.cpp#L435-L437. I think long-term it's actually a bit unclear what the default should be, and I fear that different renderers are going to want different things for OSL shaders. Anyway, that's a battle for another day.

[danieldresser-ie]

True, does look like they will accept "cycles:". It still seems like we don't have any example of a renderer that actually wants an "osl:" prefix though, right? Once we're ready to break compatibility, there isn't much benefit in the "osl:" prefix if the only thing that is being done with it is running it through pipeline code that replaces it with "arnold:", which I think is what you said is currently happening?

[danieldresser-ie]

Retargeted, squashed ( and took a pass at writing Changelog entries )