Skip to content

Conversation

SuperJappie08
Copy link

@SuperJappie08 SuperJappie08 commented Mar 17, 2025

Implementation of a scoped include-action as suggested and discussed in #801.


TODO:

  • Complete Documentation
  • Add tests to verify expected behavior (the behavior has been tested, but missing unit tests)
  • Check if file/action naming is clear enough
    Currently, the Scoped name indicator is placed in different places in the class and file name.
    This probably should be unified for consistency.

@SuperJappie08 SuperJappie08 force-pushed the rolling-scoped-include branch from 8a52136 to ead8657 Compare March 17, 2025 14:30
@SuperJappie08 SuperJappie08 force-pushed the rolling-scoped-include branch from ead8657 to 03c79ae Compare March 17, 2025 14:32
@SuperJappie08
Copy link
Author

Currently, I'm stuck due to this feature resulting in confusing behavior on the user side.
A more elaborate explanation can be found in a comment on the original issue.

Added a 'globals' launch configuration protection set, so things which
should not be scoped (such as `launch_ros` `PushRosNamespace`) can be
registered.

Signed-off-by: SuperJappie08 <[email protected]>
@SuperJappie08 SuperJappie08 marked this pull request as ready for review May 1, 2025 19:08
@SuperJappie08
Copy link
Author

SuperJappie08 commented May 1, 2025

It works, I implemented the second solution explained in the original issue.

There is now a launch local variable named globals (It isn't the best name, I'm open to suggestions).
This contains a set of launch configuration keys, which will be kept.

I have also patched PushROSNamespace and SetRemap to work with this feature (ros2/launch_ros#463).
They will be reset after, their internals are just not scoped (so they are kept as a launch configuration).

More tests are probably necessary/nice, but I would like some feedback on the current implementation before moving forward.

EDIT: Commented to eagerly

@ijnek
Copy link
Contributor

ijnek commented Jun 15, 2025

Thanks for starting this PR. I agree with the issues you've pointed out regarding the ROS namespace and the launch configurations—specifically, how remapping and namespace settings aren't properly propagated due to scoping.

Additionally, I don't think using ResetEnvironment within the scope is desirable in most cases. Many launch files and nodes rely on environment variables being set, so it's generally more appropriate to forward those variables into the included launch description rather than resetting them.

@SuperJappie08
Copy link
Author

@ijnek

Additionally, I don't think using ResetEnvironment within the scope is desirable in most cases. Many launch files and nodes rely on environment variables being set, so it's generally more appropriate to forward those variables into the included launch description rather than resetting them.

Currently, it's mirroring the behavior of GroupAction(scoping=true, forwarding=false) as this was the closest existing behavior. In this reset, only changes made during the launch process are reset and restored afterward.
This is probably closer to what a user would intend, rather than forwarding, as this could change the behavior or the included launch file in unexpected ways.

Currently, the only (standard ROS) environment variables, I can think of are RMW/communication related (e.g. implementation, settings, Domain ID), these should be set before launching, as launch_ros might start an internal node which should also use the same communication settings. (I forgot ROS_LOG_DIR, but I cover this setting later, as it had slipped my mind.)

If I'm really missing a use case, I'm willing to make the change (or add an option), however doing a quick search, I found few uses of SetEnvironmentVariable which would affect multiple nodes (mostly setting GAZEBO_MODEL_PATH which can be done via the package.xml now, setting solver configurations and some rcutils logging configurations).

The main use of a use that affects multiple nodes would be the ROS_LOG_DIR, which can be set by the SetROSLogDir action, which sets the environment variable.
If I were to use this action, I would expect it to also apply to the scoped launch file.

The more I think about it, the more it makes sense to change this, I think restoring the environment variables at the end of the scope still makes sense (e.g. revert to what it was before the scope started).
But I would like to hear your opinion.

@SuperJappie08
Copy link
Author

@emersonknapp, friendly ping.
Could I get some feedback about the direction?

@ahcorde
Copy link
Contributor

ahcorde commented Oct 2, 2025

@emersonknapp, friendly ping.

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.

4 participants