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

Fix reference BRDF implementation (#2386) #2392

Merged
merged 6 commits into from May 20, 2024

Conversation

proog128
Copy link
Contributor

This PR fixes the bug with the squared metallic term in the reference BRDF implementation in Appendix B by rewriting the pseudocode according to the proposal of @lisyarus. In addition, all extensions referencing the code are updated.

Besides, I removed a few inconsistencies in the extensions:

  • Snake case is now consistently used for values in the implementation, and camelCase for public parameters.
  • BRDFs are now called xxx_brdf instead of a mixture between f_xxx and xxx_brdf.
  • The clearcoat extension now differentiates more clearly between clearcoat normal (Nc) and material normal (N)
  • The visibility term is now referred to as $\mathcal{V}$ to distinguish it from the view direction $V$.
  • The specular extension now uses the same structure for factor * color multiplications as sheen.

Copy link
Contributor

@bsdorra bsdorra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@proog128 thanks for taking care of these changes! I left some minor comments and corrections.

specification/2.0/Specification.adoc Outdated Show resolved Hide resolved
extensions/2.0/Khronos/KHR_materials_clearcoat/README.md Outdated Show resolved Hide resolved
extensions/2.0/Khronos/KHR_materials_clearcoat/README.md Outdated Show resolved Hide resolved
specification/2.0/Specification.adoc Show resolved Hide resolved
@emackey emackey linked an issue Apr 29, 2024 that may be closed by this pull request
Copy link
Member

@emackey emackey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by PBR TSG during the May 20 meeting.

@emackey emackey merged commit be16643 into KhronosGroup:main May 20, 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.

Wrong pseudocode in BRDF reference implementation
4 participants