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.

Sign up for GitHub

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

Jump to bottom

Added exclude-topic-types to record #1582

Merged
merged 6 commits into from
Mar 28, 2024
Merged

Added exclude-topic-types to record #1582

merged 6 commits into from
Mar 28, 2024

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Mar 12, 2024

Same idea as this PR #1577 but in this case exclude by topic type

build on top of the mentioned PR

@ahcorde ahcorde self-assigned this Mar 12, 2024
@ahcorde ahcorde requested a review from a team as a code owner March 12, 2024 13:01
@ahcorde ahcorde requested review from emersonknapp and hidmic and removed request for a team March 12, 2024 13:01
@delete-merged-branch delete-merged-branch bot deleted the branch rolling March 14, 2024 12:51
@MichaelOrlov
Copy link
Contributor

@fujitatomoya
Copy link
Contributor

@ahcorde after the rebase, i am happy to review this.

@ahcorde
Added exclude-topic-types to record
5e3ac5e 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde force-pushed the ahcorde/rolling/exclude_by_topic_type branch from 1ef6e9d to 5e3ac5e Compare March 15, 2024 10:31
@ahcorde ahcorde changed the base branch from ahcorde/rolling/filter_by_topic_type to rolling March 15, 2024 10:32
@ahcorde
make linters happy
ef749ff 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
make linters happy
5424a63 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 18, 2024

@ahcorde after the rebase, i am happy to review this.

@fujitatomoya ready for review

fujitatomoya
fujitatomoya approved these changes Mar 18, 2024
View reviewed changes
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.

implementation looks good to me.

@ahcorde could you share your use case for these include and exclude topic types?

IMO, currently we can have topic with single type, right? so having the topic options would be good enough for the user configuration. are these extended options for supporting multiple types on a single topic? so that user can filter the specific topic types for the topic? i am curious about the use case. thanks in advance.

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 19, 2024

@fujitatomoya

Imaging that I have a decent amount of cameras running and I just want to avoid include all of them, with the exclude topic types, it's quite easy to do it, then I can analize my data easily

And the opposite for the other argument, I have a lot topics publishing but I only want to record my cameras, because I want to rerun my algorithm based on cameras.

does it make sense ?

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 20, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 20, 2024

@MichaelOrlov are you fine with this PR ?

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 22, 2024

@ros-pull-request-builder retest this please

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 25, 2024

friendly ping @MichaelOrlov

@MichaelOrlov
Copy link
Contributor

@ahcorde Sorry for the delay. Was busy with other PRs and issues.
I will take a look tomorrow.

@MichaelOrlov
Copy link
Contributor

@ahcorde I see some changes in the API in the rosbag2_py package.
Could you please rebase your branch and regenerate Python stubs according to the instruction from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_py/README.md ?
We recently merged Add Python stubs for rosbag2_py #1569 and now require updating Python stubs.

MichaelOrlov
MichaelOrlov requested changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@ahcorde Overall looks good among one finding in tests and one nitpick.

rosbag2_transport/src/rosbag2_transport/topic_filter.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp Show resolved Hide resolved
ahcorde added 3 commits March 27, 2024 10:03
@ahcorde
Merge remote-tracking branch 'origin/rolling' into ahcorde/rolling/ex…
6009815 
…clude_by_topic_type
@ahcorde
Added feedback
8f8a48b 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde
make linters happy
36de8c9 
Signed-off-by: Alejandro Hernández Cordero <[email protected]>
@ahcorde ahcorde requested a review from MichaelOrlov March 27, 2024 09:39
MichaelOrlov
MichaelOrlov approved these changes Mar 27, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

@ahcorde Thanks. LGTM with green CI.

@ahcorde
Copy link
Contributor Author

ahcorde commented Mar 27, 2024

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@MichaelOrlov
Copy link
Contributor

@ahcorde Please hold off from merging. Let's merge the release first #1596 since it has already in progress.

@ahcorde ahcorde merged commit 34ccb39 into rolling Mar 28, 2024
13 of 14 checks passed
@delete-merged-branch delete-merged-branch bot deleted the ahcorde/rolling/exclude_by_topic_type branch March 28, 2024 10:49
@tonynajjar tonynajjar mentioned this pull request Apr 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp was automatically assigned from ros2/tooling-reviewers

@hidmic hidmic Awaiting requested review from hidmic hidmic was automatically assigned from ros2/tooling-reviewers

Assignees

@ahcorde ahcorde

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @MichaelOrlov @fujitatomoya