[draft] merge the remapping argument into Player's NodeOption.

[fujitatomoya]

Description

Fixes #2088

Is this user-facing behavior change?

Yes, with this PR, Player constructor honors the NodeOption argument to append play_option.arguments.
in other word, play_option.arguments does not overwrite the argument for the NodeOption.

Did you use Generative AI?

Yes, Copilot Claude Sonnet 4.

Additional Information

[fujitatomoya]

this change is unrelated but i met the following error to be fixed with it.

135: Test command: /usr/bin/python3 "-u" "/root/ros2_ws/colcon_ws/install/ament_cmake_test/share/ament_cmake_test/cmake/run_test.py" "/root/ros2_ws/colcon_ws/build/rosbag2_transport/test_results/rosbag2_transport/uncrustify.xunit.xml" "--package-name" "rosbag2_transport" "--output-file" "/root/ros2_ws/colcon_ws/build/rosbag2_transport/ament_uncrustify/uncrustify.txt" "--command" "/root/ros2_ws/colcon_ws/install/ament_uncrustify/bin/ament_uncrustify" "--xunit-file" "/root/ros2_ws/colcon_ws/build/rosbag2_transport/test_results/rosbag2_transport/uncrustify.xunit.xml"
135: Working Directory: /root/ros2_ws/colcon_ws/src/ros2/rosbag2/rosbag2_transport
135: Test timeout computed to be: 60
135: -- run_test.py: invoking following command in '/root/ros2_ws/colcon_ws/src/ros2/rosbag2/rosbag2_transport':
135:  - /root/ros2_ws/colcon_ws/install/ament_uncrustify/bin/ament_uncrustify --xunit-file /root/ros2_ws/colcon_ws/build/rosbag2_transport/test_results/rosbag2_transport/uncrustify.xunit.xml
135: Code style divergence in file 'src/rosbag2_transport/player.cpp':
135:
135: --- src/rosbag2_transport/player.cpp
135: +++ src/rosbag2_transport/player.cpp.uncrustify
135: @@ -2180 +2180 @@
135: -  )
135: +)
135: @@ -2225 +2225 @@
135: -  )
135: +)
135: @@ -2264 +2264 @@
135: -  ),
135: +),
135: @@ -2267 +2267,2 @@
135: -{}
135: +{
135: +}
135:
135: 1 files with code style divergence

[fujitatomoya]

Pulls: #2089
Gist: https://gist.githubusercontent.com/fujitatomoya/4761b0e14767facd5d512bc9f30ae692/raw/6e5ebc0795e06da4fd62ddb6a150a7906403e4fa/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport
TEST args: --packages-above rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/16497

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

[Copilot]

Pull Request Overview

This PR merges NodeOptions arguments with topic remapping options in the Player constructor instead of overwriting them. The change ensures that existing arguments from NodeOptions are preserved while appending the topic remapping options, addressing issue #2088.

• Modifies three Player constructor overloads to merge arguments instead of replacing them
• Changes the argument handling from direct assignment to a lambda-based merge operation
• Preserves existing NodeOptions arguments while appending topic remapping options

[Copilot]

The identical lambda logic is duplicated across three constructors. Consider extracting this into a private helper function to improve maintainability and reduce code duplication.

Suggested change

	[&, node_options]() {
	auto args = node_options.arguments();
	args.insert(args.end(),
	play_options.topic_remapping_options.begin(),
	play_options.topic_remapping_options.end());
	return args;
	}()
	)
	)
	{
	merge_node_arguments(node_options, play_options)
	)
	)
	{
	// Helper function for merging node arguments
	auto merge_node_arguments(
	const rclcpp::NodeOptions & node_options,
	const rosbag2_transport::PlayOptions & play_options) -> std::vector<std::string> {
	auto args = node_options.arguments();
	args.insert(args.end(),
	play_options.topic_remapping_options.begin(),
	play_options.topic_remapping_options.end());
	return args;
	}

[fujitatomoya]

hmm i do not think this is necessary. waiting for what other people think about this.

[roncapat]

Hi @fujitatomoya and thank you for the patch. I will try to test it tomorrow. Apart from possible code deduplication, this seems ok in principle for me.

I have only one doubt: take as reference the following lines in a test case of this repo:

rosbag2/rosbag2_transport/test/rosbag2_transport/test_play_topic_remap.cpp

Lines 41 to 42 in 9741bfc

	play_options_.topic_remapping_options =
	{"--ros-args", "--remap", "topic_before_remap:=topic_after_remap"};

Never actually checked (but I may try to experiment with rclcpp tomorrow or dig inside the codebase) - as we chain argument lists, what if there is duplicate "--ros-args" token? Is it ok to have it let's say twice in the argument list?

[roncapat]

Another thing that I noticed right now is that the standard constructor will hit these lines:

rosbag2/rosbag2_transport/src/rosbag2_transport/config_options_from_node_params.cpp

Lines 187 to 195 in 9741bfc

	auto topic_remapping_options = node.declare_parameter<std::vector<std::string>>(
	"play.topic_remapping_options", std::vector<std::string>());
	if (!topic_remapping_options.empty()) {
	RCLCPP_WARN(
	node.get_logger(),
	"Remappings shall be applied through standard CLI/Launch options."
	"'topic_remapping_options' content will be ignored.");
	}

So basically only node arguments are accepted/kept. Shall we reason in the same way, by dropping the ones from play_options and print the same kind of warning message instead?

[MichaelOrlov]

@fujitatomoya @roncapat, Am I understanding correctly that this PR is still WIP?

[roncapat]

@MichaelOrlov IMHO yes. By the way, may I ask your opinion on my previous comments?

[MichaelOrlov]

Hi @roncapat As regards more than one --ros-args --remap options. Someone need to check if it is Okay to have them more the one in the node options. Life would be much easier if it were allowed to have more than one such argument.

As regards

So basically only node arguments are accepted/kept. Shall we reason in the same way, by dropping the ones from play_options and print the same kind of warning message instead?

I would disagree with this statement. Because otherwise, how to remap topics from CLI?
We can't do just ros2 bag play -i bag2.mcap --ros-args --remap topic1:=topic2
Please refer to the

rosbag2/ros2bag/ros2bag/verb/play.py

Lines 99 to 102 in 8607af5

	parser.add_argument(
	'--remap', '-m', default='', nargs='+',
	help='list of topics to be remapped: in the form '
	'"old_topic1:=new_topic1 old_topic2:=new_topic2 etc." ')

and

rosbag2/ros2bag/ros2bag/verb/play.py

Lines 250 to 253 in 8607af5

	topic_remapping = ['--ros-args']
	for remap_rule in args.remap:
	topic_remapping.append('--remap')
	topic_remapping.append(remap_rule)

[roncapat]

Thank you for the clarifications and feedback.
So what is missing here is:

• Tests
• Check the double --ros-args token potential issue
  It works! ✅
  Proof:

  > ros2 run tf2_ros tf2_monitor --ros-args -r /tf:=a --ros-args -r /tf_static:=b
  

  In another terminal:

  > ros2 topic list
  /a
  /b
  /parameter_events
  /rosout