Allow KinematicBase to have RobotModel passed in to save loading time #216
Conversation
| const std::string& base_frame, | ||
| const std::string& tip_frame, | ||
| double search_discretization, | ||
| const moveit::core::RobotModel* robot_model) |
There was a problem hiding this comment.
should this be a smart pointer rather than a raw one?
There was a problem hiding this comment.
Kinematic plugins are loaded within the RobotModel constructor, so it is impossible to use a RobotModel smart pointer. I instead have to use enable_shared_from_this to get the smart pointer later. This feature was very tricky to implement.
There was a problem hiding this comment.
Ah that does make sense...
|
Does this break compilation or use of existing IKFast plugins? If not, +1 from me. |
|
It does not break IKFast plugins, at least at compile time. I should probably do some more runtime testing |
| * @param robot_model - intended to replace reliance on robot_description, this allows the URDF to be loaded much quicker | ||
| * @return True if initialization was successful, false otherwise | ||
| */ | ||
| virtual bool initialize(const std::string& robot_description, |
There was a problem hiding this comment.
I don't like this new function - if we don't need robot description (since we have robot model), we should not need to pass it in. I would redesign this to just use a RobotModel ptr (with group name and search discretization).
There was a problem hiding this comment.
See comments for moveit_ros PR
There was a problem hiding this comment.
another way to do this could be to have the new initialize function be simply: initialize(robot_model, search_discretization, group_name) - make it return false by default, check on that in kinematics_plugin loader and try the other (older) initialize function then. This way things will stay cleaner and separated and we won't break anyone using the old initialize function (yet). For ROS-J, I would honestly prefer writing a new plugin with an attempt to keep the base class as it is (as far as possible) - bandaging an old plugin can get painful after a while.
|
I don't use IKFast plugins as much but are there tests for those plugins? I would prefer this PR to be accompanied by tests implemented for those plugins first. Also, make sure that the tests that are already there for the PR2 pass (which you might have checked already). Changes in this particular component of MoveIt! affect everything else so its important to be careful. |
|
I do not believe there are tests for IKFast plugins, and I am not very motivated to create them since I also do not currently use IKFast (though I do help maintain the package). I haven't run PR2 tests yet, will do. |
|
I ran the PR2 tests and they do pass. |
|
One issue with this PR I currently have - when compiling I now always get a deprecated warning because I have the new function by default calling the old function (for compatibility):
Is there a way to suppress the warning for that one instance? |
|
I have tried this PR and it broke MoveIt! in ways I could not understand - all the planning plugins disappeared. I agree that we need a way to initialize MoveIt! classes using a robot model passed in but this PR does not do it the right way. |
|
@sachinchitta I do not understand why you closed this PR when you agree MoveIt! needs this functionality. Even though this PR did not get addressed for an entire year doesn't mean its bad.
When I first created this PR I put in a lot of effort to make this backwards compatible, I tested it with several different IK solvers, and I ran the entire MoveIt! test suite. I've been using this feature for a year now in my own MoveIt fork and have had no issues. I did just merge it with the latest jade and the basic MoveIt demo.launch is not working, likely because the recent KinematicBase multi-solution changes. I could address this. But if I fix it will there be support in getting it merged in or will you close it in another year? |
|
I closed it because of the reasons mentioned when I tested it. These are the basic interaction tests I always do before merging any PR (not exhaustive): Startup a demo.launch for a robot that I have (e.g. moveit_examples here has a Fanuc: https://github.com/sachinchitta/moveit_examples)
In this PR's case, all the planners disappeared. I did spend a bunch of time trying to figure it out - note that I have pulled in other PRs and updated them myself where I thought the functionality was useful and important - e.g. moveit/moveit_ros#625. I think this functionality is important and very significant but it feels ad-hoc and weird to have RobotModel use kinematics base as a building block and vice-versa. I would prefer it if we rethink the relationship between the two in a more fundamental way - I think robot model should offer some kinematics solvers by default internally and expose a plugin model for other types of solvers. |
This allows some IK solvers, including the default KDL solver in MoveIt!, to load much faster by passing in a pre-loaded robot model instead of loading and parsing one from the parameter server each time an IK solver is initialized. This is important because multiple IK solvers are loaded, typically one for each planning group and again for each MoveIt! node. This does not currently improve loading time of IKFast solvers, however, because those robot models must be converted to Collada format and parsed by openrave.
Changes:
getParentModelPtr()to joint model groupboost::enable_shared_from_thisto allow the shared pointer to be recieved by objects that only have its raw pointerAdditionally, KinematicsBase is now forward declaring RobotState and RobotModel, so @isucan suggested:
I have not done this yet, but am thinking I could create a new library
moveit_corethat includesmoveit_robot_model,moveit_robot_state, andmoveit_kinematics_base. Thoughts? It still currently compiles without this last change though.