Enable zero-copy#171
Conversation
|
@nachovizzo this seems working for us (cc. @Fixit-Davide). Could you run again your demo bags as for the previous PR? Theoretically, no need to compose the node to demonstrate that the patch is working, just run it standalone or as you like. Of course, you should see performance improvements in the following conditions:
|
|
Well... there is a little problem with intra-process enabled. TF2 StaticTransformBroadcaster does not support intra-process on. See my previous patch against TF2 here. I will open a PR against them, but in the meantime I will also need to put in place a workaround in this KISS-ICP patch to remove the problem locally. Edit: PR opened and fixed locally with custom options. |
|
@nachovizzo have you found time to review? BTW, do you think we still need to support Foxy? |
|
@roncapat not really! But it's on my todo-list. I don't think I will have even 5 minutes to check this on the next 2 weeks or so. Apologize for the slowness :( But definitely going to look for it. PS: What would be the advantage of killing foxy support? |
|
No worries, just checking if I can do any improvements in the meantime :) Foxy does not have QoS configurability (override support) so as you see the CI job is failing. QoS override is unfortunately needed to force-disable intra-process on the StaticTransformPublisher. If we don't suppress it, we can't use true intra-process all around the node. I'm trying to figure out if a fix for foxy could exist... |
|
Hey @nachovizzo, do you have any updates? :) |
|
@roncapat I guess I will kill ROS2 foxy, since EOL was reached a few weeks ago anyways. I'm reviewing this, testing it, and merging it soon. Could you give me access to push to your branch? This way you don't need to change everything I'm asking |
|
Of course, I sent you the invite :) |
nachovizzo
left a comment
There was a problem hiding this comment.
This looks good to me. I only have 1 small question.
Turns out not to be necessary: - PRBonn#171 (review)
|
@roncapat I'm done reviewing and testing this. Thanks a lot again! If you have time, just eye-ball the last diff to make sure we are on the same page, I'll merge after you give me thumbs up 👍 Thanks! |
|
LGTM :) |
|
Great! Sorry I squeezed some small non-related changes 🙈 |
See #164.
For now, only clouds and odometry ownership is moved to IPC manager.
Path publishing for now is kept standard (of course we may create a copy of it and gift to IPC manager).
UNTESTED CODE (this is why it is still draft)