moveit / moveit_setup_assistant

THIS REPO HAS MOVED TO https://github.com/ros-planning/moveit
8 stars 20 forks source link

Added command-line SRDF updater #108

Closed mathias-luedtke closed 8 years ago

mathias-luedtke commented 8 years ago

More than three years ago I have implemented a SRDF updater that basically wraps computeDefaultCollisions. I have now updated it to indigo and added proper argument parsing.

It reads a config package path or explicit URDF and SRDF paths and recalculates the collision pairs that can be disabled. URDF and SRDF can be xacros and addtional xacro arguments can be passed through. Another notable option is to keep the existing disabled collision pairs from the input in the output.

Exampe usage:

 rosrun moveit_setup_assistant moveit_setup_assistant_updater --config-pkg PATH --trials 100000 -- --inorder

@guihomework, @rhaschke: FYI

rhaschke commented 8 years ago

Can you comment a little bit more on the use case of this extension? Can't you simply reload the moveit config in the setup assistant and retrigger computation of default collisions? What's different in your approach?

mathias-luedtke commented 8 years ago

What's different in your approach?

The updater is scriptable, runs headless and without a ROS master.

Can't you simply reload the moveit config in the setup assistant and retrigger computation of default collisions?

The setup assistant is great for initial set-up, but a little bit cumbersome if you just want to update the allowed collisions matrix. Three years ago it was even more complicated because it did not support xacro. That's why we added this feature back then.

Can you comment a little bit more on the use case of this extension?

  • Recalculation after URDF/xacro was modified (perhaps as part of a build chain?)
  • Custom package layout that is not using the assistant's templates
  • Auto-generated descriptions (URDF+SRDF) for composite robots (and their environment)

Actually this is the history of my use cases for this tool ;)

guihomework commented 8 years ago

@rhaschke This code might be the solution for the missing element of the auto-generated arm + hand combination Shadow created. One can take any SRDF arm, generated with the assistant, by the arm experts, any SRDF hand generated with the assistant by the hand expert, and then generate the missing element of the collision disable matrix of the combination of the two. @ugocupcic :FYI

ugocupcic commented 8 years ago

:+1:

rhaschke commented 8 years ago

+1 This is an additional binary not interferring with anything else. @ipa-mdl Where do you want to provide documentation for that program? Could you add some more usage information into the binary? Is there any place on the Web to add usage information too, e.g. close to the setup assistant docs?

rhaschke commented 8 years ago

The program could be more useful, if the recomputation of collision matrix can be restricted to a pair of link sets. Typically, using the setup assistant, the auto-generated matrix is fine-tuned manually afterwards. Those manual adaptions will be completely lost using the program. I guess a typical use case is to change e.g. the tool and thus recompute collision matrix for the tool with all other links. However, an already fine-tuned collision matrix for all other links shouldn't be discarded.

mathias-luedtke commented 8 years ago

Where do you want to provide documentation for that program?

The documentation can be added to README or in the (ROS) wiki. I don't want to include it in the binary.

The program could be more useful, if the recomputation of collision matrix can be restricted to a pair of link sets.

This is limitation of the computeDefaultCollisions function.

Typically, using the setup assistant, the auto-generated matrix is fine-tuned manually afterwards. Those manual adaptions will be completely lost using the program.

You can use the keep flag to keep them. (The GUI cannot do this right now)

davetcoleman commented 8 years ago

I think this is a useful tool to add to the Setup Assistant. I do think there is some duplicate functionality that would be better incorporated into the whole package instead of only here - all the file loading and saving should be the same as used in the main program.

Thanks for this contribution!

mathias-luedtke commented 8 years ago

I think this is a useful tool to add to the Setup Assistant. I do think there is some duplicate functionality that would be better incorporated into the whole package instead of only here - all the file loading and saving should be the same as used in the main program.

There are three reasons for the duplicated code:

  1. Most reading functions are located in https://github.com/ros-planning/moveit_setup_assistant/blob/indigo-devel/src/widgets/start_screen_widget.cpp and are coupled with Qt error dialogs etc.
  2. The loading functions contained duplicated code
  3. The first implementation was a copy with all Qt stuff removed (https://github.com/ipa-mdl/moveit_setup_assistant/blob/groovy-devel/src/collisions_updater.cpp)

So, the first two reasons can be addressed somehow, but not in this feature pull request.

The loadFileToString is a condensed version that could be potentially split (xacro support only for XML) and be located somewhere else.

I can move it and some other small snippets into other places. Many lines just operate on MoveItConfigData and belong there. I can add them to the class, but the GUI has to be migrated by someone else.

mathias-luedtke commented 8 years ago

I have now refactored the collision update itself and some path functions. The GUI should not be effected by these changes, but I haven't tested it yet. Please double check them!

The loading function could be move somewhere else, but I was unsure where to put it. Any ideas?

davetcoleman commented 8 years ago

The loading function could be move somewhere else, but I was unsure where to put it. Any ideas?

The GUI loading functions are currently mixed in with the QT widget code, which is less than ideal and my 4 year ago self's fault. I think they should all be moved inside the MoveItConfigData class and reused by your command line utility, but I don't blame you if you aren't up to the task of refactoring all of that. I didn't foresee wanting a command line interface back then.

sachinchitta commented 8 years ago

I think this feature is very useful. A librarized version of this capability would be very useful too. Also, documentation can go into or beside the MoveIt! Setup Assistant tutorial - http://docs.ros.org/indigo/api/moveit_setup_assistant/html/doc/tutorial.html

mathias-luedtke commented 8 years ago

I have now moved the file loading helpers into a separate location inside the tools library.

@sachinchitta: Is there anything left that is worth to be librarized? The underlying compute function was contained in the moveit_setup_assistant_tools library anyway.

sachinchitta commented 8 years ago

Looks fine to me.