Added processPbrMetallicRoughness #301
Conversation
There was a problem hiding this comment.
Great start @moneimne!
While there is some duplicate code between this and processModelMaterialsCommon, I think it's fine for now. I added some comments on more general purpose functions that could be separated out cleanly.
Also once integrated with the rest of the pipeline, start writing tests for this function. processModelMaterialsCommonSpec can serve as inspiration.
lib/processPbrMetallicRoughness.js
Outdated
| defined, | ||
| defaultValue, | ||
| WebGLConstants) { | ||
| 'use strict'; |
There was a problem hiding this comment.
To start getting this integrated into the rest of the pipeline, change the includes to work with Node. Then call processPbrMetallicRoughness in Pipeline.processJSONWithExtras right after processModelMaterialsCommon.
lib/processPbrMetallicRoughness.js
Outdated
| WebGLConstants) { | ||
| 'use strict'; | ||
|
|
||
| function webGLConstantToGlslType(webGLValue) { |
There was a problem hiding this comment.
This is also used in processModelMaterialsCommon so move to its own file.
lib/processPbrMetallicRoughness.js
Outdated
| } | ||
| } | ||
|
|
||
| function glslTypeToWebGLConstant(glslType) { |
lib/processPbrMetallicRoughness.js
Outdated
| } | ||
| } | ||
|
|
||
| function generateTechnique(gltf, material, frameState, optimizeForCesium) { |
lib/processPbrMetallicRoughness.js
Outdated
| /** | ||
| * @private | ||
| */ | ||
| function processPbrMetallicRoughness(gltf, frameState, options) { |
There was a problem hiding this comment.
By convention move this to the top of the file (also fix for processModelMaterialsCommon).
lib/processPbrMetallicRoughness.js
Outdated
| fragmentShader += 'const float M_PI = 3.141592653589793;\n'; | ||
|
|
||
| var lambertianDiffuse = ''; | ||
| lambertianDiffuse += 'vec3 lambertianDiffuse(vec3 baseColor) {\n'; |
There was a problem hiding this comment.
In GLSL code we put the { on a separate line.
lib/processPbrMetallicRoughness.js
Outdated
| fragmentShader += lambertianDiffuse + fresnelSchlick2 + fresnelSchlick + smithVisibilityG1 + smithVisibilityGGX + GGX; | ||
|
|
||
| var fragmentShaderMain = ''; | ||
| fragmentShaderMain += 'void main(void) {\n'; |
lib/processPbrMetallicRoughness.js
Outdated
| var lambertianDiffuse = ''; | ||
| lambertianDiffuse += 'vec3 lambertianDiffuse(vec3 baseColor) {\n'; | ||
| lambertianDiffuse += ' return baseColor / M_PI;\n'; | ||
| lambertianDiffuse += '}\n\n'; |
There was a problem hiding this comment.
Doing a multiline string is usually cleaner. Also use 4 spaces of indentation throughout.
var lambertianDiffuse =
'vec3 lambertianDiffuse(vec3 baseColor) {\n' +
' return baseColor / M_PI;\n' +
'}\n\n';
lib/processPbrMetallicRoughness.js
Outdated
| // Generate fragment shader's lighting block | ||
| var fragmentLightingBlock = ''; | ||
| fragmentLightingBlock += ' vec3 lightColor = vec3(1.0, 1.0, 1.0);\n'; | ||
| fragmentLightingBlock += ' vec3 l = normalize(czm_sunDirectionEC);\n'; |
There was a problem hiding this comment.
Using czm_sunDirectionEC should only happen if optimizeForCesium is true. Otherwise the light direction should originate from the camera (0.0, 0.0, 1.0) like how processModelMaterialsCommon handles it.
lib/processPbrMetallicRoughness.js
Outdated
| // Add normal mapping to fragment shader | ||
| if (hasNormals) { | ||
| fragmentShaderMain += ' vec3 ng = normalize(v_normal);\n'; | ||
| if (defined(parameterValues.normalTexture) && (hasTangents || frameState.context._standardDerivatives)) { |
There was a problem hiding this comment.
Standard derivatives will need to handled differently since we don't have this info available at conversion time. Check if there is a #define associated with the GL_OES_standard_derivatives extension, and add that in here. Check out https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Shaders/ShadowVolumeFS.glsl as an example.
lib/processPbrMetallicRoughness.js
Outdated
|
|
||
| if (optimizeForCesium) { | ||
| fragmentShader += ' vec3 diffuseIrradiance = vec3(0.5);\n'; | ||
| fragmentShader += ' vec3 specularIrradiance = textureCube(czm_cubeMap, r).rgb;\n'; |
There was a problem hiding this comment.
Should this be u_SpecularEnvSampler instead of using a Cesium builtin?
There was a problem hiding this comment.
I should probably clarify the name, but I added the specular environment map as a Cesium builtin for now since anyone using Cesium will use the same map.
lib/processPbrMetallicRoughness.js
Outdated
| fragmentShader += ' vec3 diffuseIrradiance = vec3(0.5);\n'; | ||
| fragmentShader += ' vec3 specularIrradiance = textureCube(czm_cubeMap, r).rgb;\n'; | ||
| fragmentShader += ' specularIrradiance = mix(specularIrradiance, diffuseIrradiance, roughness);\n'; // Fake LOD | ||
| fragmentShader += ' vec2 brdfLUT = texture2D(czm_brdfLUT, vec2(NdotV, 1.0 - roughness)).rg;\n'; |
There was a problem hiding this comment.
Same comment as above, should you only use a Cesium builtin if optimizeForCesium?
There was a problem hiding this comment.
Yup! Made this change in a recent commit.
lib/processPbrMetallicRoughness.js
Outdated
| fragmentShader += ' vec3 color = NdotL * lightColor * (diffuseContribution + specularContribution);\n'; | ||
|
|
||
| if (optimizeForCesium) { | ||
| fragmentShader += ' vec3 diffuseIrradiance = vec3(0.5);\n'; |
There was a problem hiding this comment.
Will there be an option to sample a diffuse environment map too? Similar to how we sample diffuse in the glTF-WebGL-PBR reference implementation.
There was a problem hiding this comment.
As for now, we are using a very simple gradient as the environment map, so I thought a diffuse gray would be acceptable for the diffuse irradiance. The change to using a proper diffuse environment map would be simple to add, but I think this can come with a switch to a more complex environment map.
|
@lilleyse, all the models from the sample repo have been tested and load correctly into Cesium when passed through the pipeline. |
|
The latest code changes look good. There are some jsHint errors and test failures. (Yes, this branch and |
|
Thanks @moneimne. Will merge this into the |
@lilleyse, this is the file that has been tested within Cesium.