AcademySoftwareFoundation / OpenPBR

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

Number and range notation #56

Closed virtualzavie closed 1 year ago

virtualzavie commented 1 year ago

Before:

image

After:

image
portsmouth commented 1 year ago

Does it make sense for color3 components to exceed 1 at the upper range? Perhaps it does and that means outside the gamut of the color space?

virtualzavie commented 1 year ago

To be honest, I am not entirely sure. For colors that correspond to an absorption (base_color in the dielectric case, coat_color, subsurface_color...) I think it does make sense as an approximation of fluorescence. For specular reflection however (base_color in the metal case, specular_color), I don't know if it makes sense, and I don't know if it may cause problems in the renderer either. Outside of this physical interpretation, yes, naybe that can be interpreted as a different color gamut.

Like @AdrienHerubel said, we don't want to restrict the range unless it breaks the rendering.

portsmouth commented 1 year ago

For colors that correspond to an absorption (base_color in the dielectric case, coat_color, subsurface_color...) I think it does make sense as an approximation of fluorescence.

If we're talking about albedos, I don't think we want to allow albedo > 1, as this means more energy is reflected than was incident, i.e. violates energy conservation (whereas fluorescence I think just means that the incident energy is redistributed and emitted at a different wavelength, e.g. UV light becomes visible. (And we're not modelling that here..).

It gets a bit murky when dealing with RGB albedos instead of an albedo per wavelength, as then the color channel means the albedo averaged over many wavelengths (with some weight function depending on the color space). Still, I think this averaging should not be able to generate an RGB albedo with channels greater than 1.

peterkutz commented 1 year ago

The problem with using albedos greater than one to approximate fluorescence is that that would repeatedly and exponentially boost the path throughput as if there's an infinite supply of invisible light to draw from along a path.

But by the way, true fluorescence could be a useful feature for a future version of the model! The appearances of even basic things like paper or shoelaces are affected by fluorescence.

peterkutz commented 1 year ago

I understand that we don't want to put unnecessary limits on parameters, but I would probably vote to constrain albedo-related colors to [0, 1] in the spec.

Other than that, these changes look good to me!

meshula commented 1 year ago

+1 on constraining color to [0,1]

virtualzavie commented 1 year ago

I've addressed the different comments:

portsmouth commented 1 year ago

I think "Norm" is not the right term, since it means something else in mathematics. Probably explicitly writing "Hard range", "Soft range" is better, assuming that doesn't screw up the table formatting (i.e. try to get Hard/Soft in a separate row above "range" to save space).

portsmouth commented 1 year ago

I think "Norm" is not the right term, since it means something else in mathematics. Probably explicitly writing "Hard range", "Soft range" is better, assuming that doesn't screw up the table formatting (i.e. try to get Hard/Soft in a separate row above "range" to save space).

Changed my mind. "Norm" is OK, since it reads as a special case when the "norm" (i.e. range of typically useful values) is a subset of the range of "possibly useful values".

jstone-lucasfilm commented 1 year ago

Closing this PR, as it has been superseded by #105