Web3d quantized attributes #3891

lasalvavida · 2016-05-02T16:10:53Z

This pull request is for reviewing an initial implementation of the WEB3D_quantized_attributes extension to glTF.

There are still a few issues to be resolved both in this implementation and the proposed specification. Feel free to try it out with the included sample model; comments and suggestions are welcome.

TODO

Merge spec updates, Updated quantized attributes extensions KhronosGroup/glTF#616
Merge gltf-pipeline implementation, Web3d quantized attributes gltf-pipeline#56
Add checkbox to online model converter

pjcozzi · 2016-05-05T13:29:26Z

Update CHANGES.md

pjcozzi · 2016-05-05T13:31:11Z

Source/Scene/Model.js

+    LoadResources.prototype.getDecodedBuffer = function(bufferView, decodeBufferAccessors) {
+        var bufferData = new Uint8Array(0);
+        var byteShift = 0;
+        for (var i in decodeBufferAccessors) {


With for...in, we always use if (decodeBufferAccessors.hasOwnProperty(i)) {. Maybe it's OK without in this case, but I would rather be consistent.

pjcozzi · 2016-05-05T13:34:14Z

Can you include a brief README.md in Specs/Data/Models/Box-Quantized that says this is the box model, but modified so the position and normal attributes are quantized?

pjcozzi · 2016-05-05T13:36:39Z

Does the extension spec say anything any quantizing accessors that are used for indices? To keep the implementation simple, it should say that the accessor must only be used for attributes.

pjcozzi · 2016-05-05T13:38:08Z

Source/Scene/Model.js

+                    var index = 0;
+                    for (var j = 0; j < chunkData.length; j += 2*numComponents) {
+                        for (var k = 0; k < numComponents; k++) {
+                            var encodedValue = (chunkData[j + k*2 + 1] << 8) + chunkData[j + k*2];


Throughout, use parentheses instead of no whitespace, e.g., (k * 2).

lasalvavida · 2016-05-05T13:38:52Z

@pjcozzi Will add a README, and yes quantization is only to be used on attributes. I'm currently working on implementing some of the feedback from @mlimper and @MarcoHutter on the glTF issue I opened.

pjcozzi · 2016-05-05T13:40:12Z

Source/Scene/Model.js

        var bufferViews = model.gltf.bufferViews;
+
+        var accessorsForBufferViewId = {};


Probably put this in its own function that returns accessorsForBufferViewId. See Statements in a function should be at a similar level of abstraction... here.

lasalvavida · 2016-05-05T13:41:00Z

Most specifically, the approach is going to change from unpacking the buffers, to just applying the decodeMatrix to the model matrix and leaving the data as-is, which should be faster and less complicated.

pjcozzi · 2016-05-05T13:42:19Z

Source/Scene/Model.js

+                } else {
+                    var inserted = false;
+                    var bufferViewAccessors = accessorsForBufferViewId[bufferViewId];
+                    for (var j in bufferViewAccessors) {


Same hasOwnProperty comment here and throughout if it is missing anywhere else.

pjcozzi · 2016-05-05T13:54:11Z

Most specifically, the approach is going to change from unpacking the buffers, to just applying the decodeMatrix to the model matrix and leaving the data as-is, which should be faster and less complicated.

Yes, exactly!

pjcozzi · 2016-05-05T13:54:18Z

Let me know when this is ready for another review.

lasalvavida · 2016-05-06T20:38:38Z

This update takes in some suggestions from this thread.

In this approach, the model shader is modified to decode the attributes and use the decoded version in places where the original was used. This may not be the best approach, and I am open to suggestions.

In my first go at this, I had tried applying the decode matrix to the model matrix directly to avoid modifying the shaders. It think this is better because it allows for attributes to be quantized or not with the same model matrix with no issues.

pjcozzi · 2016-05-10T11:10:45Z

There are jsHint warnings; check the CI - https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/128397413

pjcozzi · 2016-05-10T11:12:26Z

Did you see these comments: #3891 (comment) and #3891 (comment)?

pjcozzi · 2016-05-10T11:15:38Z

Source/Scene/Model.js

+                                                return val.toFixed(16);
+                                            });
+
+                                            var newMain = "decoded_" + attributeSemantic;


Throughout, use ' instead of ".

pjcozzi · 2016-05-10T11:21:49Z

Remind me - what attributes did you test this with? Positions? Or positions and normals?

pjcozzi · 2016-05-10T11:24:07Z

Source/Scene/Model.js

+                                            var matrixName = "decode_" + attributeSemantic + "_matrix";
+                                            var decodedAttributeVarName = attributeVarName.replace('a_', 'dec_');
+
+                                            // replace usages of the original attribute with the decoded version, but not the declaration


I don't know much about regexs, but I suspect there is an easier way to do this. @tfili would know.

You would use a negative lookbehind. Something like (?<!vec3 )a_var would match all occurences of a_var what weren't preceded by vec3. However looking it appears that this isn't supported in javascript. You may be stuck doing it this way.

That was the problem I ran into. It does need to do something differently because the current method sets off jshint, but as of right now I don't have a better answer

pjcozzi · 2016-05-10T11:27:00Z

In my first go at this, I tried applying the decode matrix to the model matrix directly to avoid modifying the shaders. It think this is better because it allows for attributes to be quantized or not with the same model matrix with no issues.

Let's discuss in person today. There are trade-off to think through, including precision and animations.

lasalvavida · 2016-06-06T15:09:17Z

@pjcozzi Updated. The MilkTruck mismatch model now has mismatched VEC4 - POSITION, VEC3 - NORMAL, and VEC2 - TEXCOORD which covers all mismatched cases. Code coverage confirms this.

pjcozzi · 2016-06-06T17:02:17Z

@mlimper once again, thanks so much for the review! It is deeply appreciated.

pjcozzi · 2016-06-06T17:06:43Z

@lasalvavida test coverage and everything else look great! I added a task list to the top of this issue so that we'll merge this when other dependencies are merged.

This will ship in Cesium 1.23 no problem!

pjcozzi · 2016-06-10T13:28:13Z

@lasalvavida merge master. There is probably a trivial conflict in CHANGES.md.

lasalvavida · 2016-06-10T13:30:32Z

Will do. I'm also going to quickly add a simple Box test for SCALAR attributes.

…ntized-attributes

lasalvavida · 2016-06-10T14:06:41Z

Updated

pjcozzi · 2016-06-10T14:56:54Z

Nice! This is close. We just need:

Add checkbox to online model converter

pjcozzi · 2016-06-18T14:36:30Z

@lasalvavida can you merge in master? There is probably a trivial conflict in CHANGES.md.

… web3d-quantized-attributes

lasalvavida · 2016-06-24T19:08:15Z

@pjcozzi The checkbox has been added. This should be good to merge whenever you're ready.

… web3d-quantized-attributes

pjcozzi · 2016-06-28T12:33:57Z

@pjcozzi The checkbox has been added. This should be good to merge whenever you're ready.

I tested with the Cesium aircraft and balloons models, and it produced the same models with and without checking the quantization box.

… web3d-quantized-attributes

lasalvavida · 2016-06-28T13:29:21Z

@pjcozzi I tried to reproduce but I actually can't get the online model converter to work at all right now. It just says An unkown error has occured.

lilleyse · 2016-06-29T14:57:23Z

I'm also seeing some errors on the page:

Cesium.js:476 Blocked script execution in 'about:blank' because the document's frame is sandboxed and the 'allow-scripts' permission is not set.

I tested with the Cesium aircraft and balloons models, and it produced the same models with and without checking the quantization box.

That's doesn't seem right, those models are being quantized fine when I run the converter locally.

pjcozzi · 2016-06-29T17:17:23Z

@lasalvavida can you merge in master?

… web3d-quantized-attributes

lasalvavida · 2016-06-29T17:19:59Z

@pjcozzi done

pjcozzi · 2016-06-29T19:08:52Z

Code coverage looks good!

This is most likely ready to merge, but I want to wait for CesiumGS/gltf-pipeline#97 (comment) and CesiumGS/gltf-pipeline#97 (comment) in case they reveal any issues in Cesium.

… web3d-quantized-attributes

lasalvavida added 4 commits April 29, 2016 09:38

Initial success with quantized model

359f2c2

Added quantized sample model

1805f24

Added test for quantized box

4c7545d

Multiple quantized accessors in the same buffer

d9393ea

lasalvavida mentioned this pull request May 2, 2016

WEB3D_quantized_attributes Specification KhronosGroup/glTF#588

Closed

pjcozzi reviewed May 5, 2016
View reviewed changes

lasalvavida added 2 commits May 6, 2016 16:26

Rewrite to use shaders for decoding

6b87855

A little cleanup

8f8ccc0

pjcozzi reviewed May 10, 2016
View reviewed changes

pjcozzi added the priority - next release label Jun 6, 2016

lasalvavida added 3 commits June 10, 2016 10:03

Added SCALAR test model

967e430

Added SCALAR support

d6442c2

Merge branch 'master' of github.com:lasalvavida/cesium into web3d-qua…

c90f4e9

…ntized-attributes

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

3863db2

… web3d-quantized-attributes

lasalvavida added 3 commits June 24, 2016 15:40

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

8a5db4c

… web3d-quantized-attributes

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

6f00bc4

… web3d-quantized-attributes

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

e19ff6d

… web3d-quantized-attributes

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

1ae20b1

… web3d-quantized-attributes

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

302d025

… web3d-quantized-attributes

lasalvavida added 2 commits June 29, 2016 18:28

Merge branch 'master' of github.com:AnalyticalGraphicsInc/cesium into…

3456746

… web3d-quantized-attributes

Fix bad merge

b4837fa

pjcozzi merged commit e6aba2f into CesiumGS:master Jun 30, 2016

pjcozzi deleted the web3d-quantized-attributes branch June 30, 2016 16:46

pjcozzi mentioned this pull request Jun 30, 2016

Add support for attribute compression CesiumGS/gltf-pipeline#97

Merged

		var bufferViews = model.gltf.bufferViews;

		var accessorsForBufferViewId = {};

Web3d quantized attributes #3891

Web3d quantized attributes #3891

Conversation

lasalvavida commented May 2, 2016 • edited Loading

pjcozzi commented May 5, 2016

Choose a reason for hiding this comment

pjcozzi commented May 5, 2016

pjcozzi commented May 5, 2016

Choose a reason for hiding this comment

lasalvavida commented May 5, 2016

Choose a reason for hiding this comment

lasalvavida commented May 5, 2016

Choose a reason for hiding this comment

pjcozzi commented May 5, 2016

pjcozzi commented May 5, 2016

lasalvavida commented May 6, 2016 • edited Loading

pjcozzi commented May 10, 2016

pjcozzi commented May 10, 2016

Choose a reason for hiding this comment

pjcozzi commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented May 10, 2016

lasalvavida commented Jun 6, 2016

pjcozzi commented Jun 6, 2016

pjcozzi commented Jun 6, 2016

pjcozzi commented Jun 10, 2016

lasalvavida commented Jun 10, 2016

lasalvavida commented Jun 10, 2016

pjcozzi commented Jun 10, 2016

pjcozzi commented Jun 18, 2016

lasalvavida commented Jun 24, 2016

pjcozzi commented Jun 28, 2016

lasalvavida commented Jun 28, 2016

lilleyse commented Jun 29, 2016 • edited Loading

pjcozzi commented Jun 29, 2016

lasalvavida commented Jun 29, 2016

pjcozzi commented Jun 29, 2016

lasalvavida commented May 2, 2016 •

edited

Loading

lasalvavida commented May 6, 2016 •

edited

Loading

lilleyse commented Jun 29, 2016 •

edited

Loading