Support for emissive-only materials #6501

lilleyse · 2018-04-26T14:39:27Z

This PR fixes two related problems:

Fix model shader generation when normals are missing #5838 - When a glTF 2.0 model does not have normals the color is black.
Emissive only materials would generate plain-gray shaders because processPbrMetallicRoughness only runs if material.pbrMetallicRoughness is defined. Effectively the emissive texture gets ignored.

The code follows @emackey's suggestion in #5838 (comment):

Lately I've been pondering whether Cesium could just show "unlit" materials (baseColor * occlusion + emissive) in the absence of normals. It's technically not what the spec advises, but it's quick to implement and would get us visibility of models that are currently pitch black.

TODO :

More testing
Update CHANGES.md
Open a PR into gltf-pipeline

cesium-concierge · 2018-04-26T14:39:28Z

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

shehzan10 · 2018-04-26T14:44:41Z

I can confirm that this fixes it using the AGI HQ with an emissive-only texture.

lilleyse · 2018-04-26T18:09:42Z

Updated. Confirmed that the models linked in #5838 are now rendering correctly using an unlit shader.

emackey · 2018-04-26T20:18:17Z

@lilleyse Just FYI, my suggestion was poorly received by the rest of the glTF WG. They would strongly prefer that engines do calculate the normal vectors, which shouldn't look bad once the rest of the 2.0 lighting issues are fixed.

That said though, certainly the glTF spec does not call for "pitch black", so, unlit isn't any more out of compliance than we are currently. I think unlit would be a good stepping stone to have in place so long as normals are still on the roadmap somewhere.

It looks like glTF pipeline 1.0 does have automatic calculation of normals in a helper class, and that class was never upgraded to 2.0.

lilleyse · 2018-04-26T20:36:59Z

Yeah I guess generating normals is the right thing to do. It would make sense then to implement KHR_materials_unlit as soon as possible to support photogrammetry.

But I agree, that doesn't necessarily need to hold up this PR. We could keep #5838 open after this is merged.

lilleyse · 2018-04-27T14:23:04Z

Diff ignoring white space: https://github.com/AnalyticalGraphicsInc/cesium/pull/6501/files?w=1

emackey · 2018-04-27T15:48:17Z

Source/ThirdParty/GltfPipeline/processPbrMetallicRoughness.js

+               defined(material.occlusionTexture) ||
+               defined(material.emissiveTexture) ||
+               defined(material.emissiveFactor) ||
+               defined(material.alphaMpde) ||


lilleyse · 2018-04-27T17:23:14Z

Updated.

emackey · 2018-04-27T18:16:46Z

Source/ThirdParty/GltfPipeline/processPbrMetallicRoughness.js

