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

As ROS2 rclcpp Component [#162] #163

Merged
merged 6 commits into from
Jun 6, 2023
Merged

As ROS2 rclcpp Component [#162] #163

merged 6 commits into from
Jun 6, 2023

Conversation

roncapat
Copy link
Contributor

See [#162].

This produced a new target: odometry_component, while odometry_node is then automagically generated by rclcpp_components_register_node CMake macro.

Copy link
Collaborator

@nachovizzo nachovizzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! Thanks for the contribution. Some minor comments and ready to merge

ros/package.xml Outdated Show resolved Hide resolved
ros/CMakeLists.txt Show resolved Hide resolved
ros/ros2/OdometryServer.hpp Outdated Show resolved Hide resolved
ros/ros2/OdometryServer.hpp Show resolved Hide resolved
ros/ros2/OdometryServer.cpp Show resolved Hide resolved
Copy link
Contributor Author

@roncapat roncapat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if the open points were all addressed from me or not. We're also doing some testing before recommending a merge.

EDIT: tested successfully on our side

@roncapat
Copy link
Contributor Author

@nachovizzo another thing... to reduce overhead and really leverage IPC while publishing PointClouds, the code shall instantiate PointClouds as std::unique_ptrs and then moved to the IPC manager by doing publish(std::move(your-point-cloud-here)). This may required some additional work around util.hpp file, so we may want to tackle it in a separate issue/PR later on IMHO.

@nachovizzo
Copy link
Collaborator

@nachovizzo another thing... to reduce overhead and really leverage IPC while publishing PointClouds, the code shall instantiate PointClouds as std::unique_ptrs and then moved to the IPC manager by doing publish(std::move(your-point-cloud-here)). This may required some additional work around util.hpp file, so we may want to tackle it in a separate issue/PR later on IMHO.

I guess it makes sense to do it on a separate MR. Great suggestion

ros/package.xml Outdated Show resolved Hide resolved
@nachovizzo
Copy link
Collaborator

Let me know if the open points were all addressed from me or not. We're also doing some testing before recommending a merge.

EDIT: tested successfully on our side

Everything looks good to me. I won't be able to test this myself until next Thursday. So I guess it will be blocked, at least, until then.

Thanks again for putting the effort into this! highly appreciatged

@nachovizzo nachovizzo linked an issue May 25, 2023 that may be closed by this pull request
@roncapat
Copy link
Contributor Author

roncapat commented May 29, 2023

@nachovizzo did you have the chance to test?

@nachovizzo
Copy link
Collaborator

@roncapat not really! Sorry for the slowness, this week we are also at ICRA. So likely to test it next week. But I'm definitely going to look at it!

@nachovizzo
Copy link
Collaborator

nachovizzo commented Jun 6, 2023

@roncapat I Tested in 3 datasets on my end and works like a charm!

If you encountered any issues, please let me know.

Thanks a lot for the contribution and bringing this to KISS-ICP!

@nachovizzo nachovizzo merged commit 836046c into PRBonn:main Jun 6, 2023
@nachovizzo nachovizzo mentioned this pull request Nov 2, 2023
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ROS2 OdometryServer composable
2 participants