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

std::unique_ptr not supported #31

Closed
AndreasAZiegler opened this issue Jul 1, 2019 · 14 comments · Fixed by #103
Closed

std::unique_ptr not supported #31

AndreasAZiegler opened this issue Jul 1, 2019 · 14 comments · Fixed by #103
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@AndreasAZiegler
Copy link

In order to get zero-copying inter-communication, std::unique_ptr are required. Unfortunately message_filters does not yet support std::unique_ptr.

@AndreasAZiegler
Copy link
Author

Is there any update?

@ZhenshengLee
Copy link

@ZhenshengLee
Copy link

can be closed.

@tonynajjar
Copy link

I'm also finding myself needing this. Any update?

can be closed.

I wouldn't close this issue, the answer provided only proposes a workaround, it would be nice to have a PR for this if it's a viable solution approved by the maintainers

@roncapat
Copy link
Contributor

I agree. I will try to implement if I have time, but I'm not making any real commitment, so feel free to do the same and open a PR pointing to this issue.

@roncapat
Copy link
Contributor

roncapat commented Sep 29, 2023

Moreover, be aware that rolling supports Type Adaptation (did a PR some time ago #95). This means that, in conjunction with intra-process comms, we would be able then to leverage the full spectrum of solutions to optimize wide ROS data exchange.

@roncapat
Copy link
Contributor

roncapat commented Oct 5, 2023

I'm giving it a look.
In my experience, I always used const MessageType::ConstSharedPtr as subscription callback type, and built MessageType::UniquePtr to be later ->publish(std::move(...))d.
message_filters make use of std::shared_ptr<MessageType const> signature, which is equivalent to MessageType::ConstSharedPtr.

It made be worth a try just to add const and see what happens as a first iteration.

@roncapat
Copy link
Contributor

roncapat commented Oct 5, 2023

To be completely honest, it seems to me that:
https://github.com/ros2/rclcpp/blob/9019a8d9b7710bd405a5a31c8d74fdc9ffa686dd/rclcpp/include/rclcpp/any_subscription_callback.hpp#L935
just requires the pointed object of the shared_ptr to be const, not necessarily the pointer itself.

May I ask to you all to verify?

@roncapat
Copy link
Contributor

roncapat commented Oct 9, 2023

@tonynajjar @AndreasAZiegler may I ask you to test #103 and see if you get any performance gain?

@tonynajjar
Copy link

Hey, I can try tomorrow. What should I be on the lookout for exactly? CPU consumption? Anything else?
As far as I know the msg memory address should be the same regardless of the change in the PR right?

@roncapat
Copy link
Contributor

roncapat commented Oct 9, 2023

@tonynajjar was the address already the same in previous test of yours?

Because otherwise I may have lost the meaning of the problem in your previous comment:

I'm also finding myself needing this. Any update?

May I ask you to clarify what was the issue/need, if different?

@tonynajjar
Copy link

tonynajjar commented Oct 10, 2023

@tonynajjar was the address already the same in previous test of yours?

When I made my first comment I hadn't tested anything yet, I was about to start working on a project in which I needed message_filters to support IPC and I took it for granted that it doesn't because of this ticket.

Now I did some tests on a particular setup of mine and without your changes, I'm able to get the same msg address for both the publisher and the message_filters subscriber. That's the only indication I know of that suggests that IPC is working. Currently I don't have a minimal reproducible example I can share, I'll try to find some time for that

@roncapat
Copy link
Contributor

Ok, nice to know. Then, I do not see any other internal copy or other blocking issue preventing message_filters to be IPC-friendly :)

@roncapat
Copy link
Contributor

@clalancette at least now we know this one has to be closed again then ;)

@clalancette clalancette linked a pull request Oct 11, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants