Model lighting options #7025

bagnell · 2018-09-10T23:44:25Z

Adds Model.iblFactor which scales the IBLColor computed in the fragment shader.
Adds Model.lightColor which changes the sunlight color and intensity. For example, settingiblFactor = 0.0 makes the model much darker so the light intensity could be increased.
Adds the same properties to Cesium3DTileset

cesium-concierge · 2018-09-10T23:44:27Z

Thanks for the pull request @bagnell!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Changes to third party files were made.
- Looks like 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.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

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

🌍 🌎 🌏

bagnell · 2018-09-10T23:46:18Z

Master:

This branch:

mramato · 2018-09-12T13:46:30Z

Source/Scene/Batched3DModel3DTileContent.js

@@ -384,7 +384,9 @@ define([
                uniformMapLoaded : batchTable.getUniformMapCallback(),
                pickIdLoaded : getPickIdCallback(content),
                addBatchIdToGeneratedShaders : (batchLength > 0), // If the batch table has values in it, generated shaders will need a batchId attribute
-                pickObject : pickObject
+                pickObject : pickObject,
+                iblFactor : tileset.iblFactor,


is ibla common enough abbreviation for our users, or should this be imageBasedLightingFactor?

mramato · 2018-09-12T13:47:01Z

That looks like a pretty awesome improvement.

emackey · 2018-09-13T21:22:34Z

Have you tried this with PBR models? The second image here shows an intense amount of contrast for what is a pretty plain white material, which makes me wonder how full PBR materials might perform. Haven't had time to try this branch myself yet.

Ideally, I would much rather see cities like this use AO instead of crazy amounts of diffuse contrast to visually separate the buildings.

pjcozzi · 2018-09-15T19:01:24Z

As discussed offline, could we please include the killer gray building visual quality example perhaps akin to what @emackey suggests?

@bagnell @lilleyse

pjcozzi · 2018-09-15T19:03:17Z

What prompted this PR? Are iblFactor and lightColor common practice in PBR/IBL? Can you point me to other engines / surveys?

pjcozzi · 2018-10-06T15:23:27Z

Is this stalled on IBL?

bagnell · 2018-10-08T21:12:32Z

What prompted this PR? Are iblFactor and lightColor common practice in PBR/IBL? Can you point me to other engines / surveys?

I was going to add an option to enable/disable IBL, but I added a scale when I saw it in the glTF reference implementation:
https://github.com/KhronosGroup/glTF-WebGL-PBR/blob/master/shaders/pbr-frag.glsl#L164

…the diffuse and specular separately.

bagnell · 2018-10-09T21:53:41Z

This is ready for another look.

Here is the PBR model with the diffuse factor set to 0.0:

For reference, here is what's in master:

Is this the difference users have mentioned on the forum?

EDIT: The difference in specular lighting is because I didn't set the clock to the same time.

pjcozzi · 2018-10-10T16:36:36Z

@lilleyse can you please review to take this over the finish line?

lilleyse · 2018-10-10T19:01:48Z

Source/Scene/Model.js

@@ -282,6 +286,8 @@ define([
     * @param {Number} [options.silhouetteSize=0.0] The size of the silhouette in pixels.
     * @param {ClippingPlaneCollection} [options.clippingPlanes] The {@link ClippingPlaneCollection} used to selectively disable rendering the model.
     * @param {Boolean} [options.dequantizeInShader=true] Determines if a {@link https://github.com/google/draco|Draco} encoded model is dequantized on the GPU. This decreases total memory usage for encoded models.
+     * @param {Number} [options.imageBasedLightingFactor=1.0] Scales the IBL lighting from the earth, sky, atmosphere and star skybox.


The doc should be Cartesian2 instead of Number.

lilleyse · 2018-10-10T19:02:45Z

Source/Scene/Model.js

+        /**
+         * The color and intensity of the sunlight used to shade the model.
+         * <p>
+         * For example, disabling additional light sources by setting <code>model.imageBasedLightingFactor = 0.0</code> will make the


Same note here.

lilleyse · 2018-10-10T19:05:10Z

Source/Scene/Model.js

+                if (value === lightColor || Color.equals(value, lightColor)) {
+                    return;
+                }
+                this._regenerateShaders = this._regenerateShaders || (defined(lightColor) && !defined(value)) || (defined(value) && !defined(lightColor));


Should it also regenerate the shader if the previous color was not (0,0,0) but the new color is (0,0,0) (and the reverse)?

Some of the other documentation makes it seem as if setting lightColor to undefined would result in using the default sun color. Should that be the behavior here?

Meant to delete this comment. See #7025 (comment) instead.

lilleyse · 2018-10-10T19:24:39Z

Source/Scene/processPbrMaterials.js

            fragmentShader += '    vec3 lightColor = vec3(1.5, 1.4, 1.2);\n';
+            fragmentShader += '#else \n';


The default light color could also share the gltf_lightColor uniform.

Then USE_CUSTOM_LIGHT_COLOR might be just USE_LIGHT_COLOR, and switched off if the user sets the color to (0,0,0).

Looking at https://github.com/AnalyticalGraphicsInc/cesium/pull/7017/files#r216167338 and the Color doc, is this because colors are not allowed to have components greater than 1?

I'm not sure about that comment. Can you explain @ggetz? Colors aren't supposed to have values >1 but you can as long as you aren't converting the components to bytes. Perhaps this should be a Cartesian3?

I'd also like to keep the uniform the way it is since the compiler could optimize out the uniform when not set.

It was because I was using a Color with components greater than 1. Cartesian3 would work. 👍

lilleyse · 2018-10-10T19:27:29Z

Specs/DataSources/ModelGraphicsSpec.js

@@ -1,5 +1,6 @@
 defineSuite([


There should also be tests for Cesium3DTileset (b3dm and i3dm) and Model. It should be enough to check that the pixel color changed after setting the lighting values.

bagnell · 2018-10-16T20:00:25Z

@lilleyse Updated. This is ready for another look.

lilleyse · 2018-10-19T17:50:07Z

Thanks @bagnell.

bagnell added 2 commits September 10, 2018 17:59

Add options to enable/disbale ibl lighting and sun light intensity.

ae40eb9

Add ibl option and sun light color options to 3D Tiles.

d741cb2

mramato reviewed Sep 12, 2018

View reviewed changes

bagnell added 6 commits October 9, 2018 05:56

Merge branch 'master' into model-lighting-options

7a3a4f4

Fix after merge.

0b2029c

Rename to imageBasedLightingFactor and it is now a vec2 which scales …

c73dbc8

…the diffuse and specular separately.

Add ibl factor to model entities.

7c0f5bc

Update tests.

9e17dc6

Add light color to model entities and update CHANGES.md.

f90dc62

lilleyse reviewed Oct 10, 2018

View reviewed changes

lilleyse mentioned this pull request Oct 12, 2018

HDR Rendering #7017

Merged

4 tasks

bagnell added 4 commits October 15, 2018 17:43

Fix doc.

d18d192

Merge branch 'master' into model-lighting-options

69437f4

Change lightColor to Cartesian3.

ca42a1b

Add tests.

7dba70b

lilleyse merged commit 6795351 into master Oct 19, 2018

lilleyse deleted the model-lighting-options branch October 19, 2018 17:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model lighting options #7025

Model lighting options #7025

bagnell commented Sep 10, 2018

cesium-concierge commented Sep 10, 2018

bagnell commented Sep 10, 2018

mramato Sep 12, 2018

mramato commented Sep 12, 2018

emackey commented Sep 13, 2018

pjcozzi commented Sep 15, 2018

pjcozzi commented Sep 15, 2018

pjcozzi commented Oct 6, 2018

bagnell commented Oct 8, 2018

bagnell commented Oct 9, 2018 •

edited

Loading

pjcozzi commented Oct 10, 2018

lilleyse Oct 10, 2018

lilleyse Oct 10, 2018

lilleyse Oct 10, 2018

lilleyse Oct 10, 2018

lilleyse Oct 10, 2018 •

edited

Loading

lilleyse Oct 12, 2018

bagnell Oct 15, 2018

bagnell Oct 15, 2018

ggetz Oct 16, 2018

lilleyse Oct 10, 2018

bagnell commented Oct 16, 2018

lilleyse commented Oct 19, 2018

		fragmentShader += ' vec3 lightColor = vec3(1.5, 1.4, 1.2);\n';
		fragmentShader += '#else \n';

Model lighting options #7025

Model lighting options #7025

Conversation

bagnell commented Sep 10, 2018

cesium-concierge commented Sep 10, 2018

bagnell commented Sep 10, 2018

Choose a reason for hiding this comment

mramato commented Sep 12, 2018

emackey commented Sep 13, 2018

pjcozzi commented Sep 15, 2018

pjcozzi commented Sep 15, 2018

pjcozzi commented Oct 6, 2018

bagnell commented Oct 8, 2018

bagnell commented Oct 9, 2018 • edited Loading

pjcozzi commented Oct 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Oct 16, 2018

lilleyse commented Oct 19, 2018

bagnell commented Oct 9, 2018 •

edited

Loading

lilleyse Oct 10, 2018 •

edited

Loading