-
Notifications
You must be signed in to change notification settings - Fork 251
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
2.0 pbr #274
Conversation
pbr->materialsCommon generation
Thanks @lasalvavida, this will get us some mileage until we are able to get full PBR integrated. Even after that, I wonder if this or some version of this would be useful to keep for slow devices. We plan to derive full PBR support from https://github.com/moneimne/WebGL-PBR with help from @moneimne, @sbtron, @snagy, and crew. |
lib/getJointCountForMaterials.js
Outdated
var jointCount = 1; | ||
if (defined(skin.inverseBindMatrices)) { | ||
jointCount = accessors[skin.inverseBindMatrices].count; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would make more sense to check skin.joints
instead?
lib/getJointCountForMaterials.js
Outdated
var mesh = meshes[meshId]; | ||
var primitives = mesh.primitives; | ||
var primitivesLength = primitives.length; | ||
for (var k = 0; k < primitivesLength; k++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForEach.meshPrimitive
@@ -0,0 +1,47 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that it's a new file, does it need a spec?
lib/pbrToMaterialsCommon.js
Outdated
diffuse : [ 0.0, 0.0, 0.0, 1.0 ], | ||
emission : [ 0.0, 0.0, 0.0, 1.0 ], | ||
specular : [ 0.0, 0.0, 0.0, 1.0], | ||
shininess : [ 0.0 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget, what's the latest on KHR_materials_common
? Is shininess an array now or still a single float. Also should textures indexes be inside an array too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a float, not an array of one float.
https://github.com/KhronosGroup/glTF/tree/2.0/extensions/Khronos/KHR_materials_common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, that's massively out of date... I don't really have a good answer other than that glTF 2.0 materials pre-PBR required all values to be arrays. I will put this in line with what is there currently, but it will probably change to something closer to this when it gets updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm actually going to keep this as is, just considering the dated nature of the KHR_materials_common spec. For example, the spec doesn't include doubleSided
or jointCount
as properties.
If anyone feels strongly opposed to that, I'm definitely open to discussing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, this will probably be replaced by: KhronosGroup/glTF#947 at some point, so I wouldn't worry about it too much
* @returns {Object} The glTF asset with materials converted to KHR_materials_common | ||
*/ | ||
function pbrToMaterialsCommon(gltf) { | ||
var materialJointCount = getJointCountForMaterials(gltf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, transparent
should be incorporated into this.
var baseColorTexture = pbrMetallicRoughness.baseColorTexture; | ||
if (defined(baseColorTexture)) { | ||
values.diffuse = [baseColorTexture.index]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to estimate specular
and shininess
from roughness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be possible to calculate, not estimate, I would expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emackey do you know of any resources for this that I should be looking at?
This is updated. I'm made the structural changes that needed to be made. As far as material changes go, I've left it mostly as-is. I would say this is good to be merged into the 2.0 branch; material and specifically KHR_materials_common changes should probably be addressed at a later time in a different PR |
Sounds good @lasalvavida. There are a couple jsHint errors. |
Updated |
Okay, this should be good now |
@lilleyse, here is the PR as discussed offline.
This adds the function
pbrToMaterialsCommon
, a utility function for doing lossy mapping from PBR -> Blinn shading. This enables partial loading of PBR models in Cesium until we have a more robust rendering solution for PBR.Damaged Helmet sketchfab model rendered this way for reference:
@emackey, any input you have here would also be appreciated