Added command line generation of KHR_materials_common #185
Conversation
|
After experimenting a bit, this still needs to create lights from the lights in the technique. That should be added shortly. |
mergeBuffers uses the old buffer name if not specified
|
This now generates lights from techniques. |
|
I found an issue where Box doesn't quite have the right lighting post-generation. I think this is because there is no ambient light generated in the KHR_materials_common sample model, but I'm not sure when it's okay to not have it since it is generated for some of the others. |
|
Good timing with this PR: CesiumGS/cesium#4669 I'll review this shortly. |
| * | ||
| * @param {Object} gltf A javascript object containing a glTF asset. | ||
| * @param {Object} [kmcOptions] KHR_materials_common options for material generation. | ||
| * @returns {Object} The glTF asset with generated normals. |
| name: 'defaultAmbient', | ||
| type: 'ambient' | ||
| }; | ||
| } |
There was a problem hiding this comment.
I found an issue where Box doesn't quite have the right lighting post-generation. I think this is because there is no ambient light generated in the KHR_materials_common sample model, but I'm not sure when it's okay to not have it since it is generated for some of the others.
I'm not sure why an ambient light needs to be added. When Cesium or other engines encounter khr_materials_common with an ambient value wouldn't they just modify the shader?
There was a problem hiding this comment.
It looks like we have to specify the light for KHR_materials_common, or the ambient part of the shader doesn't get generated. My solution for this is to detect if the technique uses the ambient parameter and create an ambient light in the extension if it does, assuming that since the technique uses the parameter, the shader does as well.
|
When going through the API rather than the command line, the technique/doubleSided are undefined if not supplied, but they should be set to the defaults somewhere. |
| gltfExtensions = {}; | ||
| gltf.extensions = gltfExtensions; | ||
| } | ||
| materialsCommon = {}; |
There was a problem hiding this comment.
Does this need deleteExtras?
There was a problem hiding this comment.
I don't think so. We don't add an extras object, which is where that property would live.
That's the model. It has ambient set to [1,1,1,1]. If you set it to [0,0,0,1] you get more what you would expect. |
|
@lilleyse, this should be ready to go now. |
Ah didn't even look there. |
|
Looks good, Thanks! |
All of the pieces are in place now to open this PR. This is being used to regenerate the 1.0 sample models before the migration to 1.1, and will likely be used for that as well.