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

Add clamp of metal Fresnel to allow for specular_weight > 1 #238

Merged

Conversation

portsmouth
Copy link
Contributor

@portsmouth portsmouth commented Sep 30, 2024

Rather than eliminating the specular_weight > 1 case (as proposed in #228), we discussed keeping this but fixing up the metal logic to ensure the Fresnel remains bounded. This makes the corresponding change needed in the spec. (Note that now the specular_weight parameter consistently, for both metal and dielectric, has soft-range $[0,1]$ and full range $[0, \infty]$).

It would be good to double check that the behaviour of the metal looks reasonable for high specular_weight values (presumably, similar to the dielectric where the Fresnel saturates).

image

@portsmouth
Copy link
Contributor Author

portsmouth commented Sep 30, 2024

The MaterialX implementation of the metal lobe has the form:

    <generalized_schlick_bsdf name="metal_bsdf" type="BSDF">
      <input name="weight" type="float" interfacename="specular_weight" />
      <input name="color0" type="color3" nodename="metal_reflectivity" />
      <input name="color82" type="color3" nodename="metal_edgecolor" />
      <input name="roughness" type="vector2" nodename="main_roughness" />
      <input name="normal" type="vector3" interfacename="geometry_normal" />
      <input name="tangent" type="vector3" interfacename="geometry_tangent" />
    </generalized_schlick_bsdf>

so simply passes the specular_weight along to the BSDF. In the current implementations, this weight is just multiplied into the BSDF (probably assuming that the weight is in [0,1]), e.g. for GLSL:

bsdf.response = D * F * G * comp * occlusion * weight / (4.0 * NdotV);

So this would need to be modified in the implementations, i.e. here the F * weight factors would be replaced with min(1.0, F * weight). Obviously, ideally the MaterialX spec itself should mention that the clamp has to be done, if the weight can exceed 1 (assuming it's OK to allow that in MaterialX), otherwise it's not clear how the implementations derive from the spec.

(Or alternatively, we could clamp the weight before suppling it to generalized_schlick_bsdf and thus the boosted Fresnel would just not be available in the MaterialX implementations).

@portsmouth
Copy link
Contributor Author

Another approach to this is to just clamp the specular_weight in [0,1] before multiplying by the metal Fresnel, which we could do. That is less consistent with how the dielectric behaves though (i.e. specular_weight > 1 would make the dielectric Fresnel brighter, but not the metallic).

@portsmouth portsmouth changed the base branch from main to dev_1.2 October 1, 2024 19:33
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.

This looks clear to me and straight to the point. 👍
I just have a cosmetic comment.

Comment on lines +564 to +565
**`specular_roughness`** | Roughness | `float` | $ [0, 1] $ | | $ 0.3 $ | Roughness of NDF of BRDF $f_\mathrm{conductor}$
**`specular_roughness_anisotropy`** | Anisotropy | `float` | $ [0, 1] $ | | $ 0 $ | Anisotropy of NDF of BRDF $f_\mathrm{conductor}$
Copy link
Contributor

Choose a reason for hiding this comment

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

If the BRDF adjective is removed here but not in the other places, it makes the wording a bit inconsistent. Both are fine, I just have a slight preference for the verbose version.
image
image

@portsmouth
Copy link
Contributor Author

Hopefully it's clear enough that in the boxed formula, the min operation is supposed to be applied component-wise, since $\mathbf{F}_{82}$ is an RGB color.

image

@virtualzavie
Copy link
Contributor

Hopefully it's clear enough that in the boxed formula, the min operation is supposed to be applied component-wise, since F 82 is an RGB color.

That's how I understood it when I read it. Let's see if anyone chimes in.

@portsmouth
Copy link
Contributor Author

portsmouth commented Oct 7, 2024

In 8fcff68 I added a footnote to make the notation for the clamp more explicit

image

image

(and also refer to it in another place where we do a similar clamp with shortened notation, instead of just saying the notation is "obvious"):

image

@portsmouth
Copy link
Contributor Author

portsmouth commented Oct 8, 2024

I implemented the clamping logic in Arnold, and show below the effect before and after, on a copper-like metal material with the specular_weight increasing exponentially. The clamp gradually saturates the Fresnel components (so the orange color shifts first to yellow, then finally to white/mirror Fresnel). Without the clamp, all the Fresnel components are boosted uniformly, producing energy gain.

This effect of saturating the Fresnel (which tends to produce progressively desaturated colors starting near the silhouette), could be seen as some loose approximation of what happens when metallic IOR is increased. This can also be simulated of course by making the base and specular color closer to white (a similar redundancy to that between changing the specular_weight and changing the IOR of the dielectric).

I don't really like having that redundancy, but it seems relatively harmless (users may not even attempt to use specular_weight > 1), and there can be cases where it is convenient.

specular_weight no clamp with clamp
0.0 clamp_off_sp0 0 clamp_on_sp0 0
0.5 clamp_off_sp0 5 clamp_on_sp0 5
1.0 clamp_off_sp1 0 clamp_on_sp1 0
2.0 clamp_off_sp2 0 clamp_on_sp2 0
4.0 clamp_off_sp4 0 clamp_on_sp4 0
8.0 clamp_off_sp8 0 clamp_on_sp8 0
16.0 clamp_off_sp16 0 clamp_on_sp16 0
32.0 clamp_off_sp32 0 clamp_on_sp32 0

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This change looks fine to me, thanks @portsmouth!

@jstone-lucasfilm jstone-lucasfilm merged commit 878a7e6 into AcademySoftwareFoundation:dev_1.2 Nov 7, 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.

3 participants