@@ -493,6 +513,9 @@ define([
        } else {
            if (defined(parameterValues.baseColorFactor)) {
                fragmentShader += '    vec4 baseColorWithAlpha = u_baseColorFactor;\n';
+            } else if (!hasNormals && (defined(parameterValues.emissiveFactor) || defined(parameterValues.emissiveTexture))) {
+                // Don't use the default base color of (1,1,1) if the material is emissive only. This handling will be better once we support KHR_materials_unlit.
+                fragmentShader += '    vec4 baseColorWithAlpha = vec4(vec3(0.0), 1.0);\n';


This breaks glTF spec, and I don't think we have any need to do it in code here. The models can and should simply specify baseColorFactor: [0, 0, 0, 1] so that all engines know to turn this off.

Yeah that is the better thing to do. Fixed.

emackey · 2018-04-27T18:17:41Z

Specs/Data/Models/PBR/BoxEmissive/BoxEmissive.gltf

+  ],
+  "materials": [
+    {
+      "emissiveTexture": {


Likewise here, we should add baseColorFactor: [0, 0, 0, 1].

emackey · 2018-04-27T18:22:41Z

CHANGES.md

@@ -41,6 +41,8 @@ Change Log
 * Fixed crash bug in PolylineCollection when a polyline was updated and removed at the same time. [#6455](https://github.com/AnalyticalGraphicsInc/cesium/pull/6455)
 * Fixed Imagery Layers Texture Filters Sandcastle example. [#6472](https://github.com/AnalyticalGraphicsInc/cesium/pull/6472).
 * Fixed a bug causing Cesium 3D Tilesets to not clip properly when tiles were unloaded and reloaded. [#6484](https://github.com/AnalyticalGraphicsInc/cesium/issues/6484)
+* Fixed rendering of glTF models that don't contain normals. [#6501](https://github.com/AnalyticalGraphicsInc/cesium/pull/6501)


"Fixed" isn't what happened here, we are just applying a temporary workaround for these models.

emackey · 2018-04-27T18:58:12Z

Travis apparently failed here. Doesn't look like the fault of this branch, but it's hard for me to tell.

mramato · 2018-04-27T19:00:17Z

The PR-side passed, so it was definitely just a travis/build hiccup.

emackey

Other than the Travis fail, I think this is good to go. Make sure the gltf-pipeline one is updated too.

emackey · 2018-04-27T19:01:27Z

OK, thanks @mramato. Merging.

lilleyse added 2 commits April 26, 2018 12:13

Support for emissive only materials and materials without normals

6ddb1f1

Updated CHANGES.md

3baeb5f

lilleyse force-pushed the emissive-fix branch from 899d845 to 24d159c Compare April 26, 2018 18:08

Added test

e0197db

lilleyse force-pushed the emissive-fix branch from 24d159c to e0197db Compare April 26, 2018 18:09

This was referenced Apr 27, 2018

Generate normals automatically for glTF 2.0 models #6506

Open

Add support for KHR_materials_unlit #6507

Closed

Merge branch 'master' into emissive-fix

1b28b58

lilleyse force-pushed the emissive-fix branch from 24cc9da to 1b28b58 Compare April 27, 2018 14:21

emackey reviewed Apr 27, 2018

View reviewed changes

lilleyse added 2 commits April 27, 2018 12:03

Fix typo

3d55a17

Fix spec model/name

9bb9271

emackey reviewed Apr 27, 2018

View reviewed changes

Updates

8efc17d

emackey approved these changes Apr 27, 2018

View reviewed changes

emackey merged commit 5f56208 into master Apr 27, 2018

emackey deleted the emissive-fix branch April 27, 2018 19:01

lilleyse mentioned this pull request Apr 27, 2018

Support for emissive-only materials CesiumGS/gltf-pipeline#366

Merged

This was referenced Apr 30, 2018

Fix model shader generation when normals are missing #5838

Closed

3D model turn black after conversion into gltf/glb CesiumGS/obj2gltf#134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for emissive-only materials #6501

Support for emissive-only materials #6501

lilleyse commented Apr 26, 2018 •

edited

cesium-concierge commented Apr 26, 2018

shehzan10 commented Apr 26, 2018

lilleyse commented Apr 26, 2018 •

edited

emackey commented Apr 26, 2018

lilleyse commented Apr 26, 2018

lilleyse commented Apr 27, 2018

emackey Apr 27, 2018

lilleyse commented Apr 27, 2018

emackey Apr 27, 2018

lilleyse Apr 27, 2018

emackey Apr 27, 2018

emackey Apr 27, 2018

emackey commented Apr 27, 2018

mramato commented Apr 27, 2018

emackey left a comment

emackey commented Apr 27, 2018

Support for emissive-only materials #6501

Support for emissive-only materials #6501

Conversation

lilleyse commented Apr 26, 2018 • edited

cesium-concierge commented Apr 26, 2018

shehzan10 commented Apr 26, 2018

lilleyse commented Apr 26, 2018 • edited

emackey commented Apr 26, 2018

lilleyse commented Apr 26, 2018

lilleyse commented Apr 27, 2018

emackey Apr 27, 2018

Choose a reason for hiding this comment

lilleyse commented Apr 27, 2018

emackey Apr 27, 2018

Choose a reason for hiding this comment

lilleyse Apr 27, 2018

Choose a reason for hiding this comment

emackey Apr 27, 2018

Choose a reason for hiding this comment

emackey Apr 27, 2018

Choose a reason for hiding this comment

emackey commented Apr 27, 2018

mramato commented Apr 27, 2018

emackey left a comment

Choose a reason for hiding this comment

emackey commented Apr 27, 2018

lilleyse commented Apr 26, 2018 •

edited

lilleyse commented Apr 26, 2018 •

edited