validating batch table #41

JudyWeng · 2016-12-23T16:08:47Z

#32
I think I have the helper methods to extract and validate the batch table, but the loadJson method from Cesium returns a promise and I'm still working on learning about promises and how to use it in the individual tile validation files.

pjcozzi · 2017-01-05T22:35:03Z

@lilleyse will you be able to look at this soon?

lilleyse · 2017-01-06T21:38:30Z

If I remember correctly, the main things to address here were:

Move the batch table JSON and batch table binary extraction to the individual validateB3dm, validateI3dm, files. While there will be some duplicate code, I think it is worth it just so each tile type is self-contained.
Avoid loading the batch table schema in validateBatchTable, since it would be much nicer to avoid promises entirely here. Rather it could be loaded right from the start in in validateTileset and eventually passed along to each tile in some sort of schemas object.

pjcozzi · 2017-01-15T21:58:33Z

@JudyWeng any update here as well?

JudyWeng · 2017-01-16T18:28:04Z

I had attempted the method of changing the tiles to promises first, which caused a lot of changes, but I'm going to try loading the batch table schema in validateTileset now instead to avoid having to change every validateB3dm, validateI3dm file.

…icsInc/3d-tiles-tools into validate-batchTable

JudyWeng · 2017-02-13T07:21:41Z

validator/specs/lib/validateB3dmSpec.js


