Refactor image_proc and stereo_image_proc launch files

[jacobperron]

• Allow passing container name to image_proc launch file.
  If a container name is provided, then load the image_proc nodes into that container. Otherwise, launch a container to load the nodes into.
• Include the image_proc launch file in the stereo_image_proc launch file
  This resolves a TODO, making the launch file have similar behavior as the version from ROS 1.
  Also expose a new launch argument for optionally providing a container (similar to image_proc's launch file).

---

These changes takes advantage of new launch conditions introduced in ros2/launch#453. Therefore, if we want this change to be compatible with Foxy, we should backport the launch additions. I'm indifferent if this change is backported to Foxy, but if it is desired I can make the backport of the launch PR.

[JWhitleyWork]

Only one minor item.

[JWhitleyWork]

Looks like there are still test errors in stereo_image_proc too.

[jacobperron]

Rebasing to get #584 fixed some errors, but it looks like test_disparity_node is flaky given that it passed the "push" triggered build but not the "pull_request" build. I'll take a look at fixing the test and open a separate PR.

[jacobperron]

@JWhitleyWork I've made addition changes

1. Making image_proc nodes optional in the launch file (e644bc1)
2. Making sure that if the namespace arguments are given, then the stereo_image_proc nodes use the right topic names (ac495ca)

Note, this change is not compatible with Foxy. I recommend we create a foxy branch that refers to the state of the code before this change, and update rosdistro accordingly.

[jacobperron]

I've opened a PR to help alleviate the flaky tests: #588

[JWhitleyWork]

I recommend we create a foxy branch that refers to the state of the code before this change, and update rosdistro accordingly.

I would prefer to not push changes which are incompatible with the current LTS release to a branch which is specifically targeting that release. The idea with our branch names is that master targets the current ROS1 LTS release and ros2 targets the current ROS2 LTS release. If you'd like to test these outside of the ros2 branch, I can create a ros2-experimental which targets the current unreleased source but this repo is somewhat central to ROS so I don't want to cause compatibility issues.

[jacobperron]

@JWhitleyWork In this case, I'm only using features from Rolling for aesthetic purposes (i.e. The LaunchConfigurationEquals and LaunchConfigurationNotEquals). I can propose an alternative that is compatible with Foxy or look into backporting those new launch conditions to Foxy.

In general, I think it would be nice to have a place to land changes for Rolling, but I'm okay if you just want to maintain compatibility with the latest release. Note, we might run into issues in the future if something changes upstream in Rolling that is not compatible with the latest release; e.g. The Rpr job might start failing and then we should either remove this package from Rolling or make a separate branch.

[JWhitleyWork]

I keep forgetting about Rolling. How does rolling work? Do you have to specifically target it with a Bloom release?

[jacobperron]

How does rolling work? Do you have to specifically target it with a Bloom release?

The idea is for Rolling to be a staging ground for the upcoming ROS release (e.g. Galactic). When we get close to the next release, the packages released into Rolling will be automatically released into the new distro (e.g. the version of image_pipeline in Rolling will be released into Galactic for you). You should make releases into Rolling independent from other distros. You can give bloom the target rolling when making a release.

For details, see REP 2002.

[jacobperron]

@JWhitleyWork I've backported ros2/launch#457 to Foxy and released it, so this PR should now be compatible with Foxy.

[jacobperron]

@JWhitleyWork Friendly ping. I think this PR is ready.

[JWhitleyWork]

Looks like the backport made it into the sync and we're good to go on that side. However, when testing the stereo_image_proc launch file locally on Foxy, I get the following error:

Future exception was never retrieved
future: <Future finished exception=AttributeError("'LaunchConfiguration' object has no attribute 'name'")>
Traceback (most recent call last):
  File "/usr/lib/python3.8/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/actions/load_composable_nodes.py", line 173, in _load_in_sequence
    self._load_node(next_composable_node_description, context)
  File "/opt/ros/foxy/lib/python3.8/site-packages/launch_ros/actions/load_composable_nodes.py", line 144, in _load_node
    container_logger = launch.logging.get_logger(self.__target_container.name)
AttributeError: 'LaunchConfiguration' object has no attribute 'name'

Since it doesn't mention the launch file in the Traceback, it doesn't seem to affect the functionality, and it doesn't happen on the image_proc launch file, I'm going to assume that it is an unrelated bug in launch_ros and let it go for now. Just thought I'd mention it.

[jacobperron]

I've looked into the Foxy launch issue, turns out it was one bug exposing another. The first is that pushing the included launch file into a namespace doesn't work in Foxy (ie. left and right image_proc nodes have the same names). There are a few launch_ros bug fixes that still need to be backported to Foxy to make this work (I'm looking into it). This exposed a small bug in the LoadComposableNode action that I've fixed in ros2/launch_ros#177 (which should fix the traceback we're seeing).

[JWhitleyWork]

@jacobperron Thanks for the follow-up and for working on a fix!