search

AcademySoftwareFoundation / OpenPBR

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

Remove provision for specular_weight to exceed 1 #228

Closed portsmouth closed 2 days ago

portsmouth commented 2 months ago

As discussed on Slack, the spec is currently inconsistent about the range of specular_weight.

For dielectrics, we allow it to exceed 1, as a sort of convenience. For metals, we don't (or at least didn't intend to, as it breaks energy conservation), but this doesn't make sense since there is only one parameter with a defined range.

This PR makes the specular_weight consistently bounded in $[0,1]$. As noted by @peterkutz

I noticed this inconsistency while implementing this specular scaling a few days ago. I wasn't sure whether it was intentional but I implemented it following the ranges in the spec, so the specular weight is uncapped for dielectrics and capped at 1 for metals.

I agree that it would be cleaner to just remove the ability to set the specular weight higher than 1. If the specular weight is being set using a 0–1 texture, it won't go higher than 1 anyway, and if it's being set via a constant or an HDR texture, then the IOR or metal colors could just be set directly.

This also simplifies the implementations slightly.

image

image

portsmouth commented 2 months ago

Corresponding change to MaterialX graph to follow..

AdrienHerubel commented 2 months ago

Since this is a potentially look breaking change without a way to restore the previous look, we need to discuss which branch/version this change would target before considering merging. It is also arguably a loss in functionality so I would like to see if there is a consensus.

portsmouth commented 2 weeks ago

Corresponding change to MaterialX graph to follow..

Done in aee8093ef98a05d6bfa0658e9108d8cdf8414004 (just removes a clamp).

portsmouth commented 1 week ago

If we decide to go with the proposal of https://github.com/AcademySoftwareFoundation/OpenPBR/pull/238, we can delete this PR.

portsmouth commented 2 days ago

Closing, as we decided to retain the specular_weight > 1 functionality.