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.

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

Support "scope" for machine tag default semantics #2059

Open
wants to merge 1 commit into
base: noetic-devel
Choose a base branch
from

Conversation

luisrayas3
Copy link

Proposed fix for #1884

@luisrayas3 luisrayas3 marked this pull request as draft September 24, 2020 02:08
@luisrayas3 luisrayas3 marked this pull request as ready for review September 24, 2020 17:21
@luisrayas3
Copy link
Author

Open to alternatives to "scope" for the value that enables this behavior. "true_while_in_scope" seemed a bit verbose

@peci1
Copy link
Contributor

peci1 commented Jul 14, 2021

I tested this PR and it works as advertised. Thanks!

I also added another value for the default argument - parent, which means that the machine becomes default in the parent context... that is useful if you want to define the machines in a separate launch file which is then included at relevant places in other launch files. It is just a few changed lines compared to this PR. Would you be interested in getting this feature here? On one hand, I consider this a useful feature, on the other hand, it might become a bit unintuitive when included files can change the default machine...

@peci1
Copy link
Contributor

peci1 commented Jul 14, 2021

For those who do not want to wait until this PR is merged, I created a package that hacks roslaunch and adds this functionality: https://github.com/ctu-vras/scoped_roslaunch .

@ameysutavani
Copy link

Hello all,
Touch basing on this PR. Is is possible to get it merged?

@DanyResasco
Copy link

hi, I need this feature too. is it possible to get merged? @peci1 I'm tried to use your patch could you please tell me how to use it? Maybe I installed it wrong.

@130s
Copy link
Member

130s commented Mar 25, 2022

I asked whether there's a room for merging a fix to the targeted issue #1884 (comment). That said, the way this PR suggests as of now seems to be adding a new capability ('scope'), which I'm not sure if we can still add to already-released core library. Just my feeling.

@peci1
Copy link
Contributor

peci1 commented Mar 27, 2022

If the wiki docs have been correct for the past few years, then this PR is just fixing a bug. It adds the "new" functionality in order to keep code using the wrong (old, wiki-nonconformant) implementation working.

@130s Do you have a reference to a decision where OR decided not adding any new functionality into released core ROS1 packages? In earlier versions, it was quite common that released distros gained new functionality - if its implementation did not break ABI and API. I know everything is ROS 2 now, but Noetic still has quite some lifespan ahead and it would be sad to know it will not gain even non-intrusive features. Also, there is recent new functionality that has been accepted to roslaunch: #1937 (both into Melodic and Noetic). So it seems that adding new features should still be possible.

@DanyResasco
Copy link

Hi,
any update on this feature? The machine tag is not working as supposed to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants