Expose functionality to generate tangents and binormals #203

mlimper · 2017-01-02T21:45:44Z

Adding command line flag and code to generate tangents and binormals for a glTF asset (for example for usage with PBR pipelines). To perform the actual computation, Cesiums GeometryPipeline.computeBinormalAndTangent is used.

Usage:

node ./bin/gltf-pipeline.js -i input.gltf --tangentsBinormals -o output.gltf

pjcozzi · 2017-01-04T15:33:15Z

lib/Pipeline.js

-    // run AO after optimizeForVertexCache since AO adds new attributes.
+    // run generation of tangents / binormals and AO after
+    // optimizeForVertexCache since those steps add new attributes.
+    if (defined(options.tangentsBinormals) && options.tangentsBinormals) {


Why not just

if (defined(options.tangentsBinormals)) {

Exchanged by just

if (options.tangentsBinormals) {

(otherwise it evaluates to true when the flag is set to false)

pjcozzi · 2017-01-04T15:33:33Z

lib/Pipeline.js

@@ -30,6 +31,7 @@ var writeGltf = require('./writeGltf');
 var writeBinaryGltf = require('./writeBinaryGltf');
 var writeSource = require('./writeSource');

+


Minor, but extra whitespace.

pjcozzi · 2017-01-04T15:33:53Z

lib/cesiumGeometryToGltfPrimitive.js

@@ -1,4 +1,4 @@
-'use strict';
+    'use strict';


Throughout, please preserve the existing formatting.

pjcozzi · 2017-01-04T15:39:55Z

lib/cesiumGeometryToGltfPrimitive.js

-    } else if (semantic === 'POSITION' || semantic === 'NORMAL') {
+    } else if (semantic.indexOf('POSITION') === 0 ||
+               semantic.indexOf('NORMAL')   === 0 ||
+               semantic.indexOf('TANGENT')  === 0 ||


The glTF spec does not define NORMAL and TANGENT attributes. See

https://github.com/KhronosGroup/glTF/tree/master/specification/1.0#parametersemantic

Perhaps we should add them for 1.1 since it is trivial?

Otherwise, the PBR extension could define them, but they would need to be prefixed with _ here so the glTF asset is still valid before the extension is added.

Let's discuss in KhronosGroup/glTF#812.

pjcozzi · 2017-01-04T15:40:34Z

lib/cesiumGeometryToGltfPrimitive.js

        type = 'VEC3';
    } else {
        throw new DeveloperError('Unsupported attribute semantic: ' + semantic);
    }
    if (semantics.length <= 0) {
-        primitive.attributes[semantic] = createAccessor(gltf, packedLength, type, WebGLConstants.FLOAT, WebGLConstants.ARRAY_BUFFER);
+
+        var id = undefined;


I don't think this will pass JSHint.

Just var id;.

pjcozzi · 2017-01-04T15:41:10Z

lib/cesiumGeometryToGltfPrimitive.js

+        var id = undefined;
+
+        // all attributes but texcoords may occur only once
+        if (semantic != 'TEXCOORD') {


This won't pass JSHint.

Use !== for exact comparison without type coercion.

pjcozzi · 2017-01-04T15:43:03Z

lib/generateTangentsBinormals.js

+    return gltf;
+}
+
+//TODO: copied from generateNormals, could be shared


Could you please move this to a separate file (mark it @private so it is not part of the public API) add a unit test for it?

After a second look, I just saw that there's also a getPrimitives function in PrimitiveHelpers.js. I used that one now in getNormalsand getTangentsBinormals:
ffa215c

The difference is, however, that this function accesses the primitives directly from the meshes, not via the nodes. I believe this makes more sense in this case - otherwise, when going over the nodes, we would potentially iterate over a single mesh multiple times, if it is being shared by several nodes?

I also moved the old function from generateNormals, which iterates over the nodes, into the PrimitiveHelpers file: ef141f0
However, right now, this function is not being used anywhere.

Does that make sense, should it be kept like this?

pjcozzi · 2017-01-04T15:43:42Z

Thanks @mlimper, this is a nice contribution! More PBR contributions are very welcome.

pjcozzi · 2017-01-06T21:50:37Z

README.md

@@ -41,6 +41,7 @@ node ./bin/gltf-pipeline.js -i ./specs/data/boxTexturedUnoptimized/CesiumTexture
 |`--compressTextureCoordinates`, `-c`|Compress the testure coordinates of this glTF asset.|No, default `false`|
 |`--removeNormals`, `-r`|Strips off existing normals, allowing them to be regenerated.|No, default `false`|
 |`--faceNormals`, `-f`|If normals are missing, they should be generated using the face normal.|No, default `false`|
+|`--tangentsBinormals`|If normals and texture coordinates are given, generate tangents and binormals.|No, default `false`|


Could you also update CHANGES.md please?

Sorry, I forgot to mention last time.

pjcozzi · 2017-01-06T21:54:37Z

lib/PrimitiveHelpers.js

@@ -185,6 +185,31 @@ function getPrimitivesByMaterialMode(primitives) {
    return primitivesByMaterialMode;
 }

+function getPrimitivesFromNodes(gltf) {


If this isn't used anywhere, I would remove it for now.

pjcozzi · 2017-01-06T21:56:21Z

Can you please add new tests to https://github.com/AnalyticalGraphicsInc/gltf-pipeline/tree/master/specs/lib?

pjcozzi · 2017-01-06T21:56:42Z

Just those comments and this is ready! Thanks again for contributing and moving forward PBR so quickly!

pjcozzi · 2017-01-07T16:10:30Z

@mlimper also note that binormal should be renamed to bitangent, KhronosGroup/glTF#812 (comment)

mlimper · 2017-01-09T14:09:09Z

Just removed the unused function, refactored to "bitangent" (instead of "binormal") throughout*, and added a test case.

(*)Exception: As Cesium is still using "binormal", the respective parts are still using the old term:
mlimper@2403a93

EDIT: Issue with conflict / duplicate commits in history fixed. Should be fine and ready for merge now.

…mals with existing getAllPrimitives (not via nodes)

… generateNormals and generateTangentsBinormals)

…ption

pjcozzi · 2017-01-10T01:45:09Z

lib/Pipeline.js

@@ -88,7 +89,7 @@ Pipeline.processJSONWithExtras  = function(gltfWithExtras, options) {
    }
    addDefaults(gltfWithExtras, options);
    RemoveUnusedProperties.removeAll(gltfWithExtras);
-    generateNormals(gltfWithExtras, options);
+    generateNormals(gltfWithExtras, options);    


In future PRs, please be mindful of this extra whitespace added to lines like this.

Oh yes, sure - thanks for the pointer, and thanks for the helpful code reviews!

pjcozzi · 2017-01-10T01:45:26Z

Great, thanks again @mlimper!

mlimper mentioned this pull request Jan 2, 2017

Senior Design Roadmap moneimne/WebGL-PBR#1

Closed

8 tasks

pjcozzi reviewed Jan 4, 2017

View reviewed changes

pjcozzi mentioned this pull request Jan 4, 2017

Add BINORMAL and TANGENT attribute semantics KhronosGroup/glTF#812

Closed

pjcozzi reviewed Jan 4, 2017

View reviewed changes

pjcozzi reviewed Jan 6, 2017

View reviewed changes

Max Limper added 14 commits January 9, 2017 21:32

Merged code to expose generation of tangents and binormals

bc2bc8a

moved tangent / binormal generation command after optimization commands

afbd3fe

proper naming for generated tangent / binormal accessors

56c848c

README option entry for tangents / binormals

e841928

smaller changes from review

5bd7665

replaced getPrimitives from generateNormals and generateTangentsBinor…

c522437

…mals with existing getAllPrimitives (not via nodes)

added (currently unused) helper getPrimitivesFromNodes (previously in…

e993c47

… generateNormals and generateTangentsBinormals)

avoiding explicit check for defined when checking tangentsBinormals o…

84f0e5d

…ption

Removed unused getPrimitivesFromNodes from PrimitiveHelpers

3205405

renamed binormal to bitangent throughout

a4ad7f1

using Cesiums binormal attribute for bitangents for now

2403a93

added code to convert glTF tangents and bitangents to cesium geometry

1dfe6b4

added test for generation of tangents and binormals

c708290

Updated changelog (tangentsBitangents option)

20d4167

mlimper force-pushed the master branch from 3b30b29 to 20d4167 Compare January 9, 2017 20:36

pjcozzi reviewed Jan 10, 2017

View reviewed changes

pjcozzi merged commit 681f453 into CesiumGS:master Jan 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose functionality to generate tangents and binormals #203

Expose functionality to generate tangents and binormals #203

mlimper commented Jan 2, 2017

pjcozzi Jan 4, 2017

mlimper Jan 6, 2017 •

edited

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

pjcozzi Jan 4, 2017

mlimper Jan 6, 2017

pjcozzi commented Jan 4, 2017

pjcozzi Jan 6, 2017

pjcozzi Jan 6, 2017

pjcozzi commented Jan 6, 2017

pjcozzi commented Jan 6, 2017

pjcozzi commented Jan 7, 2017

mlimper commented Jan 9, 2017 •

edited

pjcozzi Jan 10, 2017

mlimper Jan 10, 2017

pjcozzi commented Jan 10, 2017

		@@ -30,6 +31,7 @@ var writeGltf = require('./writeGltf');
		var writeBinaryGltf = require('./writeBinaryGltf');
		var writeSource = require('./writeSource');

Navigation Menu

Expose functionality to generate tangents and binormals #203

Expose functionality to generate tangents and binormals #203

Conversation

mlimper commented Jan 2, 2017

Choose a reason for hiding this comment

mlimper Jan 6, 2017 • edited

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

pjcozzi commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 6, 2017

pjcozzi commented Jan 6, 2017

pjcozzi commented Jan 7, 2017

mlimper commented Jan 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jan 10, 2017

mlimper Jan 6, 2017 •

edited

mlimper commented Jan 9, 2017 •

edited