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 coat darkening and saturation effects #162

Merged

Conversation

portsmouth
Copy link
Contributor

@portsmouth portsmouth commented Feb 16, 2024

This PR adds a detailed discussion to the spec of the coat darkening effect (incorporating the ideas discussed in #136 and #141), with:

  • introduction of a new coat_darkening parameter (defaulting to 1, the physically correct case).
  • a physical definition of how this parameter operates to reduce the darkening (via boosting of the base albedo)
  • a suggested practical implementation of the darkening factor for the base lobe, taking into account the IOR of the coat, and also the roughness of the base (based on the derivations and discussions linked elsewhere).
  • A suggested implementation of the darkening of colored coats at grazing angles due to longer path length in the absorbing medium.

I also include a placeholder for a discussion of how the coat roughness should affect the underlying lobes. (Probably to be done in a separate PR, and derivation of a reasonable physical approximation for this is WIP).

To make it easier to read, I broke the Coat section into sub-sections.

image

image

image

image

@portsmouth portsmouth marked this pull request as ready for review February 22, 2024 14:50
@portsmouth
Copy link
Contributor Author

portsmouth commented Apr 10, 2024

One aspect of the proposed formulas that I worry about is the effect of the base roughness on the darkening, i.e. Equation 61. This is designed so that a smooth base produces less darkening, basically blending between known formulas for the cases of a completely smooth and completely diffuse base. (See the discussion on Slack here, for background to that -- it is based on the Weta "The Path of Water" notes from SIGGRAPH 2023).

I assumed that if the base is dielectric, the roughness of the reflection from it is determined by the specular_roughness. However if the base has a scattering medium (i.e. as it does in the glossy-diffuse case, or if there is a transmission or subsurface volume) then the reflection is effectively roughened due to this scattering, apart from the initial Fresnel reflection. The proposed formula ignores this effect, so probably underestimates the darkening effect of the coat on a glossy-diffuse base or subsurface. We would need to do some proper investigation with Monte Carlo simulations to know.

Perhaps it's fine to still give these formulas merely as a convenient, reasonable approximation.

index.html Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
index.html Show resolved Hide resolved
@peterkutz
Copy link
Contributor

peterkutz commented Apr 16, 2024

I assumed that if the base is dielectric, the roughness of the reflection from it is determined by the specular_roughness. However if the base has a scattering medium (i.e. as it does in the glossy-diffuse case, or if there is a transmission or subsurface volume) then the reflection is effectively roughened due to this scattering, apart from the initial Fresnel reflection. The proposed formula ignores this effect, so probably underestimates the darkening effect of the coat on a glossy-diffuse base or subsurface.

Is this not addressed by the formula below?

$r_d = \mathrm{lerp}(1, r, \xi_s F_s)$

I thought this was considering the diffuse component to have an effective roughness of $1$.

@peterkutz
Copy link
Contributor

This is looking really good to me at a high level but I would need to do another pass to verify some of the formulas by looking more closely and by looking at the referenced equations and citations. A lot of my comments are questions and clarifications that I could potentially answer myself based on a more careful reading, however I figured I would post them anyway in case they raise any important points and in case the text can be clarified to prevent other readers from initially having the same questions.

jstone-lucasfilm pushed a commit that referenced this pull request Apr 16, 2024
…165)

We need to explain that, although physically the glossy-diffuse slab (as described) should exhibit darkening/saturation due to the same physics as the coat darkening (i.e. multiple bounces inside the gloss layer losing energy as they hit the colored base), we want to prevent this so that the user sees -- to the extent possible -- the base color she selected.

But she can't see the exact base color she selected, because the Fresnel of the gloss is superimposed on the diffuse base lobe. So we make the required color be a blend of the Fresnel and unsaturated base color (i.e. essentially the albedo-scaling formula). This is the same mechanism we used to define how the coat "un-darkening" works. Thus albedo-scaling automatically does the right thing (but if the implementation does something different, e.g. more physically correct, it is still well-defined what the goal albedo needs to be so the appearance won't break).

Note this won't make any difference to implementations that already use the albedo-scaling approximation (e.g. Arnold, MaterialX).