+        loadJson('https://raw.githubusercontent.com/AnalyticalGraphicsInc/3d-tiles/master/schema/batchTable.schema.json')
+            .then(function(schema) {


I tried to change the validate files to take the batch table schema in as a parameter to avoid having to change everything into an asynchronous process, as suggested previously. However, it seems like loadJson() is returning the "XMLHttpRequest i undefined" error. I have tried both the url above and this url: "https://github.com/AnalyticalGraphicsInc/3d-tiles/blob/master/schema/batchTable.schema.json" to load the Json. Am I not using this function properly?

Hm, XMLHttpRequest must only be supported in browsers and not Node. You could try looking at http.request.

If I choose to use http.request it would require me to change loadJson() though wouldn't it?

@lilleyse any thoughts here?

We discussed this offline, that using http.request (or alternatives) is fine to replace loadJson.

JudyWeng · 2017-03-13T07:01:30Z

I wrote the test cases for the binary table validation for b3dm, i3dm, and pnts, but not cmpt yet because that validation branch has yet to be merged into validator-master. Please let me know if there are more edge cases I have yet to cover.

lilleyse · 2017-03-13T20:00:05Z

I'll look at both later today.

lilleyse

I mostly looked at the b3dm code, so there may be areas I missed. I'll do a closer review at a later point.

So far it looks solid - mainly once the batch table extraction code is more condensed I think it will fit in better.

lilleyse · 2017-03-13T23:34:24Z

validator/lib/validateB3dm.js

+        throw new DeveloperError('b3dm content must be of type buffer');
+    }
+
+    if(content.length < 2) {


Should this be 24?

Style: if( -> if (.

Fix throughout.

lilleyse · 2017-03-13T23:48:41Z

validator/lib/validateB3dm.js

+        batchTableJSON: batchTableJSON,
+        batchTableBinary: batchTableBinary,
+        message: message
+    };


This section may be able to be condensed by extracting the sections, and then verifying they are correct.

For example - https://github.com/AnalyticalGraphicsInc/3d-tiles-tools/blob/master/tools/lib/extractB3dm.js#L44-L46. You could do something like that and then verify the buffers are the correct size. Ideally the batch table extraction/validation could even go inline with the rest of the b3dm checking.

lilleyse · 2017-03-13T23:49:00Z

validator/lib/validateBatchTable.js

@@ -0,0 +1,29 @@
+'use strict';
+var Cesium = require('cesium');
+var Ajv = require('ajv');


Add requires in alphabetical order.

lilleyse · 2017-03-13T23:51:12Z

validator/lib/validateBatchTable.js

+ * Checks if provided buffers follow the batch table schema
+ *
+ * @param {Object} schema - A JSON object containing the schema for the batch table.
+ * @param {Buffer} batchTableJSON - A buffer containing the JSON Header of a batch table


JSON Header of a batch table -> batch table JSON. It's not a buffer, right?

lilleyse · 2017-03-13T23:51:33Z

validator/lib/validateBatchTable.js

+ *
+ * @param {Object} schema - A JSON object containing the schema for the batch table.
+ * @param {Buffer} batchTableJSON - A buffer containing the JSON Header of a batch table
+ * @param {Buffer} batchTableBinary - A buffer containing the binary body of a batch table


binary body of a batch table -> batch table binary is simpler.

lilleyse · 2017-03-13T23:52:57Z

validator/specs/lib/validateB3dmSpec.js

        expect(validateB3dm(createB3dmTile()).result).toBe(true);
    });
+
+    it('validates b3dm tile with batch table JSON header matches spec', function() {


The wording for these tests is awkward.

lilleyse · 2017-03-13T23:53:41Z

validator/specs/lib/validateB3dmSpec.js

-    header.writeUInt32LE(0, 16); // batchTableBinaryByteLength
-    header.writeUInt32LE(0, 20); // batchLength
-
+    console.log('checking:\n' + header + '\n');


Remove this.

lilleyse · 2017-03-13T23:54:28Z

validator/specs/lib/validateB3dmSpec.js

+    header.writeUInt32LE(header.length + batchJSON.length, 8); // byteLength
+    header.writeUInt32LE(batchJSON.length, 12); // batchTableJSONByteLength
+
+    return Buffer.concat([header, batchJSON]);


Better to avoid condensing the name, batchTableJSON while longer is easier to follow.

The name of the function could also be createB3dmWithBatchTableJson.

Also make sure to be consistent with Json vs JSON.

lilleyse · 2017-03-13T23:55:54Z

validator/specs/lib/validateB3dmSpec.js

+        "id":[0,1,2],
+        "longitude":[-1.3196595204101946,-1.3196567190670823,-1.3196687138763508],
+        "height":[8,14,14]
+    };


No need to wrap each key in quotes.

lilleyse · 2017-03-13T23:56:18Z

validator/specs/lib/validateB3dmSpec.js

+        "height" : {
+            "byteOffset" : 12,
+            "componentType" : "UNSIGNED_INT",
+            "type" : "SCALAR"


Use single quotes for UNSIGNED_INT and SCALAR.

…icsInc/3d-tiles-tools into validate-batchTable

lilleyse

The changes look good overall, the tests are definitely shaping out nicely.

Make sure to double check everywhere that the validator is up to date with the updated b3dm format.

lilleyse · 2017-04-13T21:49:49Z

validator/lib/validateB3dm.js

+        throw new DeveloperError('b3dm content must be of type buffer');
+    }
+
+    if (content.length < 28) {


Cleaner to save 28 as a headerByteLength variable used here and below.

lilleyse · 2017-04-13T21:50:54Z

validator/lib/validateB3dm.js

+    var batchTableJSONByteLength = content.readUInt32LE(20);
+    var batchTableBinaryByteLength = content.readUInt32LE(24);
+    if (batchTableJSONByteLength > 0) {
+        var batchTableJSONHeader = content.slice(offset, offset + batchTableJSONByteLength);


Just call this batchTableJSON

lilleyse · 2017-04-13T21:51:04Z

validator/lib/validateB3dm.js

+        offset +=  batchTableJSONByteLength;
+        var batchTableBinary = content.slice(offset, offset + batchTableBinaryByteLength);
+
+        if((batchTableJSONHeader.length == batchTableJSONByteLength) && (batchTableBinary.length == batchTableBinaryByteLength))  {


lilleyse · 2017-04-13T21:53:00Z

validator/lib/validateB3dm.js

+        if((batchTableJSONHeader.length == batchTableJSONByteLength) && (batchTableBinary.length == batchTableBinaryByteLength))  {
+            var batchTableJSON = JSON.parse(batchTableJSONHeader.toString());
+            var validBatchTable = validateBatchTable(batchTableSchema, batchTableJSON, batchTableBinary);
+            if (!validBatchTable.validation) {


Rename validation to result to be consistent. Ultimately this way of passing the result will be changed, but still worth addressing.

lilleyse · 2017-04-13T21:53:23Z

validator/lib/validateB3dm.js

+            return {
+                result: false,
+                message: 'b3dm has invalid batch table lengths'
+            }


Needs semicolon.

lilleyse · 2017-04-14T14:45:00Z

validator/specs/lib/validateBatchTableSpec.js

+            "byteOffset" : 12,
+            "componentType" : 'UNSIGNED_INT',
+            "type" : 'SCALAR'
+        }


Fine to do:

width : { byteOffset : 12, componentType : 'UNSIGNED_INT', type : 'SCALAR' }

lilleyse · 2017-04-14T14:46:10Z

validator/lib/validateBatchTable.js

+                }
+
+                var propertyByteLength = batchLength * componentTypeSize[componentType] * typeSize[numberOfComponents];
+                if (propertyByteLength + byteOffset > binaryBodyLength) {


Also worth validating that componentType and type are actually valid strings.

lilleyse · 2017-04-14T14:48:42Z

validator/lib/validateBatchTable.js

+ *
+ * @param {Object} schema - A JSON object containing the schema for the batch table.
+ * @param {JSON} batchTableJSON - Batch table JSON
+ * @param {Buffer} batchTableBinary - A buffer containing the batch table binary


Since batchTableBinary is optional, change to [batchTableBinary]

lilleyse · 2017-04-14T14:50:50Z

validator/specs/lib/validateBatchTableSpec.js

+        expect(validateBatchTable(batchTableSchema, batchTableJSON, batchTableBinary).validation).toBe(false);
+    });
+
+    it('byteoffset out of bounds', function() {


Make these two slightly more descriptive - Binary property byteOffset out of bounds

lilleyse · 2017-04-14T14:54:00Z

validator/specs/lib/validateCmptSpec.js

+    var batchTable = createBatchTableBinary();
+    innerB3dmTile.writeUInt32LE(innerB3dmTile.length + batchTable.buffer.length, 8); // byteLength
+    innerB3dmTile.writeUInt32LE(batchTable.batchTableJSONByteLength, 12); // batchTableJSONByteLength
+    innerB3dmTile.writeUInt32LE(batchTable.batchTableBinaryByteLength, 16);


12 and 16 are now offsets for the feature table

JudyWeng · 2017-05-10T20:59:26Z

I've updated the old tests to the new b3dm specs and they pass when I run them. I'm not sure what else might be causing the checks to fail...

lilleyse · 2017-05-10T21:04:31Z

You can ignore that, Travis is broken for this repo right now due to the directory structure.

lilleyse · 2017-07-01T00:19:14Z

I'm late here, but thanks for all the work @JudyWeng!

validating batch table

cdb6c7a

JudyWeng added 2 commits February 12, 2017 19:45

Merge branch 'validator-master' of https://github.com/AnalyticalGraph…

c766c4f

…icsInc/3d-tiles-tools into validate-batchTable

changed validate files to take batch table schema in as param

603c4dc

JudyWeng commented Feb 13, 2017

View reviewed changes

JudyWeng added 2 commits February 21, 2017 15:13

no data in http response

85931ee

batch table validation tests for b3dm, pnts, i3dm

2303fe7

lilleyse reviewed Mar 13, 2017

View reviewed changes

JudyWeng added 8 commits March 21, 2017 00:56

Merge branch 'validator-master' of https://github.com/AnalyticalGraph…

8e0056a

…icsInc/3d-tiles-tools into validate-batchTable

validates batch table json header against batch table binary body

227bc31

uncommented all test cases

fa828b6

corrected batch table binary body test case bug

e716bf0

removed debugging statements

b01cc4c

created batch table test cases and shortened extraction code

2afa084

shortened batch table extraction code in i3dm and pnts

0cef172

updated b3dm tests to new b3dm format with featureTable

7a1a64c

lilleyse reviewed Apr 14, 2017

View reviewed changes

JudyWeng added 6 commits May 9, 2017 04:28

added featuretable for batchLength

4dd6630

added test case to validateBatchTableSpec

b57b935

changed BATCH_LENGTH to INSTANCES_LENGTH in i3dm validator

4c9f814

fixed i3dm error message

f53ef56

updated tilesets to match new b3dm specs

522abcf

added feature table json header schema

27687c5

lilleyse merged commit 2abb1fa into validator-master Jul 1, 2017

lilleyse deleted the validate-batchTable branch July 1, 2017 00:19


		loadJson('https://raw.githubusercontent.com/AnalyticalGraphicsInc/3d-tiles/master/schema/batchTable.schema.json')
		.then(function(schema) {

validating batch table #41

validating batch table #41

Conversation

JudyWeng commented Dec 23, 2016

pjcozzi commented Jan 5, 2017

lilleyse commented Jan 6, 2017

pjcozzi commented Jan 15, 2017

JudyWeng commented Jan 16, 2017

JudyWeng Feb 13, 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

JudyWeng commented Mar 13, 2017

lilleyse commented Mar 13, 2017

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

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 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse Apr 14, 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

JudyWeng commented May 10, 2017

lilleyse commented May 10, 2017

lilleyse commented Jul 1, 2017

JudyWeng Feb 13, 2017 •

edited

lilleyse Apr 14, 2017 •

edited