Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BSON is better for ROS# #198

Merged
merged 3 commits into from
May 9, 2019
Merged

BSON is better for ROS# #198

merged 3 commits into from
May 9, 2019

Conversation

akirayou
Copy link
Contributor

@akirayou akirayou commented May 8, 2019

rosbridge is now (0.11.0 or later) supports Inf and NaN float data on bson_only_mode.
RobotWebTools/rosbridge_suite#391
It's useful for LaserScan data (It often contains Nan or Inf float data).
And also BSON is also little bit faster on binary data such as PointCloud.

I just overwrite Serialize/Deserialize function,So you can not use JSON any more.
So in this Patch,you have to launch rosbridge with bson_only_mode flag.
Like this.

<launch>
	<include file="$(find rosbridge_server)/launch/rosbridge_websocket.launch">
		<arg name="port" value="9090"/>
		<arg name="bson_only_mode" value="true"/>
	</include>
</launch>

@MartinBischoff
Copy link
Collaborator

MartinBischoff commented May 8, 2019

Thanks @akirayou , this is nice.
Having a BSON (de-) serialization make sense as we already discussed in #136.
However I would not want to completely replace JSON by BSON in the ROS# master branch.
There are also reasons to use JSON and I think best would be to have both options, with JSON as default.

So I'd suggest to change the Serialize method as follows (Deserialize analogous):

private static byte[] Serialize<T>(T obj, bool UseJson = true)
{
    if (UseJson)
      <existing code>
    else // if Use BSON
       <your Bson serialization>
}

Does this make sense to you?

I also find your contirbution / modifications worth mentioning in the file's header. So would you please add a comment like:

// Adding BSON (de-)seriliazation option
// <your affilitation>, 2019, <your name> (your e-mail / github account)

one line below the header of each source file that you change, similar to as we did here?

Thank you!

@akirayou
Copy link
Contributor Author

akirayou commented May 8, 2019

I see!

I think adding the flag to the constuctor is better.
like:
public RosSocket(IProtocol protocol,bool UseJson=true)

And I'll also add the option to RosConnector.cs(in Unity3D).
Is it OK?
I'll do it in few days.

Thank you.

@MartinBischoff
Copy link
Collaborator

Right. Please add the options to RosConnector and RosSocket, that's a lot better.

@akirayou
Copy link
Contributor Author

akirayou commented May 9, 2019

I add the code ,selecting serializer.

I remove "static" on Serializer/Desirializer to pass the member variable "Serializer".
If you dislike this workaround I can rewrite by another way. (i.e. Pass it by the function's argument.)

And I used enum instead of bool. Because my friend says "MsgPack is faster!!".

Thank you

@MartinBischoff
Copy link
Collaborator

That is beautiful, thank you.

The only thing left to do is updating Unity3D/Assets/RosSharp/Plugins.RosBridgeClient.dll with you new compiled version.

I will then merge your pull request.

@akirayou
Copy link
Contributor Author

akirayou commented May 9, 2019

Oh,I forgot it.
Done! Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants