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

moving launch files for processing nodelets to a driver independent package #5

Closed
piyushk opened this issue Jul 30, 2013 · 15 comments
Closed

Comments

@piyushk
Copy link
Contributor

piyushk commented Jul 30, 2013

@jonbinney @bit-pirate

I have moved launch files to a separate rgbd_launch package. You can use this rosinstall file to test the new organization.

If things look alright to you, I will first release rgbd_launch, and then submit a pull request for openni_launch and freenect_launch once rgbd_launch makes it to shadow-fixed.

Additionally, there are a number of examples in freenect_launch that you can test out. See the documentation for reference: http://ros.org/wiki/freenect_launch

@jonbinney
Copy link
Contributor

Very cool. I just ran openni_launch and it worked great. We probably want some way for users to find the new launchfiles. For instance, if they used openni_launch/kinect_frames.launch on groovy in one of their own launchfiles, then their launchfiles wouldn't work anymore.

Maybe dummy launchfiles in openni_launch and freenect_launch which just launch a node that says "this launchfile has moved to rgbd_launch"?

@bit-pirate
Copy link
Member

Hm, sounds like too much extra work to me. I would just intentionally break anyone's launch files and leave a text file there containing a comment about the new location. Roslaunch's error message (can't find file) gives a good idea about what's going on.

Additionally, I'd like to propose renaming of launch files, which are not used directly, from .launch to *.xml []. In that way those files won't show up in roslaunch's auto-completion. Also, it gives the user an hint that those files are not intended to be used by them directly.

[*] Actually, it would be better to put an underscore (or similar) as suffix to those "include" launch files, but that hasn't been implemented yet (ros/ros_comm#254).

@jonbinney
Copy link
Contributor

An underscore sounds like a good idea to let people know the launchfiles are private.

If we just have a text file to let people know that the old launchfiles have moved, won't they have to dig around for it? They'll call roslaunch with their custom launchfile that uses some launchfile in openni_launch, for instance, and it will fail. Then, if it was me, I'd do roscd openni_launch; ls launch. I notice that hey, almost all the old launchfiles are gone. I suppose a very explicitly named textfile could work... something like openni_launch/launch/WHERE_THE_LAUNCHFILES_WENT

@bit-pirate
Copy link
Member

What about we rename them to: _somelauncher.xml In this way they won't show up in the tab completion and we already introduce the "underscore means include/private" thinking.

Adding an explicit text file should do the trick, I think. Just to be clear, your idea is much nicer - I'm just too lazy to implement it. :-)

@piyushk
Copy link
Contributor Author

piyushk commented Aug 2, 2013

All these suggestions sound great. I'll write a small node that displays a
fatal message about the launch file being moved.

I have some other work to do today and tomorrow. I'll work on this sometime
early next week and ping the thread.

Do you think we still have time to release these changes into Hydro?

On Thu, Aug 1, 2013 at 7:23 PM, Marcus Liebhardt
[email protected]:

What about we rename them to: _somelauncher.xml In this way they won't
show up in the tab completion and we already introduce the "underscore
means include/private" thinking.

Adding an explicit text file should do the trick, I think. Just to be
clear, your idea is much nicer - I'm just too lazy to implement it. :-)


Reply to this email directly or view it on GitHubhttps://github.com//issues/5#issuecomment-21979397
.

@jonbinney
Copy link
Contributor

With the node that displays the fatal message, I'm ok with releasing this
into hydro. I think that most people use only the toplevel launchfiles in
openni_launch/freenect.launch, and for the people that use the individual
launchfiles the fix shouldn't be hard.

On Thu, Aug 1, 2013 at 5:27 PM, Piyush Khandelwal
[email protected]:

All these suggestions sound great. I'll write a small node that displays a
fatal message about the launch file being moved.

I have some other work to do today and tomorrow. I'll work on this
sometime
early next week and ping the thread.

Do you think we still have time to release these changes into Hydro?

On Thu, Aug 1, 2013 at 7:23 PM, Marcus Liebhardt
[email protected]:

What about we rename them to: _somelauncher.xml In this way they won't
show up in the tab completion and we already introduce the "underscore
means include/private" thinking.

Adding an explicit text file should do the trick, I think. Just to be
clear, your idea is much nicer - I'm just too lazy to implement it. :-)


Reply to this email directly or view it on GitHub<
https://github.com/ros-drivers/openni_launch/issues/5#issuecomment-21979397>

.


Reply to this email directly or view it on GitHubhttps://github.com//issues/5#issuecomment-21979531
.

@piyushk
Copy link
Contributor Author

piyushk commented Aug 12, 2013

@jonbinney @bit-pirate @vrabaud

I've updated all 3 packages based on the current discussion. Try merging/updating from the original rosinstall file again and let me know. I will start an issue in rgbd_launch regarding launch file cleanup, but I would like to tackle migration before we try that out.

@piyushk
Copy link
Contributor Author

piyushk commented Aug 22, 2013

Hey folks! I have released a new version of freenect_launch and rgbd_launch that solves #5, #6 and #7. If you can test the system release of freenect_launch (v0.3.0) and let me know if these problems have been solved, I will push the same changes to openni_launch.

@bit-pirate
Copy link
Member

Here is a small update on the launch includes naming: ros/ros_comm#254
We are applying the same rule to Kobuki and TurtleBot packages. I think we should adapt this here as well.

@bit-pirate
Copy link
Member

@piyushk Could you please name the remaining things you like us to test?

@piyushk
Copy link
Contributor Author

piyushk commented Aug 22, 2013

@bit-pirate I have opened up #8

rgbd_launch is now available as a deb package. Can you verify that the pull request works for you, and that it solves #6 and #7?

Thanks!

@bit-pirate
Copy link
Member

@piyushk I have tested your PR and I can confirm that it fixes #6 and #7.
My test included the quick start instructions.

@jonbinney
Copy link
Contributor

I'll go ahead and merge #8 (i don't have an asus right now to test, but sounds like you guys have covered that). I'll do a release shortly.

@jonbinney
Copy link
Contributor

Just released openni_launch into hydro: ros/rosdistro#1857

@130s
Copy link
Member

130s commented Jan 6, 2018

Moved to openni_camera repo. Please follow the referenced link.

@130s 130s closed this as completed Jan 6, 2018
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

No branches or pull requests

4 participants