feat(3dtiles): add support for binary batch table

[jailln]

Description

• Add support for binary batch table attributes
• Add tests for 3d tiles binary property parsing

Motivation and Context

• Improve 3D Tiles support
• Fixes How to load such tiles? No error is reported but tilt photography is not displayed. Can you see where the error is #1826 , Problem adding 3dtile #1830 and Error loading b3dm build by FME  #919

Note: BinaryPropertyAccessor.js may also be used for parsing the binary content of the 3D Tiles feature table (not supported by itowns yet).

[gchoqueux]

Hello Vincent,
Thanks for your contribution.
Your PR is ready for review?

[jailln]

Yes it is 🙂
I will just add more tests to pass the coverage check but it won't change the code already pushed.

[jailln]

Hello @gchoqueux
It's ready to be reviewed.

[gchoqueux]

I am currently busy
I'll look at it as soon as I have some time

[mgermerie]

The constructor signature change is a breaking change. I think we could add a depreciation warning like so :

if (jsonLength + binaryLength !== buffer.byteLength) {
    // The constructor was called with its deprecated signature
    console.warn('some deprecation error message');
    registeredExtensions = batchLength;
    batchLength = binaryLength;
    binaryLength = jsonLength;
    jsonLength = buffer.byteLength - binaryLength;
}

WDYT ?

[jailln]

I think it's a good idea to add this test to check if the provided arguments seem correct but not to check if the constructor was called with the former signature since the test can pass if the constructor is called with the former signature and if the batch length equals the binary length.

However, since there is no optional argument in this function, we can test if the number of arguments is correc and add a deprecation warning and compute the values as suggested if not. WDYT?

[mgermerie]

Indeed, I didn't think of simply counting the number of arguments but it is a safer test 👍

[jailln]

After some more thinking, I think we should not add a deprecation test based on arguments length either since it may happen for other reasons than because the previous constructor was used. In addition, batch tables are not meant to be constructed directly by users of itowns but by our 3D tiles parsers so there is little chance that a user calls this constructor anyway. Therefore, I think we should keep it this way and only add BREAKING CHANGE in the commit title (and therefore in the release). WDYT ?

[jailln]

I added the deprecation warning, added the breaking change message in the commit and rebased.