[WIP] Pass RobotModel to ik plugins#1153
[WIP] Pass RobotModel to ik plugins#1153simonschmeisser wants to merge 3 commits intomoveit:kinetic-develfrom
Conversation
Did you read #150 (comment)?
Adding a virtual method breaks ABI as well.
|
|
@rhaschke Could we go back to closing PRs once discussion is finished? |
|
With this PR ownership of KinematicSolvers (ik plugins) is transfered to RobotModelLoader. So once RobotModelLoader gets released it will release the KinematicsSolvers as well as their use count is now 0. RobotModel only has a weak pointer on the KinematicSolvers which does not increase the use counter. So step 1 is done, ik plugin unloaded/released. Now during the release of the IK plugin it releases all it's shared pointers to RobotModel (as well as all RobotStates that have shared pointers to RobotModel). If nobody else (outside the kinematics code) owns further shared_pointers of RobotModel it's use count now reaches zero as well and it gets released. This PR demonstrates a way to achieve the goal of having a shared RobotModel available in plugins and does not add new shared pointer loops. That those exist elsewhere is an unrelated problem. |
Sorry, I didn't want to offend you. For me it was so obvious that this approach is wrong.
Thanks for clarifying your approach. I agree that this will work. However, I disagree to transferring ownership of KinematicSolvers from JMG to RobotModelLoader. |
rhaschke
left a comment
There was a problem hiding this comment.
IMHO this is a HACK, misusing RobotModelLoader as the storage place for IK solver instances, while they are actually owned by the RobotModel / JMG.
|
@simonschmeisser I filed #1166 as an alternative approach. Please have a look. |
|
Closing in favour of merged #1166. |
This is a proof of concept for further discussion of a way to pass
RobotModel(Ptr)to kinematics plugins. It is inspired by #150 and the recent and not so recent discussion thereinThe main objective of this (and previous PRs (and previous 4y old PRs)) is to
RobotModelPassing in
std::shared_ptr<RobotModel>(akaRobotModelPtr) tokinematics::KinematicsBasederived plugins and storing it there or in a member variableRobotStatewill however create a cyclic shared pointer loop asRobotModel->JointModelGroup->KinematicsSolver/KinematicsBase(plugin) ->RobotModelThis was discussed at length yesterday at #150 (comment) and following
The solution sketched here is to turn the shared pointer from
JointModelGroup->KinematicsSolverinto a weak pointer and have them owned byRobotModelLoaderin itsKinematicPluginLoader.KinematicPluginLoaderalready has theKinematicSolversin a cache so changes there would only be needed for clarity. We could also move/copy theKinematicSolversto a more explicit "storage" directly inRobotModelLoader(or elsewhere outside ofRobotModel)Further points open for discussion:
This PR adds a member variable of type
RobotModelPtrtokinematics::KinematicsBasefor simple access. This breaks ABI. (but not API!) Still it means that e.g. ikFast plugins from ros-I packages need to be recompiled to be able to be loaded. This could be avoided by having avirtual setRobotModel(RobotModelPtr)be called right after the constructor and not storing the pointer inKinematicsBase. Plugins could then override this function to store the pointer themselves, thus preserving ABI as well.This PR is made against kinetic-devel, mostly for convenience. Since it can change behavior it should probably only (eventually) be applied against melodic. I'm sure this can be handled by git ninjas
If during the discussion slight hints of approval emerge, I would port further plugins as well during this slightly extended World MoveIt! Week. They will continue working as is however for the time being.