Preserve the existence of non-default shader attributes #175

lasalvavida · 2016-10-21T15:29:18Z

Edit: The scope of this pull request has changed and the title has been updated to reflect that. I've kept the old text here as a record.

~~Single b3dm files can be processed via the glTF pipeline with these changes. This is based on the hard-normals branch, so that is what it is diffed against currently.~~

I am open to suggestions, but I think this is the best way to integrate b3dm support with gltf-pipeline given the current state of the public API, since it requires access to some of the internal machinery of gltf-pipeline.

~~This makes it easy to write a script to process tilesets concurrently using gltf-pipeline.~~

#!/bin/bash
for f in $(find $1/ -name '*.b3dm'); do
    echo $f
    node ./gltf-pipeline.js -i $f -o $f -f -z &
    NPROC=$(($NPROC+1))
    if [ "$NPROC" -ge 4 ]; then
        wait
        NPROC=0
    fi
done

tfili · 2016-10-24T19:53:36Z

@lilleyse Can we get this reviewed? It would be helpful to get this into hard-normals then get that into master soon.

lilleyse · 2016-10-24T21:11:58Z

I made some small fixes.

Talked with @tfili offline. We aren't sure if these b3dm steps belong in the gltf-pipeline. The main tangle point is when normals are generated a new shader is built, and the BATCH_ID needs to be re-inserted in the techniques and shaders. Technically this could be done as a post step in a b3dm specific pipeline.

lilleyse · 2016-10-24T21:18:30Z

@pjcozzi what do you think about keeping this in here vs. 3d-tiles-tools or some other repo?

lasalvavida · 2016-10-25T22:12:41Z

I've been thinking about this a bit, and it may be a better solution to just make sure that BATCHID never gets removed in the first place.

In a more general sense, I think if we generate a new shader from modelMaterialsCommon, it should declare all of the semantics from the primitive, even if it doesn't use them in the shader.

tfili · 2016-10-26T01:30:23Z

@lasalvavida Also one thing I noticed, the way the b3dm handling is currently done I can't call it from another node script. It would be nice if I could call a function in the pipeline like processJSONWithExtras, but all the removing of b3dm headers are in the bin/gltf-pipeline.js.

…pipeline into support-b3dm

Please enter the commit message for your changes. Lines starting

lasalvavida · 2016-10-26T23:40:36Z

I've finished the first part of this. In modelMaterialsCommon, we now preserve the existence of primitive semantics that don't exist by default in the generated shader. I think this is the correct approach and is not a 3d-tiles specific feature, since anyone doing dynamic shader modification using some non-default shader attribute would need this as well.

I'm still looking at the integration of reading and writing b3dms.

lasalvavida · 2016-10-26T23:48:22Z

However, I think I'm going to revert the b3dm changes in this PR for now so we can get it and face normals merged sooner. After this change it would be pretty straightforward to go either path: gltf-pipeline supporting more file types or a 3d-tiles-tool that uses gltf-pipeline.

lasalvavida · 2016-10-26T23:53:28Z

Okay, this should be ready for a look.

pjcozzi · 2016-10-31T18:36:04Z

lib/modelMaterialsCommon.js

@@ -727,6 +729,85 @@ function lightDefaults(gltf) {
    }
 }

+function getShaderVariable(accessor) {


Can you update the spacing in this file to be consistent with the rest of the codebase?

pjcozzi · 2016-10-31T18:36:26Z

Is this already tested by other unit tests?

lasalvavida · 2016-10-31T19:40:30Z

Is this already tested by other unit tests?

Doesn't look like it, I'll add a test.

lilleyse · 2016-11-01T02:13:05Z

lib/modelMaterialsCommon.js

+  if (defined(program)) {
+    vertexShaderId = program.vertexShader;
+    vertexShader = shaders[vertexShaderId];
+  }


Won't a lot of these checks always be true since it has gone through the modelMaterialsCommon? It should always have a material/technique/program. This also lets you simplify the loop.

lilleyse · 2016-11-01T02:13:09Z

lib/modelMaterialsCommon.js

+          var accessorId = attributes[semantic];
+          var accessor = accessors[accessorId];
+          var lowerCase = semantic.toLowerCase();
+          var attributeName = 'a_' + lowerCase;


For the a_batchId case this will produce a_batchid instead.

So I don't think this matters. We replace the program.attribute, technique.attribute, technique.parameter and in the shader with the same case, so Cesium should handle it correctly because of the semantic.

In Cesium we build the shader with the assumption that it's called a_batchId: https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/Cesium3DTileBatchTable.js#L498. Maybe this isn't a good assumption though - instead should we use whatever parameter has the BATCHID semantic?

lilleyse · 2016-11-01T02:13:15Z

lib/modelMaterialsCommon.js

+              var pipelineExtras = vertexShader.extras._pipeline;
+              var shaderText = pipelineExtras.source.toString();
+              shaderText = 'attribute ' + getShaderVariable(accessor) + ' ' + attributeName + ';\n' + shaderText;
+              pipelineExtras.source = shaderText.toString();


Already a string.

lilleyse · 2016-11-01T02:13:18Z

lib/modelMaterialsCommon.js

+      }
+    }
+  }
+}


If the function above be can be shortened enough, I wouldn't see a problem combining the two functions.

lilleyse · 2016-11-01T02:13:33Z

lib/modelMaterialsCommon.js

+    /*
+     * If any primitives have semantics that aren't declared in the generated
+     * shaders, we want to preserve them.
+     */


