Skip to content
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

Upgrade reference implementation to 1.39 #188

Merged
merged 7 commits into from
May 13, 2024
Merged

Upgrade reference implementation to 1.39 #188

merged 7 commits into from
May 13, 2024

Conversation

jstone-lucasfilm
Copy link
Member

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

  • Enable Hoffman/Schlick Fresnel for metals, connecting the new color82 input of generalized_schlick_bsdf.
  • Update thin-film logic to include unit conversion and more accurate layering.
  • Update swizzle nodes and channel attributes for 1.39 syntax.
  • Remove a handful of default-valued inputs for clarity.

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

- Enable Hoffman/Schlick Fresnel for metals, connecting the new color82 input of generalized_schlick_bsdf.
- Update thin-film logic to include unit conversion and more accurate layering.
- Update swizzle nodes and channel attributes for 1.39 syntax.
- Remove a handful of default-valued inputs for clarity.
@jstone-lucasfilm
Copy link
Member Author

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

Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
@portsmouth
Copy link
Contributor

portsmouth commented May 13, 2024

<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
Copy link
Contributor

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

@jstone-lucasfilm
Copy link
Member Author

@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.

- Update geometry_opacity from a color3 to a float.
- Minor fixes to recent work on anisotropy in OpenPBR.
@jstone-lucasfilm
Copy link
Member Author

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
Copy link
Contributor

portsmouth commented May 13, 2024

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
Copy link
Member Author

jstone-lucasfilm commented May 13, 2024

@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
Copy link
Contributor

portsmouth commented May 13, 2024

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.

- Remove legacy connections from specular weight to dielectric_bsdf nodes.
- Update the type of geometry_opacity in example materials.
@jstone-lucasfilm
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

jstone-lucasfilm commented May 13, 2024

@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
Copy link
Contributor

portsmouth commented May 13, 2024

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 #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).

Copy link
Contributor

@portsmouth portsmouth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Mltx reference could benefit from a bit more commentary in the file, e.g. stating how the calculations map to the equations in the spec, just for clarity. But we can certainly add that later as it would just be documentation.

LGTM!

@jstone-lucasfilm jstone-lucasfilm merged commit 8d83f86 into AcademySoftwareFoundation:main May 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants