ImageEngine / cortex

Libraries for visual effects software development
Other
531 stars 124 forks source link

USDScene : Don't treat unconnected material outputs as attributes #1385

Closed johnhaddon closed 1 year ago

johnhaddon commented 1 year ago

We were already trying to do this by testing IsAuthored(), but this wasn't catching renderer-specific outputs which had been created but not connected. Loading such outputs as empty ShaderNetworks was then causing downstream errors when trying to render them in Gaffer.

danieldresser-ie commented 1 year ago

Code looks fine. I'm less confident of the philosophical question of whether we ought to support empty shader networks, but happy to defer to you.

johnhaddon commented 1 year ago

I'm less confident of the philosophical question of whether we ought to support empty shader networks

I couldn't really come up with a philosophy I was happy with here, because of the differences between our representation and USD's. We have separate attributes for surface/displacement/etc across arnold/cycles/generic/etc, but USD smooshes those all into single materials. There's a natural mapping from material outputs to our attributes and vice versa, but this leaves us without a good mapping for ShaderNetworks which don't specify an output (of which empty networks are a subset).

On the reading side, we don't have enough information to load a shader network at all - the source for the material output is the starting point for loading, and there simply is no source. And it seems pretty clear that if an output doesn't have a source, then the intent was to not provide a shader for it. So in this case I'm confident that it's far better to not advertise any attribute, than it is to load one with a degenerate (empty) shading network in it. There's no useful information in an empty shading network. All I've done here is clear up a discrepancy in how we were treating the default surface etc outputs and how we're treating the renderer-specific <renderer>:surface outputs.

The writing side is where I'm less sure, and where I didn't touch anything. When the ShaderNetwork has no output, we're still writing out all the individual shaders and their connections, but are then unable to connect them to the material output (because we don't know what shader output to connect). This is what leads to the authored-but-not-connected outputs this PR deals with on the reading side. I think there are two choices here : write what we can, but lose it on loading, or say that a ShaderNetwork without an output is degenerate, and refuse to write it at all. Actually, I think I'd be fairly happy with the latter, if you are?

johnhaddon commented 1 year ago

The writing side is where I'm less sure...I think there are two choices here : write what we can, but lose it on loading, or say that a ShaderNetwork without an output is degenerate, and refuse to write it at all.

Summary of side-channel discussion : we think it's reasonable to stick with the status quo and keep writing what we can, even though we know it won't round-trip. It's a little easier to debug via the USD when you can see in it that there are shaders, but there is no connection to the material - because that reflects the situation on the Cortex side.