Update technique render states and support for KHR_blend #387

ggetz · 2018-06-27T17:59:05Z

Following #376, processes legacy technique render states, setting material doubleSided and alphaMode properties where neccisary, and moving blending functions to the EXT_blend extension.

@lilleyse As far as this PR, is there anything else I have to do to address material.alphaMode (specifically for MASK, and material.alphaCutoff)?

Part of #330

lilleyse

Thanks @ggetz!

Maybe also check in on KhronosGroup/glTF#1302 and let everyone know that we plan on supporting the extension in gltf-pipeline and cesium, and whether more updates are expected.

lilleyse · 2018-06-28T01:26:48Z

README.md

@@ -13,7 +13,7 @@ Content pipeline tools for optimizing [glTF](https://www.khronos.org/gltf) asset
 Supports common operations including:
 * Converting glTF to glb (and reverse)
 * Saving buffers/textures as embedded or separate files
-* Converting glTF 1.0 models to glTF 2.0 (using the [KHR_techniques_webgl](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_techniques_webgl) extension)
+* Converting glTF 1.0 models to glTF 2.0 (using the [KHR_techniques_webgl](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/KHR_techniques_webgl) and [EXT_blend](https://github.com/KhronosGroup/glTF/tree/master/extensions/2.0/Khronos/EXT_blend) extensions)


Is EXT_blend expected to be the final name, or will it be KHR_blend? It seems like a bunch of extension proposals have that prefix but I'm not sure whether that's the actual prefix or just a placeholder.

lilleyse · 2018-06-28T01:29:33Z

lib/updateVersion.js

@@ -841,6 +842,8 @@ function glTF10to20(gltf) {
    underscoreApplicationSpecificSemantics(gltf);
    // Clamp camera parameters
    clampCameraParameters(gltf);
+    // Update material properties and add blending functions to EXT_blend extension, removing technique render states
+    updateMaterialProperties(gltf);


The name of this helper could be more descriptive. Maybe moveTechniqueStatesToExtension.

Technically some properties are being set to the material and not the extension, but I think that's ok.

lilleyse · 2018-06-28T01:40:28Z

lib/updateMaterialProperties.js

+module.exports = updateMaterialProperties;
+
+function isStateEnabled(renderStates, state) {
+    return (renderStates.enable.indexOf(state) > -1);


Will need to check if enable is defined first.

lilleyse · 2018-06-28T01:41:14Z

lib/updateMaterialProperties.js

+            // If BLEND is enabled, the material should have alpha mode BLEND
+            // and save the blend functions to the EXT_blend extension
+            if (isStateEnabled(renderStates, WebGLConstants.BLEND)
+                    || defined(renderStates.blendEquationSeparate) || defined(renderStates.blendFuncSeparate)) {


Should be renderStates.functions.blendEquationSeparate and renderStates.functions.blendFuncSeparate like it is below.

lilleyse · 2018-06-28T01:53:50Z

lib/updateMaterialProperties.js

+                if (defined(renderStates.functions)) {
+                    blendingForTechnique[techniqueIndex] = {
+                        blendEquation: renderStates.functions.blendEquationSeparate,
+                        blendFactors: renderStates.functions.blendFuncSeparate


The draft extension doesn't say, but both blendEquation and blendFactors are probably required. Use default values if blendEquationSeparate or blendFuncSeparate are undefined. (the glTF 1.0 defaults are probably good).

Actually they are required by the extension, so I should definitely provide defaults.

lilleyse · 2018-06-28T02:05:47Z

lib/updateMaterialProperties.js

+                materialProperties.doubleSided = true;
+            }
+
+            delete techniqueLegacy.state;


Should be techniqueLegacy.states

(Though I don't think it actually matters since it won't be included in the final KHR_techniques_webgl technique)

I'm going to keep this here, if only because it allows this stage to run independently of later stages.

lilleyse · 2018-06-28T02:12:25Z

specs/lib/updateMaterialPropertiesSpec.js

+
+        gltf = {
+            programs: {
+                program_0: {


To make this more condensed clone the previous gltf and add the new state.

This can probably be done to make all the tests shorter. Have a single glTF above the tests that is cloned/edited by each test.

lilleyse · 2018-06-28T02:13:17Z

specs/lib/updateMaterialPropertiesSpec.js

+                }
+            ],
+            extensionsUsed: [],
+            extensionsRequired: []


Remove these two if they aren't used.

lilleyse · 2018-06-28T02:19:31Z

specs/lib/updateMaterialPropertiesSpec.js

+
+        var material = gltfWithBlendExtension.materials[0];
+        expect(material.alphaMode).toBeUndefined();
+        expect(material.extensions).toBeDefined();


At first I was confused why the extensions were defined, and then noticed KHR_techniques_webgl. Maybe just remove that.

lilleyse · 2018-06-28T02:37:56Z

lib/updateMaterialProperties.js

+                if (defined(renderStates.functions)) {
+                    blendingForTechnique[techniqueIndex] = {
+                        blendEquation: renderStates.functions.blendEquationSeparate,
+                        blendFactors: renderStates.functions.blendFuncSeparate


It looks like gltf 1 has more blend factors than the draft extension.

ZERO ONE SRC_COLOR ONE_MINUS_SRC_COLOR SRC_ALPHA ONE_MINUS_SRC_ALPHA DST_ALPHA ONE_MINUS_DST_ALPHA DST_COLOR ONE_MINUS_DST_COLOR CONSTANT_COLOR ONE_MINUS_CONSTANT_COLOR CONSTANT_ALPHA ONE_MINUS_CONSTANT_ALPHA SRC_ALPHA_SATURATE

vs

ZERO ONE SRC_COLOR ONE_MINUS_SRC_COLOR SRC_ALPHA ONE_MINUS_SRC_ALPHA DST_ALPHA ONE_MINUS_DST_ALPHA DST_COLOR ONE_MINUS_DST_COLOR

If one of the old ones are encountered maybe just switch to the default value? Or not add the extension?

lilleyse · 2018-06-28T02:43:00Z

@lilleyse As far as this PR, is there anything else I have to do to address material.alphaMode (specifically for MASK, and material.alphaCutoff)?

I think what you have here is good!

ggetz · 2018-06-28T19:01:25Z

Thanks @lilleyse, updated !

I bumped KhronosGroup/glTF#1302 to ask about the prefix and if there will be any more updates.

lilleyse

The updates look good, just some small comments on the tests.

lilleyse · 2018-06-29T15:16:56Z

specs/lib/moveTechniqueRenderStatesSpec.js

+
+describe('updateMaterialProperties', function() {
+    it('sets material.doubleSided property if CULL_FACE is not enabled', function () {
+        var gltfWithUpdatedMaterials = moveTechniqueRenderStates(gltf);


gltf should be cloned first.

This may also be causing intermittent failures when tests are run in random order, which I think jasmine does now?

lilleyse · 2018-06-29T15:18:00Z

specs/lib/moveTechniqueRenderStatesSpec.js

+        var material = gltfWithUpdatedMaterials.materials[0];
+        expect(material.doubleSided).toBeUndefined();
+
+        var gltfDoubleSided = JSON.parse(JSON.stringify(gltf));


Maybe use Cesium's clone function instead. clone(gltf, true).

I was having issues with the depth of the cloning when it came to arrays (even when using the deep clone argument). This was much more reliable.

Is that a Cesium bug? I don't think that should be happening...

The documentation isn't partially clear on what the behavior should be: https://cesiumjs.org/Cesium/Build/Documentation/clone.html

Alright, if this way works consistently then let's go with it.

lilleyse · 2018-06-29T15:38:40Z

specs/lib/moveTechniqueRenderStatesSpec.js

+    ]
+};
+
+describe('updateMaterialProperties', function() {


Update the name to moveTechniqueToRenderStates.

ggetz · 2018-06-29T16:04:16Z

@lilleyse Alright, updated! And the tests are now consistently passing.

lilleyse · 2018-06-29T16:10:58Z

Cool - will merge once we get word on the extension name.

ggetz · 2018-07-11T13:56:04Z

@lilleyse Extension name updated to KHR_blend and updated this PR accordingly.

lilleyse · 2018-07-11T14:57:07Z

Thanks @ggetz!

ggetz added 5 commits June 20, 2018 16:31

Support EXT_blend

d098f0f

Merge branch 'techniques-webgl' into ext-blend

ae6ccf1

Merge branch 'techniques-webgl' into ext-blend

8d2597b

Merge branch '2.0' into ext-blend

ba956d5

Update material properties based on renderStates

5d3fdf2

ggetz requested a review from lilleyse June 27, 2018 17:59

lilleyse reviewed Jun 28, 2018

View reviewed changes

ggetz mentioned this pull request Jun 28, 2018

KHR_blend KhronosGroup/glTF#1302

Closed

4 tasks

Cleanup moveTechniqueRenderStates

f2b9a18

lilleyse reviewed Jun 29, 2018

View reviewed changes

Cleanup specs

bfbda29

ggetz force-pushed the ext-blend branch from 05818e2 to bfbda29 Compare June 29, 2018 15:41

ggetz added 4 commits July 2, 2018 14:52

Merge branch 'build-cesium' into ext-blend

4ac3887

Merge branch 'build-cesium' into ext-blend

f7dd8a4

Remove .js when requiring modules

81a7f23

Merge branch '2.0' into ext-blend

0a8988e

ggetz changed the title ~~Update technique render states and support for EXT_blend~~ Update technique render states and support for KHR_blend Jul 11, 2018

Rename EXT_blend -> KHR_blend

3068102

ggetz force-pushed the ext-blend branch from 9e9473e to 3068102 Compare July 11, 2018 14:32

lilleyse merged commit 0e9e3f0 into 2.0 Jul 11, 2018

lilleyse deleted the ext-blend branch July 11, 2018 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update technique render states and support for KHR_blend #387

Update technique render states and support for KHR_blend #387

ggetz commented Jun 27, 2018

lilleyse left a comment •

edited

Loading

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

ggetz Jun 28, 2018

lilleyse Jun 28, 2018

ggetz Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse Jun 28, 2018

lilleyse commented Jun 28, 2018

ggetz commented Jun 28, 2018

lilleyse left a comment

lilleyse Jun 29, 2018

lilleyse Jun 29, 2018 •

edited

Loading

lilleyse Jun 29, 2018

ggetz Jun 29, 2018

lilleyse Jun 29, 2018

ggetz Jun 29, 2018

lilleyse Jun 29, 2018

lilleyse Jun 29, 2018

ggetz commented Jun 29, 2018

lilleyse commented Jun 29, 2018

ggetz commented Jul 11, 2018

lilleyse commented Jul 11, 2018

Update technique render states and support for KHR_blend #387

Update technique render states and support for KHR_blend #387

Conversation

ggetz commented Jun 27, 2018

lilleyse left a comment • 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

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

Choose a reason for hiding this comment

lilleyse commented Jun 28, 2018

ggetz commented Jun 28, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Jun 29, 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

Choose a reason for hiding this comment

ggetz commented Jun 29, 2018

lilleyse commented Jun 29, 2018

ggetz commented Jul 11, 2018

lilleyse commented Jul 11, 2018

lilleyse left a comment •

edited

Loading

lilleyse Jun 29, 2018 •

edited

Loading