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

Add support for KHR_materials_specular #124

Merged
merged 1 commit into from
Sep 16, 2020
Merged

Add support for KHR_materials_specular #124

merged 1 commit into from
Sep 16, 2020

Conversation

romainguy
Copy link
Contributor

The KHR_materials_specular extension is fairly simple and exposes three new parameters:

  • specularFactor, float, default = 1.0f
  • specularColorFactor, float3, default = {1.0f, 1.0f, 1.0f}
  • specularTexture, a 4-channel texture that combines specularFactor and specularColorFactor

This change has been tested by test_write using a Khronos sample model.

@prideout
Copy link
Contributor

Looks good. For reference, the spec is here and is a part of the "PBR Next" standardization effort.

I hope people won't confuse this with the specular-glossiness extension. The naming scheme that Romain used in the code is consistent with the Khronos spec, so I think it's fine.

@romainguy
Copy link
Contributor Author

The extension is called materials_specular, but I followed the existing convention in cgltf code to elide the materials_ prefix. In general it might be better to stick to the extensions names more closely, if only to make it easier for API users to find what they are looking for. Also, extensions are starting to proliferate and there's a question of whether they should be handled differently in cgltf.

@romainguy
Copy link
Contributor Author

Should I do anything else on this PR?

@jkuhlmann
Copy link
Owner

No, no, no, sorry, all good! 😄

@jkuhlmann jkuhlmann merged commit 468ad38 into jkuhlmann:master Sep 16, 2020
@romainguy
Copy link
Contributor Author

No need to apologize, I appreciate you taking the time to take a look. Thank you!

@jkuhlmann
Copy link
Owner

I kind of wanted to write up my thoughts on a new approach to extensions first and kept this open as a reminder... Anyway, feedback on issue #125 would be appreciated.

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.

3 participants