gltfPrimitiveToCesiumGeometry fixes #100

JoshuaStorm · 2016-06-20T18:09:24Z

Now handles texture coordinate attribute. See: #90

As of now, it is only handling one set of texture coordinates. I was unable to find a gltf model with multiple texture coordinate sets for one primitive. Do we have any?

Addressed some other small comments from #90 as well.

I'm also wondering where I should be putting the pre and post VS optimizations within the pipeline. I imagine the pre can replace a portion of what is now labeled the cacheOptimization step. When do we want to perform the post VS optimization?

lasalvavida · 2016-06-20T18:25:04Z

@JoshuaStorm One of your tests is failing here and you do have a few jsHint errors.
edit: Too quick for me

coveralls · 2016-06-20T18:27:06Z

Coverage increased (+0.2%) to 94.206% when pulling a79a2ae on graphicsTerms into 1dd869d on master.

JoshuaStorm · 2016-06-20T18:30:33Z

@lasalvavida Addressed

coveralls · 2016-06-20T18:35:34Z

Coverage increased (+0.2%) to 94.206% when pulling ab331e7 on graphicsTerms into 1dd869d on master.

lilleyse · 2016-06-20T20:40:29Z

specs/lib/gltfPrimitiveToCesiumGeometrySpec.js

@@ -11,20 +11,24 @@ var positions = new Float64Array([ -0.5, -0.5, 0.5, 0.5, -0.5, 0.5, -0.5, 0.5, 0
 var normals = new Float32Array([ 0, 0, 1, 0, 0, 1, 0, 0, 1, 0, 0, 1, 1, 0, 0, 1, 0, 0, 1, 0, 0, 1, 0, 0, 0, 1, 0, 0, 1, 
    0, 0, 1, 0, 0, 1, 0, 0, -1, 0, 0, -1, 0, 0, -1, 0, 0, -1, 0, -1, 0, 0, -1, 0, 0, -1, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0,
    -1, 0, 0, -1, 0, 0, -1 ]);
+var sts = new Float32Array([ 6, 0, 5, 0, 6, 0.9999998807907104, 5, 0.9999998807907104, 4, 0, 5, 0, 4, 1, 5, 1,
+    2, 0, 1, 0, 2, 1, 1, 1, 3, 0, 4, 0, 3, 1, 4, 1, 3, 0, 2, 0, 3, 1, 2, 1, 0, 0, 0, 0.9999998807907104, 1, 0, 1,
+    0.9999998807907104 ]);
 var indices = new Uint32Array([ 0, 1, 2, 3, 2, 1, 4, 5, 6, 7, 6, 5, 8, 9, 10, 11, 10, 9, 12, 13, 14, 15, 14, 13, 16, 17,
    18, 19, 18, 17, 20, 21, 22, 23, 22, 21 ]);
 var primitiveType = 4;


I should have brought this up before, but instead can you extract these values from the gltf like in line 71-92 of cesiumGeometryToGltfPrimitiveSpec, just so if the test model changes, the test will still pass.

lilleyse · 2016-06-20T20:42:01Z

That's all, looks good to me otherwise.

pjcozzi · 2016-06-21T12:22:41Z

As of now, it is only handling one set of texture coordinates

Are we sure that we can't just handle all attributes in a generic manner? For the ones where the semantics are know, we can map glTF to Cesium, but for the others, I thought Cesium would allow generic attributes.

JoshuaStorm · 2016-06-21T14:55:21Z

Are we sure that we can't just handle all attributes in a generic manner? For the ones where the semantics are know, we can map glTF to Cesium

I am not sure what the benefit would be to handling all attributes in a generic manner since only the attributes of known semantics will be changed when we use the geometry pipeline optimizations and our only intention is to use these geometries in intermediate steps of the gltf pipeline. Wouldn't we just be reading attributes to rewrite them exactly the same later?

coveralls · 2016-06-21T15:04:15Z

Coverage increased (+0.2%) to 94.206% when pulling a440ad5 on graphicsTerms into 1dd869d on master.

pjcozzi · 2016-06-21T16:45:21Z

I am not sure what the benefit would be to handling all attributes in a generic manner since only the attributes of known semantics will be changed when we use the geometry pipeline optimizations and our only intention is to use these geometries in intermediate steps of the gltf pipeline. Wouldn't we just be reading attributes to rewrite them exactly the same later?

For example, look at GeometryPipeline.reorderForPreVertexCache. It moves each attribute.

JoshuaStorm · 2016-06-21T21:24:03Z

Updated. I expect this PR will still need a bit of work after this refactoring, let me know if you think I'm on the right track for what you had in mind @pjcozzi.

coveralls · 2016-06-21T21:25:25Z

Coverage increased (+0.08%) to 94.047% when pulling c474e4d on graphicsTerms into 1dd869d on master.

JoshuaStorm · 2016-06-22T15:05:12Z

Actually found a bug while doing further testing. Not ready for review, my bad.

coveralls · 2016-06-22T17:38:53Z

Coverage decreased (-0.02%) to 93.947% when pulling 5f90e2e on graphicsTerms into 1dd869d on master.

JoshuaStorm · 2016-06-22T17:48:08Z

As of now, gltfPrimitiveToCesiumGeometry and cesiumGeometryToGltfPrimitive seem to work as expected. However, when adding a step in the pipeline to apply Cesium.GeometryPipeline steps, certain primitives seem to have issues.

When putting a geometry through GeometryPipeline.reorderForPostVertexCache only and rewriting it to the gltf, the model seems to work fine.

When putting a geometry through GeometryPipeline.reorderForPostVertexCache only and rewriting it ot the gltf, the model seems fine.

However, when putting a gltf through both, it seems to mangle some primitives (see below). If I flip the order of GeometryPipeline.reorderForPostVertexCache and GeometryPipeline.reorderForPostVertexCache, other primitives are mangled.

I've tried a few things and double checked that gltfPrimitiveToCesiumGeometry and cesiumGeometryToGltfPrimitive work as expected but haven't found what may be causing this issue.

JoshuaStorm · 2016-06-22T21:02:57Z

Update to the last comment: I believe the bugs are all in my use of the GeometryPipeline functions and not any of the code in this PR.

pjcozzi · 2016-06-23T11:41:54Z

@lilleyse can you please help @JoshuaStorm with the Cesium API if needed?

coveralls · 2016-06-23T16:28:54Z

Coverage decreased (-0.003%) to 93.968% when pulling eddd8c8 on graphicsTerms into 9356817 on master.

coveralls · 2016-06-23T17:08:35Z

Coverage decreased (-0.03%) to 93.937% when pulling f1d3fba on graphicsTerms into 9356817 on master.

coveralls · 2016-06-23T17:10:06Z

Coverage decreased (-0.03%) to 93.937% when pulling f1d3fba on graphicsTerms into 9356817 on master.

JoshuaStorm · 2016-06-23T17:18:19Z

Had to rebase due to pulling and accidentally pushing from one of Rob's PRs for testing.

As Sean and I discussed offline: The scope of this PR is decreasing to just implementing the GeometryPipeline.reorderForPostVertexCache since reorderForPreVertexCache is creating issues that we need to think over more before implementing fully (which we might put on the backburner for now).

A brief description of the issue: the pre vertex cache optimization shifts around both the primitive's attributes and primitive's indices. This is fine when only one primitive uses each attributes accessor, but in many cases, when the attributes accessors are shared (when multiple primitives share vertices), the model will get mangled as the shared vertices will be shifted each time a primitive accesses it resulting in indices referring to the wrong vertices.

We have a couple ideas for possible solutions, but none of which are perfect and involve quite a bit of work, so Sean and I figured that would be appropriate for another PR.

lasalvavida · 2016-06-23T17:38:15Z

@JoshuaStorm, I split off the createAccessorUsageTables utility for you as discussed offline in my buffer-fixes branch from #105. I would cache primitives by their index accessor, then iterate over the tables. If a table only has a single index accessor, you are free to perform cache optimization on that index accessor and all the related attribute accessors in the table.

lilleyse · 2016-06-23T19:24:50Z

@JoshuaStorm Do you want to try the pre-vertex cache with the new code before I look into this?

JoshuaStorm · 2016-06-23T19:25:25Z

Yeah, I'll give it a shot and let you know when this is good for review

lasalvavida · 2016-06-24T18:42:26Z

lib/gltfPrimitiveToCesiumGeometry.js

+            attributeName = 'normal';
+            finalValues = new Float32Array(packed);
+            break;
+        case 'TEXCOORD_0':


If you take a look at the glTF spec, valid attributes are of the form SEMANTIC or SEMANTIC_#. You need to check if the attributeId starts with a semantic, equality won't work for all cases. For example, this could be TEXCOORD instead of TEXCOORD_0 in a valid glTF file.

Since Cesium Geometries only directly handle one position, normal, and st, I believe it would be sufficient to just handle equality of SEMANTIC or SEMANTIC_0 and just allow the default option for generic Geometry attributes.

Is there any circumstance where the first SEMANTIC_# # wouldn't be 0?

coveralls · 2016-06-24T20:26:09Z

Coverage decreased (-0.1%) to 93.752% when pulling a40badc on graphicsTerms into 03c7733 on master.

JoshuaStorm · 2016-06-24T20:29:36Z

Updated, still seeking thoughts in regards to @lasalvavida's comment:

If you take a look at the glTF spec, valid attributes are of the form SEMANTIC or SEMANTIC_#. You need to check if the attributeId starts with a semantic, equality won't work for all cases. For example, this could be TEXCOORD instead of TEXCOORD_0 in a valid glTF file.

Since Cesium Geometries only directly handle one position, normal, and st, I believe it would be sufficient to just handle equality of SEMANTIC or SEMANTIC_0 and just allow the default option for generic Geometry attributes in all other cases.

Is there any circumstance where the first SEMANTIC_# # wouldn't be 0?

lasalvavida · 2016-06-27T14:15:07Z

lib/cacheOptimization.js

+                var primitivesOfIndicesId;
+                if (!dictionary[indicesId]) {
+                    primitivesOfIndicesId = [];
+


extra whitespace

lasalvavida · 2016-06-27T14:32:32Z

@JoshuaStorm Take a look at the specification.

Specifically:

Valid attribute semantic property names include POSITION, NORMAL, TEXCOORD, COLOR, JOINT, and WEIGHT. Attribute semantic property names can be of the form [semantic]_[set_index], e.g., TEXCOORD_0, TEXCOORD_1, etc.

The short answer to your question is yes. Maybe there are two primitives with different materials, and the author decided to make them use TEXCOORD_0 and TEXCOORD_1 to make it easier to tell them apart. That is still valid glTF. There's also nothing stopping someone from making it TEXCOORD_25, or POSITION_3 if they want to.

Just doing a check for whether the listed semantic starts with the one you're looking for should be fine for all cases.

if (attributeSemantic.indexOf(semantic) === 0) {
   ...
}

coveralls · 2016-06-27T17:17:01Z

Coverage decreased (-0.5%) to 93.412% when pulling 487a2de on graphicsTerms into 03c7733 on master.

JoshuaStorm · 2016-06-27T17:35:13Z

Updated, I hope I didn't miss anything.

I am actually handling semantics a bit differently now to avoid possible issues with different semantic naming schemes. Currently when going from gltf primitive <-> Cesium Geometry, I am just using the first semantic to map accordingly and just having the rest be generic attributes. This works fine for now, but I may need to modify it slightly once I change #82 to use the GeometryPipeline.

lasalvavida · 2016-06-27T17:38:41Z

lib/cacheOptimization.js

+                if (attributes.hasOwnProperty(semantic)) {
+                    var accessorId = attributes[semantic];
+                    if (defined(attributeAccessors[accessorId])) {
+                        if (attributeAccessors[semantic] === attributes[semantic]) {


What is this testing for? attributeAccessors doesn't have semantic keys. I think just the defined check should be sufficient here.

Must've accidentally ctrl-zed that back at some pooint, oops.

coveralls · 2016-06-27T17:54:56Z

Coverage decreased (-0.3%) to 93.582% when pulling 360b183 on graphicsTerms into 03c7733 on master.

JoshuaStorm · 2016-06-28T17:57:08Z

Aside from a final review, this should be good to go.

pjcozzi · 2016-06-29T14:15:22Z

lib/cacheOptimization.js

+function createIndicesToAttributeDictionary(gltf) {
+    var dictionary = {};
+    var meshes = gltf.meshes;
+    for (var meshId in meshes) {


Can this use NodeHelpers.forEachPrimitiveInScene?

Is this currently in master? I cannot seem to find this function.

It is not; it is in the ao branch. I think forego using it for now since it is async and doesn't have a way to notify when it completes currently.

pjcozzi · 2016-06-29T14:21:01Z

@lasalvavida for your final testing, could you please do some performance testing with a model with a large number of vertices and no textures (like Happy Buddha or the Dragon from http://graphics.stanford.edu/data/3Dscanrep/)?

In Cesium, turn off the globe, stars, and sky, (or use the Model Inspector) and see what the FPS is with and without cache optimization. You may need to load several of the same model to notice. A 10% win would be good. It is probably less.

For numbers from the original paper, see http://gfx.cs.princeton.edu/gfx/pubs/Sander_2007_%3ETR/index.php

JoshuaStorm · 2016-06-29T18:30:24Z

@pjcozzi:
To test I loaded 100 Dragon models in Cesium.

Before pipeline:
~20FPS

Through the pipeline without cache optimization:
~21FPS

Through the pipeline with cache optimization:
~44FPS

A 10% win would be good. It is probably less.

I've checked triple checked that I'm not making a mistake here, but the performance improvements seem better than we expected.

lasalvavida · 2016-06-29T18:31:54Z

@JoshuaStorm Good job, that's awesome! I will try to duplicate soon.

pjcozzi · 2016-06-29T18:43:33Z

Despite this being a heavily vertex-bound test case, I still did not expect something this good!

lasalvavida · 2016-06-30T16:30:05Z

@JoshuaStorm When I run the chinese dragon model through the pipeline with and without cache optimization enabled, I see no change. Could you please attach the models you used for your test here?

JoshuaStorm · 2016-06-30T17:09:14Z

Odd, here's the model I used:

chinesedragon.gltf.zip

lasalvavida · 2016-06-30T18:02:36Z

@JoshuaStorm I meant can you attach the ones with and without cache optimization?

JoshuaStorm · 2016-06-30T18:19:10Z

@lasalvavida

Sorry about that: below is the cache optimized, the original input, and the original input through the pipeline without the cache optimization.

Archive.zip

lasalvavida · 2016-06-30T18:40:58Z

@JoshuaStorm The model I was using had split primitives, which is why there was no change. That is the desired behavior. I tested with the models you provided and got the same results, great job!

lilleyse reviewed Jun 20, 2016
View reviewed changes

JoshuaStorm force-pushed the graphicsTerms branch from 5f90e2e to eddd8c8 Compare June 23, 2016 16:25

JoshuaStorm force-pushed the graphicsTerms branch 2 times, most recently from 3189b6a to f1d3fba Compare June 23, 2016 17:07

JoshuaStorm mentioned this pull request Jun 23, 2016

A few buffer logic fixes #105

Merged

JoshuaStorm closed this Jun 23, 2016

lasalvavida reviewed Jun 24, 2016
View reviewed changes

Addressed @lasalvavida comments

a40badc

lasalvavida reviewed Jun 27, 2016
View reviewed changes

JoshuaStorm added 2 commits June 27, 2016 13:14

Generalize attribute handling

46de2d3

One day I will remember to check jsHint before commiting

487a2de

lasalvavida reviewed Jun 27, 2016
View reviewed changes

Fixes

360b183

pjcozzi reviewed Jun 29, 2016
View reviewed changes

lasalvavida merged commit fbb9812 into master Jun 30, 2016

lasalvavida deleted the graphicsTerms branch June 30, 2016 18:41

gltfPrimitiveToCesiumGeometry fixes #100

gltfPrimitiveToCesiumGeometry fixes #100

Conversation

JoshuaStorm commented Jun 20, 2016

lasalvavida commented Jun 20, 2016 • edited Loading

coveralls commented Jun 20, 2016

JoshuaStorm commented Jun 20, 2016

coveralls commented Jun 20, 2016

Choose a reason for hiding this comment

lilleyse commented Jun 20, 2016

pjcozzi commented Jun 21, 2016

JoshuaStorm commented Jun 21, 2016

coveralls commented Jun 21, 2016

pjcozzi commented Jun 21, 2016

JoshuaStorm commented Jun 21, 2016

coveralls commented Jun 21, 2016

JoshuaStorm commented Jun 22, 2016

coveralls commented Jun 22, 2016

JoshuaStorm commented Jun 22, 2016

JoshuaStorm commented Jun 22, 2016

pjcozzi commented Jun 23, 2016

coveralls commented Jun 23, 2016

coveralls commented Jun 23, 2016

coveralls commented Jun 23, 2016

JoshuaStorm commented Jun 23, 2016

lasalvavida commented Jun 23, 2016

lilleyse commented Jun 23, 2016

JoshuaStorm commented Jun 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 24, 2016

JoshuaStorm commented Jun 24, 2016 • edited Loading

Choose a reason for hiding this comment

lasalvavida commented Jun 27, 2016

coveralls commented Jun 27, 2016

JoshuaStorm commented Jun 27, 2016

lasalvavida Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jun 27, 2016

JoshuaStorm commented Jun 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Jun 29, 2016

JoshuaStorm commented Jun 29, 2016

lasalvavida commented Jun 29, 2016

pjcozzi commented Jun 29, 2016

lasalvavida commented Jun 30, 2016

JoshuaStorm commented Jun 30, 2016

lasalvavida commented Jun 30, 2016

JoshuaStorm commented Jun 30, 2016

lasalvavida commented Jun 30, 2016

lasalvavida commented Jun 20, 2016 •

edited

Loading

JoshuaStorm commented Jun 24, 2016 •

edited

Loading

lasalvavida Jun 27, 2016 •

edited

Loading