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

Initial support for EON diffuse #1822

Merged

Conversation

jstone-lucasfilm
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm commented May 19, 2024

This changelist adds initial support for the EON diffuse model in MaterialX, enabled through the use of an energy_compensation option on the oren_nayar_diffuse_bsdf node.

For now, MaterialX shader generation only supports the new option in GLSL, ESSL, and MSL, with other shading languages to be added once this initial implementation has been vetted by the community.

See the OpenPBR whitepaper at openpbr.org for full details on the EON diffuse model, including its formal definition and goals.

This changelist adds initial support for the EON diffuse model in MaterialX, enabled through the use of an energy_compensation flag on the oren_nayar_diffuse_bsdf node.

For now, only GLSL/ESSL/MSL support the new functionality in MaterialX shader generation, with other shading languages to be added once this initial implementation has been vetted by the community.

See the OpenPBR whitepaper at openpbr.org for full details on the EON diffuse model, including its formal definition and goals.
@jstone-lucasfilm
Copy link
Member Author

CC'ing @portsmouth, @peterkutz, @fpsunflower, and @niklasharrysson for their thoughts and recommendations.

@fpsunflower
Copy link

Related PR on the OSL side: AcademySoftwareFoundation/OpenShadingLanguage#1817

I've adopted the same "energy_compensation" int parameter.

@portsmouth
Copy link

portsmouth commented May 20, 2024

I see you're implementing this as basically the existing Oren-Nayar in MaterialX (i.e. QON), augmented with the QON energy-compensation from the paper. We didn't recommend doing that.

I'd suggest instead to have the compensated Oren-Nayar be a separate model, i.e. what we called EON, which is obtained by compensating Fujii's Oren-Nayar model (FON) -- not QON.

Otherwise you will inherit from QON some artifacts, and the compensation is a bit weird since QON violates energy conservation for some input angles, meaning the compensation term then has to be negative (so in the colored case, this might not conserve energy -- since the compensation will be subtracting a smaller amount).

It seems that in the OSL version, Chris actually did what we recommended, i.e. he implements EON, not compensated QON. So the implementation here is also inconsistent with the OSL version.

@jstone-lucasfilm
Copy link
Member Author

@portsmouth That makes sense, and I was originally thinking that only the energy compensation term would differ when the energy_compensation option was enabled, with the original A and B terms remaining unchanged, but I see that the EON proposal changes more than just the energy compensation.

I'll take a second pass at this logic, taking the full extent of the EON proposal into account, and see how things look.

@jstone-lucasfilm
Copy link
Member Author

@portsmouth @fpsunflower I believe the latest logic should be ready for review, and I've now added support for multi-scatter color correction in the indirect lighting path, which uses the real-time-friendly approach of storing pre-integrated irradiance in a texture.

@portsmouth
Copy link

portsmouth commented May 23, 2024

Do we want base_weight to be multiplied into the BRDF (as the multiplicative "weight" of the MaterialX BSDF -- as you implemented it), or instead have the parameter $\rho$ be base_weight * base_color (and have no overall weight multiplier)? The latter is actually what we currently say in the OpenPBR spec, i.e. base_weight is a "scalar multiplier to base_color". (So strictly, your implementation should have the BRDF weight be 1, and feed base_weight * base_color into the color).

It depends what we want base_weight to mean though. Perhaps your the current interpretation actually makes more sense than what we say in the spec, since then the base_weight functions simply as a linear multiplier of the BRDF (and thus the albedo). Which arguably is more consistent with the behaviour of the original Oren-Nayar. (Our paper doesn't mention this issue, since there is no "weight" or "color", just the $\rho$ parameter).

I'd suggest we consider changing the OpenPBR spec itself to match that. (It would be a fairly harmless change for before 1.0 I think).

I will make an OpenPBR PR proposing this, for discussion and so we don't forget it.

cc @peterkutz @fpsunflower

@jstone-lucasfilm
Copy link
Member Author

Yes, I would expect the base_weight to be a linear multiplier on the lobe, rather than having it influence the computation of multi-scattered energy compensation within the BSDF.

If that matches your sense as well, then I'd agree that we should clarify this in the OpenPBR specification, and aim to merge that change before OpenPBR v1.0.

@jstone-lucasfilm jstone-lucasfilm merged commit d3637cd into AcademySoftwareFoundation:dev_1.39 May 23, 2024
34 checks passed
jstone-lucasfilm pushed a commit to AcademySoftwareFoundation/OpenPBR that referenced this pull request May 31, 2024
…entation (#205)

As discussed in AcademySoftwareFoundation/MaterialX#1822 (comment), the MaterialX implementation assumed that the `base_weight` acts as an overall multiplier of the EON BRDF. While in the OpenPBR spec, we had `base_weight` instead multiplying `base_color`.

In fact the MaterialX intepretation seems better, since then the `base_weight` functions as a linear modulation of the overall albedo, rather than doing something non-linear and dependent on the color. This PR makes the necessary changes to the wording/math of the spec to implement this.
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.

None yet

3 participants