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

2.0 pbr #274

Merged
merged 6 commits into from
Jun 9, 2017
Merged

2.0 pbr #274

merged 6 commits into from
Jun 9, 2017

Conversation

lasalvavida
Copy link
Contributor

@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:
damagedhelmet

@emackey, any input you have here would also be appreciated

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 91.939% when pulling d373dc2 on 2.0-pbr into 1406b32 on 2.0.

@lasalvavida
Copy link
Contributor Author

Here's a few more examples:
lantern
corset

@pjcozzi
Copy link
Contributor

pjcozzi commented May 3, 2017

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.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 3, 2017

@lilleyse just like #274, please merge when ready.

var jointCount = 1;
if (defined(skin.inverseBindMatrices)) {
jointCount = accessors[skin.inverseBindMatrices].count;
}
Copy link
Contributor

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?

var mesh = meshes[meshId];
var primitives = mesh.primitives;
var primitivesLength = primitives.length;
for (var k = 0; k < primitivesLength; k++) {
Copy link
Contributor

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';
Copy link
Contributor

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?

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 ]
Copy link
Contributor

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?

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@lasalvavida lasalvavida May 19, 2017

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

Copy link
Contributor Author

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);
Copy link
Contributor

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];
}
Copy link
Contributor

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?

Copy link

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.

Copy link
Contributor Author

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?

@lilleyse lilleyse mentioned this pull request Jun 5, 2017
14 tasks
@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jun 9, 2017

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

@lilleyse
Copy link
Contributor

lilleyse commented Jun 9, 2017

Sounds good @lasalvavida. There are a couple jsHint errors.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Jun 9, 2017

Updated
edit: failing test

@lasalvavida
Copy link
Contributor Author

Okay, this should be good now

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 91.961% when pulling 8f83cc2 on 2.0-pbr into 1406b32 on 2.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 91.961% when pulling 8f83cc2 on 2.0-pbr into 1406b32 on 2.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 91.961% when pulling 8f83cc2 on 2.0-pbr into 1406b32 on 2.0.

@lilleyse lilleyse merged commit 4308188 into 2.0 Jun 9, 2017
@lilleyse lilleyse deleted the 2.0-pbr branch June 9, 2017 17:14
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.

None yet

5 participants