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 simple bag player examples #1580

Closed
wants to merge 19 commits into from

Conversation

sangteak601
Copy link

Fixes #1449.

Added simple bag player examples in cpp and Python. I also tried to read or write bag files without specifying topic types. However, it became too complicated and was no longer a "simple" example, so I decided not to push the changes.

@sangteak601 sangteak601 requested a review from a team as a code owner March 6, 2024 06:48
@sangteak601 sangteak601 requested review from gbiggs and jhdcs and removed request for a team March 6, 2024 06:48
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
This reverts commit 24f9565.

Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
Signed-off-by: Sangtaek Lee <[email protected]>
@sangteak601 sangteak601 marked this pull request as draft March 6, 2024 14:26
@sangteak601 sangteak601 marked this pull request as ready for review March 6, 2024 14:27
Signed-off-by: Sangtaek Lee <[email protected]>
@sangteak601
Copy link
Author

Hi @MichaelOrlov, I think I screwed up with reviewers. Do you mind fixing it and reviewing this PR ?

Signed-off-by: Sangtaek Lee <[email protected]>
@MichaelOrlov
Copy link
Contributor

@sangteak601 @fujitatomoya I am curious how this PR overlaps with the existing tutorial and examples from https://docs.ros.org/en/rolling/Tutorials/Advanced/Reading-From-A-Bag-File-CPP.html ?

@fujitatomoya
Copy link
Contributor

@MichaelOrlov

I am curious how this PR overlaps with the existing tutorial and examples from https://docs.ros.org/en/rolling/Tutorials/Advanced/Reading-From-A-Bag-File-CPP.html ?

that is what i found too.

i was thinking that https://docs.ros.org/en/rolling/Tutorials/Advanced/Reading-From-A-Bag-File-CPP.html can also be updated to point the source code from this example.

@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Mar 15, 2024

i was thinking that https://docs.ros.org/en/rolling/Tutorials/Advanced/Reading-From-A-Bag-File-CPP.html can also be updated to point the source code from this example.

I would prefer to make them aligned as much as possible but in the tutorial preferable to have the full text with source code instead of just a link on it.

Also, I think that in the examples still unclear point that if the message always shall be deserialized or not before publishing.
To make it clear and show that it is optional I would suggest adding some class member variable or global variable with name like message_needs_to_be_edit_before_send and have an if-else statement showing two options for publishing. With deserialized and altering value before publishing and non-serialized publishing.

cc: @sangteak601

@sangteak601
Copy link
Author

@MichaelOrlov Thanks for the review. I will make a modification.

Based on my knowledge(correct me if I'm wrong), we can also receive serialized messages directly. Therefore, perhaps we can modify the recorder example in a similar manner?

@sangteak601
Copy link
Author

@fujitatomoya @MichaelOrlov I have addressed issues. Do you mind taking another look?

Signed-off-by: Sangtaek Lee <[email protected]>
Copy link
Contributor

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall lgtm, but i have a few comments.

besides,

I would prefer to make them aligned as much as possible but in the tutorial preferable to have the full text with source code instead of just a link on it.

i do agree with this. we need to update https://github.com/ros2/ros2_documentation accordingly to align with these samples.

self.reader = rosbag2_py.SequentialReader()
storage_options = rosbag2_py._storage.StorageOptions(
uri=bag_filename,
storage_id='sqlite3')
Copy link
Contributor

Choose a reason for hiding this comment

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

if i am not mistaken, this is intentionally enabled as sqlite3? but this bag file can be also red from simple_bag_player.cpp, because it reads the meta data from the bag file and use the appropriate storage plugin if available? just checking this because user does not meet the failure during the tutorial.

Copy link
Author

Choose a reason for hiding this comment

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

It was set as sqlite3 in simple_bag_recorder.py. I don't know the reason exactly.
It is possible to read sqlite3 files from simple_bag_player.cpp, but the opposite(reading mcap files from simple_bag_player.py) isn't.

Maybe we can delete storage_id?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, i think that is okay with current code. this also includes an example how to specify the storage id, i think that is fine.

Signed-off-by: Sangtaek Lee <[email protected]>
@sangteak601
Copy link
Author

I would prefer to make them aligned as much as possible but in the tutorial preferable to have the full text with source code instead of just a link on it.

i do agree with this. we need to update https://github.com/ros2/ros2_documentation accordingly to align with these samples.

I will update the documentation once this is merged.

: Node("simple_bag_player")
{
declare_parameter("edit", false);
message_needs_to_be_edit_before_send_ = get_parameter("edit").as_bool();
Copy link
Contributor

Choose a reason for hiding this comment

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

comment for the documentation. this needs to be described how to enable with command line parameter argument. so that user can understand we can publish either publishing serialized data as it is or editing data before publishing in the example.

publisher_->publish(*ros_msg);
std::cout << ros_msg->data << "\n";
} else {
publisher_->publish(*msg->serialized_data);
Copy link
Contributor

@MichaelOrlov MichaelOrlov Apr 13, 2024

Choose a reason for hiding this comment

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

@sangteak601 As far as I recall we need another type of publisher aka GenericPublisher to be able to publish serialized data.

@MichaelOrlov
Copy link
Contributor

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.

Add additional examples for reading ROSBag data and serialized messages
3 participants