RobotecAI / ros2-for-unity

High-performance ROS2 solution for Unity3D
Apache License 2.0
448 stars 58 forks source link

Suggestion: Option for main threaded ROS2UnityComponent #35

Closed AlgoryxJosef closed 2 years ago

AlgoryxJosef commented 2 years ago

We are running a high-frequency application (fixed update runs at 1000hz). We needed to perform tasks in the main Unity thread each time a message arrives to any of our subscribers. Since SpinOnce, and thus all message receive callback code is running on a separate worker thread, we found that we had to synchronize this by basically pushing jobs (Action) to a queue that was then executed on the main unity thread. We noticed that this approach gave a big overhead and the performance was not great.

So instead we modified the ROS2UnityComponent to run on the main thread, which was feasable after a modification to the SpinOnce in the ros2cs which allows for non-blocking, zero timeout time, see: https://github.com/RobotecAI/ros2cs/pull/16

We are currently using something like the following: ROS2UnityComponentMainThread.txt

Things to note:

My question is: would it be of interest to have a version of the ROS2UnityComponent called ROS2UnityComponentMainThread or something similar that can also be part of the scripts directory, so that users can choose what they would like, depending on their needs? Or is there perhaps some fundamental issues with doing it this way that I am currently not aware of?

If this would be of interest, I could create a pull-request with the added ROS2UnityComponentMainThread. If not, then no problem; this is a mere humble suggestion.

All the best, Josef

adamdbrw commented 2 years ago

I think that it is a reasonable justification for having an option for the user. The Asset is of course just a suggestion on how to use underlying ros2cs API.

As I see it, this is more of an option where to run the execution than a separate entity. I would opt for code reuse, as much as possible, perhaps the entire thing can be handled by a component parameter (a thread will be created by default, but if the parameter is set, we instead call the Tick() function directly from FixedUpdate). What do you think?

In any case, please feel welcome to submit a PR, we can discuss design in there.

AlgoryxJosef commented 2 years ago

Thanks for the feedback, I'll create a PR and link it from here soon. Remark: I am not entirely sure how much time I will be able to spend on this, so if it halts, feel free to close this issue in the future if nothing happens. I will try to remeber to keep it updated though.

AlgoryxJosef commented 2 years ago

I'm actually going to close this issue since I currently have no plan on creating a PR; it has since the time of creating this issue become apparent that main threaded ROS2 polling (SpinOnce) has some big drawbacks, and for us we ditched the idea completelty. It had to do with the fact that Unitys FixedUpdate() is not executed "evenly". If set to 1000hz, it will typically execute a batch of FixedUpdate() as quickly as possible, and then do nothing until the next Update() cycle. In other words; it will not run once every millisecond. It may run 10 times in 1 millisecond, do nothing for 9 milliseconds, and then start over again. This article touches on this: https://johnaustin.io/articles/2019/fix-your-unity-timestep