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

Some minor rewording improvements/clarifications #218

Open
wants to merge 28 commits into
base: dev_1.2
Choose a base branch
from

Conversation

portsmouth
Copy link
Contributor

@portsmouth portsmouth commented Jun 12, 2024

Some minor changes are needed to improve the clarity of the wording (around the implementation of the coat details). I discovered these while using the spec myself to implement the Arnold version.

image

image

image

image

jstone-lucasfilm and others added 4 commits June 8, 2024 11:25
This changelist updates the OpenPBR default example, matching its values to the latest default values of the shading model.
From 1.5 to 1.4.  

As this won't make much difference to the look in implementations that ignore the adjacent IORs of the film.

But for those that take it into account, this will make the film visible rather than invisible by default (since `specular_ior` is 1.5 by default, and `coat_ior` 1.6).
@portsmouth portsmouth changed the base branch from main to dev_1.1 June 12, 2024 16:35
This changelist enables Zeltner sheen in the reference implementation of OpenPBR, leveraging the new functionality in MaterialX 1.39.

Additionally, the open_pbr_velvet.mtlx example has been updated to account for the visual differences between Conty-Kulla and Zeltner sheen.
@portsmouth
Copy link
Contributor Author

portsmouth commented Jun 24, 2024

Needs a review. cc @jstone-lucasfilm @virtualzavie @peterkutz

portsmouth and others added 3 commits June 25, 2024 09:37
This changelist updates the types associated with physical color values for subsurface scattering in OpenPBR, aligning with the conclusions of recent threads on ASWF Slack channels.

- Change `subsurface_radius_scale` from a `vector3` to a `color3` in the specification, aligning with the MaterialX implementation of OpenPBR.
- Change the `radius` input of `subsurface_bsdf` from a `vector3` to a `color3` in the MaterialX implementation, aligning with the current definition of the `subsurface_bsdf` node in MaterialX 1.39.
Copy link
Contributor

@virtualzavie virtualzavie left a comment

Choose a reason for hiding this comment

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

Looks good to me.

index.html Outdated Show resolved Hide resolved
@portsmouth
Copy link
Contributor Author

image

@jstone-lucasfilm
Copy link
Member

This change looks great to me, though it needs to be proposed as a change to dev_1.2!

@portsmouth portsmouth changed the base branch from dev_1.1 to dev_1.2 October 1, 2024 19:31
@portsmouth
Copy link
Contributor Author

portsmouth commented Oct 5, 2024

The formula we give for the fuzz layering can be written in a more simplified form (the algebra is trivial). This makes it a bit clearer why the MaterialX form of the layering works, i.e.:

   <!-- Fuzz Layer -->
    <sheen_bsdf name="fuzz_bsdf" type="BSDF">
      <input name="weight" type="float" interfacename="fuzz_weight" />
      <input name="color" type="color3" interfacename="fuzz_color" />
      <input name="roughness" type="float" interfacename="fuzz_roughness" />
      <input name="normal" type="vector3" interfacename="geometry_normal" />
      <input name="mode" type="string" value="zeltner" />
    </sheen_bsdf>

    <layer name="fuzz_layer" type="BSDF">
      <input name="top" type="BSDF" nodename="fuzz_bsdf" />
      <input name="base" type="BSDF" nodename="coat_layer" />
    </layer>

which does $\mathbf{layer}(\texttt{F} f_\mathrm{fuzz}, f_c)$ (i.e. just layer the fuzz BRDF scaled by the fuzz weight), which is what the formula in green is doing below (in albedo-scaling form). Intuitively, layering a partially present BRDF on a base, in albedo-scaling approximation is equivalent to just scaling the BRDF by the presence weight.

Whereas it's not (immediately) obvious that red formula does that as well.

Before 30293bc
image

After 30293bc
image

Though one must assume also that the MaterialX albedo-scaling ignores the fuzz color in the fuzz brdf albedo computation (which it does in the test render implementations), since the fuzz BRDF contains a color factor which is explicitly ignored in the albedo used in the layering formula given.

@portsmouth
Copy link
Contributor Author

@jstone-lucasfilm can be merged?

@portsmouth
Copy link
Contributor Author

Added a small clarification to the text to remind that the specular_weight needs to be appropriately clamped to prevent Fresnel > 1 in the darkening calculation:

image

@portsmouth
Copy link
Contributor Author

@jstone-lucasfilm Can this be merged?

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.

3 participants