Use // here.

lilleyse · 2016-11-02T17:25:32Z

The a_batchId issue is addressed in this Cesium PR: CesiumGS/cesium#4583

tfili · 2016-11-02T17:37:29Z

@lilleyse The test has been added and I believe all the PR comments have been addressed.

lilleyse

@tfili just needs some small fixes.

lilleyse · 2016-11-02T17:42:14Z

lib/modelMaterialsCommon.js

+    for (var semantic in attributes) {
+        if (attributes.hasOwnProperty(semantic)) {
+            if (!defined(techniqueParameterForSemantic(technique, semantic))) {
+                if (defined(technique)) {


Check isn't needed.

lilleyse · 2016-11-02T17:42:16Z

lib/modelMaterialsCommon.js

+                        type: accessor.componentType
+                    };
+                    technique.attributes[attributeName] = lowerCase;
+                    if (defined(program)) {


Same comment.

lilleyse · 2016-11-02T17:42:17Z

lib/modelMaterialsCommon.js

+                    technique.attributes[attributeName] = lowerCase;
+                    if (defined(program)) {
+                        program.attributes.push(attributeName);
+                        if (defined(vertexShader)) {


Same comment.

lilleyse · 2016-11-02T17:42:21Z

specs/lib/addDefaultsSpec.js

+        var gltfClone = clone(gltf);
+        addDefaults(gltfClone);
+
+        // Uses the Cesium sun as its default light source


Remove this comment.

tfili · 2016-11-02T18:18:44Z

@lilleyse I think that should be everything.

lasalvavida added 2 commits October 21, 2016 11:26

Added support for b3dm files from the main command line

9b50494

Updated CHANGES.md and README.md

aa4a042

lilleyse added 2 commits October 24, 2016 16:31

Cleanup

c7d3639

Fix path

6b3e1f5

lasalvavida added 4 commits October 26, 2016 18:45

Merge branch 'hard-normals' of github.com:AnalyticalGraphicsInc/gltf-…

d1731d8

…pipeline into support-b3dm

Don't specifically patch batch ids

3002cdd

jsHint fixes

2fed056

Please enter the commit message for your changes. Lines starting

Make semantic existence checking safe

ac42062

lasalvavida added 2 commits October 26, 2016 19:49

Revert b3dm changes

0b789af

Updated to hard-normals branch

50bad06

lasalvavida changed the title ~~Added support for b3dm files from the main command line~~ Preserve the existence of non-default shader attributes Oct 26, 2016

pjcozzi reviewed Oct 31, 2016

View reviewed changes

lilleyse reviewed Nov 1, 2016

View reviewed changes

Tom Fili added 3 commits November 2, 2016 10:27

Merged hard-normals

ace4124

A few tweaks based on PR comments.

eb39af7

Added test for preserving non-standard semantics

94a530e

Style

c73b77d

lilleyse reviewed Nov 2, 2016

View reviewed changes

Small tweaks from PR comments.

c4b86d9

lilleyse merged commit dac565a into hard-normals Nov 2, 2016

lilleyse deleted the support-b3dm branch November 2, 2016 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve the existence of non-default shader attributes #175

Preserve the existence of non-default shader attributes #175

lasalvavida commented Oct 21, 2016 •

edited

Loading

tfili commented Oct 24, 2016

lilleyse commented Oct 24, 2016

lilleyse commented Oct 24, 2016

lasalvavida commented Oct 25, 2016 •

edited

Loading

tfili commented Oct 26, 2016 •

edited

Loading

lasalvavida commented Oct 26, 2016

lasalvavida commented Oct 26, 2016

lasalvavida commented Oct 26, 2016

pjcozzi Oct 31, 2016

pjcozzi commented Oct 31, 2016

lasalvavida commented Oct 31, 2016

lilleyse Nov 1, 2016

lilleyse Nov 1, 2016

tfili Nov 2, 2016

lilleyse Nov 2, 2016

lilleyse Nov 1, 2016

lilleyse Nov 1, 2016 •

edited

Loading

lilleyse Nov 1, 2016

lilleyse commented Nov 2, 2016 •

edited

Loading

tfili commented Nov 2, 2016

lilleyse left a comment

lilleyse Nov 2, 2016

lilleyse Nov 2, 2016

lilleyse Nov 2, 2016

lilleyse Nov 2, 2016

tfili commented Nov 2, 2016

Preserve the existence of non-default shader attributes #175

Preserve the existence of non-default shader attributes #175

Conversation

lasalvavida commented Oct 21, 2016 • edited Loading

tfili commented Oct 24, 2016

lilleyse commented Oct 24, 2016

lilleyse commented Oct 24, 2016

lasalvavida commented Oct 25, 2016 • edited Loading

tfili commented Oct 26, 2016 • edited Loading

lasalvavida commented Oct 26, 2016

lasalvavida commented Oct 26, 2016

lasalvavida commented Oct 26, 2016

Choose a reason for hiding this comment

pjcozzi commented Oct 31, 2016

lasalvavida commented Oct 31, 2016

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 Nov 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Nov 2, 2016 • edited Loading

tfili commented Nov 2, 2016

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

tfili commented Nov 2, 2016

lasalvavida commented Oct 21, 2016 •

edited

Loading

lasalvavida commented Oct 25, 2016 •

edited

Loading

tfili commented Oct 26, 2016 •

edited

Loading

lilleyse Nov 1, 2016 •

edited

Loading

lilleyse commented Nov 2, 2016 •

edited

Loading