Exclude recorded /clock topic when --clock option is specified

[kosuke55]

As mentioned in #1645, if you play a rosbag containing /clock with --clock, it will be double-published.
This PR excludes /clock when --clock is specified.

ros2 bag play sample_bag --clock 100

before

---
clock:
  sec: 1715138101
  nanosec: 880998015
---
clock:
  sec: 1666578700
  nanosec: 606046430
---
clock:
  sec: 1666578700
  nanosec: 608539499
---
clock:
  sec: 1715138101
  nanosec: 891000787
---
clock:
  sec: 1666578700
  nanosec: 611041316

after

clock:
  sec: 1715138117
  nanosec: 244567726
---
clock:
  sec: 1715138117
  nanosec: 254594666
---
clock:
  sec: 1715138117
  nanosec: 264606514
---
clock:
  sec: 1715138117
  nanosec: 274554989
---
clock:
  sec: 1715138117
  nanosec: 284571707

[MichaelOrlov]

@kosuke55 I am not convinced yet that we need this fix.
I'm curious as to why not use ros2 bag play sample_bag --clock 100 --exclude-topics /clock explicitly from CLI arguments in this case?

[kosuke55]

@MichaelOrlov
Thanks, that is correct and can be solved by explicitly excluding it in the command.
This may be more like a personal situation, but when automating some process, some rosbags include /clock and some do not.
When processing such mixed rosbags, it is necessary to check if /clock is included as a preprocessing step and separate the commands (or always add --exclude-topics option). Although it requires a bit of additional work, adding this process solves the problem.
However, I can't think of a situation where I would want to publish the duplicate /clock, and usually that would cause a bug, so I thought it would be better to automatically exclude it.

[fujitatomoya]

@kosuke55 i think automatic exclusion is user-friendly. as @MichaelOrlov mentioned, we can do this using options and checking topic information in rosbag file. but imo this provides less-burden for ros2bag user. after all, i think this is nice to have.

[fujitatomoya]

i think we should explain this behavior in --clock option help field for sure, e.g If specified, /clock topic in the bag file is excluded to publish..

[kosuke55]

thanks!! I updated option help in 2961ccc cc @MichaelOrlov

[fujitatomoya]

Can we just do the following without any other changes above?

Suggested change

	if play_options.clock_publish_frequency > 0:
	exclude_topics.append('/clock')
	play_options.exclude_topics_to_filter = exclude_topics
	if play_options.clock_publish_frequency > 0:
	play_options.exclude_topics_to_filter.append('/clock')

[kosuke55]

I haven't been able to trace the cause in detail, but I couldn't add it directly 🤔

print(f"play_options.clock_publish_frequency: {play_options.clock_publish_frequency}")
if play_options.clock_publish_frequency > 0:
    print("Before appending:", play_options.exclude_topics_to_filter)
    play_options.exclude_topics_to_filter.append('/clock')
    print("After appending:", play_options.exclude_topics_to_filter)
print(f"Final state of exclude_topics_to_filter: {play_options.exclude_topics_to_filter}")

play_options.clock_publish_frequency: 100.0
Before appending: []
After appending: []
Final state of exclude_topics_to_filter: []

[MichaelOrlov]

@kosuke55 Thanks for the detailed explanation about the reasoning for these changes.
Now, it does make sense to me.

I agree with @fujitatomoya that we need to put some notes in the CLI help section about this behavior to not surprise someone and not to be considered as a bug.

Also, I think we will need to wait until changes per #1633 will be merged and then rebase on top of it.