Add support for CBOR encoding

[mvollrath]

Goes with RobotWebTools/rosbridge_suite#364

[mvollrath]

I intend to add some tests this week.

[viktorku]

Thanks for the contribution! I was unaware of this CBOR compression method and can't wait to stress test it ;)

This PR seems to have lots of changes which are scattered among the built file and in the modules. Hence it's difficult to review and evaluate the impact on existing code vs clearly separated new code.

Relevant changes should appear only in the individual modules, and a final single "build" commit should produce the minified and non-minified file for CDN purposes. Sorry for this confusion, we need to stop hosting the build folder contents altogether.

[viktorku]

This chunk of code should not appear solely in this file, roslib.js is the result of all packed non-minified modules in the repo.

[mvollrath]

Are you saying I should remove all changes to build/ from this PR?

[viktorku]

Are you saying I should remove all changes to build/ from this PR?

Yes, the build directory is not supposed to have any manual changes - that's the result of packing all modules in src. You can freely add new modules where they seem appropriate, in core and/or util in your case mostly I think.

[mvollrath]

Added my tests and removed the build cruft.

[jihoonl]

We might want to remove build directory at some point as we have seen this minified version drags some PRs down. Not in this PR tho..

[subvertallchris]

I know I'm coming in here way late but just felt obligated to say that I really don't love the idea of adding a dependency (cbor-js) that hasn't been updated or seen a closed issue since 2016 and has benchmarks to indicate that it's significantly slower than JSON.

I'm not suggesting that this be immediately removed if people are using it but I'd love to know what kind of adoption it really has. In the meantime, is it worth considering some kind of plugin system that would let someone opt in to this or other libraries like it?

[mvollrath]

I know I'm coming in here way late but just felt obligated to say that I really don't love the idea of adding a dependency (cbor-js) that hasn't been updated or seen a closed issue since 2016 and has benchmarks to indicate that it's significantly slower than JSON.

If you have a favorite CBOR library, feel free to drop it in. I tried all of them and decided to use this one. CBOR is still a new protocol and I expect by the time there is a maintenance problem with this library a better one will be available.

The performance difference depends a lot on what kind of data is being deserialized, for all the types of data this patch was intended to optimize (large numeric arrays and untyped binary data e.g. PointCloud2 buffers) CBOR is significantly faster than JSON and its output (typed arrays) is more useful. This difference is described in the rosbridge documentation. This is why CBOR is not the default encoding for rosbridge.

[subvertallchris]

My point is more that I don’t know why everyone who uses roslibjs or rodbridge should be required to download a CBOR dependency at all. While I appreciate your interest in wanting to use a new data format, I’m not sure it’s mainstream or mature enough to justify its inclusion in the libraries. A plug-in system that makes it easy to use this and things like it would make it easier to try things like this.

…

On Mon, Feb 10, 2020 at 12:20 PM Matt Vollrath ***@***.***> wrote: I know I'm coming in here way late but just felt obligated to say that I really don't love the idea of adding a dependency (cbor-js <https://github.com/paroga/cbor-js>) that hasn't been updated or seen a closed issue since 2016 and has benchmarks <paroga/cbor-js#11> to indicate that it's significantly slower than JSON. If you have a favorite CBOR library, feel free to drop it in. I tried all of them and decided to use this one. CBOR is still a new protocol and I expect by the time there is a maintenance problem with this library a better one will be available. The performance difference depends a lot on what kind of data is being deserialized, for all the types of data this patch was intended to optimize (large numeric arrays and untyped binary data e.g. PointCloud2 buffers) CBOR is significantly faster than JSON and its output (typed arrays) is more useful. This difference is described in the rosbridge documentation. This is why CBOR is not the default encoding for rosbridge. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#303?email_source=notifications&email_token=AA7IJ5ZMITSF667XF66RJALRCGEEXA5CNFSM4GB3GAC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELJLIYI#issuecomment-584234081>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7IJ557Q5PLNRY3MSL7UXLRCGEEXANCNFSM4GB3GACQ> .

[mvollrath]

I was interested in making rosbridge fast enough so that people can send all types of data over it instead of having to develop special socket servers or republishers for things like point clouds. The people using it seem to be happy with it. I'd rather not make things more complicated for them unless there's a really good reason for it. From where I'm sitting, the CBOR feature only makes up about 10% of the total LoC&C for this client project. The runtime footprint is one if condition per message if you never invoke it. That doesn't seem like a significant burden for anybody, considering what it does for people who need it.

[subvertallchris]

I hear what you're saying. I apologize if this comes off as rude or dismissive of your interest in pushing things forward because I totally respect the work that went into all of this. I just think that it's worth being sensitive about which dependencies make it into a project and why they're added, especially if new dependencies are on unmaintained libraries. I'm not suggesting we roll back the clock and pull this out -- the ship has sailed on that. I was only chiming in so this could be considered for future PRs.