AcademySoftwareFoundation / OpenPBR

Specification and reference implementation for the OpenPBR Surface shading model
Apache License 2.0
401 stars 17 forks source link

Upgrade reference implementation to 1.39 #188

Closed jstone-lucasfilm closed 1 month ago

jstone-lucasfilm commented 1 month ago

This changelist upgrades the reference implementation of OpenPBR to MaterialX 1.39, enabling the following improvements:

jstone-lucasfilm commented 1 month ago

The latest reference implementation has been validated in the 1.39 build of the MaterialX Viewer, with two adjusted examples shown below:

Brass material with adjusted Specular Color (i.e. F82 edge tint): OpenPbr_Brass_AdjustedSpecularColor

Soap Bubble material with adjusted Thin-Film Thickness: OpenPbr_SoapBubble_AdjustedThickness

portsmouth commented 1 month ago
<multiply name="metal_edgecolor" type="color3">
      <input name="in1" type="color3" interfacename="specular_color" />
      <input name="in2" type="float" interfacename="specular_weight" />
</multiply>

Note that specular_weight is supposed to multiply the entire metallic lobe now (not just the edge tint), as in the snippet from the spec below. So in the graph, the most obvious implementation is to set the weight of the generalized_schlick_bsdf to specular_weight (if it has an overall weight, or otherwise just multiply the reflection tint by the specular_weight).

image

portsmouth commented 1 month ago

Is a version of the MaterialX viewer available for download somewhere (i.e. a Windows binary) with 1.39 support?

jstone-lucasfilm commented 1 month ago

@portsmouth Since MaterialX 1.39 is still in development, we don't provide pre-built binaries for this unreleased version of MaterialX, so developers need to build it from the source in the dev_1.39 branch:

https://github.com/AcademySoftwareFoundation/MaterialX/tree/dev_1.39

To maximize the visibility of OpenPBR, though, we've been updating the MaterialX Web Viewer to include each new release of OpenPBR, and it fully supports all of the functionality in MaterialX 1.39.

My hope is to publish a new release of OpenPBR as soon as we're happy with the latest behavior, and this will then become available in the web viewer as well.

jstone-lucasfilm commented 1 month ago

Thanks for these notes, @portsmouth, and I believe I've addressed all of them in the latest implementation.

Here are a few renders of our example materials in the MaterialX Viewer, using the updated MaterialX 1.39 implementation in this pull request:

OpenPBR Aluminum Brushed: OpenPBR_AluminumBrushed

OpenPBR Glass: OpenPBR_Glass

OpenPBR Pearl: OpenPBR_Pearl

portsmouth commented 1 month ago

Making the specular_weight very large seems to produce a blown out render, which shouldn't happen since the reflectivity should be clamped via this:

    <multiply name="scaled_specular_F0" type="float">
      <input name="in1" type="float" interfacename="specular_weight" />
      <input name="in2" type="float" nodename="specular_F0" />
    </multiply>
    <clamp name="scaled_specular_F0_clamped" type="float">
      <input name="in" type="float" nodename="scaled_specular_F0" />
      <input name="low" type="float" value="0.0" />
      <input name="high" type="float" value="0.99999" />
    </clamp>

So not sure why that is happening.

EDIT:

Ah, it's because these weights should be set to 1.0:

    <dielectric_bsdf name="dielectric_reflection" type="BSDF">
      <input name="weight" type="float" interfacename="specular_weight" />
 ...
    </dielectric_bsdf>

    <dielectric_bsdf name="dielectric_reflection_tf" type="BSDF">
      <input name="weight" type="float" interfacename="specular_weight" />
...
    </dielectric_bsdf>

As specular_weight doesn't function anymore as a simple multiplier of the BSDF (except for the metallic lobe), it works by altering the IOR.

jstone-lucasfilm commented 1 month ago

@portsmouth Since the default value for the weight input of dielectric_bsdf is already 1.0, my thinking was that we should omit this duplicate statement in these node instances:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/documents/Specification/MaterialX.PBRSpec.md#bsdf-nodes

Do you see a different result locally when you manually assign 1.0 as the weight for these nodes?

EDIT: Ah, I see what you mean now. We're currently connecting these weight inputs to specular_weight, and we should disconnect them in the latest graph. I'll go ahead and make this change now.

portsmouth commented 1 month ago

coat_darkening (using the current MaterialX approximation of the spec formulas) works reasonably as expected, when turned on, e.g. here "matt wood" turns into "varnished wood".

No coat:

image

Coat (undarkened, default)

image

Coat (darkened, optional)

image

I think it would not be harmful to have the (more physically correct) darkening be applied by default, with the non-physical "undarkening" effect the opt-in.

jstone-lucasfilm commented 1 month ago

@portsmouth I've fixed the legacy connection from specular_weight to the inputs of the dielectric_bsdf nodes, and the results look good in the MaterialX Viewer.

Let's take your additional suggestion for coat_darkening in a new conversation, as I think it's very reasonable, but I'd like to consider it separately from the current upgrade to MaterialX 1.39.

portsmouth commented 1 month ago

In my view, the new coat roughening is also a good improvement to have applied by default (in this case, we already have this as the default, with no way to turn it off). It was subtly unconvincing when applying a rough coat to see the sharp specular highlight persist.

jstone-lucasfilm commented 1 month ago

@portsmouth Agreed, we should decide upon a consistent approach to coat darkening and coat roughening in OpenPBR v1.0, whether that approach is "enabled by default with opt out" or "disabled by default with opt in", and that sounds like a good discussion for the broader group.

portsmouth commented 1 month ago

Agreed, we should decide upon a consistent approach to coat darkening and coat roughening in OpenPBR v1.0, whether that approach is "enabled by default with opt out" or "disabled by default with opt in", and that sounds like a good discussion for the broader group.

Just to note, for coat roughening the spec just says it assumes the roughening is modeled somehow. It would be better to give a specific suggestion, which we do (plus the MaterialX implementation) in https://github.com/AcademySoftwareFoundation/OpenPBR/pull/172. I suggest we get that into 1.0 as it is more logically complete then, than leaving the implementation undefined.

There's no obvious way to turn the roughening effect off in a way which can be described physically. For example, in a Monte Carlo simulation of the layering, it would be impossible to avoid. So i'd leave that for later investigation (or perhaps it's never needed).