AcademySoftwareFoundation / OpenPBR

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

Add notes about why a coat can darken an underlying surface #88

Closed peterkutz closed 11 months ago

peterkutz commented 1 year ago

These notes describe different physical mechanisms by which a coating layer can darken an underlying surface (or not).

For now, I've added these notes inside an existing relevant warning box, but it would probably be good to put some subset of these notes in the actual spec.

portsmouth commented 1 year ago

Nice notes. If we add these to the spec though, how will we connect this to the coat_affect_X parameters (assuming we're putting them back)?

E.g. should we say something about the existing ad-hoc heuristic being a useful artist-driven approximation?

portsmouth commented 11 months ago

Can we maybe add these notes but keep them hidden in an HTML comment block as below?

<!--
Add comment here
-->

Just, it probably doesn't make sense to give lots of details (in the visible text) about coat darkening effects, if we're not implementing them in the current spec.

portsmouth commented 11 months ago

What shall we do with this discussion @peterkutz, in light of the proposed changes in https://github.com/AcademySoftwareFoundation/OpenPBR/pull/103?

In that PR, we are going to give a specific formula for the coat layering, which explicitly omits the darkening/roughening effects. So we need to say something along the lines of "we would like to point out that we know these physical effects exist, and we may implement them in future iterations. But for now our recommended implementation is to just omit them, and use this specific approximate formulation of the coat layering..". But are we really going to insist e.g. that all implementations must use the naïve/simplistic non-reciprocal albedo scaling? That seems too restrictive and unrealistic, as not all implementations may actually want or be able to do that.

My original hope was that we could be completely specific about the BSDFs and volumes, so then the actual model is fully defined for physical purposes, and the ground truth would just be the correct light transport to all orders of scattering in that structure. For the coat layer we could then just say it is a dielectric with volumetric absorbing layer which has optical depth equal to the coat color, and all other effects follow. Then implementers can choose to approximate this using the naïve albedo scaling and tinting, or they can do something more sophisticated. But it is well-defined what the ground truth should be.

So I'd propose that we try to pose the spec that way -- as completely physically defined (to the extent possible) giving an unambiguous ground-truth, but agnostic about the specific implementation and approximations used, though we offer recommendations and suggestions. (See https://github.com/AcademySoftwareFoundation/OpenPBR/pull/106#discussion_r1344354038 for elaboration).

peterkutz commented 11 months ago

@portsmouth

What shall we do with this discussion @peterkutz, in light of the proposed changes in https://github.com/AcademySoftwareFoundation/OpenPBR/pull/103?

I might have missed something, but your changes in https://github.com/AcademySoftwareFoundation/OpenPBR/pull/103 don't seem to specifically address the issue of whether a non-absorbing coat is supposed to darken the underlying surface due to multiple reflection and absorption events. Although it sounds like your changes and the recent discussions imply that a ground-truth implementation would need to incorporate that light transport and therefore would darken the underlying surface in some cases.

Since the discussion in my PR is just an extension of an existing "!!! WARNING" block and is not in the main text of the spec, I would personally just merge it as is and then make a follow-up PR to condense the whole block into something that can be merged into the main text. We can also do that as part of this PR if necessary, but I don't think it would hurt to have the more detailed version in the git history.

But are we really going to insist e.g. that all implementations must use the naïve/simplistic non-reciprocal albedo scaling?

Yeah, that seems like that would be ok but not ideal.

My original hope was that we could be completely specific about the BSDFs and volumes, so then the actual model is fully defined for physical purposes, and the ground truth would just be the correct light transport to all orders of scattering in that structure. ... it is well-defined what the ground truth should be.

Me too. That seems more ideal.

So I'd propose that we try to pose the spec that way -- as completely physically defined (to the extent possible) giving an unambiguous ground-truth, but agnostic about the specific implementation and approximations used, though we offer recommendations and suggestions.

Sounds good to me.

portsmouth commented 11 months ago

In light of the recent discussion, I've closed #103 now. The discussion about the coat physics can now be part of the spec proper, so not a note or a comment.

I think though we probably should say that the effects to do with the coat "dissolving" into the base are really outside the scope of our model, i.e. we assume that the only relevant physical effects are the light transport through the given BSDFs and media. (We could still mention that effect perhaps, but only to be explicit that it's outside scope).

EDIT: you do say this already, so maybe that's enough:

Since we don't have enough knowledge about underlying substance to determine whether its internal scattering properties should be modified, we can only safely assume that the first mechanism of darkening (i.e., internal reflections), occurs. And the amount of darkening (and saturation) should be based on the known properties of the material (e.g., colors and IORs).

virtualzavie commented 11 months ago

Let's merge this PR, and do a follow up that replace the warning with a summary as part of the description of the coat.