This will need to be tweaked once we get the coat darkening PR #162 in, as it should reference that discussion.
@jstone-lucasfilm
Copy link
Member

@portsmouth I didn't get the sense that any artists liked the original Standard Surface coat darkening, and it seemed both visually unappealing and counterintuitive as a control system. So that would likely be an unnecessary step backwards for the OpenPBR project overall.

@portsmouth
Copy link
Contributor Author

portsmouth commented May 7, 2024

I mean it would have the same coat_darkening control, except adapted to turn on the Standard Surface coat darkening (via the MaterialX graph). You mean that that darkening effect is significantly worse than the OpenPBR one? It doesn't respect the IOR of the coat or base roughness, but for default IOR it should look plausible (just raises the base color to a power).

I don't really agree with the logic of "let's release a less powerful model because implementation X doesn't have the time to ship version 1.234 by such and such date".

@jstone-lucasfilm
Copy link
Member

I'm hopeful that the new OpenPBR version looks better than the original Standard Surface coat darkening, which was virtually never used by artists, but I don't yet have a real-time implementation that I can test and evaluate! The original Standard Surface version was pretty unappealing, though, from a visual and control system perspective.

@portsmouth
Copy link
Contributor Author

portsmouth commented May 7, 2024

Or if there is no alternative, I will bite the bullet and implement the MaterialX graph for the darkening logic before the deadline (Probably some script to generate the XML is faster than attempting to do it my hand).

@portsmouth
Copy link
Contributor Author

portsmouth commented May 7, 2024

I'm hopeful that the new OpenPBR version looks better than the original Standard Surface coat darkening, which was virtually never used by artists, but I don't yet have a real-time implementation that I can test and evaluate! The original Standard Surface version was pretty unappealing, though, from a visual and control system perspective.

We did agree that it looks good verbally in a past meeting, if I recall. We seemed to agree that the darkening looks more natural when turned on than off, which is the default.

As intuitively, one expects e.g. varnished wood to be darker.

@jstone-lucasfilm
Copy link
Member

@portsmouth I'm not concerned about the time that it will take to write the coat darkening logic for OpenPBR in MaterialX, but rather the time that it will take to validate it visually and artistically, as one week is not sufficient time to show the functionality to a wide set of artists and get feedback.

@portsmouth
Copy link
Contributor Author

I think this could be said of almost all the new features in the model. For example, surely the Zeltner sheen, thin film weight, fuzz on top, etc. are not really validated by artists in that sense. I don't think we can hope to achieve that by the 1.0 deadline, for the coat darkening or anything else that changed since Standard Surface.

@portsmouth
Copy link
Contributor Author

I've now merged #157, and the next step would be to resolve conflicts between these two pull requests, after which we can start on the MaterialX-side updates for this change.

So has the position changed since this, as seems like now you're pushing to omit the coat darkening from the 1.0? (On the basis that we don't have time to validate it). We've spent quite a lot of time discussing and developing this darkening parametrization, for it to be thrown out at a late stage due to MaterialX concerns. I have also prototyped it in my viewer app, which has been available for months.

@jstone-lucasfilm
Copy link
Member

@portsmouth I'd strongly support the idea of including coat darkening for a 1.0 release in late June, but at our last meeting @AdrienHerubel specifically suggested that we omit it due to the new, shorter timetable for OpenPBR release. I'd recommend bringing this up with Adrien!

@portsmouth
Copy link
Contributor Author

It's worth noting that looking at a proposed visual feature in side-by-side renders is not the same as giving the control system to an artist and asking for their feedback!

The control system just turns the darkening on and off, so not much to it..

NB, we also had Nikie in the meeting, where I recall she agreed the darkening looks good (though unfortunately we don't have the meetings recorded). So there's one artist validation data point, not sure how many we need to be confident.

I did point out on Slack that it's available to test in my viewer, but didn't get feedback on it (apart from when showing it in the meeting, where the feedback was positive as noted).

@portsmouth
Copy link
Contributor Author

portsmouth commented May 7, 2024

It would be helpful if Adobe/Autodesk implemented/prototyped this in their renderers also.

@jstone-lucasfilm
Copy link
Member

I've had a chance to catch up with @portsmouth in a side discussion, and our current recommendation is to include the coat_darkening control in OpenPBR v1.0, but to assign it a default value of zero.

This will give us the flexibility to implement coat darkening using either BSDF node/layer upgrades or graph logic approximations in future versions of MaterialX, and in the meantime we'll have a visual match between our reference implementation and custom Arnold/Eclair shaders for the common case where coat_darkening is left at its default value.

In future versions of OpenPBR, we would then have the option of assigning a new default value of one to coat_darkening, with MaterialX upgrade logic used to transparently handle documents referencing the v1.0 version of OpenPBR.

@portsmouth
Copy link
Contributor Author

As per the commentary from @jstone-lucasfilm, I've made the change to have coat_darkening default to 0. While I think it's ultimately preferable for the physical darkening effect to happen by default, I think it's reasonable to have the effect be "opt-in" for now, until we have more confidence that it is preferable to change the default.

@jstone-lucasfilm
Copy link
Member

This looks good to me, thanks @portsmouth, and my only remaining question is whether we should add the new input to our reference implementation.

Since the coat darkening is not enabled by default, I wouldn't be as concerned about using a very simple reference implementation such as the color squaring of Standard Surface, if that seems like a reasonable compromise for now.

@portsmouth
Copy link
Contributor Author

This looks good to me, thanks @portsmouth, and my only remaining question is whether we should add the new input to our reference implementation.

Since the coat darkening is not enabled by default, I wouldn't be as concerned about using a very simple reference implementation such as the color squaring of Standard Surface, if that seems like a reasonable compromise for now.

That seems reasonable, I will add it back. Though I'd note that if users decide they don't like the darkening effect on the basis of this, we need to bear in mind that the effect has not been properly implemented yet in MaterialX.

@portsmouth
Copy link
Contributor Author

portsmouth commented May 10, 2024

@jstone-lucasfilm I added an approximate MaterialX implementation of the coat darkening math (based on the spec, not on the old Standard Surface formula). This should provide a slightly more faithful implementation than the old approximation (e.g. it takes into account the coat IOR (approximately), but not the base roughness).

However, it seems the current MaterialX graph (even before the darkening addition) is failing to compile in the viewer, with error

Error in compiling fragment shader:
0(1723) : error C1503: undefined variable "base_substrate_out"
0(1724) : error C1503: undefined variable "base_substrate_out"
0(1798) : error C1503: undefined variable "base_substrate_out"
0(1799) : error C1503: undefined variable "base_substrate_out"
0(1878) : error C1503: undefined variable "base_substrate_out"
0(1879) : error C1503: undefined variable "base_substrate_out"

It's unclear what causes this, maybe it was introduced in a previous commit. Can you check and help to diagnose it?

@jstone-lucasfilm
Copy link
Member

Good catch, @portsmouth, and let me test the reference implementation in main to see what may have broken with respect to the v0.4 version.

@jstone-lucasfilm
Copy link
Member

Ok, I've tracked the issue down, and it was actually a limitation of GLSL generation in MaterialX 1.38, which has now been resolved by @niklasharrysson's refactor of the thin-film node in MaterialX 1.39.

Since we're actually targeting MaterialX 1.39 in OpenPBR, I would say that we're safe to move forward with both @virtualzavie's earlier change to thin-film controls and your current implementation of coat darkening, both of which render successfully in 1.39 builds.

In the near future, I'll add support for 1.39 features such as color82 in our reference implementation, and then fully mark this as a MaterialX 1.39 document for clarity.

@jstone-lucasfilm
Copy link
Member

For visual confirmation of your proposed change to the reference implementation, here's Anton's carpaint material with coat_roughness set to 1.0, and it looks very convincing to me:

image

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 looks great to me, thanks @portsmouth, and let's move forward with this update to OpenPBR.

@jstone-lucasfilm jstone-lucasfilm changed the title Coat darkening/saturation effects, description and parametrization Add coat darkening and saturation effects May 10, 2024
@jstone-lucasfilm jstone-lucasfilm merged commit b231843 into AcademySoftwareFoundation:main May 10, 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