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

add pbstream_to_ros_map_node #54

Merged
merged 1 commit into from
Oct 28, 2021
Merged

add pbstream_to_ros_map_node #54

merged 1 commit into from
Oct 28, 2021

Conversation

chargerKong
Copy link

@chargerKong chargerKong commented Oct 28, 2021

#53
Add a conversion node from pbstream to ros map

Now the ros map can be saved in a way similar to ROS1

ros2 service call /finish_trajectory cartographer_ros_msgs/srv/FinishTrajectory trajectory_id:\ 0
ros2 service call /write_state cartographer_ros_msgs/srv/WriteState filename:\ test.pbstream

Finally, convert the pbstream to ros map through pbstream_to_ros_map_node

ros2 run cartographer_ros pbstream_to_ros_map_node -pbstream_filename test.pbstream -map_filestem test_ros_map

which will generate the test_ros_map.pgm and test_ros_map.yaml in current folder

Copy link

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Note that I retargeted this to the ros2 branch, which I've now switched to for Rolling. This should be backportable, so if you'd like to see this for Foxy and Galactic let me know.

@clalancette clalancette merged commit 447e993 into ros2:ros2 Oct 28, 2021
@chargerKong
Copy link
Author

Yes, I think it should be in any other ROS release

@clalancette
Copy link

@Mergifyio backport dashing

mergify bot pushed a commit that referenced this pull request Oct 29, 2021
(cherry picked from commit 447e993)
@mergify
Copy link

mergify bot commented Oct 29, 2021

backport dashing

✅ Backports have been created

clalancette pushed a commit that referenced this pull request Oct 29, 2021
(cherry picked from commit 447e993)

Co-authored-by: Liangqian <[email protected]>
@doisyg
Copy link

doisyg commented Nov 18, 2021

This is redundant with the full conversion we did here: cartographer-project#1622
See discussion here: cartographer-project#1536
Cartographer is currently a mess for ros2, we are willing to help as we are using it and took care of the conversion. How can we move forward ?

@clalancette
Copy link

This is redundant with the full conversion we did here: cartographer-project#1622
See discussion here: cartographer-project#1536
Cartographer is currently a mess for ros2, we are willing to help as we are using it and took care of the conversion. How can we move forward ?

I'm honestly not sure. I would love to drop this fork entirely and just rely on the upstream (thanks for doing that conversion!). Unfortunately, I don't have any commit rights to upstream, so there isn't a whole lot I can do there. I've left an overall impression review, though.

If you can't get any traction there, we could consider opening a PR to this repository instead and moving it forward that way. It is less ideal, but at least it would make it available to the community, and be newer than the version we are currently shipping.

@doisyg
Copy link

doisyg commented Nov 18, 2021

This is redundant with the full conversion we did here: cartographer-project#1622
See discussion here: cartographer-project#1536
Cartographer is currently a mess for ros2, we are willing to help as we are using it and took care of the conversion. How can we move forward ?

I'm honestly not sure. I would love to drop this fork entirely and just rely on the upstream (thanks for doing that conversion!). Unfortunately, I don't have any commit rights to upstream, so there isn't a whole lot I can do there. I've left an overall impression review, though.

If you can't get any traction there, we could consider opening a PR to this repository instead and moving it forward that way. It is less ideal, but at least it would make it available to the community, and be newer than the version we are currently shipping.

Let's try to get the right in the original repo if possible! That was the original plan: cartographer-project#1536
That would be much less confusing.
@MichaelGrupp could you help ?

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.

3 participants