Unity-Technologies / ROS-TCP-Endpoint

ROS package used to create an endpoint to accept ROS messages sent from a Unity scene using the ROS TCP Connector scripts
Apache License 2.0
185 stars 124 forks source link

Expose the latch and queue_size functionality in the RegisterPublisher sys_command #82

Closed P3TE closed 3 years ago

P3TE commented 3 years ago

Is your feature request related to a problem? Please describe. I would like to be able to set the queue_size and the latch parameter when calling the RegisterPublisher sys_command.

Additional context Line 25 of publisher.py on the 'dev' branch has "# TODO: surface latch functionality", so I plan to implement that.

peifeng-unity commented 3 years ago

Hello @P3TE, thank you for the request and it is a good feature to have. I have created an internal ticket AIRO-990 to track it. It may take us some time to follow up and implement it. In the meantime, if you want to implement this feature, we are more than happy to help and review your changes to merge them into the codebase.

P3TE commented 3 years ago

Hello again, I thought I should provide an update to this ticket.

So after some initial investigating I found that to expose the latch and queue_size parameters would also require a change to the ROS-TCP-Connector. In fact most of the work is there.

So I went about implementing this feature, and I may have dreamed up some additional features that I thought might be nice.

These include:

So these features are mostly implemented on a new branch 'publisher-latch-dev' on my fork:

I realise that I've stepped out of the scope of just exposing the queue_size and latch functionality, but I thought I should touch base with you guys to get your opinion on these features.

mrpropellers commented 3 years ago

Hey @P3TE. Looks like this issue has fallen by the wayside a bit - but if you are still open to working on these features, feel free to open a draft PR against your base branch and tag me on it. I can go over what you have so far and discuss with the team what the best approach would be to get your code into the origin dev branch.

P3TE commented 3 years ago

@mrpropellers So the current status of this is that I have implemented most of the functionality and I've been testing it locally but I did move onto other unrelated parts of the project I'm working on. If you are interested in the features listed above in the previous post then I'll do a clean up of the code and create a PR.

mrpropellers commented 3 years ago

These both sound like good features to have, but they also sound pretty design heavy so I can't guarantee I could let them go in right away without a thorough review of the implementations. I'm happy to go though the code with you, though! Just wanted to give you the heads up that it might entail some additional iterations if there's a lot of new logic in there.

P3TE commented 3 years ago

Sounds good, additional iterations are no problem. I'll aim to get a PR tomorrow and we can discuss the changes!