Validate i3dm header #27
Conversation
|
|
||
| return { | ||
| result : true, | ||
| message: 'Tile is a valid ib3m tile ' + gltfFormatMsg |
There was a problem hiding this comment.
ib3m -> i3dm
I think it's fine to not append the "with gltf format as a url" / "with gltf format as an embedded binary gITF" message.
| result : true, | ||
| message: 'Tile is a valid ib3m tile ' + gltfFormatMsg | ||
| }; | ||
| } |
|
|
||
| describe('validateI3dm', function() { | ||
|
|
||
| it('returns true if the i3dm tile is valid, returns false if the i3dm has invalid magic', function() { |
There was a problem hiding this comment.
Remove the "returns true if the i3dm tile is valid" part from these descriptions.
| it('returns true if i3dm matches spec with glTF field of the body being a url', function() { | ||
| var validatorObject = validateI3dm(createI3dmTileGltfUrl()); | ||
| var message = validatorObject.message; | ||
| expect(validatorObject.result && message.includes("url")).toBe(true); |
There was a problem hiding this comment.
Use single quotes for strings. Shows up in a few other places as well.
| expect(validatorObject.result && message.includes("url")).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true if i3dm matches spec with glTF field of the body being an embedded binary glTF', function() { |
There was a problem hiding this comment.
The wording here can be simplified to something like:
validates an i3dm with an embedded binary glTF. Same with the url test.
Like above, it's fine not to check the message.
| header.writeUInt32LE(5, 28); // gltfFormat: invalid | ||
|
|
||
| return header; | ||
| } |
| var byteLength = content.readUInt32LE(8); | ||
| var gltfFormat = content.readUInt32LE(28); | ||
|
|
||
| var gltfFormatMsg = ""; |
There was a problem hiding this comment.
Throughout use single quotes. Check out the Coding Guide for the conventions we use.
|
|
||
| if (gltfFormat === 0) { | ||
| } else if (gltfFormat === 1) { | ||
| } else { |
There was a problem hiding this comment.
This would be cleaner as:
if (gltfFormat !== 0 && gltfFormat !== 1) {
| result : true, | ||
| message: 'valid' | ||
| }; | ||
|
|
| message: 'valid' | ||
| }; | ||
|
|
||
| } |
| var validateI3dm = require('../../lib/validateI3dm'); | ||
|
|
||
| describe('validateI3dm', function() { | ||
|
|
| header.writeUInt32LE(0, 28); // gltfFormat: 0 - url | ||
|
|
||
| return header; | ||
|
|
| header.writeUInt32LE(0, 28); // gltfFormat: 0 - url | ||
|
|
||
| return header; | ||
|
|
There was a problem hiding this comment.
Same comment for the other 2 functions.
|
Some old commits from the b3dm validation seemed to have gotten merged incorrectly into here. Can you remove those? Also make sure to leave a quick comment when the PR is ready for me to review. This looks good! Just these new comments. |
|
Hey Sean, I think the previous b3dm validation commits are here because it automatically rebases when I pull and I usually pull from the validator-master branch. Am I not supposed to be pulling from the validator-master branch while I work on these lib files? And how would I go about removing the old commits that already got duplicated here? I haven't pushed my new local changes yet just in case it complicates things. |
|
I'm not sure why rebase isn't working as expected. Personally, I almost always do To remove the commits you can try doing an interactive rebase. |
JudyWeng
force-pushed
the
validate-i3dm
branch
from
2fce485
to
107af7d
Compare
|
I tried to use the interactive rebase to drop the additional commits that got added and pushed, but it seemed to add a lot of the older validateB3dm commits instead. |
|
Hm ok, we can work through this later today. |
|
closed in favor of #31 |
#22
I've ran jsDoc, jsHint, and the tests and everything seems fine so far. I usually test every unit test possible, and therefore made 2 valid i2dm tiles with the different gITF formats. Please let me know if I should just keep it to 1 valid tile instead or if anything else needs to be changed.