[strands_movebase] Pull the 3d obstacle avoidance out of scitos_2d_navigation and calibrate_chest out of scitos_common

strands-project / strands_movebase

A repository for all the STRANDS-augmented movebase, including 3D obstacle avoidance, etc.

MIT License

10 stars 20 forks source link

[strands_movebase] Pull the 3d obstacle avoidance out of scitos_2d_navigation and calibrate_chest out of scitos_common #1

Closed nilsbore closed 10 years ago

nilsbore commented 10 years ago

This pull does the following:

Pulls the 3d move_base configuration out of scitos_2d_navigation, places it in strands_movebase/launch/config.launch
Replicates the behaviour of scitos_2d_navigation/launch/scitos_2d_nav.launch in strands_movebase/launch/movebase.launch
Moves all of the 3d processing nodes to strands_movebase.
Moves calibrate_chest to this package.
Still relies on scitos_2d_navigation for navigation if using only laser.
Make 3d obstacle avoidance default.
Add install targets.

This pull needs changes to scitos_2d_navigation and scitos_common, specifically including scitos_common/scitos_description/launch/scitos_state_publisher.launch, where the chest transform update node is launched at startup.

I will wait to merge this until the above has been fixed and the change has been reviewed. @marc-hanheide , any thoughts on this structure?

marc-hanheide commented 10 years ago

great job so far!

Comments:

usually when we have a package with the same name as the repository it is a metapackage. Here, you have quite actual stuff in strands_movebase. I'm not saying this is wrong but it's non-conventional. Also, doesn't strands_movebase also depend on calibrate_chest? It needs the chest_calibration_publisher as a run_depend doesn't it?
You are not installing the nodes, launch and parameter files. Make sure they are installed in the CMakeLists.txt. You should run catkin_make install and see if the $CATKIN_ROOT/install prefix has all you'd expect to be there.

nilsbore commented 10 years ago

Yeah, I didn't really know what to do there, but I foresee more stuff going in here so I think it's nice to have the move_base stuff as a separate package.

strands_movebase does not depend directly on calibrate_chest, calibrate_chest updates the TF and strands_movebase then checks the frame of the input cloud and gets the transform from TF.

Allright, I'll take care of that and look at the other release stuff.

Thanks for the input, I mostly wanted to get an opinion before going through with everything.

marc-hanheide commented 10 years ago

@nilsbore as you might have seen I did a few fixes to scitos_2d_navigation. This is mostly because I forgot we were to dissolve it ;-) It now has install targets, but still you should go ahead with this refactoring.

nilsbore commented 10 years ago

Hey, @marc-hanheide . Sorry, I've been very busy with my ICRA paper. I'll get on this now that it's finished.

nilsbore commented 10 years ago

@marc-hanheide I'll merge this now if OK, there's another merge for scitos_2d_navigation: https://github.com/strands-project/scitos_2d_navigation/pull/47 but I will have to go through packages and change launch files from scitos_2d_navigation to strands_movebase before that is merged.

marc-hanheide commented 10 years ago

sure, merge it