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

Shaders: renaming more functions 2 #22375

Merged
merged 1 commit into from
Aug 20, 2021
Merged

Conversation

WestLangley
Copy link
Collaborator

To be a bit less verbose...

Part A

BRDF_Diffuse_Lambert() -> BRDF_Lambert()

BRDF_Specular_BlinnPhong() -> BRDF_BlinnPhong()

BRDF_Specular_GGX() -> BRDF_GGX()

BRDF_Specular_Sheen() -> BRDF_Sheen()

Part B

BRDF_Specular_Multiscattering_Environment() -> computeMultiscattering()

The above function is not a brdf. It computes two reflectance quantities. So renaming it to computeMultiscattering for now.

BRDF_Specular_GGX_Environment -> EnvironmentBRDF

This is not a brdf either. It returns a reflectance, but "EnvironmentBRDF" is a commonly-used term, so using that for now.

@WestLangley WestLangley added this to the r132 milestone Aug 20, 2021
@WestLangley
Copy link
Collaborator Author

@mrdoob Part A is subjective, and my preference. If you don't like it, I'll revert Part A.

@mrdoob
Copy link
Owner

mrdoob commented Aug 20, 2021

I like the short names 👍

@mrdoob mrdoob merged commit 3cf2719 into mrdoob:dev Aug 20, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 20, 2021

Thanks!

@WestLangley WestLangley deleted the dev_nomenclature3 branch August 20, 2021 10:50
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Sep 10, 2021
It was renamed to `BRDF_Lambert` @ r132, mrdoob/three.js#22375
fixed by including the function for backward compat
0b5vr added a commit to pixiv/three-vrm that referenced this pull request Sep 10, 2021
It was renamed to `BRDF_Lambert` @ r132, mrdoob/three.js#22375
fixed by including the function for backward compat
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.

2 participants