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

KHR_materials_cmnConstant #1095

Conversation

donmccurdy
Copy link
Contributor

@donmccurdy donmccurdy commented Sep 10, 2017

Based on the syntax of #1075. For mobile devices trying to maintain 90+ FPS, I think it may be worth keeping an unlit shading model available.

@donmccurdy
Copy link
Contributor Author

Hm as @zellski pointed out off thread, emissiveFactor and emissiveTexture are already in the default PBR material spec, so there's nothing new here. We can set an emissive and zero out the other factors.

If an exporter specifies that something should be unlit by zeroing-out the other factors, we expect the importer to check each factor and assign the simpler shading model? I thought we'd intended for exporters to be able to provide both metal/rough and spec/gloss and let the runtime choose which to use, but this leaves us no way of specifying whether unlit shading is explicitly a fallback or not. Maybe that's OK? I'm not sure.

@donmccurdy
Copy link
Contributor Author

donmccurdy commented Sep 11, 2017

On second thought, please ignore the content of this PR for now. I will follow up on #1075.

Updated this PR, it is ready for review and feedback.

@zellski
Copy link
Contributor

zellski commented Sep 20, 2017

Perhaps open this back up, in empty/boolean form, given recent developments in #1075?

@donmccurdy donmccurdy force-pushed the feat-khr-materials-cmnConstant branch 3 times, most recently from fcab14a to 3852fc7 Compare September 20, 2017 02:02
@donmccurdy
Copy link
Contributor Author

Sounds good. I've updated this as an "empty" extension, and it should be ready for review or feedback.

@donmccurdy
Copy link
Contributor Author

Closing this; going to work toward including this along with blinnPhong as a single extension. See #1111.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants