-
Notifications
You must be signed in to change notification settings - Fork 89
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
Use thread for contact monitor computation and add option to publish contact markers #162
Use thread for contact monitor computation and add option to publish contact markers #162
Conversation
This updates #74 to the latest version and adds option to plot contact markers to visualize in rviz. |
@johnwason Do you mind taking a look to make sure I correctly fixed conflicts? |
2067308
to
0fa7d1c
Compare
0fa7d1c
to
aa2611a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started to make some comments. Some of them are below. This is probably fine the way it is. However, a couple of somewhat related thoughts
- It seems like we might want to put all of this into a class to get rid of the global variables. Having multiple contact monitors running is a thing I think we might want to have. An example would be where one has a subset of links or we have one that is publishing a different contact distance
- We might want to separate the various processes more completely. For example, I would think contactTest should go in a different thread from visualization. Then that could be running for example on the GPU. Along those lines, computeCollisionReportThread currently blocks callbackJointState. I would think the joint state callback should be really quick and then when the collision checking needs them it would very quickly lock, copy the latest, and unlock the joint states.
I agree that putting the logic into a class and breaking up functionality to different threads is a good idea. This work is outside the scope of my current project so unfortunately I can't spend time on these improvements. Is GPU collision detection available now? |
Not yet, but I know it is on the todo list(or at least the to look into list). |
aa2611a
to
9074d1f
Compare
@mpowelson Lets create an issues with your suggested improvements and mark it as an enhancement so it does not get forgotten. |
No description provided.